[PATCH v5 0/3] Add hvmops for setting and getting the suppress #VE bit

[PATCH v5 0/3] Add hvmops for setting and getting the suppress #VE bit

[Adrian Pop]

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 these
patches introduce new hvmops for setting and getting this bit and thus

Adrian Pop (3):
  x86/altp2m: Add a hvmop for setting the suppress #VE bit
  x86/altp2m: Add a hvmop for querying 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   |   4 ++
 tools/libxc/xc_altp2m.c         |  50 ++++++++++++++++
 xen/arch/x86/hvm/hvm.c          |  33 ++++++++++
 xen/arch/x86/mm/mem_access.c    | 103 +++++++++++++++++++++++++++++++-
 xen/include/public/hvm/hvm_op.h |  13 ++++
 xen/include/xen/mem_access.h    |   6 ++
 6 files changed, 207 insertions(+), 2 deletions(-)

-- 
2.18.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v5 1/3] x86/mm: Change default value for suppress #VE in set_mem_access()

[Adrian Pop]

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

[PATCH v5 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Adrian Pop]

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

[PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit

[Adrian Pop]

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

Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit

[Tamas K Lengyel]

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

Re: [PATCH v5 1/3] x86/mm: Change default value for suppress #VE in set_mem_access()

[Tamas K Lengyel]

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

Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit

[Adrian Pop]

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

Re: [PATCH v5 1/3] x86/mm: Change default value for suppress #VE in set_mem_access()

[Adrian Pop]

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

Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit

[Wei Liu]

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

Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit

[Razvan Cojocaru]

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

Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit

[Wei Liu]

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

Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit

[Wei Liu]

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

Re: [PATCH v5 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[George Dunlap]

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

Re: [PATCH v5 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Razvan Cojocaru]

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

Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit

[George Dunlap]

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

Re: [PATCH v5 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Razvan Cojocaru]

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

Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit

[Razvan Cojocaru]

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

Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit

[Razvan Cojocaru]

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

Re: [PATCH v5 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[George Dunlap]

Sorry, looks like this may not have gone through.
 -G
On Thu, Sep 20, 2018 at 5:08 PM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On Thu, Sep 20, 2018 at 4:53 PM Razvan Cojocaru
> <rcojocaru@bitdefender.com> wrote:
> >
> > 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.
>
> At the moment, it appears that you can call
> HVMOP_altp2m_vcpu_enable_notify to set a vcpu's veinfo_gfn even if the
> hardware or the p2m mode doesn't support #VE.  It would be better if
> callers got the -EOPNOTSUPP error at that point, rather than having
> vcpu_enable_notify succeed and then having set_suppress_ve fail.
>
> To be clear, I'm not requiring that as a clean-up, I'm just saying
> that such a change would be an improvement (which you may consider
> doing if you wish).
>
>  -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Razvan Cojocaru]

On 9/23/18 7:33 PM, George Dunlap wrote:
> Sorry, looks like this may not have gone through.
>  -G
> On Thu, Sep 20, 2018 at 5:08 PM George Dunlap <george.dunlap@citrix.com> wrote:
>>
>> On Thu, Sep 20, 2018 at 4:53 PM Razvan Cojocaru
>> <rcojocaru@bitdefender.com> wrote:
>>>
>>> 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.
>>
>> At the moment, it appears that you can call
>> HVMOP_altp2m_vcpu_enable_notify to set a vcpu's veinfo_gfn even if the
>> hardware or the p2m mode doesn't support #VE.  It would be better if
>> callers got the -EOPNOTSUPP error at that point, rather than having
>> vcpu_enable_notify succeed and then having set_suppress_ve fail.
>>
>> To be clear, I'm not requiring that as a clean-up, I'm just saying
>> that such a change would be an improvement (which you may consider
>> doing if you wish).

Undestood, thank you for the clarification! We'll follow up with a patch
on that.

I have in the meantime sent out a patch with the requested altp2m
cleanup (code moved to p2m.c and checks moved to p2m-ept.c. I haven't
seen a reply to my question about whether this is what you meant
(perhaps it got lost as well?), so I hope I've understood your request
correctly.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel