| Message ID | 20170609165154.23577-3-apop@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 09, 2017 at 07:51:54PM +0300, Adrian Pop wrote: > Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a > privileged domain to change the value of the #VE suppress bit for a > page. > > Add a libxc wrapper for invoking this hvmop. > > Signed-off-by: Adrian Pop <apop@bitdefender.com> > --- > tools/libxc/include/xenctrl.h | 2 ++ > tools/libxc/xc_altp2m.c | 24 +++++++++++++++++++ > xen/arch/x86/hvm/hvm.c | 14 +++++++++++ > xen/arch/x86/mm/mem_access.c | 52 +++++++++++++++++++++++++++++++++++++++++ > xen/include/public/hvm/hvm_op.h | 15 ++++++++++++ > xen/include/xen/mem_access.h | 3 +++ > 6 files changed, 110 insertions(+) > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h > index 1629f412dd..f6ba8635bf 100644 > --- a/tools/libxc/include/xenctrl.h > +++ b/tools/libxc/include/xenctrl.h > @@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid, > /* Switch all vCPUs of the domain to the specified altp2m view */ > int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, > uint16_t view_id); > +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid, > + uint16_t view_id, xen_pfn_t gfn, bool sve); > int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, > uint16_t view_id, xen_pfn_t gfn, > xenmem_access_t access); > diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c > index 0639632477..4710133918 100644 > --- a/tools/libxc/xc_altp2m.c > +++ b/tools/libxc/xc_altp2m.c > @@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, > return rc; > } > > +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid, > + uint16_t view_id, xen_pfn_t gfn, bool sve) > +{ > + int rc; > + DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg); > + > + arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg)); > + if ( arg == NULL ) > + return -1; > + > + arg->version = HVMOP_ALTP2M_INTERFACE_VERSION; > + arg->cmd = HVMOP_altp2m_set_suppress_ve; > + arg->domain = domid; > + arg->u.set_suppress_ve.view = view_id; > + arg->u.set_suppress_ve.gfn = gfn; > + arg->u.set_suppress_ve.suppress_ve = sve; > + > + rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m, > + HYPERCALL_BUFFER_AS_ARG(arg)); Indentation. With that fixed, the change to libxc looks good: Acked-by: Wei Liu <wei.liu2@citrix.com>
On Mon, Jun 12, 2017 at 04:51:48PM +0100, Wei Liu wrote: > On Fri, Jun 09, 2017 at 07:51:54PM +0300, Adrian Pop wrote: > > Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a > > privileged domain to change the value of the #VE suppress bit for a > > page. > > > > Add a libxc wrapper for invoking this hvmop. > > > > Signed-off-by: Adrian Pop <apop@bitdefender.com> > > --- > > tools/libxc/include/xenctrl.h | 2 ++ > > tools/libxc/xc_altp2m.c | 24 +++++++++++++++++++ > > xen/arch/x86/hvm/hvm.c | 14 +++++++++++ > > xen/arch/x86/mm/mem_access.c | 52 +++++++++++++++++++++++++++++++++++++++++ > > xen/include/public/hvm/hvm_op.h | 15 ++++++++++++ > > xen/include/xen/mem_access.h | 3 +++ > > 6 files changed, 110 insertions(+) > > > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h > > index 1629f412dd..f6ba8635bf 100644 > > --- a/tools/libxc/include/xenctrl.h > > +++ b/tools/libxc/include/xenctrl.h > > @@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid, > > /* Switch all vCPUs of the domain to the specified altp2m view */ > > int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, > > uint16_t view_id); > > +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid, > > + uint16_t view_id, xen_pfn_t gfn, bool sve); > > int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, > > uint16_t view_id, xen_pfn_t gfn, > > xenmem_access_t access); > > diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c > > index 0639632477..4710133918 100644 > > --- a/tools/libxc/xc_altp2m.c > > +++ b/tools/libxc/xc_altp2m.c > > @@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, > > return rc; > > } > > > > +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid, > > + uint16_t view_id, xen_pfn_t gfn, bool sve) > > +{ > > + int rc; > > + DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg); > > + > > + arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg)); > > + if ( arg == NULL ) > > + return -1; > > + > > + arg->version = HVMOP_ALTP2M_INTERFACE_VERSION; > > + arg->cmd = HVMOP_altp2m_set_suppress_ve; > > + arg->domain = domid; > > + arg->u.set_suppress_ve.view = view_id; > > + arg->u.set_suppress_ve.gfn = gfn; > > + arg->u.set_suppress_ve.suppress_ve = sve; > > + > > + rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m, > > + HYPERCALL_BUFFER_AS_ARG(arg)); > > Indentation. Oh, missed that. > With that fixed, the change to libxc looks good: > > Acked-by: Wei Liu <wei.liu2@citrix.com> Thank you!
On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote: > Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a > privileged domain to change the value of the #VE suppress bit for a > page. > > Add a libxc wrapper for invoking this hvmop. > > Signed-off-by: Adrian Pop <apop@bitdefender.com> > --- > tools/libxc/include/xenctrl.h | 2 ++ > tools/libxc/xc_altp2m.c | 24 +++++++++++++++++++ > xen/arch/x86/hvm/hvm.c | 14 +++++++++++ > xen/arch/x86/mm/mem_access.c | 52 +++++++++++++++++++++++++++++++++++++++++ > xen/include/public/hvm/hvm_op.h | 15 ++++++++++++ > xen/include/xen/mem_access.h | 3 +++ > 6 files changed, 110 insertions(+) > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h > index 1629f412dd..f6ba8635bf 100644 > --- a/tools/libxc/include/xenctrl.h > +++ b/tools/libxc/include/xenctrl.h > @@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid, > /* Switch all vCPUs of the domain to the specified altp2m view */ > int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, > uint16_t view_id); > +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid, > + uint16_t view_id, xen_pfn_t gfn, bool sve); > int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, > uint16_t view_id, xen_pfn_t gfn, > xenmem_access_t access); > diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c > index 0639632477..4710133918 100644 > --- a/tools/libxc/xc_altp2m.c > +++ b/tools/libxc/xc_altp2m.c > @@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, > return rc; > } > > +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid, > + uint16_t view_id, xen_pfn_t gfn, bool sve) > +{ > + int rc; > + DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg); > + > + arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg)); > + if ( arg == NULL ) > + return -1; > + > + arg->version = HVMOP_ALTP2M_INTERFACE_VERSION; > + arg->cmd = HVMOP_altp2m_set_suppress_ve; > + arg->domain = domid; > + arg->u.set_suppress_ve.view = view_id; > + arg->u.set_suppress_ve.gfn = gfn; > + arg->u.set_suppress_ve.suppress_ve = sve; > + > + rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m, > + HYPERCALL_BUFFER_AS_ARG(arg)); > + > + xc_hypercall_buffer_free(handle, arg); > + return rc; > +} > + > int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, > uint16_t view_id, xen_pfn_t gfn, > xenmem_access_t access) > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 70ddc81d44..dd8e205551 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -4358,6 +4358,7 @@ static int do_altp2m_op( > case HVMOP_altp2m_destroy_p2m: > case HVMOP_altp2m_switch_p2m: > case HVMOP_altp2m_set_mem_access: > + case HVMOP_altp2m_set_suppress_ve: > case HVMOP_altp2m_change_gfn: > break; > default: > @@ -4475,6 +4476,19 @@ static int do_altp2m_op( > a.u.set_mem_access.view); > break; > > + case HVMOP_altp2m_set_suppress_ve: > + if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 ) > + rc = -EINVAL; > + else > + { > + gfn_t gfn = _gfn(a.u.set_mem_access.gfn); > + unsigned int altp2m_idx = a.u.set_mem_access.view; > + bool suppress_ve = a.u.set_suppress_ve.suppress_ve; > + > + rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx); > + } > + break; > + > case HVMOP_altp2m_change_gfn: > if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 ) > rc = -EINVAL; > diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c > index d0b0767855..8c39db13e3 100644 > --- a/xen/arch/x86/mm/mem_access.c > +++ b/xen/arch/x86/mm/mem_access.c > @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access) > } > > /* > + * Set/clear the #VE suppress bit for a page. Only available on VMX. > + */ > +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, > + unsigned int altp2m_idx) > +{ > + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); > + struct p2m_domain *ap2m = NULL; > + struct p2m_domain *p2m; > + mfn_t mfn; > + p2m_access_t a; > + p2m_type_t t; > + int rc; > + > + if ( !cpu_has_vmx_virt_exceptions ) > + return -EOPNOTSUPP; > + > + /* This subop should only be used from a privileged domain. */ > + if ( !current->domain->is_privileged ) > + return -EINVAL; This check looks wrong to me. If this subop should only be used by an external (privileged) domain then I don't think this should be implemented as an HVMOP, looks more like a domctl to me. Tamas
>>> On 15.06.17 at 21:01, <tamas@tklengyel.com> wrote: > On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote: >> --- a/xen/arch/x86/mm/mem_access.c >> +++ b/xen/arch/x86/mm/mem_access.c >> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access) >> } >> >> /* >> + * Set/clear the #VE suppress bit for a page. Only available on VMX. >> + */ >> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, >> + unsigned int altp2m_idx) >> +{ >> + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); >> + struct p2m_domain *ap2m = NULL; >> + struct p2m_domain *p2m; >> + mfn_t mfn; >> + p2m_access_t a; >> + p2m_type_t t; >> + int rc; >> + >> + if ( !cpu_has_vmx_virt_exceptions ) >> + return -EOPNOTSUPP; >> + >> + /* This subop should only be used from a privileged domain. */ >> + if ( !current->domain->is_privileged ) >> + return -EINVAL; > > This check looks wrong to me. If this subop should only be used by an > external (privileged) domain then I don't think this should be > implemented as an HVMOP, looks more like a domctl to me. I think this wants to be an XSM_DM_PRIV check instead. Jan
>>> On 09.06.17 at 18:51, <apop@bitdefender.com> wrote: > Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a > privileged domain to change the value of the #VE suppress bit for a > page. > > Add a libxc wrapper for invoking this hvmop. > > Signed-off-by: Adrian Pop <apop@bitdefender.com> > --- Please properly version your patch submissions, and please put here a brief summary of what changed from the previous version. > --- a/xen/arch/x86/mm/mem_access.c > +++ b/xen/arch/x86/mm/mem_access.c > @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access) > } > > /* > + * Set/clear the #VE suppress bit for a page. Only available on VMX. > + */ > +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, > + unsigned int altp2m_idx) > +{ > + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); > + struct p2m_domain *ap2m = NULL; > + struct p2m_domain *p2m; > + mfn_t mfn; > + p2m_access_t a; > + p2m_type_t t; > + int rc; > + > + if ( !cpu_has_vmx_virt_exceptions ) > + return -EOPNOTSUPP; > + > + /* This subop should only be used from a privileged domain. */ > + if ( !current->domain->is_privileged ) > + return -EINVAL; Beyond the question of what check to use, perhaps -EPERM? > + /* #VE should be enabled for this vcpu. */ > + if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) ) > + return -EINVAL; This also doesn't really is an invalid argument error - perhaps e.g. -ENXIO or -ENOENT? Be creative, but don't use -EINVAL for everything. > --- a/xen/include/public/hvm/hvm_op.h > +++ b/xen/include/public/hvm/hvm_op.h > @@ -237,6 +237,18 @@ struct xen_hvm_altp2m_set_mem_access { > typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t; > DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t); > > +struct xen_hvm_altp2m_set_suppress_ve { > + /* view */ > + uint16_t view; > + uint8_t suppress_ve; > + uint8_t pad1; > + uint32_t pad2; > + /* gfn */ > + uint64_t gfn; Commenting fields with their field names is, I'm sorry, rather pointless. What gfn means is most likely clear without comment. For view I'm not sure (depends on conventions elsewhere), but the boolean nature of suppress_ve clearly wants commenting on (especially also to clarify behavior of values other than 0 and 1). > +}; > +typedef struct xen_hvm_altp2m_set_suppress_ve xen_hvm_altp2m_set_suppress_ve_t; > +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_suppress_ve_t); I think we should stop the habit of creating such typedefs and handles when ... > @@ -276,6 +290,7 @@ struct xen_hvm_altp2m_op { > struct xen_hvm_altp2m_vcpu_enable_notify enable_notify; > struct xen_hvm_altp2m_view view; > struct xen_hvm_altp2m_set_mem_access set_mem_access; > + struct xen_hvm_altp2m_set_suppress_ve set_suppress_ve; ... a structure isn't meant to be used on its own anyway. Jan
On Fri, Jun 16, 2017 at 09:29:49AM -0600, Jan Beulich wrote: > >>> On 09.06.17 at 18:51, <apop@bitdefender.com> wrote: > > Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a > > privileged domain to change the value of the #VE suppress bit for a > > page. > > > > Add a libxc wrapper for invoking this hvmop. > > > > Signed-off-by: Adrian Pop <apop@bitdefender.com> > > --- > > Please properly version your patch submissions, and please put > here a brief summary of what changed from the previous version. OK. I've mistakenly sent the mail without setting the patch version. I've written the change list in the cover letter, but in hindsight it would have been a better idea to add the list of changes per patch instead. > > --- a/xen/arch/x86/mm/mem_access.c > > +++ b/xen/arch/x86/mm/mem_access.c > > @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access) > > } > > > > /* > > + * Set/clear the #VE suppress bit for a page. Only available on VMX. > > + */ > > +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, > > + unsigned int altp2m_idx) > > +{ > > + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); > > + struct p2m_domain *ap2m = NULL; > > + struct p2m_domain *p2m; > > + mfn_t mfn; > > + p2m_access_t a; > > + p2m_type_t t; > > + int rc; > > + > > + if ( !cpu_has_vmx_virt_exceptions ) > > + return -EOPNOTSUPP; > > + > > + /* This subop should only be used from a privileged domain. */ > > + if ( !current->domain->is_privileged ) > > + return -EINVAL; > > Beyond the question of what check to use, perhaps -EPERM? OK. > > + /* #VE should be enabled for this vcpu. */ > > + if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) ) > > + return -EINVAL; > > This also doesn't really is an invalid argument error - perhaps e.g. > -ENXIO or -ENOENT? Be creative, but don't use -EINVAL for > everything. All right. > > --- a/xen/include/public/hvm/hvm_op.h > > +++ b/xen/include/public/hvm/hvm_op.h > > @@ -237,6 +237,18 @@ struct xen_hvm_altp2m_set_mem_access { > > typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t; > > DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t); > > > > +struct xen_hvm_altp2m_set_suppress_ve { > > + /* view */ > > + uint16_t view; > > + uint8_t suppress_ve; > > + uint8_t pad1; > > + uint32_t pad2; > > + /* gfn */ > > + uint64_t gfn; > > Commenting fields with their field names is, I'm sorry, rather pointless. > What gfn means is most likely clear without comment. For view I'm not > sure (depends on conventions elsewhere), but the boolean nature of > suppress_ve clearly wants commenting on (especially also to clarify > behavior of values other than 0 and 1). OK then. > > +}; > > +typedef struct xen_hvm_altp2m_set_suppress_ve xen_hvm_altp2m_set_suppress_ve_t; > > +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_suppress_ve_t); > > I think we should stop the habit of creating such typedefs and handles > when ... > > > @@ -276,6 +290,7 @@ struct xen_hvm_altp2m_op { > > struct xen_hvm_altp2m_vcpu_enable_notify enable_notify; > > struct xen_hvm_altp2m_view view; > > struct xen_hvm_altp2m_set_mem_access set_mem_access; > > + struct xen_hvm_altp2m_set_suppress_ve set_suppress_ve; > > ... a structure isn't meant to be used on its own anyway. Yes, I agree with that.
On Mon, Jun 12, 2017 at 04:51:48PM +0100, Wei Liu wrote: > On Fri, Jun 09, 2017 at 07:51:54PM +0300, Adrian Pop wrote: > > Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a > > privileged domain to change the value of the #VE suppress bit for a > > page. > > > > Add a libxc wrapper for invoking this hvmop. > > > > Signed-off-by: Adrian Pop <apop@bitdefender.com> > > --- > > tools/libxc/include/xenctrl.h | 2 ++ > > tools/libxc/xc_altp2m.c | 24 +++++++++++++++++++ > > xen/arch/x86/hvm/hvm.c | 14 +++++++++++ > > xen/arch/x86/mm/mem_access.c | 52 +++++++++++++++++++++++++++++++++++++++++ > > xen/include/public/hvm/hvm_op.h | 15 ++++++++++++ > > xen/include/xen/mem_access.h | 3 +++ > > 6 files changed, 110 insertions(+) > > > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h > > index 1629f412dd..f6ba8635bf 100644 > > --- a/tools/libxc/include/xenctrl.h > > +++ b/tools/libxc/include/xenctrl.h > > @@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid, > > /* Switch all vCPUs of the domain to the specified altp2m view */ > > int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, > > uint16_t view_id); > > +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid, > > + uint16_t view_id, xen_pfn_t gfn, bool sve); > > int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, > > uint16_t view_id, xen_pfn_t gfn, > > xenmem_access_t access); > > diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c > > index 0639632477..4710133918 100644 > > --- a/tools/libxc/xc_altp2m.c > > +++ b/tools/libxc/xc_altp2m.c > > @@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, > > return rc; > > } > > > > +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid, > > + uint16_t view_id, xen_pfn_t gfn, bool sve) > > +{ > > + int rc; > > + DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg); > > + > > + arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg)); > > + if ( arg == NULL ) > > + return -1; > > + > > + arg->version = HVMOP_ALTP2M_INTERFACE_VERSION; > > + arg->cmd = HVMOP_altp2m_set_suppress_ve; > > + arg->domain = domid; > > + arg->u.set_suppress_ve.view = view_id; > > + arg->u.set_suppress_ve.gfn = gfn; > > + arg->u.set_suppress_ve.suppress_ve = sve; > > + > > + rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m, > > + HYPERCALL_BUFFER_AS_ARG(arg)); > > Indentation. OK. Thanks! > With that fixed, the change to libxc looks good: > > Acked-by: Wei Liu <wei.liu2@citrix.com>
On Fri, Jun 16, 2017 at 02:39:10AM -0600, Jan Beulich wrote: > >>> On 15.06.17 at 21:01, <tamas@tklengyel.com> wrote: > > On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote: > >> --- a/xen/arch/x86/mm/mem_access.c > >> +++ b/xen/arch/x86/mm/mem_access.c > >> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access) > >> } > >> > >> /* > >> + * Set/clear the #VE suppress bit for a page. Only available on VMX. > >> + */ > >> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, > >> + unsigned int altp2m_idx) > >> +{ > >> + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); > >> + struct p2m_domain *ap2m = NULL; > >> + struct p2m_domain *p2m; > >> + mfn_t mfn; > >> + p2m_access_t a; > >> + p2m_type_t t; > >> + int rc; > >> + > >> + if ( !cpu_has_vmx_virt_exceptions ) > >> + return -EOPNOTSUPP; > >> + > >> + /* This subop should only be used from a privileged domain. */ > >> + if ( !current->domain->is_privileged ) > >> + return -EINVAL; > > > > This check looks wrong to me. If this subop should only be used by an > > external (privileged) domain then I don't think this should be > > implemented as an HVMOP, looks more like a domctl to me. > > I think this wants to be an XSM_DM_PRIV check instead. I'm not sure, but I expect that to not behave as intended security-wise if Xen is compiled without XSM. Would it? It would be great if this feature worked well without XSM too.
>>> On 22.06.17 at 14:04, <apop@bitdefender.com> wrote: > On Fri, Jun 16, 2017 at 02:39:10AM -0600, Jan Beulich wrote: >> >>> On 15.06.17 at 21:01, <tamas@tklengyel.com> wrote: >> > On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote: >> >> --- a/xen/arch/x86/mm/mem_access.c >> >> +++ b/xen/arch/x86/mm/mem_access.c >> >> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, > xenmem_access_t *access) >> >> } >> >> >> >> /* >> >> + * Set/clear the #VE suppress bit for a page. Only available on VMX. >> >> + */ >> >> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, >> >> + unsigned int altp2m_idx) >> >> +{ >> >> + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); >> >> + struct p2m_domain *ap2m = NULL; >> >> + struct p2m_domain *p2m; >> >> + mfn_t mfn; >> >> + p2m_access_t a; >> >> + p2m_type_t t; >> >> + int rc; >> >> + >> >> + if ( !cpu_has_vmx_virt_exceptions ) >> >> + return -EOPNOTSUPP; >> >> + >> >> + /* This subop should only be used from a privileged domain. */ >> >> + if ( !current->domain->is_privileged ) >> >> + return -EINVAL; >> > >> > This check looks wrong to me. If this subop should only be used by an >> > external (privileged) domain then I don't think this should be >> > implemented as an HVMOP, looks more like a domctl to me. >> >> I think this wants to be an XSM_DM_PRIV check instead. > > I'm not sure, but I expect that to not behave as intended security-wise > if Xen is compiled without XSM. Would it? It would be great if this > feature worked well without XSM too. Well, without you explaining why you think this wouldn't work without XSM, I don't really know what to answer. I suppose you've grep-ed for other uses of this and/or other XSM_* values, finding that these exist in various places where all is fine without XSM? Jan
On Thu, Jun 22, 2017 at 06:13:22AM -0600, Jan Beulich wrote: > >>> On 22.06.17 at 14:04, <apop@bitdefender.com> wrote: > > On Fri, Jun 16, 2017 at 02:39:10AM -0600, Jan Beulich wrote: > >> >>> On 15.06.17 at 21:01, <tamas@tklengyel.com> wrote: > >> > On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote: > >> >> --- a/xen/arch/x86/mm/mem_access.c > >> >> +++ b/xen/arch/x86/mm/mem_access.c > >> >> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, > > xenmem_access_t *access) > >> >> } > >> >> > >> >> /* > >> >> + * Set/clear the #VE suppress bit for a page. Only available on VMX. > >> >> + */ > >> >> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, > >> >> + unsigned int altp2m_idx) > >> >> +{ > >> >> + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); > >> >> + struct p2m_domain *ap2m = NULL; > >> >> + struct p2m_domain *p2m; > >> >> + mfn_t mfn; > >> >> + p2m_access_t a; > >> >> + p2m_type_t t; > >> >> + int rc; > >> >> + > >> >> + if ( !cpu_has_vmx_virt_exceptions ) > >> >> + return -EOPNOTSUPP; > >> >> + > >> >> + /* This subop should only be used from a privileged domain. */ > >> >> + if ( !current->domain->is_privileged ) > >> >> + return -EINVAL; > >> > > >> > This check looks wrong to me. If this subop should only be used by an > >> > external (privileged) domain then I don't think this should be > >> > implemented as an HVMOP, looks more like a domctl to me. > >> > >> I think this wants to be an XSM_DM_PRIV check instead. > > > > I'm not sure, but I expect that to not behave as intended security-wise > > if Xen is compiled without XSM. Would it? It would be great if this > > feature worked well without XSM too. > > Well, without you explaining why you think this wouldn't work > without XSM, I don't really know what to answer. I suppose > you've grep-ed for other uses of this and/or other XSM_* values, > finding that these exist in various places where all is fine without > XSM? I'll check what it does then because it's not very clear to me either.
On Thu, Jun 15, 2017 at 01:01:36PM -0600, Tamas K Lengyel wrote: > On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote: > > diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c > > index d0b0767855..8c39db13e3 100644 > > --- a/xen/arch/x86/mm/mem_access.c > > +++ b/xen/arch/x86/mm/mem_access.c > > @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access) > > } > > > > /* > > + * Set/clear the #VE suppress bit for a page. Only available on VMX. > > + */ > > +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, > > + unsigned int altp2m_idx) > > +{ > > + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); > > + struct p2m_domain *ap2m = NULL; > > + struct p2m_domain *p2m; > > + mfn_t mfn; > > + p2m_access_t a; > > + p2m_type_t t; > > + int rc; > > + > > + if ( !cpu_has_vmx_virt_exceptions ) > > + return -EOPNOTSUPP; > > + > > + /* This subop should only be used from a privileged domain. */ > > + if ( !current->domain->is_privileged ) > > + return -EINVAL; > > This check looks wrong to me. If this subop should only be used by an > external (privileged) domain then I don't think this should be > implemented as an HVMOP, looks more like a domctl to me. AFAICS this could indeed be implemented as a domctl as well.
On Thu, Jun 22, 2017 at 06:13:22AM -0600, Jan Beulich wrote: > >>> On 22.06.17 at 14:04, <apop@bitdefender.com> wrote: > > On Fri, Jun 16, 2017 at 02:39:10AM -0600, Jan Beulich wrote: > >> >>> On 15.06.17 at 21:01, <tamas@tklengyel.com> wrote: > >> > On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote: > >> >> --- a/xen/arch/x86/mm/mem_access.c > >> >> +++ b/xen/arch/x86/mm/mem_access.c > >> >> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, > > xenmem_access_t *access) > >> >> } > >> >> > >> >> /* > >> >> + * Set/clear the #VE suppress bit for a page. Only available on VMX. > >> >> + */ > >> >> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, > >> >> + unsigned int altp2m_idx) > >> >> +{ > >> >> + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); > >> >> + struct p2m_domain *ap2m = NULL; > >> >> + struct p2m_domain *p2m; > >> >> + mfn_t mfn; > >> >> + p2m_access_t a; > >> >> + p2m_type_t t; > >> >> + int rc; > >> >> + > >> >> + if ( !cpu_has_vmx_virt_exceptions ) > >> >> + return -EOPNOTSUPP; > >> >> + > >> >> + /* This subop should only be used from a privileged domain. */ > >> >> + if ( !current->domain->is_privileged ) > >> >> + return -EINVAL; > >> > > >> > This check looks wrong to me. If this subop should only be used by an > >> > external (privileged) domain then I don't think this should be > >> > implemented as an HVMOP, looks more like a domctl to me. > >> > >> I think this wants to be an XSM_DM_PRIV check instead. > > > > I'm not sure, but I expect that to not behave as intended security-wise > > if Xen is compiled without XSM. Would it? It would be great if this > > feature worked well without XSM too. > > Well, without you explaining why you think this wouldn't work > without XSM, I don't really know what to answer. I suppose > you've grep-ed for other uses of this and/or other XSM_* values, > finding that these exist in various places where all is fine without > XSM? OK; it indeed does what it should without XSM as well.
On Thu, Jun 15, 2017 at 8:01 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote: > On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote: >> Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a >> privileged domain to change the value of the #VE suppress bit for a >> page. >> >> Add a libxc wrapper for invoking this hvmop. >> >> Signed-off-by: Adrian Pop <apop@bitdefender.com> >> --- >> tools/libxc/include/xenctrl.h | 2 ++ >> tools/libxc/xc_altp2m.c | 24 +++++++++++++++++++ >> xen/arch/x86/hvm/hvm.c | 14 +++++++++++ >> xen/arch/x86/mm/mem_access.c | 52 +++++++++++++++++++++++++++++++++++++++++ >> xen/include/public/hvm/hvm_op.h | 15 ++++++++++++ >> xen/include/xen/mem_access.h | 3 +++ >> 6 files changed, 110 insertions(+) >> >> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h >> index 1629f412dd..f6ba8635bf 100644 >> --- a/tools/libxc/include/xenctrl.h >> +++ b/tools/libxc/include/xenctrl.h >> @@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid, >> /* Switch all vCPUs of the domain to the specified altp2m view */ >> int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, >> uint16_t view_id); >> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid, >> + uint16_t view_id, xen_pfn_t gfn, bool sve); >> int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, >> uint16_t view_id, xen_pfn_t gfn, >> xenmem_access_t access); >> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c >> index 0639632477..4710133918 100644 >> --- a/tools/libxc/xc_altp2m.c >> +++ b/tools/libxc/xc_altp2m.c >> @@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, >> return rc; >> } >> >> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid, >> + uint16_t view_id, xen_pfn_t gfn, bool sve) >> +{ >> + int rc; >> + DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg); >> + >> + arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg)); >> + if ( arg == NULL ) >> + return -1; >> + >> + arg->version = HVMOP_ALTP2M_INTERFACE_VERSION; >> + arg->cmd = HVMOP_altp2m_set_suppress_ve; >> + arg->domain = domid; >> + arg->u.set_suppress_ve.view = view_id; >> + arg->u.set_suppress_ve.gfn = gfn; >> + arg->u.set_suppress_ve.suppress_ve = sve; >> + >> + rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m, >> + HYPERCALL_BUFFER_AS_ARG(arg)); >> + >> + xc_hypercall_buffer_free(handle, arg); >> + return rc; >> +} >> + >> int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, >> uint16_t view_id, xen_pfn_t gfn, >> xenmem_access_t access) >> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c >> index 70ddc81d44..dd8e205551 100644 >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -4358,6 +4358,7 @@ static int do_altp2m_op( >> case HVMOP_altp2m_destroy_p2m: >> case HVMOP_altp2m_switch_p2m: >> case HVMOP_altp2m_set_mem_access: >> + case HVMOP_altp2m_set_suppress_ve: >> case HVMOP_altp2m_change_gfn: >> break; >> default: >> @@ -4475,6 +4476,19 @@ static int do_altp2m_op( >> a.u.set_mem_access.view); >> break; >> >> + case HVMOP_altp2m_set_suppress_ve: >> + if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 ) >> + rc = -EINVAL; >> + else >> + { >> + gfn_t gfn = _gfn(a.u.set_mem_access.gfn); >> + unsigned int altp2m_idx = a.u.set_mem_access.view; >> + bool suppress_ve = a.u.set_suppress_ve.suppress_ve; >> + >> + rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx); >> + } >> + break; >> + >> case HVMOP_altp2m_change_gfn: >> if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 ) >> rc = -EINVAL; >> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c >> index d0b0767855..8c39db13e3 100644 >> --- a/xen/arch/x86/mm/mem_access.c >> +++ b/xen/arch/x86/mm/mem_access.c >> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access) >> } >> >> /* >> + * Set/clear the #VE suppress bit for a page. Only available on VMX. >> + */ >> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, >> + unsigned int altp2m_idx) >> +{ >> + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); >> + struct p2m_domain *ap2m = NULL; >> + struct p2m_domain *p2m; >> + mfn_t mfn; >> + p2m_access_t a; >> + p2m_type_t t; >> + int rc; >> + >> + if ( !cpu_has_vmx_virt_exceptions ) >> + return -EOPNOTSUPP; >> + >> + /* This subop should only be used from a privileged domain. */ >> + if ( !current->domain->is_privileged ) >> + return -EINVAL; > > This check looks wrong to me. If this subop should only be used by an > external (privileged) domain then I don't think this should be > implemented as an HVMOP, looks more like a domctl to me. Well after patch 1, isn't it the case that a guest has no way of clearing the suppress_ve bit? I was going to say we want the XSM_TARGET "default action" (which allows a guest to do things on itself, or a privileged domain to do it to any domain); but I think really we probably we don't want a guest to be able to *clear* the suppress_ve bit on a page for which a privileged domain has *set*; this would allow a domain to prevent the other domain from effectively introspecting on a page. This is starting to sound like another conversation I think I remember recently about making sure that *only* the guest *or* an introspection engine can use the altp2m functionality, but I can't seem to find that with a quick look. Tamas, does that ring any bells? -George
On Thu, Jul 20, 2017 at 9:11 AM, George Dunlap <George.Dunlap@eu.citrix.com> wrote: > On Thu, Jun 15, 2017 at 8:01 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote: >> On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote: >>> Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a >>> privileged domain to change the value of the #VE suppress bit for a >>> page. >>> >>> Add a libxc wrapper for invoking this hvmop. >>> >>> Signed-off-by: Adrian Pop <apop@bitdefender.com> >>> --- >>> tools/libxc/include/xenctrl.h | 2 ++ >>> tools/libxc/xc_altp2m.c | 24 +++++++++++++++++++ >>> xen/arch/x86/hvm/hvm.c | 14 +++++++++++ >>> xen/arch/x86/mm/mem_access.c | 52 +++++++++++++++++++++++++++++++++++++++++ >>> xen/include/public/hvm/hvm_op.h | 15 ++++++++++++ >>> xen/include/xen/mem_access.h | 3 +++ >>> 6 files changed, 110 insertions(+) >>> >>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h >>> index 1629f412dd..f6ba8635bf 100644 >>> --- a/tools/libxc/include/xenctrl.h >>> +++ b/tools/libxc/include/xenctrl.h >>> @@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid, >>> /* Switch all vCPUs of the domain to the specified altp2m view */ >>> int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, >>> uint16_t view_id); >>> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid, >>> + uint16_t view_id, xen_pfn_t gfn, bool sve); >>> int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, >>> uint16_t view_id, xen_pfn_t gfn, >>> xenmem_access_t access); >>> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c >>> index 0639632477..4710133918 100644 >>> --- a/tools/libxc/xc_altp2m.c >>> +++ b/tools/libxc/xc_altp2m.c >>> @@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, >>> return rc; >>> } >>> >>> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid, >>> + uint16_t view_id, xen_pfn_t gfn, bool sve) >>> +{ >>> + int rc; >>> + DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg); >>> + >>> + arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg)); >>> + if ( arg == NULL ) >>> + return -1; >>> + >>> + arg->version = HVMOP_ALTP2M_INTERFACE_VERSION; >>> + arg->cmd = HVMOP_altp2m_set_suppress_ve; >>> + arg->domain = domid; >>> + arg->u.set_suppress_ve.view = view_id; >>> + arg->u.set_suppress_ve.gfn = gfn; >>> + arg->u.set_suppress_ve.suppress_ve = sve; >>> + >>> + rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m, >>> + HYPERCALL_BUFFER_AS_ARG(arg)); >>> + >>> + xc_hypercall_buffer_free(handle, arg); >>> + return rc; >>> +} >>> + >>> int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, >>> uint16_t view_id, xen_pfn_t gfn, >>> xenmem_access_t access) >>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c >>> index 70ddc81d44..dd8e205551 100644 >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -4358,6 +4358,7 @@ static int do_altp2m_op( >>> case HVMOP_altp2m_destroy_p2m: >>> case HVMOP_altp2m_switch_p2m: >>> case HVMOP_altp2m_set_mem_access: >>> + case HVMOP_altp2m_set_suppress_ve: >>> case HVMOP_altp2m_change_gfn: >>> break; >>> default: >>> @@ -4475,6 +4476,19 @@ static int do_altp2m_op( >>> a.u.set_mem_access.view); >>> break; >>> >>> + case HVMOP_altp2m_set_suppress_ve: >>> + if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 ) >>> + rc = -EINVAL; >>> + else >>> + { >>> + gfn_t gfn = _gfn(a.u.set_mem_access.gfn); >>> + unsigned int altp2m_idx = a.u.set_mem_access.view; >>> + bool suppress_ve = a.u.set_suppress_ve.suppress_ve; >>> + >>> + rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx); >>> + } >>> + break; >>> + >>> case HVMOP_altp2m_change_gfn: >>> if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 ) >>> rc = -EINVAL; >>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c >>> index d0b0767855..8c39db13e3 100644 >>> --- a/xen/arch/x86/mm/mem_access.c >>> +++ b/xen/arch/x86/mm/mem_access.c >>> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access) >>> } >>> >>> /* >>> + * Set/clear the #VE suppress bit for a page. Only available on VMX. >>> + */ >>> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, >>> + unsigned int altp2m_idx) >>> +{ >>> + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); >>> + struct p2m_domain *ap2m = NULL; >>> + struct p2m_domain *p2m; >>> + mfn_t mfn; >>> + p2m_access_t a; >>> + p2m_type_t t; >>> + int rc; >>> + >>> + if ( !cpu_has_vmx_virt_exceptions ) >>> + return -EOPNOTSUPP; >>> + >>> + /* This subop should only be used from a privileged domain. */ >>> + if ( !current->domain->is_privileged ) >>> + return -EINVAL; >> >> This check looks wrong to me. If this subop should only be used by an >> external (privileged) domain then I don't think this should be >> implemented as an HVMOP, looks more like a domctl to me. > > Well after patch 1, isn't it the case that a guest has no way of > clearing the suppress_ve bit? > > I was going to say we want the XSM_TARGET "default action" (which > allows a guest to do things on itself, or a privileged domain to do it > to any domain); but I think really we probably we don't want a guest > to be able to *clear* the suppress_ve bit on a page for which a > privileged domain has *set*; this would allow a domain to prevent the > other domain from effectively introspecting on a page. That sounds right, that would be a scenario that would probably have to be avoided. That said, it becomes quite complex if we want to have two entities having access to altp2m, one external and one in-guest. I don't think that setup is something that was considered when altp2m was introduced. > > This is starting to sound like another conversation I think I remember > recently about making sure that *only* the guest *or* an introspection > engine can use the altp2m functionality, but I can't seem to find that > with a quick look. Tamas, does that ring any bells? You may be thinking of the discussions regarding the externel_only mode for altp2m I've added in https://lists.xen.org/archives/html/xen-devel/2017-04/msg00373.html. Tamas
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index 1629f412dd..f6ba8635bf 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid, /* Switch all vCPUs of the domain to the specified altp2m view */ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, uint16_t view_id); +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid, + uint16_t view_id, xen_pfn_t gfn, bool sve); int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, uint16_t view_id, xen_pfn_t gfn, xenmem_access_t access); diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c index 0639632477..4710133918 100644 --- a/tools/libxc/xc_altp2m.c +++ b/tools/libxc/xc_altp2m.c @@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, return rc; } +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid, + uint16_t view_id, xen_pfn_t gfn, bool sve) +{ + int rc; + DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg); + + arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg)); + if ( arg == NULL ) + return -1; + + arg->version = HVMOP_ALTP2M_INTERFACE_VERSION; + arg->cmd = HVMOP_altp2m_set_suppress_ve; + arg->domain = domid; + arg->u.set_suppress_ve.view = view_id; + arg->u.set_suppress_ve.gfn = gfn; + arg->u.set_suppress_ve.suppress_ve = sve; + + rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m, + HYPERCALL_BUFFER_AS_ARG(arg)); + + xc_hypercall_buffer_free(handle, arg); + return rc; +} + int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, uint16_t view_id, xen_pfn_t gfn, xenmem_access_t access) diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 70ddc81d44..dd8e205551 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4358,6 +4358,7 @@ static int do_altp2m_op( case HVMOP_altp2m_destroy_p2m: case HVMOP_altp2m_switch_p2m: case HVMOP_altp2m_set_mem_access: + case HVMOP_altp2m_set_suppress_ve: case HVMOP_altp2m_change_gfn: break; default: @@ -4475,6 +4476,19 @@ static int do_altp2m_op( a.u.set_mem_access.view); break; + case HVMOP_altp2m_set_suppress_ve: + if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 ) + rc = -EINVAL; + else + { + gfn_t gfn = _gfn(a.u.set_mem_access.gfn); + unsigned int altp2m_idx = a.u.set_mem_access.view; + bool suppress_ve = a.u.set_suppress_ve.suppress_ve; + + rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx); + } + break; + case HVMOP_altp2m_change_gfn: if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 ) rc = -EINVAL; diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c index d0b0767855..8c39db13e3 100644 --- a/xen/arch/x86/mm/mem_access.c +++ b/xen/arch/x86/mm/mem_access.c @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access) } /* + * Set/clear the #VE suppress bit for a page. Only available on VMX. + */ +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, + unsigned int altp2m_idx) +{ + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); + struct p2m_domain *ap2m = NULL; + struct p2m_domain *p2m; + mfn_t mfn; + p2m_access_t a; + p2m_type_t t; + int rc; + + if ( !cpu_has_vmx_virt_exceptions ) + return -EOPNOTSUPP; + + /* This subop should only be used from a privileged domain. */ + if ( !current->domain->is_privileged ) + return -EINVAL; + + /* #VE should be enabled for this vcpu. */ + if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) ) + return -EINVAL; + + if ( altp2m_idx > 0 ) + { + if ( altp2m_idx >= MAX_ALTP2M || + d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) + return -EINVAL; + + p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx]; + } + else + p2m = host_p2m; + + gfn_lock(host_p2m, gfn, 0); + if ( ap2m ) + p2m_lock(ap2m); + + mfn = p2m->get_entry(p2m, gfn_x(gfn), &t, &a, 0, NULL, NULL); + if ( !mfn_valid(mfn) ) + return -ESRCH; + rc = p2m->set_entry(p2m, gfn_x(gfn), mfn, PAGE_ORDER_4K, t, a, + suppress_ve); + if ( ap2m ) + p2m_unlock(ap2m); + gfn_unlock(host_p2m, gfn, 0); + + return rc; +} + +/* * Local variables: * mode: C * c-file-style: "BSD" diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h index 0bdafdf59a..f0b3d8e4d3 100644 --- a/xen/include/public/hvm/hvm_op.h +++ b/xen/include/public/hvm/hvm_op.h @@ -237,6 +237,18 @@ struct xen_hvm_altp2m_set_mem_access { typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t; DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t); +struct xen_hvm_altp2m_set_suppress_ve { + /* view */ + uint16_t view; + uint8_t suppress_ve; + uint8_t pad1; + uint32_t pad2; + /* gfn */ + uint64_t gfn; +}; +typedef struct xen_hvm_altp2m_set_suppress_ve xen_hvm_altp2m_set_suppress_ve_t; +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_suppress_ve_t); + struct xen_hvm_altp2m_change_gfn { /* view */ uint16_t view; @@ -268,6 +280,8 @@ struct xen_hvm_altp2m_op { #define HVMOP_altp2m_set_mem_access 7 /* Change a p2m entry to have a different gfn->mfn mapping */ #define HVMOP_altp2m_change_gfn 8 +/* Set the "Suppress #VE" bit on a page */ +#define HVMOP_altp2m_set_suppress_ve 9 domid_t domain; uint16_t pad1; uint32_t pad2; @@ -276,6 +290,7 @@ struct xen_hvm_altp2m_op { struct xen_hvm_altp2m_vcpu_enable_notify enable_notify; struct xen_hvm_altp2m_view view; struct xen_hvm_altp2m_set_mem_access set_mem_access; + struct xen_hvm_altp2m_set_suppress_ve set_suppress_ve; struct xen_hvm_altp2m_change_gfn change_gfn; uint8_t pad[64]; } u; diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h index 5ab34c1553..0c6717d80f 100644 --- a/xen/include/xen/mem_access.h +++ b/xen/include/xen/mem_access.h @@ -78,6 +78,9 @@ long p2m_set_mem_access_multi(struct domain *d, */ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access); +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, + unsigned int altp2m_idx); + #ifdef CONFIG_HAS_MEM_ACCESS int mem_access_memop(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg);
Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a privileged domain to change the value of the #VE suppress bit for a page. Add a libxc wrapper for invoking this hvmop. Signed-off-by: Adrian Pop <apop@bitdefender.com> --- tools/libxc/include/xenctrl.h | 2 ++ tools/libxc/xc_altp2m.c | 24 +++++++++++++++++++ xen/arch/x86/hvm/hvm.c | 14 +++++++++++ xen/arch/x86/mm/mem_access.c | 52 +++++++++++++++++++++++++++++++++++++++++ xen/include/public/hvm/hvm_op.h | 15 ++++++++++++ xen/include/xen/mem_access.h | 3 +++ 6 files changed, 110 insertions(+)