All of lore.kernel.org
 help / color / mirror / Atom feed
* [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
@ 2019-11-06 15:35 Alexandru Stefan ISAILA
  2019-11-06 15:35 ` [Xen-devel] [PATCH V2 2/2] x86/mm: Make use of the default access param from xc_altp2m_create_view Alexandru Stefan ISAILA
                   ` (3 more replies)
  0 siblings, 4 replies; 25+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-11-06 15:35 UTC (permalink / raw)
  To: xen-devel
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, Razvan COJOCARU, wl,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson, tamas,
	jbeulich, Alexandru Stefan ISAILA, roger.pau

By default the sve bits are not set.
This patch adds a new hypercall, xc_altp2m_set_supress_ve_multi(),
to set a range of sve bits.
The core function, p2m_set_suppress_ve_multi(), does not brake in case
of a error and it is doing a best effort for setting the bits in the
given range. A check for continuation is made in order to have
preemption on big ranges.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>

---
Changes since V1:
	- Remove "continue"
	- Add a new field in xen_hvm_altp2m_suppress_ve to store the
continuation value
	- Have p2m_set_suppress_ve_multi() take
xen_hvm_altp2m_suppress_ve as a param.
---
 tools/libxc/include/xenctrl.h   |  3 ++
 tools/libxc/xc_altp2m.c         | 25 ++++++++++++++
 xen/arch/x86/hvm/hvm.c          | 20 ++++++++++--
 xen/arch/x86/mm/p2m.c           | 58 +++++++++++++++++++++++++++++++++
 xen/include/public/hvm/hvm_op.h |  5 ++-
 xen/include/xen/mem_access.h    |  3 ++
 6 files changed, 111 insertions(+), 3 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index f4431687b3..21b644f459 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1923,6 +1923,9 @@ 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_supress_ve_multi(xc_interface *handle, uint32_t domid,
+                                   uint16_t view_id, xen_pfn_t start_gfn,
+                                   uint32_t nr, 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,
diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index 09dad0355e..6605d9abbe 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -234,6 +234,31 @@ int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
     return rc;
 }
 
+int xc_altp2m_set_supress_ve_multi(xc_interface *handle, uint32_t domid,
+                                   uint16_t view_id, xen_pfn_t start_gfn,
+                                   uint32_t nr, 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_multi;
+    arg->domain = domid;
+    arg->u.suppress_ve.view = view_id;
+    arg->u.suppress_ve.gfn = start_gfn;
+    arg->u.suppress_ve.suppress_ve = sve;
+    arg->u.suppress_ve.nr = nr;
+
+    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 06a7b40107..66ed8b8e3e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4535,6 +4535,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_set_suppress_ve_multi:
     case HVMOP_altp2m_get_suppress_ve:
     case HVMOP_altp2m_set_mem_access:
     case HVMOP_altp2m_set_mem_access_multi:
@@ -4681,7 +4682,7 @@ static int do_altp2m_op(
         break;
 
     case HVMOP_altp2m_set_suppress_ve:
-        if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
+        if ( a.u.suppress_ve.pad1 )
             rc = -EINVAL;
         else
         {
@@ -4693,8 +4694,23 @@ static int do_altp2m_op(
         }
         break;
 
+    case HVMOP_altp2m_set_suppress_ve_multi:
+        if ( a.u.suppress_ve.pad1 || !a.u.suppress_ve.nr )
+            rc = -EINVAL;
+        else
+        {
+            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve);
+
+            if ( rc == -ERESTART )
+                if ( __copy_field_to_guest(guest_handle_cast(arg,
+                                           xen_hvm_altp2m_op_t),
+                                           &a, u.suppress_ve.opaque) )
+                    rc = -EFAULT;
+        }
+        break;
+
     case HVMOP_altp2m_get_suppress_ve:
-        if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
+        if ( a.u.suppress_ve.pad1 )
             rc = -EINVAL;
         else
         {
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index e5e4349dea..9e1335065d 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -3054,6 +3054,64 @@ out:
     return rc;
 }
 
+/*
+ * Set/clear the #VE suppress bit for multiple pages.  Only available on VMX.
+ */
+int p2m_set_suppress_ve_multi(struct domain *d,
+                              struct xen_hvm_altp2m_suppress_ve* sve)
+{
+    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
+    struct p2m_domain *ap2m = NULL;
+    struct p2m_domain *p2m;
+    uint64_t start = sve->opaque ?: sve->gfn;
+    int rc = 0;
+
+    if ( sve->view > 0 )
+    {
+        if ( sve->view >= MAX_ALTP2M ||
+             d->arch.altp2m_eptp[sve->view] == mfn_x(INVALID_MFN) )
+            return -EINVAL;
+
+        p2m = ap2m = d->arch.altp2m_p2m[sve->view];
+    }
+    else
+        p2m = host_p2m;
+
+    p2m_lock(host_p2m);
+
+    if ( ap2m )
+        p2m_lock(ap2m);
+
+
+    while ( start < sve->nr )
+    {
+        p2m_access_t a;
+        p2m_type_t t;
+        mfn_t mfn;
+
+        if ( altp2m_get_effective_entry(p2m, _gfn(start), &mfn, &t, &a, AP2MGET_query) )
+            a = p2m->default_access;
+
+        p2m->set_entry(p2m, _gfn(start), mfn, PAGE_ORDER_4K, t, a, sve->suppress_ve);
+
+        /* Check for continuation if it's not the last iteration. */
+        if ( sve->nr > ++start && hypercall_preempt_check() )
+        {
+            rc = -ERESTART;
+            break;
+        }
+    }
+
+    sve->opaque = start;
+
+    if ( ap2m )
+        p2m_unlock(ap2m);
+
+    p2m_unlock(host_p2m);
+
+    return rc;
+}
+
 int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
                         unsigned int altp2m_idx)
 {
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index 353f8034d9..9834ce0aea 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -42,8 +42,9 @@ struct xen_hvm_altp2m_suppress_ve {
     uint16_t view;
     uint8_t suppress_ve; /* Boolean type. */
     uint8_t pad1;
-    uint32_t pad2;
+    uint32_t nr;
     uint64_t gfn;
+    uint64_t opaque;
 };
 
 #if __XEN_INTERFACE_VERSION__ < 0x00040900
@@ -339,6 +340,8 @@ struct xen_hvm_altp2m_op {
 #define HVMOP_altp2m_vcpu_disable_notify  13
 /* Get the active vcpu p2m index */
 #define HVMOP_altp2m_get_p2m_idx          14
+/* Set the "Supress #VE" bit for a range of pages */
+#define HVMOP_altp2m_set_suppress_ve_multi 15
     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 e4d24502e0..ffecd2650e 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_set_suppress_ve_multi(struct domain *d,
+                              struct xen_hvm_altp2m_suppress_ve* suppress_ve);
+
 int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
                         unsigned int altp2m_idx);
 
-- 
2.17.1


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

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Xen-devel] [PATCH V2 2/2] x86/mm: Make use of the default access param from xc_altp2m_create_view
  2019-11-06 15:35 [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits Alexandru Stefan ISAILA
@ 2019-11-06 15:35 ` Alexandru Stefan ISAILA
  2019-11-12 12:02   ` Jan Beulich
  2019-11-06 21:06 ` [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits Tamas K Lengyel
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 25+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-11-06 15:35 UTC (permalink / raw)
  To: xen-devel
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, Razvan COJOCARU, wl,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson, tamas,
	jbeulich, Alexandru Stefan ISAILA, roger.pau

At this moment the default_access param from xc_altp2m_create_view is
not used.

This patch assigns default_access to p2m->default_access at the time of
initializing a new altp2m view.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
Reviewed-by: Tamas K Lengyel <tamas@tklengyel.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
---
 xen/arch/x86/hvm/hvm.c            |  3 ++-
 xen/arch/x86/mm/p2m-ept.c         |  5 +++--
 xen/arch/x86/mm/p2m.c             | 31 ++++++++++++++++++++++++++-----
 xen/include/asm-x86/hvm/vmx/vmx.h |  3 ++-
 xen/include/asm-x86/p2m.h         |  3 ++-
 xen/include/public/hvm/hvm_op.h   |  2 --
 6 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 66ed8b8e3e..84f836a5c9 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4669,7 +4669,8 @@ static int do_altp2m_op(
     }
 
     case HVMOP_altp2m_create_p2m:
-        if ( !(rc = p2m_init_next_altp2m(d, &a.u.view.view)) )
+        if ( !(rc = p2m_init_next_altp2m(d, &a.u.view.view,
+                                         a.u.view.hvmmem_default_access)) )
             rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
         break;
 
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 220990f017..e62e749ec5 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1345,13 +1345,14 @@ void setup_ept_dump(void)
     register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
 }
 
-void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
+void p2m_init_altp2m_ept(struct domain *d, unsigned int i,
+                         p2m_access_t default_access)
 {
     struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
     struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
     struct ept_data *ept;
 
-    p2m->default_access = hostp2m->default_access;
+    p2m->default_access = default_access;
     p2m->domain = hostp2m->domain;
 
     p2m->global_logdirty = hostp2m->global_logdirty;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 9e1335065d..e45a2c3c44 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2528,7 +2528,8 @@ void p2m_flush_altp2m(struct domain *d)
     altp2m_list_unlock(d);
 }
 
-static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
+static int p2m_activate_altp2m(struct domain *d, unsigned int idx,
+                               p2m_access_t hvmmem_default_access)
 {
     struct p2m_domain *hostp2m, *p2m;
     int rc;
@@ -2554,7 +2555,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
         goto out;
     }
 
-    p2m_init_altp2m_ept(d, idx);
+    p2m_init_altp2m_ept(d, idx, hvmmem_default_access);
 
  out:
     p2m_unlock(p2m);
@@ -2565,6 +2566,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
 int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
 {
     int rc = -EINVAL;
+    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
 
     if ( idx >= MAX_ALTP2M )
         return rc;
@@ -2572,17 +2574,36 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
     altp2m_list_lock(d);
 
     if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
-        rc = p2m_activate_altp2m(d, idx);
+        rc = p2m_activate_altp2m(d, idx, hostp2m->default_access);
 
     altp2m_list_unlock(d);
     return rc;
 }
 
-int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
+int p2m_init_next_altp2m(struct domain *d, uint16_t *idx,
+                         uint16_t hvmmem_default_access)
 {
     int rc = -EINVAL;
     unsigned int i;
 
+    static const p2m_access_t memaccess[] = {
+#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
+        ACCESS(n),
+        ACCESS(r),
+        ACCESS(w),
+        ACCESS(rw),
+        ACCESS(x),
+        ACCESS(rx),
+        ACCESS(wx),
+        ACCESS(rwx),
+        ACCESS(rx2rw),
+        ACCESS(n2rwx),
+#undef ACCESS
+    };
+
+    if ( hvmmem_default_access > XENMEM_access_default )
+        return rc;
+
     altp2m_list_lock(d);
 
     for ( i = 0; i < MAX_ALTP2M; i++ )
@@ -2590,7 +2611,7 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
         if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
             continue;
 
-        rc = p2m_activate_altp2m(d, i);
+        rc = p2m_activate_altp2m(d, i, memaccess[hvmmem_default_access]);
 
         if ( !rc )
             *idx = i;
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index ebaa74449b..a9601e8f7e 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -598,7 +598,8 @@ void ept_p2m_uninit(struct p2m_domain *p2m);
 void ept_walk_table(struct domain *d, unsigned long gfn);
 bool_t ept_handle_misconfig(uint64_t gpa);
 void setup_ept_dump(void);
-void p2m_init_altp2m_ept(struct domain *d, unsigned int i);
+void p2m_init_altp2m_ept(struct domain *d, unsigned int i,
+                         p2m_access_t default_access);
 /* Locate an alternate p2m by its EPTP */
 unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp);
 
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 94285db1b4..321d5e70a8 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -884,7 +884,8 @@ bool p2m_altp2m_get_or_propagate(struct p2m_domain *ap2m, unsigned long gfn_l,
 int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx);
 
 /* Find an available alternate p2m and make it valid */
-int p2m_init_next_altp2m(struct domain *d, uint16_t *idx);
+int p2m_init_next_altp2m(struct domain *d, uint16_t *idx,
+                         uint16_t hvmmem_default_access);
 
 /* Make a specific alternate p2m invalid */
 int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx);
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index 9834ce0aea..fbe4b53d8d 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -242,8 +242,6 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_vcpu_disable_notify_t);
 struct xen_hvm_altp2m_view {
     /* IN/OUT variable */
     uint16_t view;
-    /* Create view only: default access type
-     * NOTE: currently ignored */
     uint16_t hvmmem_default_access; /* xenmem_access_t */
 };
 typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;
-- 
2.17.1


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

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-06 15:35 [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits Alexandru Stefan ISAILA
  2019-11-06 15:35 ` [Xen-devel] [PATCH V2 2/2] x86/mm: Make use of the default access param from xc_altp2m_create_view Alexandru Stefan ISAILA
@ 2019-11-06 21:06 ` Tamas K Lengyel
  2019-11-07  7:46   ` Alexandru Stefan ISAILA
  2019-11-08  8:31 ` Alexandru Stefan ISAILA
  2019-11-12 11:54 ` Jan Beulich
  3 siblings, 1 reply; 25+ messages in thread
From: Tamas K Lengyel @ 2019-11-06 21:06 UTC (permalink / raw)
  To: Alexandru Stefan ISAILA
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, Razvan COJOCARU, wl,
	konrad.wilk, George.Dunlap, andrew.cooper3, ian.jackson,
	jbeulich, xen-devel, roger.pau

On Wed, Nov 6, 2019 at 7:35 AM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> wrote:
>
> By default the sve bits are not set.
> This patch adds a new hypercall, xc_altp2m_set_supress_ve_multi(),
> to set a range of sve bits.
> The core function, p2m_set_suppress_ve_multi(), does not brake in case
> of a error and it is doing a best effort for setting the bits in the
> given range. A check for continuation is made in order to have
> preemption on big ranges.
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>
> ---
> Changes since V1:
>         - Remove "continue"
>         - Add a new field in xen_hvm_altp2m_suppress_ve to store the
> continuation value
>         - Have p2m_set_suppress_ve_multi() take
> xen_hvm_altp2m_suppress_ve as a param.
> ---
>  tools/libxc/include/xenctrl.h   |  3 ++
>  tools/libxc/xc_altp2m.c         | 25 ++++++++++++++
>  xen/arch/x86/hvm/hvm.c          | 20 ++++++++++--
>  xen/arch/x86/mm/p2m.c           | 58 +++++++++++++++++++++++++++++++++
>  xen/include/public/hvm/hvm_op.h |  5 ++-
>  xen/include/xen/mem_access.h    |  3 ++
>  6 files changed, 111 insertions(+), 3 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index f4431687b3..21b644f459 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1923,6 +1923,9 @@ 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_supress_ve_multi(xc_interface *handle, uint32_t domid,
> +                                   uint16_t view_id, xen_pfn_t start_gfn,
> +                                   uint32_t nr, 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,
> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
> index 09dad0355e..6605d9abbe 100644
> --- a/tools/libxc/xc_altp2m.c
> +++ b/tools/libxc/xc_altp2m.c
> @@ -234,6 +234,31 @@ int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
>      return rc;
>  }
>
> +int xc_altp2m_set_supress_ve_multi(xc_interface *handle, uint32_t domid,
> +                                   uint16_t view_id, xen_pfn_t start_gfn,
> +                                   uint32_t nr, bool sve)
> +{
> +    int rc;
> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
> +
> +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));

Does xc_hypercall_buffer_alloc null-initialize the structure?

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-06 21:06 ` [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits Tamas K Lengyel
@ 2019-11-07  7:46   ` Alexandru Stefan ISAILA
  2019-11-07 15:00     ` Tamas K Lengyel
  0 siblings, 1 reply; 25+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-11-07  7:46 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, Razvan COJOCARU, wl,
	konrad.wilk, George.Dunlap, andrew.cooper3, ian.jackson,
	jbeulich, xen-devel, roger.pau



On 06.11.2019 23:06, Tamas K Lengyel wrote:
> On Wed, Nov 6, 2019 at 7:35 AM Alexandru Stefan ISAILA
> <aisaila@bitdefender.com> wrote:
>>
>> By default the sve bits are not set.
>> This patch adds a new hypercall, xc_altp2m_set_supress_ve_multi(),
>> to set a range of sve bits.
>> The core function, p2m_set_suppress_ve_multi(), does not brake in case
>> of a error and it is doing a best effort for setting the bits in the
>> given range. A check for continuation is made in order to have
>> preemption on big ranges.
>>
>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>>
>> ---
>> Changes since V1:
>>          - Remove "continue"
>>          - Add a new field in xen_hvm_altp2m_suppress_ve to store the
>> continuation value
>>          - Have p2m_set_suppress_ve_multi() take
>> xen_hvm_altp2m_suppress_ve as a param.
>> ---
>>   tools/libxc/include/xenctrl.h   |  3 ++
>>   tools/libxc/xc_altp2m.c         | 25 ++++++++++++++
>>   xen/arch/x86/hvm/hvm.c          | 20 ++++++++++--
>>   xen/arch/x86/mm/p2m.c           | 58 +++++++++++++++++++++++++++++++++
>>   xen/include/public/hvm/hvm_op.h |  5 ++-
>>   xen/include/xen/mem_access.h    |  3 ++
>>   6 files changed, 111 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index f4431687b3..21b644f459 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -1923,6 +1923,9 @@ 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_supress_ve_multi(xc_interface *handle, uint32_t domid,
>> +                                   uint16_t view_id, xen_pfn_t start_gfn,
>> +                                   uint32_t nr, 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,
>> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
>> index 09dad0355e..6605d9abbe 100644
>> --- a/tools/libxc/xc_altp2m.c
>> +++ b/tools/libxc/xc_altp2m.c
>> @@ -234,6 +234,31 @@ int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
>>       return rc;
>>   }
>>
>> +int xc_altp2m_set_supress_ve_multi(xc_interface *handle, uint32_t domid,
>> +                                   uint16_t view_id, xen_pfn_t start_gfn,
>> +                                   uint32_t nr, bool sve)
>> +{
>> +    int rc;
>> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
>> +
>> +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
> 
> Does xc_hypercall_buffer_alloc null-initialize the structure?
> 

It calls xencall_alloc_buffer_pages() which calls memset(p, 0, nr_pages 
* PAGE_SIZE) before returning.

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-07  7:46   ` Alexandru Stefan ISAILA
@ 2019-11-07 15:00     ` Tamas K Lengyel
  0 siblings, 0 replies; 25+ messages in thread
From: Tamas K Lengyel @ 2019-11-07 15:00 UTC (permalink / raw)
  To: Alexandru Stefan ISAILA
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, Razvan COJOCARU, wl,
	konrad.wilk, George.Dunlap, andrew.cooper3, ian.jackson,
	jbeulich, xen-devel, roger.pau

On Wed, Nov 6, 2019 at 11:46 PM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> wrote:
>
>
>
> On 06.11.2019 23:06, Tamas K Lengyel wrote:
> > On Wed, Nov 6, 2019 at 7:35 AM Alexandru Stefan ISAILA
> > <aisaila@bitdefender.com> wrote:
> >>
> >> By default the sve bits are not set.
> >> This patch adds a new hypercall, xc_altp2m_set_supress_ve_multi(),
> >> to set a range of sve bits.
> >> The core function, p2m_set_suppress_ve_multi(), does not brake in case
> >> of a error and it is doing a best effort for setting the bits in the
> >> given range. A check for continuation is made in order to have
> >> preemption on big ranges.
> >>
> >> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> >>
> >> ---
> >> Changes since V1:
> >>          - Remove "continue"
> >>          - Add a new field in xen_hvm_altp2m_suppress_ve to store the
> >> continuation value
> >>          - Have p2m_set_suppress_ve_multi() take
> >> xen_hvm_altp2m_suppress_ve as a param.
> >> ---
> >>   tools/libxc/include/xenctrl.h   |  3 ++
> >>   tools/libxc/xc_altp2m.c         | 25 ++++++++++++++
> >>   xen/arch/x86/hvm/hvm.c          | 20 ++++++++++--
> >>   xen/arch/x86/mm/p2m.c           | 58 +++++++++++++++++++++++++++++++++
> >>   xen/include/public/hvm/hvm_op.h |  5 ++-
> >>   xen/include/xen/mem_access.h    |  3 ++
> >>   6 files changed, 111 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> >> index f4431687b3..21b644f459 100644
> >> --- a/tools/libxc/include/xenctrl.h
> >> +++ b/tools/libxc/include/xenctrl.h
> >> @@ -1923,6 +1923,9 @@ 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_supress_ve_multi(xc_interface *handle, uint32_t domid,
> >> +                                   uint16_t view_id, xen_pfn_t start_gfn,
> >> +                                   uint32_t nr, 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,
> >> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
> >> index 09dad0355e..6605d9abbe 100644
> >> --- a/tools/libxc/xc_altp2m.c
> >> +++ b/tools/libxc/xc_altp2m.c
> >> @@ -234,6 +234,31 @@ int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
> >>       return rc;
> >>   }
> >>
> >> +int xc_altp2m_set_supress_ve_multi(xc_interface *handle, uint32_t domid,
> >> +                                   uint16_t view_id, xen_pfn_t start_gfn,
> >> +                                   uint32_t nr, bool sve)
> >> +{
> >> +    int rc;
> >> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
> >> +
> >> +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
> >
> > Does xc_hypercall_buffer_alloc null-initialize the structure?
> >
>
> It calls xencall_alloc_buffer_pages() which calls memset(p, 0, nr_pages
> * PAGE_SIZE) before returning.

Thanks!

Reviewed-by: Tamas K Lengyel <tamas@tklengyel.com>

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-06 15:35 [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits Alexandru Stefan ISAILA
  2019-11-06 15:35 ` [Xen-devel] [PATCH V2 2/2] x86/mm: Make use of the default access param from xc_altp2m_create_view Alexandru Stefan ISAILA
  2019-11-06 21:06 ` [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits Tamas K Lengyel
@ 2019-11-08  8:31 ` Alexandru Stefan ISAILA
  2019-11-12 11:54 ` Jan Beulich
  3 siblings, 0 replies; 25+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-11-08  8:31 UTC (permalink / raw)
  To: xen-devel, George.Dunlap
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, Razvan COJOCARU, wl,
	konrad.wilk, andrew.cooper3, ian.jackson, tamas, jbeulich,
	roger.pau

Hi George,

Sorry for the early reminder but v1 you said "Everything else looks OK 
to me." and you did not give a specific ACK. Can you take a look at the 
changes when you have the time?

Thanks,
Alex

On 06.11.2019 17:35, Alexandru Stefan ISAILA wrote:
> By default the sve bits are not set.
> This patch adds a new hypercall, xc_altp2m_set_supress_ve_multi(),
> to set a range of sve bits.
> The core function, p2m_set_suppress_ve_multi(), does not brake in case
> of a error and it is doing a best effort for setting the bits in the
> given range. A check for continuation is made in order to have
> preemption on big ranges.
> 
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> 
> ---
> Changes since V1:
> 	- Remove "continue"
> 	- Add a new field in xen_hvm_altp2m_suppress_ve to store the
> continuation value
> 	- Have p2m_set_suppress_ve_multi() take
> xen_hvm_altp2m_suppress_ve as a param.
> ---
>   tools/libxc/include/xenctrl.h   |  3 ++
>   tools/libxc/xc_altp2m.c         | 25 ++++++++++++++
>   xen/arch/x86/hvm/hvm.c          | 20 ++++++++++--
>   xen/arch/x86/mm/p2m.c           | 58 +++++++++++++++++++++++++++++++++
>   xen/include/public/hvm/hvm_op.h |  5 ++-
>   xen/include/xen/mem_access.h    |  3 ++
>   6 files changed, 111 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index f4431687b3..21b644f459 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1923,6 +1923,9 @@ 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_supress_ve_multi(xc_interface *handle, uint32_t domid,
> +                                   uint16_t view_id, xen_pfn_t start_gfn,
> +                                   uint32_t nr, 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,
> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
> index 09dad0355e..6605d9abbe 100644
> --- a/tools/libxc/xc_altp2m.c
> +++ b/tools/libxc/xc_altp2m.c
> @@ -234,6 +234,31 @@ int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
>       return rc;
>   }
>   
> +int xc_altp2m_set_supress_ve_multi(xc_interface *handle, uint32_t domid,
> +                                   uint16_t view_id, xen_pfn_t start_gfn,
> +                                   uint32_t nr, 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_multi;
> +    arg->domain = domid;
> +    arg->u.suppress_ve.view = view_id;
> +    arg->u.suppress_ve.gfn = start_gfn;
> +    arg->u.suppress_ve.suppress_ve = sve;
> +    arg->u.suppress_ve.nr = nr;
> +
> +    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 06a7b40107..66ed8b8e3e 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4535,6 +4535,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_set_suppress_ve_multi:
>       case HVMOP_altp2m_get_suppress_ve:
>       case HVMOP_altp2m_set_mem_access:
>       case HVMOP_altp2m_set_mem_access_multi:
> @@ -4681,7 +4682,7 @@ static int do_altp2m_op(
>           break;
>   
>       case HVMOP_altp2m_set_suppress_ve:
> -        if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
> +        if ( a.u.suppress_ve.pad1 )
>               rc = -EINVAL;
>           else
>           {
> @@ -4693,8 +4694,23 @@ static int do_altp2m_op(
>           }
>           break;
>   
> +    case HVMOP_altp2m_set_suppress_ve_multi:
> +        if ( a.u.suppress_ve.pad1 || !a.u.suppress_ve.nr )
> +            rc = -EINVAL;
> +        else
> +        {
> +            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve);
> +
> +            if ( rc == -ERESTART )
> +                if ( __copy_field_to_guest(guest_handle_cast(arg,
> +                                           xen_hvm_altp2m_op_t),
> +                                           &a, u.suppress_ve.opaque) )
> +                    rc = -EFAULT;
> +        }
> +        break;
> +
>       case HVMOP_altp2m_get_suppress_ve:
> -        if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
> +        if ( a.u.suppress_ve.pad1 )
>               rc = -EINVAL;
>           else
>           {
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index e5e4349dea..9e1335065d 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -3054,6 +3054,64 @@ out:
>       return rc;
>   }
>   
> +/*
> + * Set/clear the #VE suppress bit for multiple pages.  Only available on VMX.
> + */
> +int p2m_set_suppress_ve_multi(struct domain *d,
> +                              struct xen_hvm_altp2m_suppress_ve* sve)
> +{
> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *ap2m = NULL;
> +    struct p2m_domain *p2m;
> +    uint64_t start = sve->opaque ?: sve->gfn;
> +    int rc = 0;
> +
> +    if ( sve->view > 0 )
> +    {
> +        if ( sve->view >= MAX_ALTP2M ||
> +             d->arch.altp2m_eptp[sve->view] == mfn_x(INVALID_MFN) )
> +            return -EINVAL;
> +
> +        p2m = ap2m = d->arch.altp2m_p2m[sve->view];
> +    }
> +    else
> +        p2m = host_p2m;
> +
> +    p2m_lock(host_p2m);
> +
> +    if ( ap2m )
> +        p2m_lock(ap2m);
> +
> +
> +    while ( start < sve->nr )
> +    {
> +        p2m_access_t a;
> +        p2m_type_t t;
> +        mfn_t mfn;
> +
> +        if ( altp2m_get_effective_entry(p2m, _gfn(start), &mfn, &t, &a, AP2MGET_query) )
> +            a = p2m->default_access;
> +
> +        p2m->set_entry(p2m, _gfn(start), mfn, PAGE_ORDER_4K, t, a, sve->suppress_ve);
> +
> +        /* Check for continuation if it's not the last iteration. */
> +        if ( sve->nr > ++start && hypercall_preempt_check() )
> +        {
> +            rc = -ERESTART;
> +            break;
> +        }
> +    }
> +
> +    sve->opaque = start;
> +
> +    if ( ap2m )
> +        p2m_unlock(ap2m);
> +
> +    p2m_unlock(host_p2m);
> +
> +    return rc;
> +}
> +
>   int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
>                           unsigned int altp2m_idx)
>   {
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index 353f8034d9..9834ce0aea 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -42,8 +42,9 @@ struct xen_hvm_altp2m_suppress_ve {
>       uint16_t view;
>       uint8_t suppress_ve; /* Boolean type. */
>       uint8_t pad1;
> -    uint32_t pad2;
> +    uint32_t nr;
>       uint64_t gfn;
> +    uint64_t opaque;
>   };
>   
>   #if __XEN_INTERFACE_VERSION__ < 0x00040900
> @@ -339,6 +340,8 @@ struct xen_hvm_altp2m_op {
>   #define HVMOP_altp2m_vcpu_disable_notify  13
>   /* Get the active vcpu p2m index */
>   #define HVMOP_altp2m_get_p2m_idx          14
> +/* Set the "Supress #VE" bit for a range of pages */
> +#define HVMOP_altp2m_set_suppress_ve_multi 15
>       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 e4d24502e0..ffecd2650e 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_set_suppress_ve_multi(struct domain *d,
> +                              struct xen_hvm_altp2m_suppress_ve* suppress_ve);
> +
>   int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
>                           unsigned int altp2m_idx);
>   
> 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-06 15:35 [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits Alexandru Stefan ISAILA
                   ` (2 preceding siblings ...)
  2019-11-08  8:31 ` Alexandru Stefan ISAILA
@ 2019-11-12 11:54 ` Jan Beulich
  2019-11-12 14:05   ` Tamas K Lengyel
                     ` (2 more replies)
  3 siblings, 3 replies; 25+ messages in thread
From: Jan Beulich @ 2019-11-12 11:54 UTC (permalink / raw)
  To: Alexandru Stefan ISAILA
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson, tamas,
	xen-devel, roger.pau

On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
> @@ -4681,7 +4682,7 @@ static int do_altp2m_op(
>          break;
>  
>      case HVMOP_altp2m_set_suppress_ve:
> -        if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
> +        if ( a.u.suppress_ve.pad1 )

Just because the field changes its name doesn't mean you can
drop the check. You even add a new field not used (yet) by
this sub-function, which then also would need checking here.

> @@ -4693,8 +4694,23 @@ static int do_altp2m_op(
>          }
>          break;
>  
> +    case HVMOP_altp2m_set_suppress_ve_multi:
> +        if ( a.u.suppress_ve.pad1 || !a.u.suppress_ve.nr )

A count of zero typically is taken as a no-op, not an error.

> +            rc = -EINVAL;
> +        else
> +        {
> +            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve);
> +
> +            if ( rc == -ERESTART )
> +                if ( __copy_field_to_guest(guest_handle_cast(arg,
> +                                           xen_hvm_altp2m_op_t),
> +                                           &a, u.suppress_ve.opaque) )
> +                    rc = -EFAULT;

If the operation is best effort, _some_ indication of failure should
still be handed back to the caller. Whether that's through the opaque
field or by some other means is secondary. If not via that field
(which would make the outer of the two if()-s disappear), please fold
the if()-s.

> +        }
> +        break;
> +
>      case HVMOP_altp2m_get_suppress_ve:
> -        if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
> +        if ( a.u.suppress_ve.pad1 )

See above.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -3054,6 +3054,64 @@ out:
>      return rc;
>  }
>  
> +/*
> + * Set/clear the #VE suppress bit for multiple pages.  Only available on VMX.
> + */
> +int p2m_set_suppress_ve_multi(struct domain *d,
> +                              struct xen_hvm_altp2m_suppress_ve* sve)

Misplaced *.

> +{
> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *ap2m = NULL;
> +    struct p2m_domain *p2m;
> +    uint64_t start = sve->opaque ?: sve->gfn;

According to this start (and hence ->opaque) are GFNs.

> +    int rc = 0;
> +
> +    if ( sve->view > 0 )
> +    {
> +        if ( sve->view >= MAX_ALTP2M ||
> +             d->arch.altp2m_eptp[sve->view] == mfn_x(INVALID_MFN) )
> +            return -EINVAL;
> +
> +        p2m = ap2m = d->arch.altp2m_p2m[sve->view];
> +    }
> +    else
> +        p2m = host_p2m;
> +
> +    p2m_lock(host_p2m);
> +
> +    if ( ap2m )
> +        p2m_lock(ap2m);
> +
> +
> +    while ( start < sve->nr )

According to this, start is an index. Which of the two do you
mean?

> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -42,8 +42,9 @@ struct xen_hvm_altp2m_suppress_ve {
>      uint16_t view;
>      uint8_t suppress_ve; /* Boolean type. */
>      uint8_t pad1;
> -    uint32_t pad2;
> +    uint32_t nr;
>      uint64_t gfn;
> +    uint64_t opaque;
>  };

How is this addition of a field going to work compatibly with old
and new callers on old and new hypervisors? Recall in particular
that these operations are (almost?) all potentially usable by the
guest itself.

Jan

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 2/2] x86/mm: Make use of the default access param from xc_altp2m_create_view
  2019-11-06 15:35 ` [Xen-devel] [PATCH V2 2/2] x86/mm: Make use of the default access param from xc_altp2m_create_view Alexandru Stefan ISAILA
@ 2019-11-12 12:02   ` Jan Beulich
  2019-11-18  8:38     ` Alexandru Stefan ISAILA
  0 siblings, 1 reply; 25+ messages in thread
From: Jan Beulich @ 2019-11-12 12:02 UTC (permalink / raw)
  To: Alexandru Stefan ISAILA
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson, tamas,
	xen-devel, roger.pau

On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1345,13 +1345,14 @@ void setup_ept_dump(void)
>      register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
>  }
>  
> -void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
> +void p2m_init_altp2m_ept(struct domain *d, unsigned int i,
> +                         p2m_access_t default_access)
>  {
>      struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>      struct ept_data *ept;
>  
> -    p2m->default_access = hostp2m->default_access;
> +    p2m->default_access = default_access;
>      p2m->domain = hostp2m->domain;
>  
>      p2m->global_logdirty = hostp2m->global_logdirty;

All of this is not EPT-specific. Before adding more infrastructure to
cover for this (here: another function parameter), how about moving
these parts into vendor-independent code?

> @@ -2572,17 +2574,36 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
>      altp2m_list_lock(d);
>  
>      if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
> -        rc = p2m_activate_altp2m(d, idx);
> +        rc = p2m_activate_altp2m(d, idx, hostp2m->default_access);
>  
>      altp2m_list_unlock(d);
>      return rc;
>  }
>  
> -int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
> +int p2m_init_next_altp2m(struct domain *d, uint16_t *idx,
> +                         uint16_t hvmmem_default_access)
>  {
>      int rc = -EINVAL;
>      unsigned int i;
>  
> +    static const p2m_access_t memaccess[] = {
> +#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
> +        ACCESS(n),
> +        ACCESS(r),
> +        ACCESS(w),
> +        ACCESS(rw),
> +        ACCESS(x),
> +        ACCESS(rx),
> +        ACCESS(wx),
> +        ACCESS(rwx),
> +        ACCESS(rx2rw),
> +        ACCESS(n2rwx),
> +#undef ACCESS
> +    };
> +
> +    if ( hvmmem_default_access > XENMEM_access_default )
> +        return rc;
> +
>      altp2m_list_lock(d);
>  
>      for ( i = 0; i < MAX_ALTP2M; i++ )
> @@ -2590,7 +2611,7 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
>          if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
>              continue;
>  
> -        rc = p2m_activate_altp2m(d, i);
> +        rc = p2m_activate_altp2m(d, i, memaccess[hvmmem_default_access]);

Aren't you open-coding xenmem_access_to_p2m_access() here? In
no event should there be two instances of the same static array.

Jan

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-12 11:54 ` Jan Beulich
@ 2019-11-12 14:05   ` Tamas K Lengyel
  2019-11-12 14:31     ` Jan Beulich
  2019-11-18 13:39   ` Alexandru Stefan ISAILA
  2019-11-18 13:39   ` Alexandru Stefan ISAILA
  2 siblings, 1 reply; 25+ messages in thread
From: Tamas K Lengyel @ 2019-11-12 14:05 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson,
	Alexandru Stefan ISAILA, xen-devel, roger.pau

On Tue, Nov 12, 2019 at 4:54 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
> > @@ -4681,7 +4682,7 @@ static int do_altp2m_op(
> >          break;
> >
> >      case HVMOP_altp2m_set_suppress_ve:
> > -        if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
> > +        if ( a.u.suppress_ve.pad1 )
>
> Just because the field changes its name doesn't mean you can
> drop the check. You even add a new field not used (yet) by
> this sub-function, which then also would need checking here.
>
> > @@ -4693,8 +4694,23 @@ static int do_altp2m_op(
> >          }
> >          break;
> >
> > +    case HVMOP_altp2m_set_suppress_ve_multi:
> > +        if ( a.u.suppress_ve.pad1 || !a.u.suppress_ve.nr )
>
> A count of zero typically is taken as a no-op, not an error.
>
> > +            rc = -EINVAL;
> > +        else
> > +        {
> > +            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve);
> > +
> > +            if ( rc == -ERESTART )
> > +                if ( __copy_field_to_guest(guest_handle_cast(arg,
> > +                                           xen_hvm_altp2m_op_t),
> > +                                           &a, u.suppress_ve.opaque) )
> > +                    rc = -EFAULT;
>
> If the operation is best effort, _some_ indication of failure should
> still be handed back to the caller. Whether that's through the opaque
> field or by some other means is secondary. If not via that field
> (which would make the outer of the two if()-s disappear), please fold
> the if()-s.

At least for mem_sharing_range_op we also do a best-effort and don't
return an error for pages where it wasn't possible to share. So I
don't think it's absolutely necessary to do that, especially if the
caller can't do anything about those errors anyway.

Tamas

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-12 14:05   ` Tamas K Lengyel
@ 2019-11-12 14:31     ` Jan Beulich
  2019-11-13 14:51       ` Tamas K Lengyel
  0 siblings, 1 reply; 25+ messages in thread
From: Jan Beulich @ 2019-11-12 14:31 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson,
	Alexandru Stefan ISAILA, xen-devel, roger.pau

On 12.11.2019 15:05, Tamas K Lengyel wrote:
> On Tue, Nov 12, 2019 at 4:54 AM Jan Beulich <jbeulich@suse.com> wrote:
>> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
>>> +        else
>>> +        {
>>> +            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve);
>>> +
>>> +            if ( rc == -ERESTART )
>>> +                if ( __copy_field_to_guest(guest_handle_cast(arg,
>>> +                                           xen_hvm_altp2m_op_t),
>>> +                                           &a, u.suppress_ve.opaque) )
>>> +                    rc = -EFAULT;
>>
>> If the operation is best effort, _some_ indication of failure should
>> still be handed back to the caller. Whether that's through the opaque
>> field or by some other means is secondary. If not via that field
>> (which would make the outer of the two if()-s disappear), please fold
>> the if()-s.
> 
> At least for mem_sharing_range_op we also do a best-effort and don't
> return an error for pages where it wasn't possible to share. So I
> don't think it's absolutely necessary to do that, especially if the
> caller can't do anything about those errors anyway.

mem-sharing is a little different in nature, isn't it? If you
can't share a page, both involved guests will continue to run
with their own instances. If you want to suppress #VE delivery
and it fails, behavior won't be transparently correct, as
there'll potentially be #VE when there should be none. Whether
that's benign to the guest very much depends on its handler.

Jan

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-12 14:31     ` Jan Beulich
@ 2019-11-13 14:51       ` Tamas K Lengyel
  2019-11-13 14:57         ` Tamas K Lengyel
  0 siblings, 1 reply; 25+ messages in thread
From: Tamas K Lengyel @ 2019-11-13 14:51 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson,
	Alexandru Stefan ISAILA, xen-devel, roger.pau

On Tue, Nov 12, 2019 at 7:31 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 12.11.2019 15:05, Tamas K Lengyel wrote:
> > On Tue, Nov 12, 2019 at 4:54 AM Jan Beulich <jbeulich@suse.com> wrote:
> >> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
> >>> +        else
> >>> +        {
> >>> +            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve);
> >>> +
> >>> +            if ( rc == -ERESTART )
> >>> +                if ( __copy_field_to_guest(guest_handle_cast(arg,
> >>> +                                           xen_hvm_altp2m_op_t),
> >>> +                                           &a, u.suppress_ve.opaque) )
> >>> +                    rc = -EFAULT;
> >>
> >> If the operation is best effort, _some_ indication of failure should
> >> still be handed back to the caller. Whether that's through the opaque
> >> field or by some other means is secondary. If not via that field
> >> (which would make the outer of the two if()-s disappear), please fold
> >> the if()-s.
> >
> > At least for mem_sharing_range_op we also do a best-effort and don't
> > return an error for pages where it wasn't possible to share. So I
> > don't think it's absolutely necessary to do that, especially if the
> > caller can't do anything about those errors anyway.
>
> mem-sharing is a little different in nature, isn't it? If you
> can't share a page, both involved guests will continue to run
> with their own instances. If you want to suppress #VE delivery
> and it fails, behavior won't be transparently correct, as
> there'll potentially be #VE when there should be none. Whether
> that's benign to the guest very much depends on its handler.

Makes me wonder whether it would make more sense to flip this thing on
its head and have supress_ve be set by default (since its ignored by
default) and then have pages for which the EPT violation should be
convertible to #VE be specifically enabled by turning suppress_ve off.
That would eliminate the possibility of having the in-guest handler
getting #VE for pages it is not ready to handle. The hypervisor (and
the external VMI toolstack) OTOH should always be in a position to
handle EPT violations it itself causes by changing the page
permissions.

Tamas

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-13 14:51       ` Tamas K Lengyel
@ 2019-11-13 14:57         ` Tamas K Lengyel
  2019-11-13 16:52           ` Jan Beulich
  0 siblings, 1 reply; 25+ messages in thread
From: Tamas K Lengyel @ 2019-11-13 14:57 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson,
	Alexandru Stefan ISAILA, xen-devel, roger.pau

On Wed, Nov 13, 2019 at 7:51 AM Tamas K Lengyel <tamas@tklengyel.com> wrote:
>
> On Tue, Nov 12, 2019 at 7:31 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 12.11.2019 15:05, Tamas K Lengyel wrote:
> > > On Tue, Nov 12, 2019 at 4:54 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
> > >>> +        else
> > >>> +        {
> > >>> +            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve);
> > >>> +
> > >>> +            if ( rc == -ERESTART )
> > >>> +                if ( __copy_field_to_guest(guest_handle_cast(arg,
> > >>> +                                           xen_hvm_altp2m_op_t),
> > >>> +                                           &a, u.suppress_ve.opaque) )
> > >>> +                    rc = -EFAULT;
> > >>
> > >> If the operation is best effort, _some_ indication of failure should
> > >> still be handed back to the caller. Whether that's through the opaque
> > >> field or by some other means is secondary. If not via that field
> > >> (which would make the outer of the two if()-s disappear), please fold
> > >> the if()-s.
> > >
> > > At least for mem_sharing_range_op we also do a best-effort and don't
> > > return an error for pages where it wasn't possible to share. So I
> > > don't think it's absolutely necessary to do that, especially if the
> > > caller can't do anything about those errors anyway.
> >
> > mem-sharing is a little different in nature, isn't it? If you
> > can't share a page, both involved guests will continue to run
> > with their own instances. If you want to suppress #VE delivery
> > and it fails, behavior won't be transparently correct, as
> > there'll potentially be #VE when there should be none. Whether
> > that's benign to the guest very much depends on its handler.
>
> Makes me wonder whether it would make more sense to flip this thing on
> its head and have supress_ve be set by default (since its ignored by
> default) and then have pages for which the EPT violation should be
> convertible to #VE be specifically enabled by turning suppress_ve off.
> That would eliminate the possibility of having the in-guest handler
> getting #VE for pages it is not ready to handle. The hypervisor (and
> the external VMI toolstack) OTOH should always be in a position to
> handle EPT violations it itself causes by changing the page
> permissions.

Actually, now that I looked at it, that's _exactly_ what we do
already. The suppress_ve bit is always set for all EPT pages. So this
operation here is going to be used to enable #VE for pages, not the
other way around. So there wouldn't be a case of "potentially be #VE
when there should be none".

Tamas

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-13 14:57         ` Tamas K Lengyel
@ 2019-11-13 16:52           ` Jan Beulich
  2019-11-13 18:38             ` Tamas K Lengyel
  0 siblings, 1 reply; 25+ messages in thread
From: Jan Beulich @ 2019-11-13 16:52 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson,
	Alexandru Stefan ISAILA, xen-devel, roger.pau

On 13.11.2019 15:57, Tamas K Lengyel wrote:
> On Wed, Nov 13, 2019 at 7:51 AM Tamas K Lengyel <tamas@tklengyel.com> wrote:
>>
>> On Tue, Nov 12, 2019 at 7:31 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 12.11.2019 15:05, Tamas K Lengyel wrote:
>>>> On Tue, Nov 12, 2019 at 4:54 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
>>>>>> +        else
>>>>>> +        {
>>>>>> +            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve);
>>>>>> +
>>>>>> +            if ( rc == -ERESTART )
>>>>>> +                if ( __copy_field_to_guest(guest_handle_cast(arg,
>>>>>> +                                           xen_hvm_altp2m_op_t),
>>>>>> +                                           &a, u.suppress_ve.opaque) )
>>>>>> +                    rc = -EFAULT;
>>>>>
>>>>> If the operation is best effort, _some_ indication of failure should
>>>>> still be handed back to the caller. Whether that's through the opaque
>>>>> field or by some other means is secondary. If not via that field
>>>>> (which would make the outer of the two if()-s disappear), please fold
>>>>> the if()-s.
>>>>
>>>> At least for mem_sharing_range_op we also do a best-effort and don't
>>>> return an error for pages where it wasn't possible to share. So I
>>>> don't think it's absolutely necessary to do that, especially if the
>>>> caller can't do anything about those errors anyway.
>>>
>>> mem-sharing is a little different in nature, isn't it? If you
>>> can't share a page, both involved guests will continue to run
>>> with their own instances. If you want to suppress #VE delivery
>>> and it fails, behavior won't be transparently correct, as
>>> there'll potentially be #VE when there should be none. Whether
>>> that's benign to the guest very much depends on its handler.
>>
>> Makes me wonder whether it would make more sense to flip this thing on
>> its head and have supress_ve be set by default (since its ignored by
>> default) and then have pages for which the EPT violation should be
>> convertible to #VE be specifically enabled by turning suppress_ve off.
>> That would eliminate the possibility of having the in-guest handler
>> getting #VE for pages it is not ready to handle. The hypervisor (and
>> the external VMI toolstack) OTOH should always be in a position to
>> handle EPT violations it itself causes by changing the page
>> permissions.
> 
> Actually, now that I looked at it, that's _exactly_ what we do
> already. The suppress_ve bit is always set for all EPT pages. So this
> operation here is going to be used to enable #VE for pages, not the
> other way around. So there wouldn't be a case of "potentially be #VE
> when there should be none".

But this doesn't change the bottom line of my earlier comment: It's
as bad to an OS to see too many #VE as it is to miss any that are
expected.

Jan

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-13 16:52           ` Jan Beulich
@ 2019-11-13 18:38             ` Tamas K Lengyel
  0 siblings, 0 replies; 25+ messages in thread
From: Tamas K Lengyel @ 2019-11-13 18:38 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson,
	Alexandru Stefan ISAILA, xen-devel, roger.pau

On Wed, Nov 13, 2019 at 9:52 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 13.11.2019 15:57, Tamas K Lengyel wrote:
> > On Wed, Nov 13, 2019 at 7:51 AM Tamas K Lengyel <tamas@tklengyel.com> wrote:
> >>
> >> On Tue, Nov 12, 2019 at 7:31 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>
> >>> On 12.11.2019 15:05, Tamas K Lengyel wrote:
> >>>> On Tue, Nov 12, 2019 at 4:54 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
> >>>>>> +        else
> >>>>>> +        {
> >>>>>> +            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve);
> >>>>>> +
> >>>>>> +            if ( rc == -ERESTART )
> >>>>>> +                if ( __copy_field_to_guest(guest_handle_cast(arg,
> >>>>>> +                                           xen_hvm_altp2m_op_t),
> >>>>>> +                                           &a, u.suppress_ve.opaque) )
> >>>>>> +                    rc = -EFAULT;
> >>>>>
> >>>>> If the operation is best effort, _some_ indication of failure should
> >>>>> still be handed back to the caller. Whether that's through the opaque
> >>>>> field or by some other means is secondary. If not via that field
> >>>>> (which would make the outer of the two if()-s disappear), please fold
> >>>>> the if()-s.
> >>>>
> >>>> At least for mem_sharing_range_op we also do a best-effort and don't
> >>>> return an error for pages where it wasn't possible to share. So I
> >>>> don't think it's absolutely necessary to do that, especially if the
> >>>> caller can't do anything about those errors anyway.
> >>>
> >>> mem-sharing is a little different in nature, isn't it? If you
> >>> can't share a page, both involved guests will continue to run
> >>> with their own instances. If you want to suppress #VE delivery
> >>> and it fails, behavior won't be transparently correct, as
> >>> there'll potentially be #VE when there should be none. Whether
> >>> that's benign to the guest very much depends on its handler.
> >>
> >> Makes me wonder whether it would make more sense to flip this thing on
> >> its head and have supress_ve be set by default (since its ignored by
> >> default) and then have pages for which the EPT violation should be
> >> convertible to #VE be specifically enabled by turning suppress_ve off.
> >> That would eliminate the possibility of having the in-guest handler
> >> getting #VE for pages it is not ready to handle. The hypervisor (and
> >> the external VMI toolstack) OTOH should always be in a position to
> >> handle EPT violations it itself causes by changing the page
> >> permissions.
> >
> > Actually, now that I looked at it, that's _exactly_ what we do
> > already. The suppress_ve bit is always set for all EPT pages. So this
> > operation here is going to be used to enable #VE for pages, not the
> > other way around. So there wouldn't be a case of "potentially be #VE
> > when there should be none".
>
> But this doesn't change the bottom line of my earlier comment: It's
> as bad to an OS to see too many #VE as it is to miss any that are
> expected.

Fair enough.

Tamas

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 2/2] x86/mm: Make use of the default access param from xc_altp2m_create_view
  2019-11-12 12:02   ` Jan Beulich
@ 2019-11-18  8:38     ` Alexandru Stefan ISAILA
  2019-11-18  9:53       ` Jan Beulich
  0 siblings, 1 reply; 25+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-11-18  8:38 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson, tamas,
	xen-devel, roger.pau



On 12.11.2019 14:02, Jan Beulich wrote:
> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -1345,13 +1345,14 @@ void setup_ept_dump(void)
>>       register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
>>   }
>>   
>> -void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>> +void p2m_init_altp2m_ept(struct domain *d, unsigned int i,
>> +                         p2m_access_t default_access)
>>   {
>>       struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>>       struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>       struct ept_data *ept;
>>   
>> -    p2m->default_access = hostp2m->default_access;
>> +    p2m->default_access = default_access;
>>       p2m->domain = hostp2m->domain;
>>   
>>       p2m->global_logdirty = hostp2m->global_logdirty;
> 
> All of this is not EPT-specific. Before adding more infrastructure to
> cover for this (here: another function parameter), how about moving
> these parts into vendor-independent code?

Ok, I will move the non ept code in p2m_activate_altp2m().

> 
>> @@ -2572,17 +2574,36 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
>>       altp2m_list_lock(d);
>>   
>>       if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>> -        rc = p2m_activate_altp2m(d, idx);
>> +        rc = p2m_activate_altp2m(d, idx, hostp2m->default_access);
>>   
>>       altp2m_list_unlock(d);
>>       return rc;
>>   }
>>   
>> -int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
>> +int p2m_init_next_altp2m(struct domain *d, uint16_t *idx,
>> +                         uint16_t hvmmem_default_access)
>>   {
>>       int rc = -EINVAL;
>>       unsigned int i;
>>   
>> +    static const p2m_access_t memaccess[] = {
>> +#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
>> +        ACCESS(n),
>> +        ACCESS(r),
>> +        ACCESS(w),
>> +        ACCESS(rw),
>> +        ACCESS(x),
>> +        ACCESS(rx),
>> +        ACCESS(wx),
>> +        ACCESS(rwx),
>> +        ACCESS(rx2rw),
>> +        ACCESS(n2rwx),
>> +#undef ACCESS
>> +    };
>> +
>> +    if ( hvmmem_default_access > XENMEM_access_default )
>> +        return rc;
>> +
>>       altp2m_list_lock(d);
>>   
>>       for ( i = 0; i < MAX_ALTP2M; i++ )
>> @@ -2590,7 +2611,7 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
>>           if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
>>               continue;
>>   
>> -        rc = p2m_activate_altp2m(d, i);
>> +        rc = p2m_activate_altp2m(d, i, memaccess[hvmmem_default_access]);
> 
> Aren't you open-coding xenmem_access_to_p2m_access() here? In
> no event should there be two instances of the same static array.

I did this because xenmem_access_to_p2m_access() is defined static in 
x86/mm/mem_access.c. If it's ok to have it defined in mem_access.h then 
I can go with that and drop this part of the code.

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 2/2] x86/mm: Make use of the default access param from xc_altp2m_create_view
  2019-11-18  8:38     ` Alexandru Stefan ISAILA
@ 2019-11-18  9:53       ` Jan Beulich
  2019-11-19 19:31         ` Tamas K Lengyel
  0 siblings, 1 reply; 25+ messages in thread
From: Jan Beulich @ 2019-11-18  9:53 UTC (permalink / raw)
  To: Alexandru Stefan ISAILA
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson, tamas,
	xen-devel, roger.pau

On 18.11.2019 09:38, Alexandru Stefan ISAILA wrote:
> On 12.11.2019 14:02, Jan Beulich wrote:
>> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
>>> @@ -2572,17 +2574,36 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
>>>       altp2m_list_lock(d);
>>>   
>>>       if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>>> -        rc = p2m_activate_altp2m(d, idx);
>>> +        rc = p2m_activate_altp2m(d, idx, hostp2m->default_access);
>>>   
>>>       altp2m_list_unlock(d);
>>>       return rc;
>>>   }
>>>   
>>> -int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
>>> +int p2m_init_next_altp2m(struct domain *d, uint16_t *idx,
>>> +                         uint16_t hvmmem_default_access)
>>>   {
>>>       int rc = -EINVAL;
>>>       unsigned int i;
>>>   
>>> +    static const p2m_access_t memaccess[] = {
>>> +#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
>>> +        ACCESS(n),
>>> +        ACCESS(r),
>>> +        ACCESS(w),
>>> +        ACCESS(rw),
>>> +        ACCESS(x),
>>> +        ACCESS(rx),
>>> +        ACCESS(wx),
>>> +        ACCESS(rwx),
>>> +        ACCESS(rx2rw),
>>> +        ACCESS(n2rwx),
>>> +#undef ACCESS
>>> +    };
>>> +
>>> +    if ( hvmmem_default_access > XENMEM_access_default )
>>> +        return rc;
>>> +
>>>       altp2m_list_lock(d);
>>>   
>>>       for ( i = 0; i < MAX_ALTP2M; i++ )
>>> @@ -2590,7 +2611,7 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
>>>           if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
>>>               continue;
>>>   
>>> -        rc = p2m_activate_altp2m(d, i);
>>> +        rc = p2m_activate_altp2m(d, i, memaccess[hvmmem_default_access]);
>>
>> Aren't you open-coding xenmem_access_to_p2m_access() here? In
>> no event should there be two instances of the same static array.
> 
> I did this because xenmem_access_to_p2m_access() is defined static in 
> x86/mm/mem_access.c. If it's ok to have it defined in mem_access.h then 
> I can go with that and drop this part of the code.

I see no reason why this wouldn't be a reasonable step, allowing to
avoid code duplication. Looks like the function is even suitably
named already for making non-static.

Jan

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-12 11:54 ` Jan Beulich
  2019-11-12 14:05   ` Tamas K Lengyel
@ 2019-11-18 13:39   ` Alexandru Stefan ISAILA
  2019-11-18 13:39   ` Alexandru Stefan ISAILA
  2 siblings, 0 replies; 25+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-11-18 13:39 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson, tamas,
	xen-devel, roger.pau



On 12.11.2019 13:54, Jan Beulich wrote:
> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
>> @@ -4681,7 +4682,7 @@ static int do_altp2m_op(
>>           break;
>>   
>>       case HVMOP_altp2m_set_suppress_ve:
>> -        if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
>> +        if ( a.u.suppress_ve.pad1 )
> 
> Just because the field changes its name doesn't mean you can
> drop the check. You even add a new field not used (yet) by
> this sub-function, which then also would need checking here.

I will revert the change and check the new field.

> 
>> @@ -4693,8 +4694,23 @@ static int do_altp2m_op(
>>           }
>>           break;
>>   
>> +    case HVMOP_altp2m_set_suppress_ve_multi:
>> +        if ( a.u.suppress_ve.pad1 || !a.u.suppress_ve.nr )
> 
> A count of zero typically is taken as a no-op, not an error.

I will return -EPERM for !nr.

> 
>> +            rc = -EINVAL;
>> +        else
>> +        {
>> +            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve);
>> +
>> +            if ( rc == -ERESTART )
>> +                if ( __copy_field_to_guest(guest_handle_cast(arg,
>> +                                           xen_hvm_altp2m_op_t),
>> +                                           &a, u.suppress_ve.opaque) )
>> +                    rc = -EFAULT;
> 
> If the operation is best effort, _some_ indication of failure should
> still be handed back to the caller. Whether that's through the opaque
> field or by some other means is secondary. If not via that field
> (which would make the outer of the two if()-s disappear), please fold
> the if()-s.

This can be solved by having a int error_list that will get 
"copy_to_guest_offset()" at the end.

> 
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -3054,6 +3054,64 @@ out:
>>       return rc;
>>   }
>>   
>> +/*
>> + * Set/clear the #VE suppress bit for multiple pages.  Only available on VMX.
>> + */
>> +int p2m_set_suppress_ve_multi(struct domain *d,
>> +                              struct xen_hvm_altp2m_suppress_ve* sve)
> 
> Misplaced *.

I've missed that, I'll have it the right way.

> 
>> +{
>> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
>> +    struct p2m_domain *ap2m = NULL;
>> +    struct p2m_domain *p2m;
>> +    uint64_t start = sve->opaque ?: sve->gfn;
> 
> According to this start (and hence ->opaque) are GFNs.
> 
>> +    int rc = 0;
>> +
>> +    if ( sve->view > 0 )
>> +    {
>> +        if ( sve->view >= MAX_ALTP2M ||
>> +             d->arch.altp2m_eptp[sve->view] == mfn_x(INVALID_MFN) )
>> +            return -EINVAL;
>> +
>> +        p2m = ap2m = d->arch.altp2m_p2m[sve->view];
>> +    }
>> +    else
>> +        p2m = host_p2m;
>> +
>> +    p2m_lock(host_p2m);
>> +
>> +    if ( ap2m )
>> +        p2m_lock(ap2m);
>> +
>> +
>> +    while ( start < sve->nr )
> 
> According to this, start is an index. Which of the two do you
> mean?

Start is a GFN.

> 
>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.h
>> @@ -42,8 +42,9 @@ struct xen_hvm_altp2m_suppress_ve {
>>       uint16_t view;
>>       uint8_t suppress_ve; /* Boolean type. */
>>       uint8_t pad1;
>> -    uint32_t pad2;
>> +    uint32_t nr;
>>       uint64_t gfn;
>> +    uint64_t opaque;
>>   };
> 
> How is this addition of a field going to work compatibly with old
> and new callers on old and new hypervisors? Recall in particular
> that these operations are (almost?) all potentially usable by the
> guest itself.
> 

For this HVMOP_ALTP2M_INTERFACE_VERSION shout be increased. I will leave 
it to Tamas to decide if we will need a different structure for 
xen_hvm_altp2m_suppress_ve_multi to keep the compatibility.

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-12 11:54 ` Jan Beulich
  2019-11-12 14:05   ` Tamas K Lengyel
  2019-11-18 13:39   ` Alexandru Stefan ISAILA
@ 2019-11-18 13:39   ` Alexandru Stefan ISAILA
  2019-11-18 14:09     ` Jan Beulich
  2 siblings, 1 reply; 25+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-11-18 13:39 UTC (permalink / raw)
  To: Jan Beulich, tamas
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson,
	xen-devel, roger.pau



On 12.11.2019 13:54, Jan Beulich wrote:
> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
>> @@ -4681,7 +4682,7 @@ static int do_altp2m_op(
>>           break;
>>   
>>       case HVMOP_altp2m_set_suppress_ve:
>> -        if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
>> +        if ( a.u.suppress_ve.pad1 )
> 
> Just because the field changes its name doesn't mean you can
> drop the check. You even add a new field not used (yet) by
> this sub-function, which then also would need checking here.

I will revert the change and check the new field.

> 
>> @@ -4693,8 +4694,23 @@ static int do_altp2m_op(
>>           }
>>           break;
>>   
>> +    case HVMOP_altp2m_set_suppress_ve_multi:
>> +        if ( a.u.suppress_ve.pad1 || !a.u.suppress_ve.nr )
> 
> A count of zero typically is taken as a no-op, not an error.

I will return -EPERM for !nr.

> 
>> +            rc = -EINVAL;
>> +        else
>> +        {
>> +            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve);
>> +
>> +            if ( rc == -ERESTART )
>> +                if ( __copy_field_to_guest(guest_handle_cast(arg,
>> +                                           xen_hvm_altp2m_op_t),
>> +                                           &a, u.suppress_ve.opaque) )
>> +                    rc = -EFAULT;
> 
> If the operation is best effort, _some_ indication of failure should
> still be handed back to the caller. Whether that's through the opaque
> field or by some other means is secondary. If not via that field
> (which would make the outer of the two if()-s disappear), please fold
> the if()-s.

This can be solved by having a int error_list that will get 
"copy_to_guest_offset()" at the end.

> 
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -3054,6 +3054,64 @@ out:
>>       return rc;
>>   }
>>   
>> +/*
>> + * Set/clear the #VE suppress bit for multiple pages.  Only available on VMX.
>> + */
>> +int p2m_set_suppress_ve_multi(struct domain *d,
>> +                              struct xen_hvm_altp2m_suppress_ve* sve)
> 
> Misplaced *.

I've missed that, I'll have it the right way.

> 
>> +{
>> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
>> +    struct p2m_domain *ap2m = NULL;
>> +    struct p2m_domain *p2m;
>> +    uint64_t start = sve->opaque ?: sve->gfn;
> 
> According to this start (and hence ->opaque) are GFNs.
> 
>> +    int rc = 0;
>> +
>> +    if ( sve->view > 0 )
>> +    {
>> +        if ( sve->view >= MAX_ALTP2M ||
>> +             d->arch.altp2m_eptp[sve->view] == mfn_x(INVALID_MFN) )
>> +            return -EINVAL;
>> +
>> +        p2m = ap2m = d->arch.altp2m_p2m[sve->view];
>> +    }
>> +    else
>> +        p2m = host_p2m;
>> +
>> +    p2m_lock(host_p2m);
>> +
>> +    if ( ap2m )
>> +        p2m_lock(ap2m);
>> +
>> +
>> +    while ( start < sve->nr )
> 
> According to this, start is an index. Which of the two do you
> mean?

Start is a GFN.

> 
>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.h
>> @@ -42,8 +42,9 @@ struct xen_hvm_altp2m_suppress_ve {
>>       uint16_t view;
>>       uint8_t suppress_ve; /* Boolean type. */
>>       uint8_t pad1;
>> -    uint32_t pad2;
>> +    uint32_t nr;
>>       uint64_t gfn;
>> +    uint64_t opaque;
>>   };
> 
> How is this addition of a field going to work compatibly with old
> and new callers on old and new hypervisors? Recall in particular
> that these operations are (almost?) all potentially usable by the
> guest itself.
> 

For this HVMOP_ALTP2M_INTERFACE_VERSION shout be increased. I will leave 
it to Tamas to decide if we will need a different structure for 
xen_hvm_altp2m_suppress_ve_multi to keep the compatibility.

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-18 13:39   ` Alexandru Stefan ISAILA
@ 2019-11-18 14:09     ` Jan Beulich
  2019-11-19  9:05       ` Alexandru Stefan ISAILA
  0 siblings, 1 reply; 25+ messages in thread
From: Jan Beulich @ 2019-11-18 14:09 UTC (permalink / raw)
  To: Alexandru Stefan ISAILA
  Cc: Petre Ovidiu PIRCALABU, tamas, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson,
	sstabellini, xen-devel, roger.pau

On 18.11.2019 14:39, Alexandru Stefan ISAILA wrote:
> On 12.11.2019 13:54, Jan Beulich wrote:
>> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
>>> @@ -4693,8 +4694,23 @@ static int do_altp2m_op(
>>>           }
>>>           break;
>>>   
>>> +    case HVMOP_altp2m_set_suppress_ve_multi:
>>> +        if ( a.u.suppress_ve.pad1 || !a.u.suppress_ve.nr )
>>
>> A count of zero typically is taken as a no-op, not an error.
> 
> I will return -EPERM for !nr.

How is -EPERM better than ...

>>> +            rc = -EINVAL;

... this, and hence how is it addressing my remark?

>>> +        else
>>> +        {
>>> +            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve);
>>> +
>>> +            if ( rc == -ERESTART )
>>> +                if ( __copy_field_to_guest(guest_handle_cast(arg,
>>> +                                           xen_hvm_altp2m_op_t),
>>> +                                           &a, u.suppress_ve.opaque) )
>>> +                    rc = -EFAULT;
>>
>> If the operation is best effort, _some_ indication of failure should
>> still be handed back to the caller. Whether that's through the opaque
>> field or by some other means is secondary. If not via that field
>> (which would make the outer of the two if()-s disappear), please fold
>> the if()-s.
> 
> This can be solved by having a int error_list that will get 
> "copy_to_guest_offset()" at the end.

I was actually not meaning to suggest to go _that_ far, but I
wouldn't mind such a full solution. Since there's a "get"
counterpart, I was rather thinking that an indication of "there
was _some_ error" might suffice, suggesting to the caller to
inspect which settings actually took effect. Such an indication
could e.g. be an index value identifying the first failed
operation.

>>> --- a/xen/include/public/hvm/hvm_op.h
>>> +++ b/xen/include/public/hvm/hvm_op.h
>>> @@ -42,8 +42,9 @@ struct xen_hvm_altp2m_suppress_ve {
>>>       uint16_t view;
>>>       uint8_t suppress_ve; /* Boolean type. */
>>>       uint8_t pad1;
>>> -    uint32_t pad2;
>>> +    uint32_t nr;
>>>       uint64_t gfn;
>>> +    uint64_t opaque;
>>>   };
>>
>> How is this addition of a field going to work compatibly with old
>> and new callers on old and new hypervisors? Recall in particular
>> that these operations are (almost?) all potentially usable by the
>> guest itself.
> 
> For this HVMOP_ALTP2M_INTERFACE_VERSION shout be increased. I will leave 
> it to Tamas to decide if we will need a different structure for 
> xen_hvm_altp2m_suppress_ve_multi to keep the compatibility.

Wasn't is that due to the possible guest exposure it was decided
that the interface version approach was not suitable here, and hence
it shouldn't be bumped any further?

Jan

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-18 14:09     ` Jan Beulich
@ 2019-11-19  9:05       ` Alexandru Stefan ISAILA
  2019-11-19  9:23         ` Jan Beulich
  0 siblings, 1 reply; 25+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-11-19  9:05 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Petre Ovidiu PIRCALABU, tamas, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson,
	sstabellini, xen-devel, roger.pau



On 18.11.2019 16:09, Jan Beulich wrote:
> On 18.11.2019 14:39, Alexandru Stefan ISAILA wrote:
>> On 12.11.2019 13:54, Jan Beulich wrote:
>>> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
>>>> @@ -4693,8 +4694,23 @@ static int do_altp2m_op(
>>>>            }
>>>>            break;
>>>>    
>>>> +    case HVMOP_altp2m_set_suppress_ve_multi:
>>>> +        if ( a.u.suppress_ve.pad1 || !a.u.suppress_ve.nr )
>>>
>>> A count of zero typically is taken as a no-op, not an error.
>>
>> I will return -EPERM for !nr.
> 
> How is -EPERM better than ...
> 
>>>> +            rc = -EINVAL;
> 
> ... this, and hence how is it addressing my remark?
> 
>>>> +        else
>>>> +        {
>>>> +            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve);
>>>> +
>>>> +            if ( rc == -ERESTART )
>>>> +                if ( __copy_field_to_guest(guest_handle_cast(arg,
>>>> +                                           xen_hvm_altp2m_op_t),
>>>> +                                           &a, u.suppress_ve.opaque) )
>>>> +                    rc = -EFAULT;
>>>
>>> If the operation is best effort, _some_ indication of failure should
>>> still be handed back to the caller. Whether that's through the opaque
>>> field or by some other means is secondary. If not via that field
>>> (which would make the outer of the two if()-s disappear), please fold
>>> the if()-s.
>>
>> This can be solved by having a int error_list that will get
>> "copy_to_guest_offset()" at the end.
> 
> I was actually not meaning to suggest to go _that_ far, but I
> wouldn't mind such a full solution. Since there's a "get"
> counterpart, I was rather thinking that an indication of "there
> was _some_ error" might suffice, suggesting to the caller to
> inspect which settings actually took effect. Such an indication
> could e.g. be an index value identifying the first failed
> operation.

This sound good, I can use the return for this or have a separate field 
in the structure.

> 
>>>> --- a/xen/include/public/hvm/hvm_op.h
>>>> +++ b/xen/include/public/hvm/hvm_op.h
>>>> @@ -42,8 +42,9 @@ struct xen_hvm_altp2m_suppress_ve {
>>>>        uint16_t view;
>>>>        uint8_t suppress_ve; /* Boolean type. */
>>>>        uint8_t pad1;
>>>> -    uint32_t pad2;
>>>> +    uint32_t nr;
>>>>        uint64_t gfn;
>>>> +    uint64_t opaque;
>>>>    };
>>>
>>> How is this addition of a field going to work compatibly with old
>>> and new callers on old and new hypervisors? Recall in particular
>>> that these operations are (almost?) all potentially usable by the
>>> guest itself.
>>
>> For this HVMOP_ALTP2M_INTERFACE_VERSION shout be increased. I will leave
>> it to Tamas to decide if we will need a different structure for
>> xen_hvm_altp2m_suppress_ve_multi to keep the compatibility.
> 
> Wasn't is that due to the possible guest exposure it was decided
> that the interface version approach was not suitable here, and hence
> it shouldn't be bumped any further?
> 

That is correct but there was also requested to add the new opaque field 
so I don't know how to have that an still keep the same version.

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-19  9:05       ` Alexandru Stefan ISAILA
@ 2019-11-19  9:23         ` Jan Beulich
  2019-11-20  8:29           ` Alexandru Stefan ISAILA
  0 siblings, 1 reply; 25+ messages in thread
From: Jan Beulich @ 2019-11-19  9:23 UTC (permalink / raw)
  To: Alexandru Stefan ISAILA
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson, tamas,
	xen-devel, roger.pau

On 19.11.2019 10:05, Alexandru Stefan ISAILA wrote:
> On 18.11.2019 16:09, Jan Beulich wrote:
>> On 18.11.2019 14:39, Alexandru Stefan ISAILA wrote:
>>> For this HVMOP_ALTP2M_INTERFACE_VERSION shout be increased. I will leave
>>> it to Tamas to decide if we will need a different structure for
>>> xen_hvm_altp2m_suppress_ve_multi to keep the compatibility.
>>
>> Wasn't is that due to the possible guest exposure it was decided
>> that the interface version approach was not suitable here, and hence
>> it shouldn't be bumped any further?
>>
> 
> That is correct but there was also requested to add the new opaque field 
> so I don't know how to have that an still keep the same version.

New sub-op?

Jan

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 2/2] x86/mm: Make use of the default access param from xc_altp2m_create_view
  2019-11-18  9:53       ` Jan Beulich
@ 2019-11-19 19:31         ` Tamas K Lengyel
  0 siblings, 0 replies; 25+ messages in thread
From: Tamas K Lengyel @ 2019-11-19 19:31 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson,
	Alexandru Stefan ISAILA, xen-devel, roger.pau

On Mon, Nov 18, 2019 at 2:53 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 18.11.2019 09:38, Alexandru Stefan ISAILA wrote:
> > On 12.11.2019 14:02, Jan Beulich wrote:
> >> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
> >>> @@ -2572,17 +2574,36 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
> >>>       altp2m_list_lock(d);
> >>>
> >>>       if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
> >>> -        rc = p2m_activate_altp2m(d, idx);
> >>> +        rc = p2m_activate_altp2m(d, idx, hostp2m->default_access);
> >>>
> >>>       altp2m_list_unlock(d);
> >>>       return rc;
> >>>   }
> >>>
> >>> -int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
> >>> +int p2m_init_next_altp2m(struct domain *d, uint16_t *idx,
> >>> +                         uint16_t hvmmem_default_access)
> >>>   {
> >>>       int rc = -EINVAL;
> >>>       unsigned int i;
> >>>
> >>> +    static const p2m_access_t memaccess[] = {
> >>> +#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
> >>> +        ACCESS(n),
> >>> +        ACCESS(r),
> >>> +        ACCESS(w),
> >>> +        ACCESS(rw),
> >>> +        ACCESS(x),
> >>> +        ACCESS(rx),
> >>> +        ACCESS(wx),
> >>> +        ACCESS(rwx),
> >>> +        ACCESS(rx2rw),
> >>> +        ACCESS(n2rwx),
> >>> +#undef ACCESS
> >>> +    };
> >>> +
> >>> +    if ( hvmmem_default_access > XENMEM_access_default )
> >>> +        return rc;
> >>> +
> >>>       altp2m_list_lock(d);
> >>>
> >>>       for ( i = 0; i < MAX_ALTP2M; i++ )
> >>> @@ -2590,7 +2611,7 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
> >>>           if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
> >>>               continue;
> >>>
> >>> -        rc = p2m_activate_altp2m(d, i);
> >>> +        rc = p2m_activate_altp2m(d, i, memaccess[hvmmem_default_access]);
> >>
> >> Aren't you open-coding xenmem_access_to_p2m_access() here? In
> >> no event should there be two instances of the same static array.
> >
> > I did this because xenmem_access_to_p2m_access() is defined static in
> > x86/mm/mem_access.c. If it's ok to have it defined in mem_access.h then
> > I can go with that and drop this part of the code.
>
> I see no reason why this wouldn't be a reasonable step, allowing to
> avoid code duplication. Looks like the function is even suitably
> named already for making non-static.

Sounds fine to me too.

Thanks,
Tamas

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-19  9:23         ` Jan Beulich
@ 2019-11-20  8:29           ` Alexandru Stefan ISAILA
  2019-11-20  8:41             ` Jan Beulich
  0 siblings, 1 reply; 25+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-11-20  8:29 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson, tamas,
	xen-devel, roger.pau

On 19.11.2019 11:23, Jan Beulich wrote:
> On 19.11.2019 10:05, Alexandru Stefan ISAILA wrote:
>> On 18.11.2019 16:09, Jan Beulich wrote:
>>> On 18.11.2019 14:39, Alexandru Stefan ISAILA wrote:
>>>> For this HVMOP_ALTP2M_INTERFACE_VERSION shout be increased. I will leave
>>>> it to Tamas to decide if we will need a different structure for
>>>> xen_hvm_altp2m_suppress_ve_multi to keep the compatibility.
>>>
>>> Wasn't is that due to the possible guest exposure it was decided
>>> that the interface version approach was not suitable here, and hence
>>> it shouldn't be bumped any further?
>>>
>>
>> That is correct but there was also requested to add the new opaque field
>> so I don't know how to have that an still keep the same version.
> 
> New sub-op?
> 

Wouldn't it be simpler to have a new structure to use for this, 
something like "struct xen_hvm_altp2m_suppress_ve_multi" and then the 
version will be unchanged

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-20  8:29           ` Alexandru Stefan ISAILA
@ 2019-11-20  8:41             ` Jan Beulich
  2019-11-20  8:48               ` Alexandru Stefan ISAILA
  0 siblings, 1 reply; 25+ messages in thread
From: Jan Beulich @ 2019-11-20  8:41 UTC (permalink / raw)
  To: Alexandru Stefan ISAILA
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson, tamas,
	xen-devel, roger.pau

On 20.11.2019 09:29, Alexandru Stefan ISAILA wrote:
> On 19.11.2019 11:23, Jan Beulich wrote:
>> On 19.11.2019 10:05, Alexandru Stefan ISAILA wrote:
>>> On 18.11.2019 16:09, Jan Beulich wrote:
>>>> On 18.11.2019 14:39, Alexandru Stefan ISAILA wrote:
>>>>> For this HVMOP_ALTP2M_INTERFACE_VERSION shout be increased. I will leave
>>>>> it to Tamas to decide if we will need a different structure for
>>>>> xen_hvm_altp2m_suppress_ve_multi to keep the compatibility.
>>>>
>>>> Wasn't is that due to the possible guest exposure it was decided
>>>> that the interface version approach was not suitable here, and hence
>>>> it shouldn't be bumped any further?
>>>>
>>>
>>> That is correct but there was also requested to add the new opaque field
>>> so I don't know how to have that an still keep the same version.
>>
>> New sub-op?
> 
> Wouldn't it be simpler to have a new structure to use for this, 
> something like "struct xen_hvm_altp2m_suppress_ve_multi" and then the 
> version will be unchanged

Well, if the original sub-op is to remain entirely untouched,
then yes, sure. I have to admit that in the course of the
discussion it became decreasingly clear whether the original
sub-op also wanted some for of adjustment.

Jan

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits
  2019-11-20  8:41             ` Jan Beulich
@ 2019-11-20  8:48               ` Alexandru Stefan ISAILA
  0 siblings, 0 replies; 25+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-11-20  8:48 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Petre Ovidiu PIRCALABU, sstabellini, julien, wl, Razvan COJOCARU,
	konrad.wilk, george.dunlap, andrew.cooper3, ian.jackson, tamas,
	xen-devel, roger.pau



On 20.11.2019 10:41, Jan Beulich wrote:
> On 20.11.2019 09:29, Alexandru Stefan ISAILA wrote:
>> On 19.11.2019 11:23, Jan Beulich wrote:
>>> On 19.11.2019 10:05, Alexandru Stefan ISAILA wrote:
>>>> On 18.11.2019 16:09, Jan Beulich wrote:
>>>>> On 18.11.2019 14:39, Alexandru Stefan ISAILA wrote:
>>>>>> For this HVMOP_ALTP2M_INTERFACE_VERSION shout be increased. I will leave
>>>>>> it to Tamas to decide if we will need a different structure for
>>>>>> xen_hvm_altp2m_suppress_ve_multi to keep the compatibility.
>>>>>
>>>>> Wasn't is that due to the possible guest exposure it was decided
>>>>> that the interface version approach was not suitable here, and hence
>>>>> it shouldn't be bumped any further?
>>>>>
>>>>
>>>> That is correct but there was also requested to add the new opaque field
>>>> so I don't know how to have that an still keep the same version.
>>>
>>> New sub-op?
>>
>> Wouldn't it be simpler to have a new structure to use for this,
>> something like "struct xen_hvm_altp2m_suppress_ve_multi" and then the
>> version will be unchanged
> 
> Well, if the original sub-op is to remain entirely untouched,
> then yes, sure. I have to admit that in the course of the
> discussion it became decreasingly clear whether the original
> sub-op also wanted some for of adjustment.
> 

I agree with the clearness here. So the original sub-op will remain 
untouched.

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2019-11-20  8:48 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-06 15:35 [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits Alexandru Stefan ISAILA
2019-11-06 15:35 ` [Xen-devel] [PATCH V2 2/2] x86/mm: Make use of the default access param from xc_altp2m_create_view Alexandru Stefan ISAILA
2019-11-12 12:02   ` Jan Beulich
2019-11-18  8:38     ` Alexandru Stefan ISAILA
2019-11-18  9:53       ` Jan Beulich
2019-11-19 19:31         ` Tamas K Lengyel
2019-11-06 21:06 ` [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits Tamas K Lengyel
2019-11-07  7:46   ` Alexandru Stefan ISAILA
2019-11-07 15:00     ` Tamas K Lengyel
2019-11-08  8:31 ` Alexandru Stefan ISAILA
2019-11-12 11:54 ` Jan Beulich
2019-11-12 14:05   ` Tamas K Lengyel
2019-11-12 14:31     ` Jan Beulich
2019-11-13 14:51       ` Tamas K Lengyel
2019-11-13 14:57         ` Tamas K Lengyel
2019-11-13 16:52           ` Jan Beulich
2019-11-13 18:38             ` Tamas K Lengyel
2019-11-18 13:39   ` Alexandru Stefan ISAILA
2019-11-18 13:39   ` Alexandru Stefan ISAILA
2019-11-18 14:09     ` Jan Beulich
2019-11-19  9:05       ` Alexandru Stefan ISAILA
2019-11-19  9:23         ` Jan Beulich
2019-11-20  8:29           ` Alexandru Stefan ISAILA
2019-11-20  8:41             ` Jan Beulich
2019-11-20  8:48               ` Alexandru Stefan ISAILA

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.