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

[PATCH 0/2] x86: Add a hvmop for setting the #VE suppress 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 this
patch introduces a new hvmop to set this bit and thus have control over
which pages generate #VE and which VM-Exit.

I'm not sure whether it's best to define p2m_set_suppress_ve() in
mem_access.c since this file contains common functions for x86 (vmx &
svm) and the function is Intel-specific.

changes in v2:
- check if #VE has been enabled on the target domain (Tamas K Lengyel)
- check if the cpu has the #VE feature
- make the suppress_ve argument boolean (Jan Beulich)
- initialize only local variables that need initializing (Jan Beulich)
- use fewer local variables (Jan Beulich)
- fix indentation (Jan Beulich)
- remove unnecessary braces (Jan Beulich)
- use gfn_lock() instead of p2m_lock() in the non-altp2m case (Jan
  Beulich)
- allow only privileged domains to use this hvmop
- merge patch #2 and patch #3 (Jan Beulich)

Adrian Pop (1):
  x86/altp2m: Add a hvmop for setting the suppress #VE bit

Vlad Ioan Topan (1):
  x86/mm: Change default value for suppress #VE in set_mem_access()

 tools/libxc/include/xenctrl.h   |  2 ++
 tools/libxc/xc_altp2m.c         | 24 ++++++++++++++++++
 xen/arch/x86/hvm/hvm.c          | 14 +++++++++++
 xen/arch/x86/mm/mem_access.c    | 55 +++++++++++++++++++++++++++++++++++++++--
 xen/include/public/hvm/hvm_op.h | 15 +++++++++++
 xen/include/xen/mem_access.h    |  3 +++
 6 files changed, 111 insertions(+), 2 deletions(-)

-- 
2.13.0


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

[PATCH 1/2] 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 5adaf6df90..d0b0767855 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -273,8 +273,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
         }
     }
 
-    return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
-                         (current->domain != d));
+    return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1);
 }
 
 static int set_mem_access(struct domain *d, struct p2m_domain *p2m,
-- 
2.13.0


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

[PATCH 2/2] 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
privileged domain to change the value of the #VE suppress bit for a
page.

Add a libxc wrapper for invoking this hvmop.

Signed-off-by: Adrian Pop <apop@bitdefender.com>
---
 tools/libxc/include/xenctrl.h   |  2 ++
 tools/libxc/xc_altp2m.c         | 24 +++++++++++++++++++
 xen/arch/x86/hvm/hvm.c          | 14 +++++++++++
 xen/arch/x86/mm/mem_access.c    | 52 +++++++++++++++++++++++++++++++++++++++++
 xen/include/public/hvm/hvm_op.h | 15 ++++++++++++
 xen/include/xen/mem_access.h    |  3 +++
 6 files changed, 110 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 1629f412dd..f6ba8635bf 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid,
 /* Switch all vCPUs of the domain to the specified altp2m view */
 int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
                              uint16_t view_id);
+int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
+                              uint16_t view_id, xen_pfn_t gfn, bool sve);
 int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
                              uint16_t view_id, xen_pfn_t gfn,
                              xenmem_access_t access);
diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index 0639632477..4710133918 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
     return rc;
 }
 
+int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
+                              uint16_t view_id, xen_pfn_t gfn, bool sve)
+{
+    int rc;
+    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
+
+    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
+    if ( arg == NULL )
+        return -1;
+
+    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
+    arg->cmd = HVMOP_altp2m_set_suppress_ve;
+    arg->domain = domid;
+    arg->u.set_suppress_ve.view = view_id;
+    arg->u.set_suppress_ve.gfn = gfn;
+    arg->u.set_suppress_ve.suppress_ve = sve;
+
+    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
+		  HYPERCALL_BUFFER_AS_ARG(arg));
+
+    xc_hypercall_buffer_free(handle, arg);
+    return rc;
+}
+
 int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
                              uint16_t view_id, xen_pfn_t gfn,
                              xenmem_access_t access)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 70ddc81d44..dd8e205551 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4358,6 +4358,7 @@ static int do_altp2m_op(
     case HVMOP_altp2m_destroy_p2m:
     case HVMOP_altp2m_switch_p2m:
     case HVMOP_altp2m_set_mem_access:
+    case HVMOP_altp2m_set_suppress_ve:
     case HVMOP_altp2m_change_gfn:
         break;
     default:
@@ -4475,6 +4476,19 @@ static int do_altp2m_op(
                                     a.u.set_mem_access.view);
         break;
 
+    case HVMOP_altp2m_set_suppress_ve:
+        if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 )
+            rc = -EINVAL;
+        else
+        {
+            gfn_t gfn = _gfn(a.u.set_mem_access.gfn);
+            unsigned int altp2m_idx = a.u.set_mem_access.view;
+            bool suppress_ve = a.u.set_suppress_ve.suppress_ve;
+
+            rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx);
+        }
+        break;
+
     case HVMOP_altp2m_change_gfn:
         if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
             rc = -EINVAL;
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index d0b0767855..8c39db13e3 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
 }
 
 /*
+ * Set/clear the #VE suppress bit for a page.  Only available on VMX.
+ */
+int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
+                        unsigned int altp2m_idx)
+{
+    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
+    struct p2m_domain *ap2m = NULL;
+    struct p2m_domain *p2m;
+    mfn_t mfn;
+    p2m_access_t a;
+    p2m_type_t t;
+    int rc;
+
+    if ( !cpu_has_vmx_virt_exceptions )
+        return -EOPNOTSUPP;
+
+    /* This subop should only be used from a privileged domain. */
+    if ( !current->domain->is_privileged )
+        return -EINVAL;
+
+    /* #VE should be enabled for this vcpu. */
+    if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) )
+        return -EINVAL;
+
+    if ( altp2m_idx > 0 )
+    {
+        if ( altp2m_idx >= MAX_ALTP2M ||
+             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
+            return -EINVAL;
+
+        p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx];
+    }
+    else
+        p2m = host_p2m;
+
+    gfn_lock(host_p2m, gfn, 0);
+    if ( ap2m )
+        p2m_lock(ap2m);
+
+    mfn = p2m->get_entry(p2m, gfn_x(gfn), &t, &a, 0, NULL, NULL);
+    if ( !mfn_valid(mfn) )
+        return -ESRCH;
+    rc = p2m->set_entry(p2m, gfn_x(gfn), mfn, PAGE_ORDER_4K, t, a,
+                        suppress_ve);
+    if ( ap2m )
+        p2m_unlock(ap2m);
+    gfn_unlock(host_p2m, gfn, 0);
+
+    return rc;
+}
+
+/*
  * Local variables:
  * mode: C
  * c-file-style: "BSD"
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index 0bdafdf59a..f0b3d8e4d3 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -237,6 +237,18 @@ struct xen_hvm_altp2m_set_mem_access {
 typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
 DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
 
+struct xen_hvm_altp2m_set_suppress_ve {
+    /* view */
+    uint16_t view;
+    uint8_t suppress_ve;
+    uint8_t pad1;
+    uint32_t pad2;
+    /* gfn */
+    uint64_t gfn;
+};
+typedef struct xen_hvm_altp2m_set_suppress_ve xen_hvm_altp2m_set_suppress_ve_t;
+DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_suppress_ve_t);
+
 struct xen_hvm_altp2m_change_gfn {
     /* view */
     uint16_t view;
@@ -268,6 +280,8 @@ struct xen_hvm_altp2m_op {
 #define HVMOP_altp2m_set_mem_access       7
 /* Change a p2m entry to have a different gfn->mfn mapping */
 #define HVMOP_altp2m_change_gfn           8
+/* Set the "Suppress #VE" bit on a page */
+#define HVMOP_altp2m_set_suppress_ve      9
     domid_t domain;
     uint16_t pad1;
     uint32_t pad2;
@@ -276,6 +290,7 @@ struct xen_hvm_altp2m_op {
         struct xen_hvm_altp2m_vcpu_enable_notify enable_notify;
         struct xen_hvm_altp2m_view               view;
         struct xen_hvm_altp2m_set_mem_access     set_mem_access;
+        struct xen_hvm_altp2m_set_suppress_ve    set_suppress_ve;
         struct xen_hvm_altp2m_change_gfn         change_gfn;
         uint8_t pad[64];
     } u;
diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
index 5ab34c1553..0c6717d80f 100644
--- a/xen/include/xen/mem_access.h
+++ b/xen/include/xen/mem_access.h
@@ -78,6 +78,9 @@ long p2m_set_mem_access_multi(struct domain *d,
  */
 int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access);
 
+int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
+                        unsigned int altp2m_idx);
+
 #ifdef CONFIG_HAS_MEM_ACCESS
 int mem_access_memop(unsigned long cmd,
                      XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg);
-- 
2.13.0


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

Re: [PATCH v2 0/2] x86: Add a hvmop for setting the #VE suppress bit

[Adrian Pop]

I've just noticed I had forgotten to update the version of the patch in
the email subject.  Sorry!

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

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

[Wei Liu]

On Fri, Jun 09, 2017 at 07:51:54PM +0300, Adrian Pop wrote:
> Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
> privileged domain to change the value of the #VE suppress bit for a
> page.
> 
> Add a libxc wrapper for invoking this hvmop.
> 
> Signed-off-by: Adrian Pop <apop@bitdefender.com>
> ---
>  tools/libxc/include/xenctrl.h   |  2 ++
>  tools/libxc/xc_altp2m.c         | 24 +++++++++++++++++++
>  xen/arch/x86/hvm/hvm.c          | 14 +++++++++++
>  xen/arch/x86/mm/mem_access.c    | 52 +++++++++++++++++++++++++++++++++++++++++
>  xen/include/public/hvm/hvm_op.h | 15 ++++++++++++
>  xen/include/xen/mem_access.h    |  3 +++
>  6 files changed, 110 insertions(+)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 1629f412dd..f6ba8635bf 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid,
>  /* Switch all vCPUs of the domain to the specified altp2m view */
>  int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>                               uint16_t view_id);
> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
> +                              uint16_t view_id, xen_pfn_t gfn, bool sve);
>  int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>                               uint16_t view_id, xen_pfn_t gfn,
>                               xenmem_access_t access);
> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
> index 0639632477..4710133918 100644
> --- a/tools/libxc/xc_altp2m.c
> +++ b/tools/libxc/xc_altp2m.c
> @@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>      return rc;
>  }
>  
> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
> +                              uint16_t view_id, xen_pfn_t gfn, bool sve)
> +{
> +    int rc;
> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
> +
> +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
> +    if ( arg == NULL )
> +        return -1;
> +
> +    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
> +    arg->cmd = HVMOP_altp2m_set_suppress_ve;
> +    arg->domain = domid;
> +    arg->u.set_suppress_ve.view = view_id;
> +    arg->u.set_suppress_ve.gfn = gfn;
> +    arg->u.set_suppress_ve.suppress_ve = sve;
> +
> +    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
> +		  HYPERCALL_BUFFER_AS_ARG(arg));

Indentation.

With that fixed, the change to libxc looks good:

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

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

[Adrian Pop]

On Mon, Jun 12, 2017 at 04:51:48PM +0100, Wei Liu wrote:
> On Fri, Jun 09, 2017 at 07:51:54PM +0300, Adrian Pop wrote:
> > Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
> > privileged domain to change the value of the #VE suppress bit for a
> > page.
> > 
> > Add a libxc wrapper for invoking this hvmop.
> > 
> > Signed-off-by: Adrian Pop <apop@bitdefender.com>
> > ---
> >  tools/libxc/include/xenctrl.h   |  2 ++
> >  tools/libxc/xc_altp2m.c         | 24 +++++++++++++++++++
> >  xen/arch/x86/hvm/hvm.c          | 14 +++++++++++
> >  xen/arch/x86/mm/mem_access.c    | 52 +++++++++++++++++++++++++++++++++++++++++
> >  xen/include/public/hvm/hvm_op.h | 15 ++++++++++++
> >  xen/include/xen/mem_access.h    |  3 +++
> >  6 files changed, 110 insertions(+)
> > 
> > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> > index 1629f412dd..f6ba8635bf 100644
> > --- a/tools/libxc/include/xenctrl.h
> > +++ b/tools/libxc/include/xenctrl.h
> > @@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid,
> >  /* Switch all vCPUs of the domain to the specified altp2m view */
> >  int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
> >                               uint16_t view_id);
> > +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
> > +                              uint16_t view_id, xen_pfn_t gfn, bool sve);
> >  int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
> >                               uint16_t view_id, xen_pfn_t gfn,
> >                               xenmem_access_t access);
> > diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
> > index 0639632477..4710133918 100644
> > --- a/tools/libxc/xc_altp2m.c
> > +++ b/tools/libxc/xc_altp2m.c
> > @@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
> >      return rc;
> >  }
> >  
> > +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
> > +                              uint16_t view_id, xen_pfn_t gfn, bool sve)
> > +{
> > +    int rc;
> > +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
> > +
> > +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
> > +    if ( arg == NULL )
> > +        return -1;
> > +
> > +    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
> > +    arg->cmd = HVMOP_altp2m_set_suppress_ve;
> > +    arg->domain = domid;
> > +    arg->u.set_suppress_ve.view = view_id;
> > +    arg->u.set_suppress_ve.gfn = gfn;
> > +    arg->u.set_suppress_ve.suppress_ve = sve;
> > +
> > +    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
> > +		  HYPERCALL_BUFFER_AS_ARG(arg));
> 
> Indentation.

Oh, missed that.
 
> With that fixed, the change to libxc looks good:
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Thank you!

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

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

[Tamas K Lengyel]

On Fri, Jun 9, 2017 at 10:51 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.

Could you elaborate on why do you think it is safer and more reliable
to switch the behavior? I believe the original idea was that the
domain should only be allowed to clear an SVE bit set by an external
tool. With this change it will allow the guest to request VE for any
page the external tool hasn't itself reserved specifically.

Tamas

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

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

[Tamas K Lengyel]

On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote:
> Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
> privileged domain to change the value of the #VE suppress bit for a
> page.
>
> Add a libxc wrapper for invoking this hvmop.
>
> Signed-off-by: Adrian Pop <apop@bitdefender.com>
> ---
>  tools/libxc/include/xenctrl.h   |  2 ++
>  tools/libxc/xc_altp2m.c         | 24 +++++++++++++++++++
>  xen/arch/x86/hvm/hvm.c          | 14 +++++++++++
>  xen/arch/x86/mm/mem_access.c    | 52 +++++++++++++++++++++++++++++++++++++++++
>  xen/include/public/hvm/hvm_op.h | 15 ++++++++++++
>  xen/include/xen/mem_access.h    |  3 +++
>  6 files changed, 110 insertions(+)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 1629f412dd..f6ba8635bf 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid,
>  /* Switch all vCPUs of the domain to the specified altp2m view */
>  int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>                               uint16_t view_id);
> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
> +                              uint16_t view_id, xen_pfn_t gfn, bool sve);
>  int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>                               uint16_t view_id, xen_pfn_t gfn,
>                               xenmem_access_t access);
> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
> index 0639632477..4710133918 100644
> --- a/tools/libxc/xc_altp2m.c
> +++ b/tools/libxc/xc_altp2m.c
> @@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>      return rc;
>  }
>
> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
> +                              uint16_t view_id, xen_pfn_t gfn, bool sve)
> +{
> +    int rc;
> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
> +
> +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
> +    if ( arg == NULL )
> +        return -1;
> +
> +    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
> +    arg->cmd = HVMOP_altp2m_set_suppress_ve;
> +    arg->domain = domid;
> +    arg->u.set_suppress_ve.view = view_id;
> +    arg->u.set_suppress_ve.gfn = gfn;
> +    arg->u.set_suppress_ve.suppress_ve = sve;
> +
> +    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
> +                 HYPERCALL_BUFFER_AS_ARG(arg));
> +
> +    xc_hypercall_buffer_free(handle, arg);
> +    return rc;
> +}
> +
>  int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>                               uint16_t view_id, xen_pfn_t gfn,
>                               xenmem_access_t access)
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 70ddc81d44..dd8e205551 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4358,6 +4358,7 @@ static int do_altp2m_op(
>      case HVMOP_altp2m_destroy_p2m:
>      case HVMOP_altp2m_switch_p2m:
>      case HVMOP_altp2m_set_mem_access:
> +    case HVMOP_altp2m_set_suppress_ve:
>      case HVMOP_altp2m_change_gfn:
>          break;
>      default:
> @@ -4475,6 +4476,19 @@ static int do_altp2m_op(
>                                      a.u.set_mem_access.view);
>          break;
>
> +    case HVMOP_altp2m_set_suppress_ve:
> +        if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 )
> +            rc = -EINVAL;
> +        else
> +        {
> +            gfn_t gfn = _gfn(a.u.set_mem_access.gfn);
> +            unsigned int altp2m_idx = a.u.set_mem_access.view;
> +            bool suppress_ve = a.u.set_suppress_ve.suppress_ve;
> +
> +            rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx);
> +        }
> +        break;
> +
>      case HVMOP_altp2m_change_gfn:
>          if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
>              rc = -EINVAL;
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index d0b0767855..8c39db13e3 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
>  }
>
>  /*
> + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
> + */
> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
> +                        unsigned int altp2m_idx)
> +{
> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *ap2m = NULL;
> +    struct p2m_domain *p2m;
> +    mfn_t mfn;
> +    p2m_access_t a;
> +    p2m_type_t t;
> +    int rc;
> +
> +    if ( !cpu_has_vmx_virt_exceptions )
> +        return -EOPNOTSUPP;
> +
> +    /* This subop should only be used from a privileged domain. */
> +    if ( !current->domain->is_privileged )
> +        return -EINVAL;

This check looks wrong to me. If this subop should only be used by an
external (privileged) domain then I don't think this should be
implemented as an HVMOP, looks more like a domctl to me.

Tamas

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

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

[Jan Beulich]

>>> On 15.06.17 at 21:01, <tamas@tklengyel.com> wrote:
> On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote:
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
>>  }
>>
>>  /*
>> + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
>> + */
>> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
>> +                        unsigned int altp2m_idx)
>> +{
>> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
>> +    struct p2m_domain *ap2m = NULL;
>> +    struct p2m_domain *p2m;
>> +    mfn_t mfn;
>> +    p2m_access_t a;
>> +    p2m_type_t t;
>> +    int rc;
>> +
>> +    if ( !cpu_has_vmx_virt_exceptions )
>> +        return -EOPNOTSUPP;
>> +
>> +    /* This subop should only be used from a privileged domain. */
>> +    if ( !current->domain->is_privileged )
>> +        return -EINVAL;
> 
> This check looks wrong to me. If this subop should only be used by an
> external (privileged) domain then I don't think this should be
> implemented as an HVMOP, looks more like a domctl to me.

I think this wants to be an XSM_DM_PRIV check instead.

Jan


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

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

[Jan Beulich]

>>> On 09.06.17 at 18:51, <apop@bitdefender.com> wrote:
> Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
> privileged domain to change the value of the #VE suppress bit for a
> page.
> 
> Add a libxc wrapper for invoking this hvmop.
> 
> Signed-off-by: Adrian Pop <apop@bitdefender.com>
> ---

Please properly version your patch submissions, and please put
here a brief summary of what changed from the previous version.

> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
>  }
>  
>  /*
> + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
> + */
> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
> +                        unsigned int altp2m_idx)
> +{
> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *ap2m = NULL;
> +    struct p2m_domain *p2m;
> +    mfn_t mfn;
> +    p2m_access_t a;
> +    p2m_type_t t;
> +    int rc;
> +
> +    if ( !cpu_has_vmx_virt_exceptions )
> +        return -EOPNOTSUPP;
> +
> +    /* This subop should only be used from a privileged domain. */
> +    if ( !current->domain->is_privileged )
> +        return -EINVAL;

Beyond the question of what check to use, perhaps -EPERM?

> +    /* #VE should be enabled for this vcpu. */
> +    if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) )
> +        return -EINVAL;

This also doesn't really is an invalid argument error - perhaps e.g.
-ENXIO or -ENOENT? Be creative, but don't use -EINVAL for
everything.

> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -237,6 +237,18 @@ struct xen_hvm_altp2m_set_mem_access {
>  typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
>  
> +struct xen_hvm_altp2m_set_suppress_ve {
> +    /* view */
> +    uint16_t view;
> +    uint8_t suppress_ve;
> +    uint8_t pad1;
> +    uint32_t pad2;
> +    /* gfn */
> +    uint64_t gfn;

Commenting fields with their field names is, I'm sorry, rather pointless.
What gfn means is most likely clear without comment. For view I'm not
sure (depends on conventions elsewhere), but the boolean nature of
suppress_ve clearly wants commenting on (especially also to clarify
behavior of values other than 0 and 1).

> +};
> +typedef struct xen_hvm_altp2m_set_suppress_ve xen_hvm_altp2m_set_suppress_ve_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_suppress_ve_t);

I think we should stop the habit of creating such typedefs and handles
when ...

> @@ -276,6 +290,7 @@ struct xen_hvm_altp2m_op {
>          struct xen_hvm_altp2m_vcpu_enable_notify enable_notify;
>          struct xen_hvm_altp2m_view               view;
>          struct xen_hvm_altp2m_set_mem_access     set_mem_access;
> +        struct xen_hvm_altp2m_set_suppress_ve    set_suppress_ve;

... a structure isn't meant to be used on its own anyway.

Jan


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

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

[Adrian Pop]

On Fri, Jun 16, 2017 at 09:29:49AM -0600, Jan Beulich wrote:
> >>> On 09.06.17 at 18:51, <apop@bitdefender.com> wrote:
> > Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
> > privileged domain to change the value of the #VE suppress bit for a
> > page.
> > 
> > Add a libxc wrapper for invoking this hvmop.
> > 
> > Signed-off-by: Adrian Pop <apop@bitdefender.com>
> > ---
> 
> Please properly version your patch submissions, and please put
> here a brief summary of what changed from the previous version.
 
OK.  I've mistakenly sent the mail without setting the patch version.
I've written the change list in the cover letter, but in hindsight it
would have been a better idea to add the list of changes per patch
instead.

> > --- a/xen/arch/x86/mm/mem_access.c
> > +++ b/xen/arch/x86/mm/mem_access.c
> > @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
> >  }
> >  
> >  /*
> > + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
> > + */
> > +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
> > +                        unsigned int altp2m_idx)
> > +{
> > +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> > +    struct p2m_domain *ap2m = NULL;
> > +    struct p2m_domain *p2m;
> > +    mfn_t mfn;
> > +    p2m_access_t a;
> > +    p2m_type_t t;
> > +    int rc;
> > +
> > +    if ( !cpu_has_vmx_virt_exceptions )
> > +        return -EOPNOTSUPP;
> > +
> > +    /* This subop should only be used from a privileged domain. */
> > +    if ( !current->domain->is_privileged )
> > +        return -EINVAL;
> 
> Beyond the question of what check to use, perhaps -EPERM?
 
OK.

> > +    /* #VE should be enabled for this vcpu. */
> > +    if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) )
> > +        return -EINVAL;
> 
> This also doesn't really is an invalid argument error - perhaps e.g.
> -ENXIO or -ENOENT? Be creative, but don't use -EINVAL for
> everything.

All right.

> > --- a/xen/include/public/hvm/hvm_op.h
> > +++ b/xen/include/public/hvm/hvm_op.h
> > @@ -237,6 +237,18 @@ struct xen_hvm_altp2m_set_mem_access {
> >  typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
> >  
> > +struct xen_hvm_altp2m_set_suppress_ve {
> > +    /* view */
> > +    uint16_t view;
> > +    uint8_t suppress_ve;
> > +    uint8_t pad1;
> > +    uint32_t pad2;
> > +    /* gfn */
> > +    uint64_t gfn;
> 
> Commenting fields with their field names is, I'm sorry, rather pointless.
> What gfn means is most likely clear without comment. For view I'm not
> sure (depends on conventions elsewhere), but the boolean nature of
> suppress_ve clearly wants commenting on (especially also to clarify
> behavior of values other than 0 and 1).

OK then.

> > +};
> > +typedef struct xen_hvm_altp2m_set_suppress_ve xen_hvm_altp2m_set_suppress_ve_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_suppress_ve_t);
> 
> I think we should stop the habit of creating such typedefs and handles
> when ...
> 
> > @@ -276,6 +290,7 @@ struct xen_hvm_altp2m_op {
> >          struct xen_hvm_altp2m_vcpu_enable_notify enable_notify;
> >          struct xen_hvm_altp2m_view               view;
> >          struct xen_hvm_altp2m_set_mem_access     set_mem_access;
> > +        struct xen_hvm_altp2m_set_suppress_ve    set_suppress_ve;
> 
> ... a structure isn't meant to be used on its own anyway.
 
Yes, I agree with that.

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

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

[Adrian Pop]

On Mon, Jun 12, 2017 at 04:51:48PM +0100, Wei Liu wrote:
> On Fri, Jun 09, 2017 at 07:51:54PM +0300, Adrian Pop wrote:
> > Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
> > privileged domain to change the value of the #VE suppress bit for a
> > page.
> > 
> > Add a libxc wrapper for invoking this hvmop.
> > 
> > Signed-off-by: Adrian Pop <apop@bitdefender.com>
> > ---
> >  tools/libxc/include/xenctrl.h   |  2 ++
> >  tools/libxc/xc_altp2m.c         | 24 +++++++++++++++++++
> >  xen/arch/x86/hvm/hvm.c          | 14 +++++++++++
> >  xen/arch/x86/mm/mem_access.c    | 52 +++++++++++++++++++++++++++++++++++++++++
> >  xen/include/public/hvm/hvm_op.h | 15 ++++++++++++
> >  xen/include/xen/mem_access.h    |  3 +++
> >  6 files changed, 110 insertions(+)
> > 
> > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> > index 1629f412dd..f6ba8635bf 100644
> > --- a/tools/libxc/include/xenctrl.h
> > +++ b/tools/libxc/include/xenctrl.h
> > @@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid,
> >  /* Switch all vCPUs of the domain to the specified altp2m view */
> >  int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
> >                               uint16_t view_id);
> > +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
> > +                              uint16_t view_id, xen_pfn_t gfn, bool sve);
> >  int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
> >                               uint16_t view_id, xen_pfn_t gfn,
> >                               xenmem_access_t access);
> > diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
> > index 0639632477..4710133918 100644
> > --- a/tools/libxc/xc_altp2m.c
> > +++ b/tools/libxc/xc_altp2m.c
> > @@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
> >      return rc;
> >  }
> >  
> > +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
> > +                              uint16_t view_id, xen_pfn_t gfn, bool sve)
> > +{
> > +    int rc;
> > +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
> > +
> > +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
> > +    if ( arg == NULL )
> > +        return -1;
> > +
> > +    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
> > +    arg->cmd = HVMOP_altp2m_set_suppress_ve;
> > +    arg->domain = domid;
> > +    arg->u.set_suppress_ve.view = view_id;
> > +    arg->u.set_suppress_ve.gfn = gfn;
> > +    arg->u.set_suppress_ve.suppress_ve = sve;
> > +
> > +    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
> > +		  HYPERCALL_BUFFER_AS_ARG(arg));
> 
> Indentation.

OK.  Thanks!

> With that fixed, the change to libxc looks good:
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

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

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

[Adrian Pop]

On Fri, Jun 16, 2017 at 02:39:10AM -0600, Jan Beulich wrote:
> >>> On 15.06.17 at 21:01, <tamas@tklengyel.com> wrote:
> > On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote:
> >> --- a/xen/arch/x86/mm/mem_access.c
> >> +++ b/xen/arch/x86/mm/mem_access.c
> >> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
> >>  }
> >>
> >>  /*
> >> + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
> >> + */
> >> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
> >> +                        unsigned int altp2m_idx)
> >> +{
> >> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> >> +    struct p2m_domain *ap2m = NULL;
> >> +    struct p2m_domain *p2m;
> >> +    mfn_t mfn;
> >> +    p2m_access_t a;
> >> +    p2m_type_t t;
> >> +    int rc;
> >> +
> >> +    if ( !cpu_has_vmx_virt_exceptions )
> >> +        return -EOPNOTSUPP;
> >> +
> >> +    /* This subop should only be used from a privileged domain. */
> >> +    if ( !current->domain->is_privileged )
> >> +        return -EINVAL;
> > 
> > This check looks wrong to me. If this subop should only be used by an
> > external (privileged) domain then I don't think this should be
> > implemented as an HVMOP, looks more like a domctl to me.
> 
> I think this wants to be an XSM_DM_PRIV check instead.

I'm not sure, but I expect that to not behave as intended security-wise
if Xen is compiled without XSM.  Would it?  It would be great if this
feature worked well without XSM too.

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

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

[Jan Beulich]

>>> On 22.06.17 at 14:04, <apop@bitdefender.com> wrote:
> On Fri, Jun 16, 2017 at 02:39:10AM -0600, Jan Beulich wrote:
>> >>> On 15.06.17 at 21:01, <tamas@tklengyel.com> wrote:
>> > On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote:
>> >> --- a/xen/arch/x86/mm/mem_access.c
>> >> +++ b/xen/arch/x86/mm/mem_access.c
>> >> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, 
> xenmem_access_t *access)
>> >>  }
>> >>
>> >>  /*
>> >> + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
>> >> + */
>> >> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
>> >> +                        unsigned int altp2m_idx)
>> >> +{
>> >> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
>> >> +    struct p2m_domain *ap2m = NULL;
>> >> +    struct p2m_domain *p2m;
>> >> +    mfn_t mfn;
>> >> +    p2m_access_t a;
>> >> +    p2m_type_t t;
>> >> +    int rc;
>> >> +
>> >> +    if ( !cpu_has_vmx_virt_exceptions )
>> >> +        return -EOPNOTSUPP;
>> >> +
>> >> +    /* This subop should only be used from a privileged domain. */
>> >> +    if ( !current->domain->is_privileged )
>> >> +        return -EINVAL;
>> > 
>> > This check looks wrong to me. If this subop should only be used by an
>> > external (privileged) domain then I don't think this should be
>> > implemented as an HVMOP, looks more like a domctl to me.
>> 
>> I think this wants to be an XSM_DM_PRIV check instead.
> 
> I'm not sure, but I expect that to not behave as intended security-wise
> if Xen is compiled without XSM.  Would it?  It would be great if this
> feature worked well without XSM too.

Well, without you explaining why you think this wouldn't work
without XSM, I don't really know what to answer. I suppose
you've grep-ed for other uses of this and/or other XSM_* values,
finding that these exist in various places where all is fine without
XSM?

Jan


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

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

[Adrian Pop]

On Thu, Jun 22, 2017 at 06:13:22AM -0600, Jan Beulich wrote:
> >>> On 22.06.17 at 14:04, <apop@bitdefender.com> wrote:
> > On Fri, Jun 16, 2017 at 02:39:10AM -0600, Jan Beulich wrote:
> >> >>> On 15.06.17 at 21:01, <tamas@tklengyel.com> wrote:
> >> > On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote:
> >> >> --- a/xen/arch/x86/mm/mem_access.c
> >> >> +++ b/xen/arch/x86/mm/mem_access.c
> >> >> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, 
> > xenmem_access_t *access)
> >> >>  }
> >> >>
> >> >>  /*
> >> >> + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
> >> >> + */
> >> >> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
> >> >> +                        unsigned int altp2m_idx)
> >> >> +{
> >> >> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> >> >> +    struct p2m_domain *ap2m = NULL;
> >> >> +    struct p2m_domain *p2m;
> >> >> +    mfn_t mfn;
> >> >> +    p2m_access_t a;
> >> >> +    p2m_type_t t;
> >> >> +    int rc;
> >> >> +
> >> >> +    if ( !cpu_has_vmx_virt_exceptions )
> >> >> +        return -EOPNOTSUPP;
> >> >> +
> >> >> +    /* This subop should only be used from a privileged domain. */
> >> >> +    if ( !current->domain->is_privileged )
> >> >> +        return -EINVAL;
> >> > 
> >> > This check looks wrong to me. If this subop should only be used by an
> >> > external (privileged) domain then I don't think this should be
> >> > implemented as an HVMOP, looks more like a domctl to me.
> >> 
> >> I think this wants to be an XSM_DM_PRIV check instead.
> > 
> > I'm not sure, but I expect that to not behave as intended security-wise
> > if Xen is compiled without XSM.  Would it?  It would be great if this
> > feature worked well without XSM too.
> 
> Well, without you explaining why you think this wouldn't work
> without XSM, I don't really know what to answer. I suppose
> you've grep-ed for other uses of this and/or other XSM_* values,
> finding that these exist in various places where all is fine without
> XSM?

I'll check what it does then because it's not very clear to me either.

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

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

[Adrian Pop]

On Thu, Jun 15, 2017 at 01:01:36PM -0600, Tamas K Lengyel wrote:
> On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote:
> > diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> > index d0b0767855..8c39db13e3 100644
> > --- a/xen/arch/x86/mm/mem_access.c
> > +++ b/xen/arch/x86/mm/mem_access.c
> > @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
> >  }
> >
> >  /*
> > + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
> > + */
> > +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
> > +                        unsigned int altp2m_idx)
> > +{
> > +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> > +    struct p2m_domain *ap2m = NULL;
> > +    struct p2m_domain *p2m;
> > +    mfn_t mfn;
> > +    p2m_access_t a;
> > +    p2m_type_t t;
> > +    int rc;
> > +
> > +    if ( !cpu_has_vmx_virt_exceptions )
> > +        return -EOPNOTSUPP;
> > +
> > +    /* This subop should only be used from a privileged domain. */
> > +    if ( !current->domain->is_privileged )
> > +        return -EINVAL;
> 
> This check looks wrong to me. If this subop should only be used by an
> external (privileged) domain then I don't think this should be
> implemented as an HVMOP, looks more like a domctl to me.

AFAICS this could indeed be implemented as a domctl as well.

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

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

[Adrian Pop]

On Thu, Jun 22, 2017 at 06:13:22AM -0600, Jan Beulich wrote:
> >>> On 22.06.17 at 14:04, <apop@bitdefender.com> wrote:
> > On Fri, Jun 16, 2017 at 02:39:10AM -0600, Jan Beulich wrote:
> >> >>> On 15.06.17 at 21:01, <tamas@tklengyel.com> wrote:
> >> > On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote:
> >> >> --- a/xen/arch/x86/mm/mem_access.c
> >> >> +++ b/xen/arch/x86/mm/mem_access.c
> >> >> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, 
> > xenmem_access_t *access)
> >> >>  }
> >> >>
> >> >>  /*
> >> >> + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
> >> >> + */
> >> >> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
> >> >> +                        unsigned int altp2m_idx)
> >> >> +{
> >> >> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> >> >> +    struct p2m_domain *ap2m = NULL;
> >> >> +    struct p2m_domain *p2m;
> >> >> +    mfn_t mfn;
> >> >> +    p2m_access_t a;
> >> >> +    p2m_type_t t;
> >> >> +    int rc;
> >> >> +
> >> >> +    if ( !cpu_has_vmx_virt_exceptions )
> >> >> +        return -EOPNOTSUPP;
> >> >> +
> >> >> +    /* This subop should only be used from a privileged domain. */
> >> >> +    if ( !current->domain->is_privileged )
> >> >> +        return -EINVAL;
> >> > 
> >> > This check looks wrong to me. If this subop should only be used by an
> >> > external (privileged) domain then I don't think this should be
> >> > implemented as an HVMOP, looks more like a domctl to me.
> >> 
> >> I think this wants to be an XSM_DM_PRIV check instead.
> > 
> > I'm not sure, but I expect that to not behave as intended security-wise
> > if Xen is compiled without XSM.  Would it?  It would be great if this
> > feature worked well without XSM too.
> 
> Well, without you explaining why you think this wouldn't work
> without XSM, I don't really know what to answer. I suppose
> you've grep-ed for other uses of this and/or other XSM_* values,
> finding that these exist in various places where all is fine without
> XSM?

OK; it indeed does what it should without XSM as well.

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

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

[George Dunlap]

On Thu, Jun 15, 2017 at 7:49 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
> On Fri, Jun 9, 2017 at 10:51 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.
>
> Could you elaborate on why do you think it is safer and more reliable
> to switch the behavior? I believe the original idea was that the
> domain should only be allowed to clear an SVE bit set by an external
> tool. With this change it will allow the guest to request VE for any
> page the external tool hasn't itself reserved specifically.

Hmm?  This patch by itself simply prevents the guest from changing the
VE bit at all (either setting or clearing it).

Or did you mean, "This patch series"?

 -George

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

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

[George Dunlap]

On Thu, Jun 15, 2017 at 8:01 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
> On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote:
>> Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
>> privileged domain to change the value of the #VE suppress bit for a
>> page.
>>
>> Add a libxc wrapper for invoking this hvmop.
>>
>> Signed-off-by: Adrian Pop <apop@bitdefender.com>
>> ---
>>  tools/libxc/include/xenctrl.h   |  2 ++
>>  tools/libxc/xc_altp2m.c         | 24 +++++++++++++++++++
>>  xen/arch/x86/hvm/hvm.c          | 14 +++++++++++
>>  xen/arch/x86/mm/mem_access.c    | 52 +++++++++++++++++++++++++++++++++++++++++
>>  xen/include/public/hvm/hvm_op.h | 15 ++++++++++++
>>  xen/include/xen/mem_access.h    |  3 +++
>>  6 files changed, 110 insertions(+)
>>
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index 1629f412dd..f6ba8635bf 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid,
>>  /* Switch all vCPUs of the domain to the specified altp2m view */
>>  int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>>                               uint16_t view_id);
>> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
>> +                              uint16_t view_id, xen_pfn_t gfn, bool sve);
>>  int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>                               uint16_t view_id, xen_pfn_t gfn,
>>                               xenmem_access_t access);
>> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
>> index 0639632477..4710133918 100644
>> --- a/tools/libxc/xc_altp2m.c
>> +++ b/tools/libxc/xc_altp2m.c
>> @@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>>      return rc;
>>  }
>>
>> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
>> +                              uint16_t view_id, xen_pfn_t gfn, bool sve)
>> +{
>> +    int rc;
>> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
>> +
>> +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
>> +    if ( arg == NULL )
>> +        return -1;
>> +
>> +    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
>> +    arg->cmd = HVMOP_altp2m_set_suppress_ve;
>> +    arg->domain = domid;
>> +    arg->u.set_suppress_ve.view = view_id;
>> +    arg->u.set_suppress_ve.gfn = gfn;
>> +    arg->u.set_suppress_ve.suppress_ve = sve;
>> +
>> +    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
>> +                 HYPERCALL_BUFFER_AS_ARG(arg));
>> +
>> +    xc_hypercall_buffer_free(handle, arg);
>> +    return rc;
>> +}
>> +
>>  int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>                               uint16_t view_id, xen_pfn_t gfn,
>>                               xenmem_access_t access)
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 70ddc81d44..dd8e205551 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4358,6 +4358,7 @@ static int do_altp2m_op(
>>      case HVMOP_altp2m_destroy_p2m:
>>      case HVMOP_altp2m_switch_p2m:
>>      case HVMOP_altp2m_set_mem_access:
>> +    case HVMOP_altp2m_set_suppress_ve:
>>      case HVMOP_altp2m_change_gfn:
>>          break;
>>      default:
>> @@ -4475,6 +4476,19 @@ static int do_altp2m_op(
>>                                      a.u.set_mem_access.view);
>>          break;
>>
>> +    case HVMOP_altp2m_set_suppress_ve:
>> +        if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 )
>> +            rc = -EINVAL;
>> +        else
>> +        {
>> +            gfn_t gfn = _gfn(a.u.set_mem_access.gfn);
>> +            unsigned int altp2m_idx = a.u.set_mem_access.view;
>> +            bool suppress_ve = a.u.set_suppress_ve.suppress_ve;
>> +
>> +            rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx);
>> +        }
>> +        break;
>> +
>>      case HVMOP_altp2m_change_gfn:
>>          if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
>>              rc = -EINVAL;
>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>> index d0b0767855..8c39db13e3 100644
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
>>  }
>>
>>  /*
>> + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
>> + */
>> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
>> +                        unsigned int altp2m_idx)
>> +{
>> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
>> +    struct p2m_domain *ap2m = NULL;
>> +    struct p2m_domain *p2m;
>> +    mfn_t mfn;
>> +    p2m_access_t a;
>> +    p2m_type_t t;
>> +    int rc;
>> +
>> +    if ( !cpu_has_vmx_virt_exceptions )
>> +        return -EOPNOTSUPP;
>> +
>> +    /* This subop should only be used from a privileged domain. */
>> +    if ( !current->domain->is_privileged )
>> +        return -EINVAL;
>
> This check looks wrong to me. If this subop should only be used by an
> external (privileged) domain then I don't think this should be
> implemented as an HVMOP, looks more like a domctl to me.

Well after patch 1, isn't it the case that a guest has no way of
clearing the suppress_ve bit?

I was going to say we want the XSM_TARGET "default action" (which
allows a guest to do things on itself, or a privileged domain to do it
to any domain); but I think really we probably we don't want a guest
to be able to *clear* the suppress_ve bit on a page for which a
privileged domain has *set*; this would allow a domain to prevent the
other domain from effectively introspecting on a page.

This is starting to sound like another conversation I think I remember
recently about making sure that *only* the guest *or* an introspection
engine can use the altp2m functionality, but I can't seem to find that
with a quick look. Tamas, does that ring any bells?

 -George

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

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

[Tamas K Lengyel]

On Thu, Jul 20, 2017 at 8:38 AM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> On Thu, Jun 15, 2017 at 7:49 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
>> On Fri, Jun 9, 2017 at 10:51 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.
>>
>> Could you elaborate on why do you think it is safer and more reliable
>> to switch the behavior? I believe the original idea was that the
>> domain should only be allowed to clear an SVE bit set by an external
>> tool. With this change it will allow the guest to request VE for any
>> page the external tool hasn't itself reserved specifically.
>
> Hmm?  This patch by itself simply prevents the guest from changing the
> VE bit at all (either setting or clearing it).
>
> Or did you mean, "This patch series"?

No, technically the other patch is fine by itself. It can only be used
to set the SVE bit from a privileged domain, but by itself that is
fine. Only this patch is problematic if we want to allow a setup where
there is only an in-guest tool without a corresponding vm_event
mem_access listener.

Tamas

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

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

[Tamas K Lengyel]

On Thu, Jul 20, 2017 at 9:11 AM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> On Thu, Jun 15, 2017 at 8:01 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
>> On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@bitdefender.com> wrote:
>>> Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
>>> privileged domain to change the value of the #VE suppress bit for a
>>> page.
>>>
>>> Add a libxc wrapper for invoking this hvmop.
>>>
>>> Signed-off-by: Adrian Pop <apop@bitdefender.com>
>>> ---
>>>  tools/libxc/include/xenctrl.h   |  2 ++
>>>  tools/libxc/xc_altp2m.c         | 24 +++++++++++++++++++
>>>  xen/arch/x86/hvm/hvm.c          | 14 +++++++++++
>>>  xen/arch/x86/mm/mem_access.c    | 52 +++++++++++++++++++++++++++++++++++++++++
>>>  xen/include/public/hvm/hvm_op.h | 15 ++++++++++++
>>>  xen/include/xen/mem_access.h    |  3 +++
>>>  6 files changed, 110 insertions(+)
>>>
>>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>>> index 1629f412dd..f6ba8635bf 100644
>>> --- a/tools/libxc/include/xenctrl.h
>>> +++ b/tools/libxc/include/xenctrl.h
>>> @@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid,
>>>  /* Switch all vCPUs of the domain to the specified altp2m view */
>>>  int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>>>                               uint16_t view_id);
>>> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
>>> +                              uint16_t view_id, xen_pfn_t gfn, bool sve);
>>>  int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>>                               uint16_t view_id, xen_pfn_t gfn,
>>>                               xenmem_access_t access);
>>> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
>>> index 0639632477..4710133918 100644
>>> --- a/tools/libxc/xc_altp2m.c
>>> +++ b/tools/libxc/xc_altp2m.c
>>> @@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>>>      return rc;
>>>  }
>>>
>>> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
>>> +                              uint16_t view_id, xen_pfn_t gfn, bool sve)
>>> +{
>>> +    int rc;
>>> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
>>> +
>>> +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
>>> +    if ( arg == NULL )
>>> +        return -1;
>>> +
>>> +    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
>>> +    arg->cmd = HVMOP_altp2m_set_suppress_ve;
>>> +    arg->domain = domid;
>>> +    arg->u.set_suppress_ve.view = view_id;
>>> +    arg->u.set_suppress_ve.gfn = gfn;
>>> +    arg->u.set_suppress_ve.suppress_ve = sve;
>>> +
>>> +    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
>>> +                 HYPERCALL_BUFFER_AS_ARG(arg));
>>> +
>>> +    xc_hypercall_buffer_free(handle, arg);
>>> +    return rc;
>>> +}
>>> +
>>>  int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>>                               uint16_t view_id, xen_pfn_t gfn,
>>>                               xenmem_access_t access)
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index 70ddc81d44..dd8e205551 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -4358,6 +4358,7 @@ static int do_altp2m_op(
>>>      case HVMOP_altp2m_destroy_p2m:
>>>      case HVMOP_altp2m_switch_p2m:
>>>      case HVMOP_altp2m_set_mem_access:
>>> +    case HVMOP_altp2m_set_suppress_ve:
>>>      case HVMOP_altp2m_change_gfn:
>>>          break;
>>>      default:
>>> @@ -4475,6 +4476,19 @@ static int do_altp2m_op(
>>>                                      a.u.set_mem_access.view);
>>>          break;
>>>
>>> +    case HVMOP_altp2m_set_suppress_ve:
>>> +        if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 )
>>> +            rc = -EINVAL;
>>> +        else
>>> +        {
>>> +            gfn_t gfn = _gfn(a.u.set_mem_access.gfn);
>>> +            unsigned int altp2m_idx = a.u.set_mem_access.view;
>>> +            bool suppress_ve = a.u.set_suppress_ve.suppress_ve;
>>> +
>>> +            rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx);
>>> +        }
>>> +        break;
>>> +
>>>      case HVMOP_altp2m_change_gfn:
>>>          if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
>>>              rc = -EINVAL;
>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>>> index d0b0767855..8c39db13e3 100644
>>> --- a/xen/arch/x86/mm/mem_access.c
>>> +++ b/xen/arch/x86/mm/mem_access.c
>>> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
>>>  }
>>>
>>>  /*
>>> + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
>>> + */
>>> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
>>> +                        unsigned int altp2m_idx)
>>> +{
>>> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
>>> +    struct p2m_domain *ap2m = NULL;
>>> +    struct p2m_domain *p2m;
>>> +    mfn_t mfn;
>>> +    p2m_access_t a;
>>> +    p2m_type_t t;
>>> +    int rc;
>>> +
>>> +    if ( !cpu_has_vmx_virt_exceptions )
>>> +        return -EOPNOTSUPP;
>>> +
>>> +    /* This subop should only be used from a privileged domain. */
>>> +    if ( !current->domain->is_privileged )
>>> +        return -EINVAL;
>>
>> This check looks wrong to me. If this subop should only be used by an
>> external (privileged) domain then I don't think this should be
>> implemented as an HVMOP, looks more like a domctl to me.
>
> Well after patch 1, isn't it the case that a guest has no way of
> clearing the suppress_ve bit?
>
> I was going to say we want the XSM_TARGET "default action" (which
> allows a guest to do things on itself, or a privileged domain to do it
> to any domain); but I think really we probably we don't want a guest
> to be able to *clear* the suppress_ve bit on a page for which a
> privileged domain has *set*; this would allow a domain to prevent the
> other domain from effectively introspecting on a page.

That sounds right, that would be a scenario that would probably have
to be avoided. That said, it becomes quite complex if we want to have
two entities having access to altp2m, one external and one in-guest. I
don't think that setup is something that was considered when altp2m
was introduced.

>
> This is starting to sound like another conversation I think I remember
> recently about making sure that *only* the guest *or* an introspection
> engine can use the altp2m functionality, but I can't seem to find that
> with a quick look. Tamas, does that ring any bells?

You may be thinking of the discussions regarding the externel_only
mode for altp2m I've added in
https://lists.xen.org/archives/html/xen-devel/2017-04/msg00373.html.

Tamas

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