* [PATCH v5 1/3] x86/mm: Change default value for suppress #VE in set_mem_access()
2018-09-03 15:48 [PATCH v5 0/3] Add hvmops for setting and getting the suppress #VE bit Adrian Pop
@ 2018-09-03 15:48 ` Adrian Pop
2018-09-11 18:08 ` Tamas K Lengyel
2018-09-03 15:48 ` [PATCH v5 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit Adrian Pop
2018-09-03 15:48 ` [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying " Adrian Pop
2 siblings, 1 reply; 20+ messages in thread
From: Adrian Pop @ 2018-09-03 15:48 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Adrian Pop, Ian Jackson, Tim Deegan, Julien Grall,
Tamas K Lengyel, Jan Beulich, Sergej Proskurin, Vlad Ioan Topan
From: Vlad Ioan Topan <itopan@bitdefender.com>
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 <itopan@bitdefender.com>
Signed-off-by: Adrian Pop <apop@bitdefender.com>
---
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 84d260ebd8..e1522a0b75 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -284,8 +284,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
}
}
- return ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a,
- current->domain != d);
+ return ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a, -1);
}
static int set_mem_access(struct domain *d, struct p2m_domain *p2m,
--
2.18.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v5 1/3] x86/mm: Change default value for suppress #VE in set_mem_access()
2018-09-03 15:48 ` [PATCH v5 1/3] x86/mm: Change default value for suppress #VE in set_mem_access() Adrian Pop
@ 2018-09-11 18:08 ` Tamas K Lengyel
2018-09-12 4:02 ` Adrian Pop
0 siblings, 1 reply; 20+ messages in thread
From: Tamas K Lengyel @ 2018-09-11 18:08 UTC (permalink / raw)
To: Adrian Pop
Cc: Stefano Stabellini, Wei Liu, Razvan Cojocaru,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Vlad-Ioan TOPAN, Julien Grall, Jan Beulich,
Sergej Proskurin, Xen-devel
On Mon, Sep 3, 2018 at 9:48 AM Adrian Pop <apop@bitdefender.com> wrote:
>
> From: Vlad Ioan Topan <itopan@bitdefender.com>
>
> 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 <itopan@bitdefender.com>
> Signed-off-by: Adrian Pop <apop@bitdefender.com>
> ---
> 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 84d260ebd8..e1522a0b75 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -284,8 +284,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
> }
> }
>
> - return ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a,
> - current->domain != d);
Could you add a comment here saying "inherit the old suppress #VE bit
value if it is already set and to set it to 1 otherwise" to explain
the meaning of the magic -1 value?
Thanks,
Tamas
> + return ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a, -1);
> }
>
> static int set_mem_access(struct domain *d, struct p2m_domain *p2m,
> --
> 2.18.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/3] x86/mm: Change default value for suppress #VE in set_mem_access()
2018-09-11 18:08 ` Tamas K Lengyel
@ 2018-09-12 4:02 ` Adrian Pop
0 siblings, 0 replies; 20+ messages in thread
From: Adrian Pop @ 2018-09-12 4:02 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Stefano Stabellini, Wei Liu, Razvan Cojocaru,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Vlad-Ioan TOPAN, Julien Grall, Jan Beulich,
Sergej Proskurin, Xen-devel
On Tue, Sep 11, 2018 at 12:08:42PM -0600, Tamas K Lengyel wrote:
> On Mon, Sep 3, 2018 at 9:48 AM Adrian Pop <apop@bitdefender.com> wrote:
> >
> > From: Vlad Ioan Topan <itopan@bitdefender.com>
> >
> > 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 <itopan@bitdefender.com>
> > Signed-off-by: Adrian Pop <apop@bitdefender.com>
> > ---
> > 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 84d260ebd8..e1522a0b75 100644
> > --- a/xen/arch/x86/mm/mem_access.c
> > +++ b/xen/arch/x86/mm/mem_access.c
> > @@ -284,8 +284,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
> > }
> > }
> >
> > - return ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a,
> > - current->domain != d);
>
> Could you add a comment here saying "inherit the old suppress #VE bit
> value if it is already set and to set it to 1 otherwise" to explain
> the meaning of the magic -1 value?
Sure. Thank you!
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v5 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit
2018-09-03 15:48 [PATCH v5 0/3] Add hvmops for setting and getting the suppress #VE bit Adrian Pop
2018-09-03 15:48 ` [PATCH v5 1/3] x86/mm: Change default value for suppress #VE in set_mem_access() Adrian Pop
@ 2018-09-03 15:48 ` Adrian Pop
2018-09-20 11:34 ` George Dunlap
2018-09-03 15:48 ` [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying " Adrian Pop
2 siblings, 1 reply; 20+ messages in thread
From: Adrian Pop @ 2018-09-03 15:48 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Adrian Pop, Ian Jackson, Tim Deegan, Julien Grall,
Tamas K Lengyel, Jan Beulich, Sergej Proskurin
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.
Add a libxc wrapper for invoking this hvmop.
Signed-off-by: Adrian Pop <apop@bitdefender.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
---
Changes in v5:
- remove the "set_" from struct xen_hvm_altp2m_set_suppress_ve
Changes in v4:
- fix a deadlock:
If p2m_set_suppress_ve() is called on invalid pages the code path
wrongly returns without releasing the lock, resulting in a deadlock.
- remove the privileged domain check
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)
- 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 | 55 +++++++++++++++++++++++++++++++++
xen/include/public/hvm/hvm_op.h | 11 +++++++
xen/include/xen/mem_access.h | 3 ++
6 files changed, 109 insertions(+)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index c626984aba..cc8b3e7dce 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1952,6 +1952,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, uint32_t domid,
/* Switch all vCPUs of the domain to the specified altp2m view */
int xc_altp2m_switch_to_view(xc_interface *handle, uint32_t domid,
uint16_t view_id);
+int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
+ uint16_t view_id, xen_pfn_t gfn, bool sve);
int xc_altp2m_set_mem_access(xc_interface *handle, uint32_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 ce4a1e4d60..f883d0b392 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, uint32_t domid,
return rc;
}
+int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_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.suppress_ve.view = view_id;
+ arg->u.suppress_ve.gfn = gfn;
+ arg->u.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, uint32_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 72c51faecb..64ab36ff53 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4524,6 +4524,7 @@ static int do_altp2m_op(
case HVMOP_altp2m_create_p2m:
case HVMOP_altp2m_destroy_p2m:
case HVMOP_altp2m_switch_p2m:
+ case HVMOP_altp2m_set_suppress_ve:
case HVMOP_altp2m_set_mem_access:
case HVMOP_altp2m_set_mem_access_multi:
case HVMOP_altp2m_change_gfn:
@@ -4641,6 +4642,19 @@ static int do_altp2m_op(
rc = p2m_switch_domain_altp2m_by_id(d, a.u.view.view);
break;
+ case HVMOP_altp2m_set_suppress_ve:
+ if ( a.u.suppress_ve.pad1 || a.u.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.suppress_ve.suppress_ve;
+
+ rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx);
+ }
+ break;
+
case HVMOP_altp2m_set_mem_access:
if ( a.u.set_mem_access.pad )
rc = -EINVAL;
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index e1522a0b75..4d49025cbe 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -495,6 +495,61 @@ void arch_p2m_set_access_required(struct domain *d, bool access_required)
}
}
+/*
+ * 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;
+
+ /* #VE should be enabled for this vcpu. */
+ if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) )
+ return -ENXIO;
+
+ 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, &t, &a, 0, NULL, NULL);
+ if ( !mfn_valid(mfn) )
+ {
+ rc = -ESRCH;
+ goto out;
+ }
+
+ rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, t, a, suppress_ve);
+
+out:
+ if ( ap2m )
+ p2m_unlock(ap2m);
+
+ gfn_unlock(host_p2m, gfn, 0);
+
+ return rc;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index bbba99e5f5..14d29d1700 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -38,6 +38,14 @@ struct xen_hvm_param {
typedef struct xen_hvm_param xen_hvm_param_t;
DEFINE_XEN_GUEST_HANDLE(xen_hvm_param_t);
+struct xen_hvm_altp2m_suppress_ve {
+ uint16_t view;
+ uint8_t suppress_ve; /* Boolean type. */
+ uint8_t pad1;
+ uint32_t pad2;
+ uint64_t gfn;
+};
+
#if __XEN_INTERFACE_VERSION__ < 0x00040900
/* Set the logical level of one of a domain's PCI INTx wires. */
@@ -296,6 +304,8 @@ struct xen_hvm_altp2m_op {
#define HVMOP_altp2m_change_gfn 8
/* Set access for an array of pages */
#define HVMOP_altp2m_set_mem_access_multi 9
+/* Set the "Suppress #VE" bit on a page */
+#define HVMOP_altp2m_set_suppress_ve 10
domid_t domain;
uint16_t pad1;
uint32_t pad2;
@@ -306,6 +316,7 @@ struct xen_hvm_altp2m_op {
struct xen_hvm_altp2m_set_mem_access set_mem_access;
struct xen_hvm_altp2m_change_gfn change_gfn;
struct xen_hvm_altp2m_set_mem_access_multi set_mem_access_multi;
+ struct xen_hvm_altp2m_suppress_ve suppress_ve;
uint8_t pad[64];
} u;
};
diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
index 7e95eab81c..a8d38b90e6 100644
--- a/xen/include/xen/mem_access.h
+++ b/xen/include/xen/mem_access.h
@@ -72,6 +72,9 @@ long p2m_set_mem_access_multi(struct domain *d,
uint32_t nr, uint32_t start, uint32_t mask,
unsigned int altp2m_idx);
+int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
+ unsigned int altp2m_idx);
+
/*
* Get access type for a gfn.
* If gfn == INVALID_GFN, gets the default access type.
--
2.18.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v5 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit
2018-09-03 15:48 ` [PATCH v5 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit Adrian Pop
@ 2018-09-20 11:34 ` George Dunlap
2018-09-20 11:38 ` Razvan Cojocaru
2018-09-20 15:52 ` Razvan Cojocaru
0 siblings, 2 replies; 20+ messages in thread
From: George Dunlap @ 2018-09-20 11:34 UTC (permalink / raw)
To: Adrian Pop, xen-devel
Cc: Stefano Stabellini, Wei Liu, Razvan Cojocaru,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Tamas K Lengyel, Jan Beulich,
Sergej Proskurin
On 09/03/2018 04:48 PM, 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.
>
> Add a libxc wrapper for invoking this hvmop.
>
> Signed-off-by: Adrian Pop <apop@bitdefender.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
Sorry I haven't had a chance to weigh in on this one yet. A couple fo
comments...
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index e1522a0b75..4d49025cbe 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -495,6 +495,61 @@ void arch_p2m_set_access_required(struct domain *d, bool access_required)
> }
> }
>
> +/*
> + * 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)
This should clearly be in p2m.c, and...
> +{
> + 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;
We should avoid Intel-specific checks in common code.
In fact, this is wrong, because you can choose to run a guest in shadow
mode even on a system with virt exceptions -- in which case you'd
trigger the ASSERT() in p2m-pt.c:p2m_pt_set_entry().
Probably what should happen is that we should move the vmx check into
p2m-ept.c:p2m_ept_set_entry(), and replace the ASSERT(sve = 0) in
p2m-pt.c:p2m_pt_set_entry() with "if ( sve != 0 ) return -ENOTSUPP;".
Although what should probably *really* happen is that
`HVMOP_altp2m_vcpu_enable_notify` should fail with -EOPNOTSUPP instead
of silently succeeding.
Everything else looks good.
I realize this has been checked in already; no need to revert if you can
send a follow-up patch in the next couple of (business) days.
Thanks,
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit
2018-09-20 11:34 ` George Dunlap
@ 2018-09-20 11:38 ` Razvan Cojocaru
2018-09-20 15:52 ` Razvan Cojocaru
1 sibling, 0 replies; 20+ messages in thread
From: Razvan Cojocaru @ 2018-09-20 11:38 UTC (permalink / raw)
To: George Dunlap, Adrian Pop, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Tamas K Lengyel, Jan Beulich, Sergej Proskurin
On 9/20/18 2:34 PM, George Dunlap wrote:
> On 09/03/2018 04:48 PM, 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.
>>
>> Add a libxc wrapper for invoking this hvmop.
>>
>> Signed-off-by: Adrian Pop <apop@bitdefender.com>
>> Acked-by: Wei Liu <wei.liu2@citrix.com>
>> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
>
> Sorry I haven't had a chance to weigh in on this one yet. A couple fo
> comments...
>
>
>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>> index e1522a0b75..4d49025cbe 100644
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -495,6 +495,61 @@ void arch_p2m_set_access_required(struct domain *d, bool access_required)
>> }
>> }
>>
>> +/*
>> + * 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)
>
> This should clearly be in p2m.c, and...
>
>> +{
>> + 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;
>
> We should avoid Intel-specific checks in common code.
>
> In fact, this is wrong, because you can choose to run a guest in shadow
> mode even on a system with virt exceptions -- in which case you'd
> trigger the ASSERT() in p2m-pt.c:p2m_pt_set_entry().
>
> Probably what should happen is that we should move the vmx check into
> p2m-ept.c:p2m_ept_set_entry(), and replace the ASSERT(sve = 0) in
> p2m-pt.c:p2m_pt_set_entry() with "if ( sve != 0 ) return -ENOTSUPP;".
>
> Although what should probably *really* happen is that
> `HVMOP_altp2m_vcpu_enable_notify` should fail with -EOPNOTSUPP instead
> of silently succeeding.
>
> Everything else looks good.
>
> I realize this has been checked in already; no need to revert if you can
> send a follow-up patch in the next couple of (business) days.
Thanks for the review! I'll submit a follow up patch as soon as possible.
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit
2018-09-20 11:34 ` George Dunlap
2018-09-20 11:38 ` Razvan Cojocaru
@ 2018-09-20 15:52 ` Razvan Cojocaru
[not found] ` <CAFLBxZY0am2E01iLpZDcboFG7a1-S47Dwnd-qjp5YqQxGAdejQ@mail.gmail.com>
1 sibling, 1 reply; 20+ messages in thread
From: Razvan Cojocaru @ 2018-09-20 15:52 UTC (permalink / raw)
To: George Dunlap, Adrian Pop, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Tamas K Lengyel, Jan Beulich, Sergej Proskurin
On 9/20/18 2:34 PM, George Dunlap wrote:
>> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
>> + unsigned int altp2m_idx)
> This should clearly be in p2m.c, and...
>
>> +{
>> + 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;
> We should avoid Intel-specific checks in common code.
>
> In fact, this is wrong, because you can choose to run a guest in shadow
> mode even on a system with virt exceptions -- in which case you'd
> trigger the ASSERT() in p2m-pt.c:p2m_pt_set_entry().
>
> Probably what should happen is that we should move the vmx check into
> p2m-ept.c:p2m_ept_set_entry(), and replace the ASSERT(sve = 0) in
> p2m-pt.c:p2m_pt_set_entry() with "if ( sve != 0 ) return -ENOTSUPP;".
>
> Although what should probably *really* happen is that
> `HVMOP_altp2m_vcpu_enable_notify` should fail with -EOPNOTSUPP instead
> of silently succeeding.
Do you mean HVMOP_altp2m_set_suppress_ve here, or am I misunderstanding
your comment? I'm happy to do the exact modifications you've requested
above but I'm afraid I don't follow the HVMOP_altp2m_vcpu_enable_notify
comment.
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit
2018-09-03 15:48 [PATCH v5 0/3] Add hvmops for setting and getting the suppress #VE bit Adrian Pop
2018-09-03 15:48 ` [PATCH v5 1/3] x86/mm: Change default value for suppress #VE in set_mem_access() Adrian Pop
2018-09-03 15:48 ` [PATCH v5 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit Adrian Pop
@ 2018-09-03 15:48 ` Adrian Pop
2018-09-11 18:02 ` Tamas K Lengyel
` (2 more replies)
2 siblings, 3 replies; 20+ messages in thread
From: Adrian Pop @ 2018-09-03 15:48 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Adrian Pop, Ian Jackson, Tim Deegan, Julien Grall,
Tamas K Lengyel, Jan Beulich, Sergej Proskurin
Signed-off-by: Adrian Pop <apop@bitdefender.com>
---
tools/libxc/include/xenctrl.h | 2 ++
tools/libxc/xc_altp2m.c | 26 +++++++++++++++++++
xen/arch/x86/hvm/hvm.c | 19 ++++++++++++++
xen/arch/x86/mm/mem_access.c | 45 +++++++++++++++++++++++++++++++++
xen/include/public/hvm/hvm_op.h | 2 ++
xen/include/xen/mem_access.h | 3 +++
6 files changed, 97 insertions(+)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index cc8b3e7dce..59955f0357 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1954,6 +1954,8 @@ int xc_altp2m_switch_to_view(xc_interface *handle, uint32_t domid,
uint16_t view_id);
int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
uint16_t view_id, xen_pfn_t gfn, bool sve);
+int xc_altp2m_get_suppress_ve(xc_interface *handle, uint32_t domid,
+ uint16_t view_id, xen_pfn_t gfn, bool *sve);
int xc_altp2m_set_mem_access(xc_interface *handle, uint32_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 f883d0b392..1c9b572e2b 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -163,6 +163,32 @@ int xc_altp2m_switch_to_view(xc_interface *handle, uint32_t domid,
return rc;
}
+int xc_altp2m_get_suppress_ve(xc_interface *handle, uint32_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_get_suppress_ve;
+ arg->domain = domid;
+ arg->u.suppress_ve.view = view_id;
+ arg->u.suppress_ve.gfn = gfn;
+
+ rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
+ HYPERCALL_BUFFER_AS_ARG(arg));
+
+ if ( !rc )
+ *sve = arg->u.suppress_ve.suppress_ve;
+
+ xc_hypercall_buffer_free(handle, arg);
+ return rc;
+}
+
int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
uint16_t view_id, xen_pfn_t gfn, bool sve)
{
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 64ab36ff53..6f6efb0d8a 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4525,6 +4525,7 @@ static int do_altp2m_op(
case HVMOP_altp2m_destroy_p2m:
case HVMOP_altp2m_switch_p2m:
case HVMOP_altp2m_set_suppress_ve:
+ case HVMOP_altp2m_get_suppress_ve:
case HVMOP_altp2m_set_mem_access:
case HVMOP_altp2m_set_mem_access_multi:
case HVMOP_altp2m_change_gfn:
@@ -4655,6 +4656,24 @@ static int do_altp2m_op(
}
break;
+ case HVMOP_altp2m_get_suppress_ve:
+ if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
+ rc = -EINVAL;
+ else
+ {
+ gfn_t gfn = _gfn(a.u.suppress_ve.gfn);
+ unsigned int altp2m_idx = a.u.suppress_ve.view;
+ bool suppress_ve;
+
+ rc = p2m_get_suppress_ve(d, gfn, &suppress_ve, altp2m_idx);
+ if ( !rc )
+ {
+ a.u.suppress_ve.suppress_ve = suppress_ve;
+ rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+ }
+ }
+ break;
+
case HVMOP_altp2m_set_mem_access:
if ( a.u.set_mem_access.pad )
rc = -EINVAL;
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 4d49025cbe..df78c93cfd 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -550,6 +550,51 @@ out:
return rc;
}
+int p2m_get_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;
+
+ if ( !cpu_has_vmx_virt_exceptions )
+ return -EOPNOTSUPP;
+
+ /* #VE should be enabled for this vcpu. */
+ if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) )
+ return -ENXIO;
+
+ 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, &t, &a, 0, NULL, suppress_ve);
+ if ( !mfn_valid(mfn) )
+ return -ESRCH;
+
+ if ( ap2m )
+ p2m_unlock(ap2m);
+
+ gfn_unlock(host_p2m, gfn, 0);
+
+ return 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index 14d29d1700..cf00cad164 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -306,6 +306,8 @@ struct xen_hvm_altp2m_op {
#define HVMOP_altp2m_set_mem_access_multi 9
/* Set the "Suppress #VE" bit on a page */
#define HVMOP_altp2m_set_suppress_ve 10
+/* Get the "Suppress #VE" bit of a page */
+#define HVMOP_altp2m_get_suppress_ve 11
domid_t domain;
uint16_t pad1;
uint32_t pad2;
diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
index a8d38b90e6..28cab673da 100644
--- a/xen/include/xen/mem_access.h
+++ b/xen/include/xen/mem_access.h
@@ -75,6 +75,9 @@ long p2m_set_mem_access_multi(struct domain *d,
int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
unsigned int altp2m_idx);
+int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
+ unsigned int altp2m_idx);
+
/*
* Get access type for a gfn.
* If gfn == INVALID_GFN, gets the default access type.
--
2.18.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit
2018-09-03 15:48 ` [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying " Adrian Pop
@ 2018-09-11 18:02 ` Tamas K Lengyel
2018-09-12 4:01 ` Adrian Pop
2018-09-19 14:48 ` Wei Liu
2018-09-20 12:50 ` George Dunlap
2 siblings, 1 reply; 20+ messages in thread
From: Tamas K Lengyel @ 2018-09-11 18:02 UTC (permalink / raw)
To: Adrian Pop
Cc: Stefano Stabellini, Wei Liu, Razvan Cojocaru,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Jan Beulich, Sergej Proskurin,
Xen-devel
On Mon, Sep 3, 2018 at 9:48 AM Adrian Pop <apop@bitdefender.com> wrote:
>
> Signed-off-by: Adrian Pop <apop@bitdefender.com>
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
> ---
> tools/libxc/include/xenctrl.h | 2 ++
> tools/libxc/xc_altp2m.c | 26 +++++++++++++++++++
> xen/arch/x86/hvm/hvm.c | 19 ++++++++++++++
> xen/arch/x86/mm/mem_access.c | 45 +++++++++++++++++++++++++++++++++
> xen/include/public/hvm/hvm_op.h | 2 ++
> xen/include/xen/mem_access.h | 3 +++
> 6 files changed, 97 insertions(+)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index cc8b3e7dce..59955f0357 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1954,6 +1954,8 @@ int xc_altp2m_switch_to_view(xc_interface *handle, uint32_t domid,
> uint16_t view_id);
> int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
> uint16_t view_id, xen_pfn_t gfn, bool sve);
> +int xc_altp2m_get_suppress_ve(xc_interface *handle, uint32_t domid,
> + uint16_t view_id, xen_pfn_t gfn, bool *sve);
> int xc_altp2m_set_mem_access(xc_interface *handle, uint32_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 f883d0b392..1c9b572e2b 100644
> --- a/tools/libxc/xc_altp2m.c
> +++ b/tools/libxc/xc_altp2m.c
> @@ -163,6 +163,32 @@ int xc_altp2m_switch_to_view(xc_interface *handle, uint32_t domid,
> return rc;
> }
>
> +int xc_altp2m_get_suppress_ve(xc_interface *handle, uint32_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_get_suppress_ve;
> + arg->domain = domid;
> + arg->u.suppress_ve.view = view_id;
> + arg->u.suppress_ve.gfn = gfn;
> +
> + rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
> + HYPERCALL_BUFFER_AS_ARG(arg));
> +
> + if ( !rc )
> + *sve = arg->u.suppress_ve.suppress_ve;
> +
> + xc_hypercall_buffer_free(handle, arg);
> + return rc;
> +}
> +
> int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
> uint16_t view_id, xen_pfn_t gfn, bool sve)
> {
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 64ab36ff53..6f6efb0d8a 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4525,6 +4525,7 @@ static int do_altp2m_op(
> case HVMOP_altp2m_destroy_p2m:
> case HVMOP_altp2m_switch_p2m:
> case HVMOP_altp2m_set_suppress_ve:
> + case HVMOP_altp2m_get_suppress_ve:
> case HVMOP_altp2m_set_mem_access:
> case HVMOP_altp2m_set_mem_access_multi:
> case HVMOP_altp2m_change_gfn:
> @@ -4655,6 +4656,24 @@ static int do_altp2m_op(
> }
> break;
>
> + case HVMOP_altp2m_get_suppress_ve:
> + if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
> + rc = -EINVAL;
> + else
> + {
> + gfn_t gfn = _gfn(a.u.suppress_ve.gfn);
> + unsigned int altp2m_idx = a.u.suppress_ve.view;
> + bool suppress_ve;
> +
> + rc = p2m_get_suppress_ve(d, gfn, &suppress_ve, altp2m_idx);
> + if ( !rc )
> + {
> + a.u.suppress_ve.suppress_ve = suppress_ve;
> + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> + }
> + }
> + break;
> +
> case HVMOP_altp2m_set_mem_access:
> if ( a.u.set_mem_access.pad )
> rc = -EINVAL;
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 4d49025cbe..df78c93cfd 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -550,6 +550,51 @@ out:
> return rc;
> }
>
> +int p2m_get_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;
> +
> + if ( !cpu_has_vmx_virt_exceptions )
> + return -EOPNOTSUPP;
> +
> + /* #VE should be enabled for this vcpu. */
> + if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) )
> + return -ENXIO;
> +
> + 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, &t, &a, 0, NULL, suppress_ve);
> + if ( !mfn_valid(mfn) )
> + return -ESRCH;
> +
> + if ( ap2m )
> + p2m_unlock(ap2m);
> +
> + gfn_unlock(host_p2m, gfn, 0);
> +
> + return 0;
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index 14d29d1700..cf00cad164 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -306,6 +306,8 @@ struct xen_hvm_altp2m_op {
> #define HVMOP_altp2m_set_mem_access_multi 9
> /* Set the "Suppress #VE" bit on a page */
> #define HVMOP_altp2m_set_suppress_ve 10
> +/* Get the "Suppress #VE" bit of a page */
> +#define HVMOP_altp2m_get_suppress_ve 11
> domid_t domain;
> uint16_t pad1;
> uint32_t pad2;
> diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
> index a8d38b90e6..28cab673da 100644
> --- a/xen/include/xen/mem_access.h
> +++ b/xen/include/xen/mem_access.h
> @@ -75,6 +75,9 @@ long p2m_set_mem_access_multi(struct domain *d,
> int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
> unsigned int altp2m_idx);
>
> +int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
> + unsigned int altp2m_idx);
> +
> /*
> * Get access type for a gfn.
> * If gfn == INVALID_GFN, gets the default access type.
> --
> 2.18.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit
2018-09-11 18:02 ` Tamas K Lengyel
@ 2018-09-12 4:01 ` Adrian Pop
0 siblings, 0 replies; 20+ messages in thread
From: Adrian Pop @ 2018-09-12 4:01 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Stefano Stabellini, Wei Liu, Razvan Cojocaru,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Jan Beulich, Sergej Proskurin,
Xen-devel
On Tue, Sep 11, 2018 at 12:02:39PM -0600, Tamas K Lengyel wrote:
> On Mon, Sep 3, 2018 at 9:48 AM Adrian Pop <apop@bitdefender.com> wrote:
> >
> > Signed-off-by: Adrian Pop <apop@bitdefender.com>
>
> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
Thanks!
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit
2018-09-03 15:48 ` [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying " Adrian Pop
2018-09-11 18:02 ` Tamas K Lengyel
@ 2018-09-19 14:48 ` Wei Liu
2018-09-20 7:46 ` Razvan Cojocaru
2018-09-20 12:50 ` George Dunlap
2 siblings, 1 reply; 20+ messages in thread
From: Wei Liu @ 2018-09-19 14:48 UTC (permalink / raw)
To: Adrian Pop
Cc: Stefano Stabellini, Wei Liu, Razvan Cojocaru,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Tamas K Lengyel, Jan Beulich,
Sergej Proskurin, xen-devel
On Mon, Sep 03, 2018 at 06:48:36PM +0300, Adrian Pop wrote:
> Signed-off-by: Adrian Pop <apop@bitdefender.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
FAOD I'm expecting you to address Tamas' comment in patch 1 and resend.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit
2018-09-19 14:48 ` Wei Liu
@ 2018-09-20 7:46 ` Razvan Cojocaru
2018-09-20 7:53 ` Wei Liu
0 siblings, 1 reply; 20+ messages in thread
From: Razvan Cojocaru @ 2018-09-20 7:46 UTC (permalink / raw)
To: Wei Liu, Adrian Pop
Cc: Stefano Stabellini, Konrad Rzeszutek Wilk, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall,
Tamas K Lengyel, Jan Beulich, Sergej Proskurin, xen-devel
On 9/19/18 5:48 PM, Wei Liu wrote:
> On Mon, Sep 03, 2018 at 06:48:36PM +0300, Adrian Pop wrote:
>> Signed-off-by: Adrian Pop <apop@bitdefender.com>
>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
>
> FAOD I'm expecting you to address Tamas' comment in patch 1 and resend.
Hello Wei,
That has already been addressed in V6, and the patch is already in:
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=eea4ec2b66dad87ec745778ab9f00e12ef0f2760
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit
2018-09-20 7:46 ` Razvan Cojocaru
@ 2018-09-20 7:53 ` Wei Liu
2018-09-20 8:38 ` Wei Liu
0 siblings, 1 reply; 20+ messages in thread
From: Wei Liu @ 2018-09-20 7:53 UTC (permalink / raw)
To: Razvan Cojocaru
Cc: Stefano Stabellini, Wei Liu, Adrian Pop, George Dunlap,
Andrew Cooper, Konrad Rzeszutek Wilk, Ian Jackson, Tim Deegan,
Julien Grall, Tamas K Lengyel, Jan Beulich, Sergej Proskurin,
xen-devel
On Thu, Sep 20, 2018 at 10:46:01AM +0300, Razvan Cojocaru wrote:
> On 9/19/18 5:48 PM, Wei Liu wrote:
> > On Mon, Sep 03, 2018 at 06:48:36PM +0300, Adrian Pop wrote:
> >> Signed-off-by: Adrian Pop <apop@bitdefender.com>
> >
> > Acked-by: Wei Liu <wei.liu2@citrix.com>
> >
> > FAOD I'm expecting you to address Tamas' comment in patch 1 and resend.
>
> Hello Wei,
>
> That has already been addressed in V6, and the patch is already in:
>
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=eea4ec2b66dad87ec745778ab9f00e12ef0f2760
Yep. Saw that yesterday and I committed the last two patches of v6.
Wei.
>
>
> Thanks,
> Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit
2018-09-20 7:53 ` Wei Liu
@ 2018-09-20 8:38 ` Wei Liu
0 siblings, 0 replies; 20+ messages in thread
From: Wei Liu @ 2018-09-20 8:38 UTC (permalink / raw)
To: Razvan Cojocaru
Cc: Stefano Stabellini, Wei Liu, Adrian Pop, George Dunlap,
Andrew Cooper, Konrad Rzeszutek Wilk, Ian Jackson, Tim Deegan,
Julien Grall, Tamas K Lengyel, Jan Beulich, Sergej Proskurin,
xen-devel
On Thu, Sep 20, 2018 at 08:53:26AM +0100, Wei Liu wrote:
> On Thu, Sep 20, 2018 at 10:46:01AM +0300, Razvan Cojocaru wrote:
> > On 9/19/18 5:48 PM, Wei Liu wrote:
> > > On Mon, Sep 03, 2018 at 06:48:36PM +0300, Adrian Pop wrote:
> > >> Signed-off-by: Adrian Pop <apop@bitdefender.com>
> > >
> > > Acked-by: Wei Liu <wei.liu2@citrix.com>
> > >
> > > FAOD I'm expecting you to address Tamas' comment in patch 1 and resend.
> >
> > Hello Wei,
> >
> > That has already been addressed in V6, and the patch is already in:
> >
> > http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=eea4ec2b66dad87ec745778ab9f00e12ef0f2760
>
> Yep. Saw that yesterday and I committed the last two patches of v6.
Actually those patches will need acks from George, too. I will wait
until EOD to revert them. Sorry, my memory is completely wrong regarding
the maintainership of altp2m code.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit
2018-09-03 15:48 ` [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying " Adrian Pop
2018-09-11 18:02 ` Tamas K Lengyel
2018-09-19 14:48 ` Wei Liu
@ 2018-09-20 12:50 ` George Dunlap
2018-09-21 13:16 ` Razvan Cojocaru
2 siblings, 1 reply; 20+ messages in thread
From: George Dunlap @ 2018-09-20 12:50 UTC (permalink / raw)
To: Adrian Pop, xen-devel
Cc: Stefano Stabellini, Wei Liu, Razvan Cojocaru,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Tamas K Lengyel, Jan Beulich,
Sergej Proskurin
On 09/03/2018 04:48 PM, Adrian Pop wrote:
> Signed-off-by: Adrian Pop <apop@bitdefender.com>
> ---
> tools/libxc/include/xenctrl.h | 2 ++
> tools/libxc/xc_altp2m.c | 26 +++++++++++++++++++
> xen/arch/x86/hvm/hvm.c | 19 ++++++++++++++
> xen/arch/x86/mm/mem_access.c | 45 +++++++++++++++++++++++++++++++++
> xen/include/public/hvm/hvm_op.h | 2 ++
> xen/include/xen/mem_access.h | 3 +++
> 6 files changed, 97 insertions(+)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index cc8b3e7dce..59955f0357 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1954,6 +1954,8 @@ int xc_altp2m_switch_to_view(xc_interface *handle, uint32_t domid,
> uint16_t view_id);
> int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
> uint16_t view_id, xen_pfn_t gfn, bool sve);
> +int xc_altp2m_get_suppress_ve(xc_interface *handle, uint32_t domid,
> + uint16_t view_id, xen_pfn_t gfn, bool *sve);
> int xc_altp2m_set_mem_access(xc_interface *handle, uint32_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 f883d0b392..1c9b572e2b 100644
> --- a/tools/libxc/xc_altp2m.c
> +++ b/tools/libxc/xc_altp2m.c
> @@ -163,6 +163,32 @@ int xc_altp2m_switch_to_view(xc_interface *handle, uint32_t domid,
> return rc;
> }
>
> +int xc_altp2m_get_suppress_ve(xc_interface *handle, uint32_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_get_suppress_ve;
> + arg->domain = domid;
> + arg->u.suppress_ve.view = view_id;
> + arg->u.suppress_ve.gfn = gfn;
> +
> + rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
> + HYPERCALL_BUFFER_AS_ARG(arg));
> +
> + if ( !rc )
> + *sve = arg->u.suppress_ve.suppress_ve;
> +
> + xc_hypercall_buffer_free(handle, arg);
> + return rc;
> +}
> +
> int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
> uint16_t view_id, xen_pfn_t gfn, bool sve)
> {
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 64ab36ff53..6f6efb0d8a 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4525,6 +4525,7 @@ static int do_altp2m_op(
> case HVMOP_altp2m_destroy_p2m:
> case HVMOP_altp2m_switch_p2m:
> case HVMOP_altp2m_set_suppress_ve:
> + case HVMOP_altp2m_get_suppress_ve:
> case HVMOP_altp2m_set_mem_access:
> case HVMOP_altp2m_set_mem_access_multi:
> case HVMOP_altp2m_change_gfn:
> @@ -4655,6 +4656,24 @@ static int do_altp2m_op(
> }
> break;
>
> + case HVMOP_altp2m_get_suppress_ve:
> + if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
> + rc = -EINVAL;
> + else
> + {
> + gfn_t gfn = _gfn(a.u.suppress_ve.gfn);
> + unsigned int altp2m_idx = a.u.suppress_ve.view;
> + bool suppress_ve;
> +
> + rc = p2m_get_suppress_ve(d, gfn, &suppress_ve, altp2m_idx);
> + if ( !rc )
> + {
> + a.u.suppress_ve.suppress_ve = suppress_ve;
> + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> + }
> + }
> + break;
> +
> case HVMOP_altp2m_set_mem_access:
> if ( a.u.set_mem_access.pad )
> rc = -EINVAL;
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 4d49025cbe..df78c93cfd 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -550,6 +550,51 @@ out:
> return rc;
> }
>
> +int p2m_get_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;
> +
> + if ( !cpu_has_vmx_virt_exceptions )
> + return -EOPNOTSUPP;
> +
> + /* #VE should be enabled for this vcpu. */
> + if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) )
> + return -ENXIO;
Basically the same comments as for 2/3: Move to p2m.c, and get rid of
the vmx-ism.
Anothre idea is to get rid of these checks altogether -- returning
'false' when the feature isn't supported or enabled shouldn't be a big
deal. But I don't feel strongly enough about it to argue either way.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit
2018-09-20 12:50 ` George Dunlap
@ 2018-09-21 13:16 ` Razvan Cojocaru
2018-09-21 13:17 ` Razvan Cojocaru
0 siblings, 1 reply; 20+ messages in thread
From: Razvan Cojocaru @ 2018-09-21 13:16 UTC (permalink / raw)
To: George Dunlap, Adrian Pop, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Tamas K Lengyel, Jan Beulich, Sergej Proskurin
On 9/20/18 3:50 PM, George Dunlap wrote:
> On 09/03/2018 04:48 PM, Adrian Pop wrote:
>> Signed-off-by: Adrian Pop <apop@bitdefender.com>
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -550,6 +550,51 @@ out:
>> return rc;
>> }
>>
>> +int p2m_get_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;
>> +
>> + if ( !cpu_has_vmx_virt_exceptions )
>> + return -EOPNOTSUPP;
>> +
>> + /* #VE should be enabled for this vcpu. */
>> + if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) )
>> + return -ENXIO;
>
> Basically the same comments as for 2/3: Move to p2m.c, and get rid of
> the vmx-ism.
>
> Anothre idea is to get rid of these checks altogether -- returning
> 'false' when the feature isn't supported or enabled shouldn't be a big
> deal. But I don't feel strongly enough about it to argue either way.
I've moved the functions to p2m.c and started moving the checks when it
occured to me that it might be cha to move them to ept_get_entry() /
ept_set_entry() in p2m-ept.c.
ept_get_entry() uses a bool_t *sve pointer, so I could check if the
pointer is not NULL, and if it's not perform the checks then, and so
require the caller to always pass a NULL pointer in non-#VE cases.
Still, ept_get_entry() returns a mfn_t, so all can do there is return
INVALID_MFN, which is not very helpful.
ept_set_entry() is more complicated: it takes "int sve" as a parameter
(which makes sense because it can also be -1) - but then really there's
no special value which should trigger a check. Any value could be a
legitimate attempt to set that bit - although 0 (clearing it) might be
viewed as a special case, like it is in the NPT code.
I could indeed remove all the checks and allow setting and getting that
bit regardless of whether #VE is enabled and / or available - if that's
acceptable (though I believe someone has required the checks at some
point in Adrian's work with the patches).
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit
2018-09-21 13:16 ` Razvan Cojocaru
@ 2018-09-21 13:17 ` Razvan Cojocaru
0 siblings, 0 replies; 20+ messages in thread
From: Razvan Cojocaru @ 2018-09-21 13:17 UTC (permalink / raw)
To: George Dunlap, Adrian Pop, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Tamas K Lengyel, Jan Beulich, Sergej Proskurin
On 9/21/18 4:16 PM, Razvan Cojocaru wrote:
> On 9/20/18 3:50 PM, George Dunlap wrote:
>> On 09/03/2018 04:48 PM, Adrian Pop wrote:
>>> Signed-off-by: Adrian Pop <apop@bitdefender.com>
>>> +++ b/xen/arch/x86/mm/mem_access.c
>>> @@ -550,6 +550,51 @@ out:
>>> return rc;
>>> }
>>>
>>> +int p2m_get_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;
>>> +
>>> + if ( !cpu_has_vmx_virt_exceptions )
>>> + return -EOPNOTSUPP;
>>> +
>>> + /* #VE should be enabled for this vcpu. */
>>> + if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) )
>>> + return -ENXIO;
>>
>> Basically the same comments as for 2/3: Move to p2m.c, and get rid of
>> the vmx-ism.
>>
>> Anothre idea is to get rid of these checks altogether -- returning
>> 'false' when the feature isn't supported or enabled shouldn't be a big
>> deal. But I don't feel strongly enough about it to argue either way.
>
> I've moved the functions to p2m.c and started moving the checks when it
> occured to me that it might be cha to move them to ept_get_entry() /
> ept_set_entry() in p2m-ept.c.
Sorry, wrote "challenging" here, my touchpad got in the way and removed
a part of the word.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread