[Xen-devel] [PATCH 1/3] x86/mm: Change default value for suppress #VE in set_mem_access()
From: Vlad Ioan Topan The default value for the "suppress #VE" bit set by set_mem_access() currently depends on whether the call is made from the same domain (the bit is set when called from another domain and cleared if called from the same domain). This patch changes that behavior to inherit the old suppress #VE bit value if it is already set and to set it to 1 otherwise, which is safer and more reliable. Signed-off-by: Vlad Ioan Topan Signed-off-by: Adrian Pop --- xen/arch/x86/mm/mem_access.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c index 5adaf6df90..d0b0767855 100644 --- a/xen/arch/x86/mm/mem_access.c +++ b/xen/arch/x86/mm/mem_access.c @@ -273,8 +273,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, } } -return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, - (current->domain != d)); +return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1); } static int set_mem_access(struct domain *d, struct p2m_domain *p2m, -- 2.12.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit
Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
domain to change the value of the #VE suppress bit for a page.
Signed-off-by: Adrian Pop
---
xen/arch/x86/hvm/hvm.c | 14
xen/arch/x86/mm/mem_access.c| 48 +
xen/include/public/hvm/hvm_op.h | 15 +
xen/include/xen/mem_access.h| 3 +++
4 files changed, 80 insertions(+)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 2e76c2345b..eb01527c5b 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4356,6 +4356,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:
@@ -4472,6 +4473,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;
+uint8_t 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..b9e611d3db 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -466,6 +466,54 @@ 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, uint8_t suppress_ve,
+unsigned int altp2m_idx)
+{
+struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
+struct p2m_domain *ap2m = NULL;
+struct p2m_domain *p2m = NULL;
+mfn_t mfn;
+p2m_access_t a;
+p2m_type_t t;
+unsigned long gfn_l;
+int rc = 0;
+
+if ( !cpu_has_vmx )
+return -EOPNOTSUPP;
+
+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;
+}
+
+p2m_lock(host_p2m);
+if ( ap2m )
+p2m_lock(ap2m);
+
+gfn_l = gfn_x(gfn);
+mfn = p2m->get_entry(p2m, gfn_l, &t, &a, 0, NULL, NULL);
+if ( !mfn_valid(mfn) )
+return -ESRCH;
+rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
+suppress_ve);
+if ( ap2m )
+p2m_unlock(ap2m);
+p2m_unlock(host_p2m);
+
+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 bc00ef0e65..9736092f58 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -231,6 +231,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;
@@ -262,6 +274,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;
@@ -270,6 +284,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_veset_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..b6e6a7650a 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);
+in
[Xen-devel] [PATCH 0/3] x86: Add a hvmop for setting the #VE suppress bit
As the code stands right now, after DomU has enabled #VE using HVMOP_altp2m_vcpu_enable_notify, all its pages have the #VE suppress bit cleared, generating #VEs for any EPT violation. There is currently no way to change the value of the #VE suppress bit for a page from a domain; it can only be done in Xen internally using ept_set_entry(). Following the discussion from https://lists.xen.org/archives/html/xen-devel/2017-03/msg01312.html this patch introduces a new hvmop to set this bit and thus have control over which pages generate #VE and which VM-Exit. I'm not sure whether it's best to define p2m_set_suppress_ve() in mem_access.c since this file contains common functions for x86 (vmx & svm) and the function is Intel-specific. Adrian Pop (2): x86/altp2m: Add a hvmop for setting the suppress #VE bit libxc: Add support for the altp2m suppress #VE hvmop Vlad Ioan Topan (1): x86/mm: Change default value for suppress #VE in set_mem_access() 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| 51 +++-- xen/include/public/hvm/hvm_op.h | 15 xen/include/xen/mem_access.h| 3 +++ 6 files changed, 107 insertions(+), 2 deletions(-) -- 2.12.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 3/3] libxc: Add support for the altp2m suppress #VE hvmop
This adds a wrapper for issuing HVMOP_altp2m_set_suppress_ve from a
domain.
Signed-off-by: Adrian Pop
---
tools/libxc/include/xenctrl.h | 2 ++
tools/libxc/xc_altp2m.c | 24
2 files changed, 26 insertions(+)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 2d97d36c38..5e1e4cfa81 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1923,6 +1923,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, uint8_t 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..b0f3e344af 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, uint8_t 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)
--
2.12.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit
On Thu, May 18, 2017 at 11:27:44AM -0600, Tamas K Lengyel wrote:
> On Thu, May 18, 2017 at 9:07 AM, Adrian Pop wrote:
> > Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
> > domain to change the value of the #VE suppress bit for a page.
> >
> > Signed-off-by: Adrian Pop
> > ---
> > xen/arch/x86/hvm/hvm.c | 14
> > xen/arch/x86/mm/mem_access.c| 48
> > +
> > xen/include/public/hvm/hvm_op.h | 15 +
> > xen/include/xen/mem_access.h| 3 +++
> > 4 files changed, 80 insertions(+)
> >
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index 2e76c2345b..eb01527c5b 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -4356,6 +4356,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:
> > @@ -4472,6 +4473,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;
> > +uint8_t 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..b9e611d3db 100644
> > --- a/xen/arch/x86/mm/mem_access.c
> > +++ b/xen/arch/x86/mm/mem_access.c
> > @@ -466,6 +466,54 @@ 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, uint8_t suppress_ve,
> > +unsigned int altp2m_idx)
> > +{
> > +struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> > +struct p2m_domain *ap2m = NULL;
> > +struct p2m_domain *p2m = NULL;
> > +mfn_t mfn;
> > +p2m_access_t a;
> > +p2m_type_t t;
> > +unsigned long gfn_l;
> > +int rc = 0;
> > +
> > +if ( !cpu_has_vmx )
> > +return -EOPNOTSUPP;
> > +
> > +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;
> > +}
>
>
> IMHO there should be some further checks here to verify that the
> domain has issued HVMOP_altp2m_vcpu_enable_notify before and that it
> was allowed (ie. this hypercall should not be able to enable the
> suppress_bit if there is no veinfo_gfn). That said, is there anything
Ok, a check can be added easily. The reasoning behind not adding it in
the first place was that should the #VE be disabled in the VM's VMCS,
setting/clearing the suppress #VE bit would do nothing (it is ignored).
> that would prevent a malicious application issuing rouge altp2m HVMOPs
> from messing with this if it is activated (which I guess stands for
> the rest of the altp2m ops too)?
AFAIK there isn't any safeguard of this sort. I might just be
excessively ignorant, though. On the other hand the current default
behavior is to enable #VE for all the pages. The default with these
patches would be to issue a VM-Exit, and either a SVA, the Dom0 or the
target DomU itself could modify this behavior to generate #VE instead of
VM-Exit. In any case I'll investigate some more.
Thanks!
> > +
> > +p2m_lock(host_p2m);
> > +if ( ap2m )
> > +p2m_lock(ap2m);
> > +
> > +gfn_l = gfn_x(gfn);
> > +mfn = p2m->get_entry(p2m, gfn_l, &t, &a, 0, NULL, NULL);
> > +if ( !mfn_valid(mfn) )
> > +retur
Re: [Xen-devel] [PATCH 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit
Hello,
On Mon, May 29, 2017 at 08:38:33AM -0600, Jan Beulich wrote:
> >>> On 18.05.17 at 17:07, wrote:
> > --- a/xen/arch/x86/mm/mem_access.c
> > +++ b/xen/arch/x86/mm/mem_access.c
> > @@ -466,6 +466,54 @@ 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, uint8_t suppress_ve,
>
> suppress_ve presumably is meant to be boolean.
Yes. It can be changed to bool.
> > +unsigned int altp2m_idx)
> > +{
> > +struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> > +struct p2m_domain *ap2m = NULL;
> > +struct p2m_domain *p2m = NULL;
>
> Pointless initializer.
Ok.
> > +mfn_t mfn;
> > +p2m_access_t a;
> > +p2m_type_t t;
> > +unsigned long gfn_l;
>
> Please avoid this local variable and use gfn_x() in the two places
> you need to.
Sure.
> > +int rc = 0;
>
> Pointless initializer again.
Right.
> > +
> > +if ( !cpu_has_vmx )
> > +return -EOPNOTSUPP;
>
> Is this enough? Wouldn't it be better to signal the caller whenever
> hardware (or even software) isn't going to honor the request?
Well, the caller checks the return value. The libxc function
xc_altp2m_set_suppress_ve for instance will return a negative in this
case.
> > +if ( altp2m_idx > 0 )
> > +{
> > +if ( altp2m_idx >= MAX_ALTP2M ||
> > +d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
>
> Indentation.
Ok.
> > +return -EINVAL;
> > +
> > +p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx];
> > +}
> > +else
> > +{
> > +p2m = host_p2m;
> > +}
>
> Unnecessary braces.
Ok.
> > +p2m_lock(host_p2m);
> > +if ( ap2m )
> > +p2m_lock(ap2m);
> > +
> > +gfn_l = gfn_x(gfn);
> > +mfn = p2m->get_entry(p2m, gfn_l, &t, &a, 0, NULL, NULL);
> > +if ( !mfn_valid(mfn) )
> > +return -ESRCH;
> > +rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
> > +suppress_ve);
> > +if ( ap2m )
> > +p2m_unlock(ap2m);
> > +p2m_unlock(host_p2m);
>
> To fiddle with a single gfn, this looks to be very heavy locking.
> While for now gfn_lock() is the same as p2m_lock(), from an
> abstract perspective I'd expect gfn_lock() to suffice here at
> least in the non-altp2m case.
Ok.
> And then there are two general questions: Without a libxc layer
> function, how is one supposed to use this new sub-op? Is it
> really intended to permit a guest to call this for itself?
Well, the sub-op could be used from a Linux kernel module if libxc is
not available if struct xen_hvm_altp2m_op and struct
xen_hvm_altp2m_set_suppress_ve are defined.
Our use case, though, involves either Dom0 or a "privileged" DomU
altering the suppress #VE bit for the target guest.
> Jan
>
Thanks!
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit
On Tue, Jun 06, 2017 at 07:08:43AM -0600, Jan Beulich wrote: > >>> On 06.06.17 at 15:00, wrote: > > On Mon, May 29, 2017 at 08:38:33AM -0600, Jan Beulich wrote: > >> >>> On 18.05.17 at 17:07, wrote: > >> > + > >> > +if ( !cpu_has_vmx ) > >> > +return -EOPNOTSUPP; > >> > >> Is this enough? Wouldn't it be better to signal the caller whenever > >> hardware (or even software) isn't going to honor the request? > > > > Well, the caller checks the return value. The libxc function > > xc_altp2m_set_suppress_ve for instance will return a negative in this > > case. > > The question wasn't what the caller does but whether checking just > cpu_has_vmx is enough. What if you're running on a machine with > VMX but no #VE support? Oh, all right. I misinterpreted it. Yes, at least using something like cpu_has_vmx_virt_exceptions instead of cpu_has_vmx would definitely be more appropriate in this case. Do you think there should be a more thorough check? > >> And then there are two general questions: Without a libxc layer > >> function, how is one supposed to use this new sub-op? Is it > >> really intended to permit a guest to call this for itself? > > > > Well, the sub-op could be used from a Linux kernel module if libxc is > > not available if struct xen_hvm_altp2m_op and struct > > xen_hvm_altp2m_set_suppress_ve are defined. > > > > Our use case, though, involves either Dom0 or a "privileged" DomU > > altering the suppress #VE bit for the target guest. > > This doesn't really answer the question: What are the security > implications if a guest can invoke this on itself? Indeed it would be desirable that the guest doesn't use this hvmop on itself. It's also less than ideal that a DomU can call this on other DomUs. After some talks it turns out that restricting this hvmop to a privileged domain solves this issue and still works for our use case. What do you think? > (FTR I think my first question was kind of pointless, as patch 3 > looks like it does introduce a libxc function; I simply didn't realize > that back then, because I'd generally have expected the > consumer of the hypercall to be introduce together with the > producer.) I can merge these two patches for v2 if that's what you want. > Jan Thank you! ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit
On Thu, Jun 08, 2017 at 08:08:56AM -0600, Jan Beulich wrote: > >>> On 08.06.17 at 15:49, wrote: > > On Tue, Jun 06, 2017 at 07:08:43AM -0600, Jan Beulich wrote: > >> >>> On 06.06.17 at 15:00, wrote: > >> > On Mon, May 29, 2017 at 08:38:33AM -0600, Jan Beulich wrote: > >> >> >>> On 18.05.17 at 17:07, wrote: > >> >> > + > >> >> > +if ( !cpu_has_vmx ) > >> >> > +return -EOPNOTSUPP; > >> >> > >> >> Is this enough? Wouldn't it be better to signal the caller whenever > >> >> hardware (or even software) isn't going to honor the request? > >> > > >> > Well, the caller checks the return value. The libxc function > >> > xc_altp2m_set_suppress_ve for instance will return a negative in this > >> > case. > >> > >> The question wasn't what the caller does but whether checking just > >> cpu_has_vmx is enough. What if you're running on a machine with > >> VMX but no #VE support? > > > > Oh, all right. I misinterpreted it. Yes, at least using something like > > cpu_has_vmx_virt_exceptions instead of cpu_has_vmx would definitely be > > more appropriate in this case. Do you think there should be a more > > thorough check? > > Depends on what "more thorough" means: You'll want to check all > features the code requires; I'm not certain if virt_exceptions is all > it needs. The checks so far would be: - is the domain invoking this hvmop privileged? - does the cpu have the #VE feature? - is #VE enabled on this vcpu? > >> >> And then there are two general questions: Without a libxc layer > >> >> function, how is one supposed to use this new sub-op? Is it > >> >> really intended to permit a guest to call this for itself? > >> > > >> > Well, the sub-op could be used from a Linux kernel module if libxc is > >> > not available if struct xen_hvm_altp2m_op and struct > >> > xen_hvm_altp2m_set_suppress_ve are defined. > >> > > >> > Our use case, though, involves either Dom0 or a "privileged" DomU > >> > altering the suppress #VE bit for the target guest. > >> > >> This doesn't really answer the question: What are the security > >> implications if a guest can invoke this on itself? > > > > Indeed it would be desirable that the guest doesn't use this hvmop on > > itself. It's also less than ideal that a DomU can call this on other > > DomUs. > > The latter is an absolute no-go. Indeed. > > After some talks it turns out that restricting this hvmop to a > > privileged domain solves this issue and still works for our use case. > > What do you think? > > Restrictions should generally be put in place because of > abstract considerations, not because of them not harming > one's particular use case. Of course. > >> (FTR I think my first question was kind of pointless, as patch 3 > >> looks like it does introduce a libxc function; I simply didn't realize > >> that back then, because I'd generally have expected the > >> consumer of the hypercall to be introduce together with the > >> producer.) > > > > I can merge these two patches for v2 if that's what you want. > > I'd prefer that, but others may have differing opinions. And > there are certainly benefits in keeping hypervisor and tools > changes separate. Ok then. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 1/2] x86/mm: Change default value for suppress #VE in set_mem_access()
From: Vlad Ioan Topan The default value for the "suppress #VE" bit set by set_mem_access() currently depends on whether the call is made from the same domain (the bit is set when called from another domain and cleared if called from the same domain). This patch changes that behavior to inherit the old suppress #VE bit value if it is already set and to set it to 1 otherwise, which is safer and more reliable. Signed-off-by: Vlad Ioan Topan Signed-off-by: Adrian Pop --- xen/arch/x86/mm/mem_access.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c index 5adaf6df90..d0b0767855 100644 --- a/xen/arch/x86/mm/mem_access.c +++ b/xen/arch/x86/mm/mem_access.c @@ -273,8 +273,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, } } -return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, - (current->domain != d)); +return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1); } static int set_mem_access(struct domain *d, struct p2m_domain *p2m, -- 2.13.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 2/2] x86/altp2m: Add a hvmop for setting the suppress #VE bit
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
---
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;
+
+/* #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];
+
[Xen-devel] [PATCH 0/2] x86: Add a hvmop for setting the #VE suppress bit
As the code stands right now, after DomU has enabled #VE using HVMOP_altp2m_vcpu_enable_notify, all its pages have the #VE suppress bit cleared, generating #VEs for any EPT violation. There is currently no way to change the value of the #VE suppress bit for a page from a domain; it can only be done in Xen internally using ept_set_entry(). Following the discussion from https://lists.xen.org/archives/html/xen-devel/2017-03/msg01312.html this patch introduces a new hvmop to set this bit and thus have control over which pages generate #VE and which VM-Exit. I'm not sure whether it's best to define p2m_set_suppress_ve() in mem_access.c since this file contains common functions for x86 (vmx & svm) and the function is Intel-specific. changes in v2: - check if #VE has been enabled on the target domain (Tamas K Lengyel) - check if the cpu has the #VE feature - make the suppress_ve argument boolean (Jan Beulich) - initialize only local variables that need initializing (Jan Beulich) - use fewer local variables (Jan Beulich) - fix indentation (Jan Beulich) - remove unnecessary braces (Jan Beulich) - use gfn_lock() instead of p2m_lock() in the non-altp2m case (Jan Beulich) - allow only privileged domains to use this hvmop - merge patch #2 and patch #3 (Jan Beulich) Adrian Pop (1): x86/altp2m: Add a hvmop for setting the suppress #VE bit Vlad Ioan Topan (1): x86/mm: Change default value for suppress #VE in set_mem_access() 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| 55 +++-- xen/include/public/hvm/hvm_op.h | 15 +++ xen/include/xen/mem_access.h| 3 +++ 6 files changed, 111 insertions(+), 2 deletions(-) -- 2.13.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 0/2] x86: Add a hvmop for setting the #VE suppress bit
I've just noticed I had forgotten to update the version of the patch in the email subject. Sorry! ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] x86/altp2m: Add a hvmop for setting the suppress #VE bit
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
> > ---
> > 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
Thank you!
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH] x86/hvm: Fix rcu_unlock_domain call bypass
rcu_lock_current_domain is called at the beginning of do_altp2m_op, but
the altp2m_vcpu_enable_notify subop handler might skip calling
rcu_unlock_domain, possibly hanging the domain altogether.
Signed-off-by: Adrian Pop
---
xen/arch/x86/hvm/hvm.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 205b4cb685..0af498a312 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4534,12 +4534,18 @@ static int do_altp2m_op(
if ( a.u.enable_notify.pad || a.domain != DOMID_SELF ||
a.u.enable_notify.vcpu_id != curr->vcpu_id )
+{
rc = -EINVAL;
+break;
+}
if ( !gfn_eq(vcpu_altp2m(curr).veinfo_gfn, INVALID_GFN) ||
mfn_eq(get_gfn_query_unlocked(curr->domain,
a.u.enable_notify.gfn, &p2mt), INVALID_MFN) )
-return -EINVAL;
+{
+rc = -EINVAL;
+break;
+}
vcpu_altp2m(curr).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
altp2m_vcpu_update_vmfunc_ve(curr);
--
2.15.0
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86/hvm: Fix rcu_unlock_domain call bypass
Hello,
On Tue, Nov 14, 2017 at 08:25:57AM -0700, Jan Beulich wrote:
> >>> On 14.11.17 at 16:11, wrote:
> > rcu_lock_current_domain is called at the beginning of do_altp2m_op, but
> > the altp2m_vcpu_enable_notify subop handler might skip calling
> > rcu_unlock_domain, possibly hanging the domain altogether.
>
> I fully agree with the change, but the description needs improvement.
> For one, why would the domain be hanging with
>
> static inline struct domain *rcu_lock_current_domain(void)
> {
> return /*rcu_lock_domain*/(current->domain);
> }
>
> ? And even if the lock function invocation wasn't commented
> out, all it does is preempt_disable(). That may cause an
> assertion to trigger in debug builds, but that's not a domain
> hang. Plus ...
Sorry, I was indeed referring to the preempt_count() assertion, only
using poor wording. I had tested something else using
rcu_lock_domain_by_id() instead of rcu_lock_current_domain() which
triggered the assertion.
>
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -4534,12 +4534,18 @@ static int do_altp2m_op(
> >
> > if ( a.u.enable_notify.pad || a.domain != DOMID_SELF ||
> > a.u.enable_notify.vcpu_id != curr->vcpu_id )
> > +{
> > rc = -EINVAL;
> > +break;
> > +}
>
> ... you also change flow here, which is a second bug you address,
> but you fail to mention it.
OK.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2] x86/hvm: Fix altp2m_vcpu_enable_notify error handling
The altp2m_vcpu_enable_notify subop handler might skip calling
rcu_unlock_domain() after rcu_lock_current_domain(). Albeit since both
rcu functions are no-ops when run on the current domain, this doesn't
really have repercussions.
The second change is adding a missing break that would have potentially
enabled #VE for the current domain even if it had intended to enable it
for another one (not a supported functionality).
Signed-off-by: Adrian Pop
Reviewed-by: Andrew Cooper
---
changes in v2:
- reword the commit message
---
xen/arch/x86/hvm/hvm.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 205b4cb685..0af498a312 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4534,12 +4534,18 @@ static int do_altp2m_op(
if ( a.u.enable_notify.pad || a.domain != DOMID_SELF ||
a.u.enable_notify.vcpu_id != curr->vcpu_id )
+{
rc = -EINVAL;
+break;
+}
if ( !gfn_eq(vcpu_altp2m(curr).veinfo_gfn, INVALID_GFN) ||
mfn_eq(get_gfn_query_unlocked(curr->domain,
a.u.enable_notify.gfn, &p2mt), INVALID_MFN) )
-return -EINVAL;
+{
+rc = -EINVAL;
+break;
+}
vcpu_altp2m(curr).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
altp2m_vcpu_update_vmfunc_ve(curr);
--
2.15.0
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3 1/2] x86/mm: Change default value for suppress #VE in set_mem_access()
From: Vlad Ioan Topan The default value for the "suppress #VE" bit set by set_mem_access() currently depends on whether the call is made from the same domain (the bit is set when called from another domain and cleared if called from the same domain). This patch changes that behavior to inherit the old suppress #VE bit value if it is already set and to set it to 1 otherwise, which is safer and more reliable. Signed-off-by: Vlad Ioan Topan Signed-off-by: Adrian Pop --- xen/arch/x86/mm/mem_access.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c index 5adaf6df90..d0b0767855 100644 --- a/xen/arch/x86/mm/mem_access.c +++ b/xen/arch/x86/mm/mem_access.c @@ -273,8 +273,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, } } -return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, - (current->domain != d)); +return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1); } static int set_mem_access(struct domain *d, struct p2m_domain *p2m, -- 2.13.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3 0/2] Add a hvmop for setting the #VE suppress bit
As the code stands right now, after DomU has enabled #VE using HVMOP_altp2m_vcpu_enable_notify, all its pages have the #VE suppress bit cleared, generating #VEs for any EPT violation. There is currently no way to change the value of the #VE suppress bit for a page from a domain; it can only be done in Xen internally using ept_set_entry(). Following the discussion from https://lists.xen.org/archives/html/xen-devel/2017-03/msg01312.html this patch introduces a new hvmop to set this bit and thus have control over which pages generate #VE and which VM-Exit. Adrian Pop (1): x86/altp2m: Add a hvmop for setting the suppress #VE bit Vlad Ioan Topan (1): x86/mm: Change default value for suppress #VE in set_mem_access() 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| 56 +++-- xen/include/public/hvm/hvm_op.h | 11 xen/include/xen/mem_access.h| 3 +++ 6 files changed, 108 insertions(+), 2 deletions(-) -- 2.13.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3 2/2] x86/altp2m: Add a hvmop for setting the suppress #VE bit
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
Acked-by: Wei Liu
---
changes in v3:
- fix indentation (Wei Liu)
- use return values other than EINVAL where appropriate (Ian Beulich)
- remove the irrelevant comments from the
xen_hvm_altp2m_set_suppress_ve struct (Ian Beulich)
- add comment for the suppress_ve field in the struct above (Ian
Beulich)
- remove the typedef and DEFINE_XEN_GUEST_HANDLE for
xen_hvm_altp2m_set_suppress_ve (Ian Beulich)
- use XSM_DM_PRIV check instead of domain->is_privileged (Ian Beulich)
changes in v2:
- check if #VE has been enabled on the target domain (Tamas K Lengyel)
- check if the cpu has the #VE feature
- make the suppress_ve argument boolean (Jan Beulich)
- initialize only local variables that need initializing (Jan Beulich)
- use fewer local variables (Jan Beulich)
- fix indentation (Jan Beulich)
- remove unnecessary braces (Jan Beulich)
- use gfn_lock() instead of p2m_lock() in the non-altp2m case (Jan
Beulich)
- allow only privileged domains to use this hvmop
- merge patch #2 and patch #3 (Jan Beulich)
---
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| 53 +
xen/include/public/hvm/hvm_op.h | 11 +
xen/include/xen/mem_access.h| 3 +++
6 files changed, 107 insertions(+)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 552a4fd47d..ea985696e4 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1938,6 +1938,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..26e3a56fb1 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 8145385747..b7ea945f78 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4413,6 +4413,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:
@@ -4530,6 +4531,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..87a92a632b 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#include "mm
Re: [Xen-devel] [PATCH v3 1/2] x86/mm: Change default value for suppress #VE in set_mem_access()
Hello, On Tue, Jul 18, 2017 at 11:26:45AM -0600, Tamas K Lengyel wrote: > On Tue, Jul 18, 2017 at 9:25 AM, Adrian Pop wrote: > > From: Vlad Ioan Topan > > > > The default value for the "suppress #VE" bit set by set_mem_access() > > currently depends on whether the call is made from the same domain (the > > bit is set when called from another domain and cleared if called from > > the same domain). This patch changes that behavior to inherit the old > > suppress #VE bit value if it is already set and to set it to 1 > > otherwise, which is safer and more reliable. > > With the way things are currently if the in-guest tool calls > set_mem_access for an altp2m view, it implies it wants to receive #VE > for it. Wouldn't this change in this patch effectively make it > impossible for an in-guest tool to decide which pages it wants to > receive #VE for? The new HVMOP you are introducing is only accessible > from a privileged domain.. Yes, this change, along with the restrictions from the new HVMOP would virtually prevent a guest from changing the suppress #VE bit for its pages. The current set_mem_access functionality, if I'm not mistaken, is a bit odd since the guest can only clear the sve, but to set it, another domain would have to call set_mem_access for it. I think the issue would be whether to allow a domain to set/clear the suppress #VE bit for its pages by calling the new HVMOP on itself. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 2/2] x86/altp2m: Add a hvmop for setting the suppress #VE bit
On Tue, Jul 18, 2017 at 11:19:07AM -0600, Tamas K Lengyel wrote: > On Tue, Jul 18, 2017 at 9:25 AM, 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 > > Acked-by: Wei Liu > > Acked-by: Tamas K Lengyel Thanks! ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] x86/altp2m: Add a hvmop for setting the suppress #VE bit
On Fri, Jun 16, 2017 at 09:29:49AM -0600, Jan Beulich wrote:
> >>> On 09.06.17 at 18:51, 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
> > ---
>
> 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_veset_suppress_ve;
>
> ... a structure isn't meant to be used on its own anyway.
Yes, I agree with that.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] x86/altp2m: Add a hvmop for setting the suppress #VE bit
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
> > ---
> > 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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] x86/altp2m: Add a hvmop for setting the suppress #VE bit
On Fri, Jun 16, 2017 at 02:39:10AM -0600, Jan Beulich wrote:
> >>> On 15.06.17 at 21:01, wrote:
> > On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop 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.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] x86/altp2m: Add a hvmop for setting the suppress #VE bit
On Thu, Jun 22, 2017 at 06:13:22AM -0600, Jan Beulich wrote:
> >>> On 22.06.17 at 14:04, wrote:
> > On Fri, Jun 16, 2017 at 02:39:10AM -0600, Jan Beulich wrote:
> >> >>> On 15.06.17 at 21:01, wrote:
> >> > On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop 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.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] x86/altp2m: Add a hvmop for setting the suppress #VE bit
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 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.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] x86/altp2m: Add a hvmop for setting the suppress #VE bit
On Thu, Jun 22, 2017 at 06:13:22AM -0600, Jan Beulich wrote:
> >>> On 22.06.17 at 14:04, wrote:
> > On Fri, Jun 16, 2017 at 02:39:10AM -0600, Jan Beulich wrote:
> >> >>> On 15.06.17 at 21:01, wrote:
> >> > On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop 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.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2] x86/monitor: add support for descriptor access events
Adds monitor support for descriptor access events (reads & writes of
IDTR/GDTR/LDTR/TR) for the x86 architecture (VMX and SVM).
Signed-off-by: Adrian Pop
---
changes in v2:
- use two sub-structs (vmx/svm) for vm_event_desc_access (Tamas K
Lengyel)
- more compact version of the descriptor VM-Exit handling on svm (Boris
Ostrovsky)
- use bool instead of bool_t (Jan Beulich)
- use curr, currd for the struct vcpu and struct domain local variables
(Jan Beulich)
- move hvm_emulate_ctxt into a narrower scope (Jan Beulich)
- remove leftover dead code (Jan Beulich)
- order the monitor events numerically (Jan Beulich)
- remove VM_EVENT_DESC_INVALID (Jan Beulich)
- crash the domain if the descriptor access instruction can't be
emulated (Jan Beulich)
- call hvm_inject_event without checking for pending events (Andrew
Cooper)
- introduce structures for the VM-Exit instruction information used for
the descriptor instructions and use fewer magic numbers (Andrew
Cooper, Jan Beulich)
---
tools/libxc/include/xenctrl.h | 2 ++
tools/libxc/xc_monitor.c| 14 ++
tools/tests/xen-access/xen-access.c | 29 -
xen/arch/x86/hvm/hvm.c | 37 ++
xen/arch/x86/hvm/monitor.c | 22
xen/arch/x86/hvm/svm/svm.c | 42 ++
xen/arch/x86/hvm/vmx/vmx.c | 52 +
xen/arch/x86/monitor.c | 18 +
xen/arch/x86/vm_event.c | 6 +
xen/include/asm-x86/domain.h| 1 +
xen/include/asm-x86/hvm/hvm.h | 1 +
xen/include/asm-x86/hvm/monitor.h | 3 +++
xen/include/asm-x86/hvm/support.h | 3 +++
xen/include/asm-x86/hvm/vmx/vmx.h | 42 ++
xen/include/asm-x86/monitor.h | 3 ++-
xen/include/public/domctl.h | 1 +
xen/include/public/vm_event.h | 25 ++
17 files changed, 299 insertions(+), 2 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 2d97d36c38..2248041c5a 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2007,6 +2007,8 @@ int xc_monitor_mov_to_msr(xc_interface *xch, domid_t
domain_id, uint32_t msr,
int xc_monitor_singlestep(xc_interface *xch, domid_t domain_id, bool enable);
int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id,
bool enable);
+int xc_monitor_descriptor_access(xc_interface *xch, domid_t domain_id,
+ bool enable);
int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
bool enable, bool sync);
int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 15a7c32d52..f99b6e3a33 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -129,6 +129,20 @@ int xc_monitor_singlestep(xc_interface *xch, domid_t
domain_id,
return do_domctl(xch, &domctl);
}
+int xc_monitor_descriptor_access(xc_interface *xch, domid_t domain_id,
+ bool enable)
+{
+DECLARE_DOMCTL;
+
+domctl.cmd = XEN_DOMCTL_monitor_op;
+domctl.domain = domain_id;
+domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
+: XEN_DOMCTL_MONITOR_OP_DISABLE;
+domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS;
+
+return do_domctl(xch, &domctl);
+}
+
int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, bool enable,
bool sync)
{
diff --git a/tools/tests/xen-access/xen-access.c
b/tools/tests/xen-access/xen-access.c
index 9d4f95756b..ff4d289b45 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -337,7 +337,7 @@ void usage(char* progname)
{
fprintf(stderr, "Usage: %s [-m] write|exec", progname);
#if defined(__i386__) || defined(__x86_64__)
-fprintf(stderr,
"|breakpoint|altp2m_write|altp2m_exec|debug|cpuid");
+fprintf(stderr,
"|breakpoint|altp2m_write|altp2m_exec|debug|cpuid|desc_access");
#elif defined(__arm__) || defined(__aarch64__)
fprintf(stderr, "|privcall");
#endif
@@ -368,6 +368,7 @@ int main(int argc, char *argv[])
int altp2m = 0;
int debug = 0;
int cpuid = 0;
+int desc_access = 0;
uint16_t altp2m_view_id = 0;
char* progname = argv[0];
@@ -434,6 +435,10 @@ int main(int argc, char *argv[])
{
cpuid = 1;
}
+else if ( !strcmp(argv[0], "desc_access") )
+{
+desc_access = 1;
+}
#elif defined(__arm__) || defined(__aarch64__)
else if ( !strcmp(argv[0], "privcall") )
{
@@ -571,6 +576,16 @@ int main(int argc, char *argv[])
}
}
+if ( desc
Re: [Xen-devel] [PATCH v2] x86/monitor: add support for descriptor access events
On Tue, Apr 04, 2017 at 01:52:24PM +0300, Razvan Cojocaru wrote: > You've forgotten Wei Liu's ack for the first version of the patch. > > For the vm_event bits: > > Acked-by: Razvan Cojocaru Okay, will include them next time. Thanks! ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] x86/monitor: add support for descriptor access events
On Tue, Apr 04, 2017 at 09:23:28AM -0400, Boris Ostrovsky wrote:
> On 04/04/2017 05:57 AM, Adrian Pop wrote:
> > Adds monitor support for descriptor access events (reads & writes of
> > IDTR/GDTR/LDTR/TR) for the x86 architecture (VMX and SVM).
> >
> > Signed-off-by: Adrian Pop
>
>
> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -856,6 +856,23 @@ static void svm_set_rdtsc_exiting(struct vcpu *v,
> > bool_t enable)
> > vmcb_set_general2_intercepts(vmcb, general2_intercepts);
> > }
> >
> > +static void svm_set_descriptor_access_exiting(struct vcpu *v, bool enable)
> > +{
> > +struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> > +u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
> > +u32 mask = GENERAL1_INTERCEPT_IDTR_READ | GENERAL1_INTERCEPT_GDTR_READ
> > \
> > +| GENERAL1_INTERCEPT_LDTR_READ | GENERAL1_INTERCEPT_TR_READ \
> > +| GENERAL1_INTERCEPT_IDTR_WRITE |
> > GENERAL1_INTERCEPT_GDTR_WRITE \
> > +| GENERAL1_INTERCEPT_LDTR_WRITE | GENERAL1_INTERCEPT_TR_WRITE;
>
> I didn't notice this last time --- there is no need for line
> continuation here.
>
> With that fixed,
>
> Reviewed-by: Boris Ostrovsky
Oh, yes. I'll remove the escapes for the next version. Thank you!
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] x86/monitor: add support for descriptor access events
On Tue, Apr 04, 2017 at 04:26:24PM +0100, Andrew Cooper wrote:
> On 04/04/17 10:57, Adrian Pop wrote:
> > diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
> > index f5cd245771..d60e4afd0c 100644
> > --- a/xen/arch/x86/hvm/monitor.c
> > +++ b/xen/arch/x86/hvm/monitor.c
> > @@ -72,6 +72,28 @@ void hvm_monitor_msr(unsigned int msr, uint64_t value)
> > }
> > }
> >
> > +void hvm_monitor_descriptor_access(uint64_t exit_info,
> > + uint64_t vmx_exit_qualification,
> > + uint8_t descriptor, bool is_write)
> > +{
> > +struct vcpu *curr = current;
> > +vm_event_request_t req = {
> > +.reason = VM_EVENT_REASON_DESCRIPTOR_ACCESS,
> > +.u.desc_access.descriptor = descriptor,
> > +.u.desc_access.is_write = is_write,
> > +};
>
> Newline here
Ok.
> > +if ( cpu_has_vmx )
> > +{
> > +req.u.desc_access.arch.vmx.instr_info = exit_info;
> > +req.u.desc_access.arch.vmx.exit_qualification =
> > vmx_exit_qualification;
> > +}
> > +else
> > +{
> > +req.u.desc_access.arch.svm.exitinfo = exit_info;
> > +}
>
> And here please.
Ok.
> > +monitor_traps(curr, 1, &req);
> > +}
> > +
> > static inline unsigned long gfn_of_rip(unsigned long rip)
> > {
> > struct vcpu *curr = current;
> > diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h
> > b/xen/include/asm-x86/hvm/vmx/vmx.h
> > index 2b781ab120..b00ba52443 100644
> > --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> > +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> > @@ -628,4 +628,46 @@ typedef struct {
> > u16 eptp_index;
> > } ve_info_t;
> >
> > +/* VM-Exit instruction info for LIDT, LGDT, SIDT, SGDT */
> > +typedef union idt_or_gdt_instr_info {
> > +unsigned long raw;
> > +struct {
> > +unsigned long scaling :2, /* bits 0:1 - Scaling */
> > +:5, /* bits 6:2 - Undefined */
> > +addr_size :3, /* bits 9:7 - Address size */
> > +:1, /* bit 10 - Cleared to 0 */
> > +operand_size:1, /* bit 11 - Operand size */
> > +:3, /* bits 14:12 - Undefined */
> > +segment_reg :3, /* bits 17:15 - Segment register */
> > +index_reg :4, /* bits 21:18 - Index register */
> > +index_reg_invalid :1, /* bit 22 - Index register invalid */
> > +base_reg:4, /* bits 26:23 - Base register */
> > +base_reg_invalid:1, /* bit 27 - Base register invalid */
> > +instr_identity :1, /* bit 28 - 0:GDT, 1:IDT */
> > +instr_write :1, /* bit 29 - 0:store, 1:load */
> > +:2; /* bits 30:31 - Undefined */
>
> I think you need a :32 /* undefined */ in each of these, to avoid
> breaking the Clang build, which cares that each half of the union have
> the same bit size.
All right.
> All of these can be fixed on commit if there are no other issues.
> Otherwise, Reviewed-by: Andrew Cooper
Thanks!
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [RFC 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit
Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
domain to change the value of the #VE suppress bit for a page.
Signed-off-by: Adrian Pop
---
xen/arch/x86/hvm/hvm.c | 14
xen/arch/x86/mm/mem_access.c| 48 +
xen/include/public/hvm/hvm_op.h | 15 +
xen/include/xen/mem_access.h| 3 +++
4 files changed, 80 insertions(+)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 2e76c2345b..eb01527c5b 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4356,6 +4356,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:
@@ -4472,6 +4473,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;
+uint8_t 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..b9e611d3db 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -466,6 +466,54 @@ 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, uint8_t suppress_ve,
+unsigned int altp2m_idx)
+{
+struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
+struct p2m_domain *ap2m = NULL;
+struct p2m_domain *p2m = NULL;
+mfn_t mfn;
+p2m_access_t a;
+p2m_type_t t;
+unsigned long gfn_l;
+int rc = 0;
+
+if ( !cpu_has_vmx )
+return -EOPNOTSUPP;
+
+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;
+}
+
+p2m_lock(host_p2m);
+if ( ap2m )
+p2m_lock(ap2m);
+
+gfn_l = gfn_x(gfn);
+mfn = p2m->get_entry(p2m, gfn_l, &t, &a, 0, NULL, NULL);
+if ( !mfn_valid(mfn) )
+return -ESRCH;
+rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
+suppress_ve);
+if ( ap2m )
+p2m_unlock(ap2m);
+p2m_unlock(host_p2m);
+
+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 bc00ef0e65..9736092f58 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -231,6 +231,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;
@@ -262,6 +274,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;
@@ -270,6 +284,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_veset_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..b6e6a7650a 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);
+in
[Xen-devel] [RFC 0/3] x86: Add a hvmop for setting the #VE suppress bit
After DomU has enabled #VE using HVMOP_altp2m_vcpu_enable_notify, all its pages have the #VE suppress bit cleared, generating #VEs for any EPT violation. There is currently no way to change the value of the #VE suppress bit for a page from a domain; it can only be done in Xen internally using ept_set_entry(). Following the discussion from https://lists.xen.org/archives/html/xen-devel/2017-03/msg01312.html this patch introduces a new hvmop to set this bit. I'm not quite sure where it would be best to define p2m_set_suppress_ve(). It is currently in mem_access.c, but this file contains common functions for x86 (vmx & svm), while this function is Intel-specific. Adrian Pop (2): x86/altp2m: Add a hvmop for setting the suppress #VE bit libxc: Add support for the altp2m suppress #VE hvmop Vlad Ioan Topan (1): x86/mm: Change default value for suppress #VE in set_mem_access() 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| 51 +++-- xen/include/public/hvm/hvm_op.h | 15 xen/include/xen/mem_access.h| 3 +++ 6 files changed, 107 insertions(+), 2 deletions(-) -- 2.12.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [RFC 3/3] libxc: Add support for the altp2m suppress #VE hvmop
This adds a wrapper for issuing HVMOP_altp2m_set_suppress_ve from a
domain.
Signed-off-by: Adrian Pop
---
tools/libxc/include/xenctrl.h | 2 ++
tools/libxc/xc_altp2m.c | 24
2 files changed, 26 insertions(+)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 2d97d36c38..5e1e4cfa81 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1923,6 +1923,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, uint8_t 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..b0f3e344af 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, uint8_t 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)
--
2.12.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [RFC 1/3] x86/mm: Change default value for suppress #VE in set_mem_access()
From: Vlad Ioan Topan The default value for the "suppress #VE" bit set by set_mem_access() currently depends on whether the call is made from the same domain (the bit is set when called from another domain and cleared if called from the same domain). This patch changes that behavior to inherit the old suppress #VE bit value if it is already set and to set it to 1 otherwise, which is safer and more reliable. Signed-off-by: Vlad Ioan Topan --- xen/arch/x86/mm/mem_access.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c index 5adaf6df90..d0b0767855 100644 --- a/xen/arch/x86/mm/mem_access.c +++ b/xen/arch/x86/mm/mem_access.c @@ -273,8 +273,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, } } -return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, - (current->domain != d)); +return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1); } static int set_mem_access(struct domain *d, struct p2m_domain *p2m, -- 2.12.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] x86/monitor: add support for descriptor access events
Hello,
On Wed, Apr 05, 2017 at 08:26:27AM -0600, Jan Beulich wrote:
> >>> On 04.04.17 at 11:57, wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -3572,6 +3572,43 @@ gp_fault:
> > return X86EMUL_EXCEPTION;
> > }
> >
> > +int hvm_descriptor_access_intercept(uint64_t exit_info,
> > +uint64_t vmx_exit_qualification,
> > +uint8_t descriptor, bool is_write)
>
> Why uint8_t?
The descriptor type from struct vm_event_desc_access is uint8_t since
there are only 4 possible descriptors:
> > +#define VM_EVENT_DESC_IDTR 1
> > +#define VM_EVENT_DESC_GDTR 2
> > +#define VM_EVENT_DESC_LDTR 3
> > +#define VM_EVENT_DESC_TR 4
Should it be something else?
> > +{
> > +struct vcpu *curr = current;
> > +struct domain *currd = curr->domain;
> > +int rc;
> > +
> > +if ( currd->arch.monitor.descriptor_access_enabled )
> > +{
> > +ASSERT(curr->arch.vm_event);
> > +hvm_monitor_descriptor_access(exit_info, vmx_exit_qualification,
> > + descriptor, is_write);
> > +}
> > +else
> > +{
> > +struct hvm_emulate_ctxt ctxt = {};
> > +
> > +hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
> > +rc = hvm_emulate_one(&ctxt);
> > +switch ( rc )
>
> You don't really need to go through a local variable here.
Ok.
> > --- a/xen/arch/x86/hvm/monitor.c
> > +++ b/xen/arch/x86/hvm/monitor.c
> > @@ -72,6 +72,28 @@ void hvm_monitor_msr(unsigned int msr, uint64_t value)
> > }
> > }
> >
> > +void hvm_monitor_descriptor_access(uint64_t exit_info,
> > + uint64_t vmx_exit_qualification,
> > + uint8_t descriptor, bool is_write)
> > +{
> > +struct vcpu *curr = current;
> > +vm_event_request_t req = {
> > +.reason = VM_EVENT_REASON_DESCRIPTOR_ACCESS,
> > +.u.desc_access.descriptor = descriptor,
> > +.u.desc_access.is_write = is_write,
> > +};
> > +if ( cpu_has_vmx )
> > +{
> > +req.u.desc_access.arch.vmx.instr_info = exit_info;
> > +req.u.desc_access.arch.vmx.exit_qualification =
> > vmx_exit_qualification;
> > +}
> > +else
> > +{
> > +req.u.desc_access.arch.svm.exitinfo = exit_info;
> > +}
> > +monitor_traps(curr, 1, &req);
>
> true
Ok.
> > @@ -3361,6 +3376,40 @@ static void vmx_handle_xrstors(void)
> > domain_crash(current->domain);
> > }
> >
> > +static void vmx_handle_idt_or_gdt(idt_or_gdt_instr_info_t instr_info,
> > + uint64_t exit_qualification)
> > +{
> > +uint8_t descriptor = instr_info.instr_identity
> > +? VM_EVENT_DESC_IDTR : VM_EVENT_DESC_GDTR;
> > +
> > +hvm_descriptor_access_intercept(instr_info.raw, exit_qualification,
> > +descriptor, instr_info.instr_write);
> > +}
> > +
> > +static void vmx_handle_ldt_or_tr(ldt_or_tr_instr_info_t instr_info,
> > + uint64_t exit_qualification)
> > +{
> > +uint8_t descriptor = instr_info.instr_identity
> > +? VM_EVENT_DESC_TR : VM_EVENT_DESC_LDTR;
> > +
> > +hvm_descriptor_access_intercept(instr_info.raw, exit_qualification,
> > +descriptor, instr_info.instr_write);
> > +}
>
> I think these should be folded into their only caller (at once
> eliminating the need to make those unions transparent ones).
Ok.
> And again - why uint8_t?
Same as above.
> Jan
Thank you!
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] x86/monitor: add support for descriptor access events
On Thu, Apr 06, 2017 at 03:20:21AM -0600, Jan Beulich wrote:
> >>> On 06.04.17 at 10:59, wrote:
> > On Wed, Apr 05, 2017 at 08:26:27AM -0600, Jan Beulich wrote:
> >> >>> On 04.04.17 at 11:57, wrote:
> >> > --- a/xen/arch/x86/hvm/hvm.c
> >> > +++ b/xen/arch/x86/hvm/hvm.c
> >> > @@ -3572,6 +3572,43 @@ gp_fault:
> >> > return X86EMUL_EXCEPTION;
> >> > }
> >> >
> >> > +int hvm_descriptor_access_intercept(uint64_t exit_info,
> >> > +uint64_t vmx_exit_qualification,
> >> > +uint8_t descriptor, bool is_write)
> >>
> >> Why uint8_t?
> >
> > The descriptor type from struct vm_event_desc_access is uint8_t since
> > there are only 4 possible descriptors:
> >
> >> > +#define VM_EVENT_DESC_IDTR 1
> >> > +#define VM_EVENT_DESC_GDTR 2
> >> > +#define VM_EVENT_DESC_LDTR 3
> >> > +#define VM_EVENT_DESC_TR 4
> >
> > Should it be something else?
>
> Well, you should avoid fixed width types where they're not really
> needed (their use should signal a true dependency on the specified
> width). "unsigned int" would be quite fine here afaict.
So should it be changed in the struct definition as well?
> >> > +struct vm_event_desc_access {
> >> > +union {
> >> > +struct {
> >> > +uint32_t instr_info; /* VMX: VMCS
> >> > Instruction-Information */
> >> > +uint32_t _pad1;
> >> > +uint64_t exit_qualification; /* VMX: VMCS Exit
> >> > Qualification */
> >> > +} vmx;
> >> > +struct {
> >> > +uint64_t exitinfo; /* SVM: VMCB EXITINFO */
> >> > +uint64_t _pad2;
> >> > +} svm;
> >> > +} arch;
> >> > +uint8_t descriptor; /* VM_EVENT_DESC_* */
> >> > +uint8_t is_write;
> >> > +uint8_t _pad[6];
> >> > +};
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] x86/monitor: add support for descriptor access events
On Thu, Apr 06, 2017 at 08:09:23AM -0600, Jan Beulich wrote: > >>> On 06.04.17 at 11:37, wrote: > > On Thu, Apr 06, 2017 at 03:20:21AM -0600, Jan Beulich wrote: > >> >>> On 06.04.17 at 10:59, wrote: > >> > On Wed, Apr 05, 2017 at 08:26:27AM -0600, Jan Beulich wrote: > >> >> >>> On 04.04.17 at 11:57, wrote: > >> >> > --- a/xen/arch/x86/hvm/hvm.c > >> >> > +++ b/xen/arch/x86/hvm/hvm.c > >> >> > @@ -3572,6 +3572,43 @@ gp_fault: > >> >> > return X86EMUL_EXCEPTION; > >> >> > } > >> >> > > >> >> > +int hvm_descriptor_access_intercept(uint64_t exit_info, > >> >> > +uint64_t vmx_exit_qualification, > >> >> > +uint8_t descriptor, bool > >> >> > is_write) > >> >> > >> >> Why uint8_t? > >> > > >> > The descriptor type from struct vm_event_desc_access is uint8_t since > >> > there are only 4 possible descriptors: > >> > > >> >> > +#define VM_EVENT_DESC_IDTR 1 > >> >> > +#define VM_EVENT_DESC_GDTR 2 > >> >> > +#define VM_EVENT_DESC_LDTR 3 > >> >> > +#define VM_EVENT_DESC_TR 4 > >> > > >> > Should it be something else? > >> > >> Well, you should avoid fixed width types where they're not really > >> needed (their use should signal a true dependency on the specified > >> width). "unsigned int" would be quite fine here afaict. > > > > So should it be changed in the struct definition as well? > > You mean in the public interface? No, there you _need_ to use fixed > width types. Ok, I'll make the changes and resend. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3] x86/monitor: add support for descriptor access events
Adds monitor support for descriptor access events (reads & writes of
IDTR/GDTR/LDTR/TR) for the x86 architecture (VMX and SVM).
Signed-off-by: Adrian Pop
Acked-by: Wei Liu
Acked-by: Razvan Cojocaru
Reviewed-by: Boris Ostrovsky
Reviewed-by: Kevin Tian
---
changes in v3:
- remove the unnecessary newline escapes (Boris Ostrovsky)
- make both halves of the _instr_info union the same size to
avoid breaking the Clang build (Andrew Cooper)
- hvm_descriptor_access_intercept(): don't use the rc local variable
(Jan Beulich)
- use true when calling monitor_traps (Jan Beulich)
- fold vmx_handle_idt_or_gdt() and vmx_handle_ldt_or_tr() in their
caller and remove the transparent attribute from the unions (Jan
Beulich)
- avoid using fixed width types if not necessary (Jan Beulich)
changes in v2:
- use two sub-structs (vmx/svm) for vm_event_desc_access (Tamas K
Lengyel)
- more compact version of the descriptor VM-Exit handling on svm (Boris
Ostrovsky)
- use bool instead of bool_t (Jan Beulich)
- use curr, currd for the struct vcpu and struct domain local variables
(Jan Beulich)
- move hvm_emulate_ctxt into a narrower scope (Jan Beulich)
- remove leftover dead code (Jan Beulich)
- order the monitor events numerically (Jan Beulich)
- remove VM_EVENT_DESC_INVALID (Jan Beulich)
- crash the domain if the descriptor access instruction can't be
emulated (Jan Beulich)
- call hvm_inject_event without checking for pending events (Andrew
Cooper)
- introduce structures for the VM-Exit instruction information used for
the descriptor instructions and use fewer magic numbers (Andrew
Cooper, Jan Beulich)
---
tools/libxc/include/xenctrl.h | 2 ++
tools/libxc/xc_monitor.c| 14
tools/tests/xen-access/xen-access.c | 29 +++-
xen/arch/x86/hvm/hvm.c | 35 +
xen/arch/x86/hvm/monitor.c | 24
xen/arch/x86/hvm/svm/svm.c | 42 ++
xen/arch/x86/hvm/vmx/vmx.c | 45 +
xen/arch/x86/monitor.c | 18 +++
xen/arch/x86/vm_event.c | 6 +
xen/include/asm-x86/domain.h| 1 +
xen/include/asm-x86/hvm/hvm.h | 1 +
xen/include/asm-x86/hvm/monitor.h | 3 +++
xen/include/asm-x86/hvm/support.h | 3 +++
xen/include/asm-x86/hvm/vmx/vmx.h | 44
xen/include/asm-x86/monitor.h | 3 ++-
xen/include/public/domctl.h | 1 +
xen/include/public/vm_event.h | 25 +
17 files changed, 294 insertions(+), 2 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 01f8dfe513..1629f412dd 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2010,6 +2010,8 @@ int xc_monitor_mov_to_msr(xc_interface *xch, domid_t
domain_id, uint32_t msr,
int xc_monitor_singlestep(xc_interface *xch, domid_t domain_id, bool enable);
int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id,
bool enable);
+int xc_monitor_descriptor_access(xc_interface *xch, domid_t domain_id,
+ bool enable);
int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
bool enable, bool sync);
int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 15a7c32d52..f99b6e3a33 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -129,6 +129,20 @@ int xc_monitor_singlestep(xc_interface *xch, domid_t
domain_id,
return do_domctl(xch, &domctl);
}
+int xc_monitor_descriptor_access(xc_interface *xch, domid_t domain_id,
+ bool enable)
+{
+DECLARE_DOMCTL;
+
+domctl.cmd = XEN_DOMCTL_monitor_op;
+domctl.domain = domain_id;
+domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
+: XEN_DOMCTL_MONITOR_OP_DISABLE;
+domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS;
+
+return do_domctl(xch, &domctl);
+}
+
int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, bool enable,
bool sync)
{
diff --git a/tools/tests/xen-access/xen-access.c
b/tools/tests/xen-access/xen-access.c
index 9d4f95756b..ff4d289b45 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -337,7 +337,7 @@ void usage(char* progname)
{
fprintf(stderr, "Usage: %s [-m] write|exec", progname);
#if defined(__i386__) || defined(__x86_64__)
-fprintf(stderr,
"|breakpoint|altp2m_write|altp2m_exec|debug|cpuid");
+fprintf(stderr,
"|breakpoint|altp2m_write|altp2m_exec|debug|cpuid|desc_access");
#elif defined(__arm__) || defined(__aarch64__
Re: [Xen-devel] [PATCH v3] x86/monitor: add support for descriptor access events
On Fri, Apr 07, 2017 at 07:18:26AM -0600, Jan Beulich wrote:
> >>> On 07.04.17 at 12:17, wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -3589,6 +3589,41 @@ gp_fault:
> > return X86EMUL_EXCEPTION;
> > }
> >
> > +int hvm_descriptor_access_intercept(uint64_t exit_info,
> > +uint64_t vmx_exit_qualification,
> > +unsigned int descriptor, bool is_write)
> > +{
> > +struct vcpu *curr = current;
> > +struct domain *currd = curr->domain;
> > +
> > +if ( currd->arch.monitor.descriptor_access_enabled )
> > +{
> > +ASSERT(curr->arch.vm_event);
> > +hvm_monitor_descriptor_access(exit_info, vmx_exit_qualification,
> > + descriptor, is_write);
> > +}
> > +else
> > +{
> > +struct hvm_emulate_ctxt ctxt = {};
>
> Pointless initializer - this function ...
>
> > +hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
>
> ... memset()s the whole structure.
Indeed.
> > --- a/xen/arch/x86/hvm/monitor.c
> > +++ b/xen/arch/x86/hvm/monitor.c
> > @@ -72,6 +72,30 @@ void hvm_monitor_msr(unsigned int msr, uint64_t value)
> > }
> > }
> >
> > +void hvm_monitor_descriptor_access(uint64_t exit_info,
> > + uint64_t vmx_exit_qualification,
> > + uint8_t descriptor, bool is_write)
> > +{
> > +struct vcpu *curr = current;
>
> Pointless local variable, it is being use just once ...
>
> > +vm_event_request_t req = {
> > +.reason = VM_EVENT_REASON_DESCRIPTOR_ACCESS,
> > +.u.desc_access.descriptor = descriptor,
> > +.u.desc_access.is_write = is_write,
> > +};
> > +
> > +if ( cpu_has_vmx )
> > +{
> > +req.u.desc_access.arch.vmx.instr_info = exit_info;
> > +req.u.desc_access.arch.vmx.exit_qualification =
> > vmx_exit_qualification;
> > +}
> > +else
> > +{
> > +req.u.desc_access.arch.svm.exitinfo = exit_info;
> > +}
> > +
> > +monitor_traps(curr, true, &req);
>
> ... here afaics.
That's right. Using current directly would be fine.
> > --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> > +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> > @@ -628,4 +628,48 @@ typedef struct {
> > u16 eptp_index;
> > } ve_info_t;
> >
> > +/* VM-Exit instruction info for LIDT, LGDT, SIDT, SGDT */
> > +typedef union idt_or_gdt_instr_info {
> > +unsigned long raw;
> > +struct {
> > +unsigned long scaling :2, /* bits 0:1 - Scaling */
> > +:5, /* bits 6:2 - Undefined */
> > +addr_size :3, /* bits 9:7 - Address size */
> > +:1, /* bit 10 - Cleared to 0 */
> > +operand_size:1, /* bit 11 - Operand size */
> > +:3, /* bits 14:12 - Undefined */
> > +segment_reg :3, /* bits 17:15 - Segment register */
> > +index_reg :4, /* bits 21:18 - Index register */
> > +index_reg_invalid :1, /* bit 22 - Index register invalid */
> > +base_reg:4, /* bits 26:23 - Base register */
> > +base_reg_invalid:1, /* bit 27 - Base register invalid */
> > +instr_identity :1, /* bit 28 - 0:GDT, 1:IDT */
> > +instr_write :1, /* bit 29 - 0:store, 1:load */
> > +:2, /* bits 30:31 - Undefined */
> > +:32; /* bits 32:63 - Undefined */
>
> Is there anything wrong with :34?
Nothing wrong with :34.
> With these cosmetic issues addressed (which I guess I'll take the
> liberty of doing while committing)
> Reviewed-by: Jan Beulich
Thanks!
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
