[PATCH v4] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[PATCH v4] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Petre Pircalabu]

From: Razvan Cojocaru <rcojocaru@bitdefender.com>

For the default EPT view we have xc_set_mem_access_multi(), which
is able to set an array of pages to an array of access rights with
a single hypercall. However, this functionality was lacking for the
altp2m subsystem, which could only set page restrictions for one
page at a time. This patch addresses the gap.

HVMOP_altp2m_set_mem_access_multi has been added as a HVMOP (as opposed to a
DOMCTL) for consistency with its HVMOP_altp2m_set_mem_access counterpart (and
hence with the original altp2m design, where domains are allowed - with the
proper altp2m access rights - to alter these settings), in the absence of an
official position on the issue from the original altp2m designers.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>

---

Changed since v2:
    * Added support for compat arguments translation

Changed since v3:
    * Replaced  __copy_to_guest with __copy_field_to_guest
    * Removed the un-needed parentheses.
    * Fixed xlat.lst ordering
    * Added comment to patch description explaining why the
    functionality was added as an HVMOP.
    * Guard using XEN_GENERATING_COMPAT_HEADERS the hvmmem_type_t definition.
    This will prevent suplicate definitions to be generated for the
    compat equivalent.
    * Added comment describing the manual translation of
    xen_hvm_altp2m_op_t generic fields from compat_hvm_altp2m_opt.
---
 tools/libxc/include/xenctrl.h   |  3 ++
 tools/libxc/xc_altp2m.c         | 41 +++++++++++++++++++++
 xen/arch/x86/hvm/hvm.c          | 80 ++++++++++++++++++++++++++++++++++++++++-
 xen/include/Makefile            |  1 +
 xen/include/public/hvm/hvm_op.h | 39 +++++++++++++++++---
 xen/include/xlat.lst            |  1 +
 6 files changed, 159 insertions(+), 6 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 3bcab3c..4e2ce64 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1971,6 +1971,9 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
 int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
                              uint16_t view_id, xen_pfn_t gfn,
                              xenmem_access_t access);
+int xc_altp2m_set_mem_access_multi(xc_interface *handle, domid_t domid,
+                                   uint16_t view_id, uint8_t *access,
+                                   uint64_t *pages, uint32_t nr);
 int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
                          uint16_t view_id, xen_pfn_t old_gfn,
                          xen_pfn_t new_gfn);
diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index 0639632..f202ca1 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -188,6 +188,47 @@ int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
     return rc;
 }
 
+int xc_altp2m_set_mem_access_multi(xc_interface *xch, domid_t domid,
+                                   uint16_t view_id, uint8_t *access,
+                                   uint64_t *pages, uint32_t nr)
+{
+    int rc;
+
+    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
+    DECLARE_HYPERCALL_BOUNCE(access, nr, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+    DECLARE_HYPERCALL_BOUNCE(pages, nr * sizeof(uint64_t),
+                             XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+    arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
+    if ( arg == NULL )
+        return -1;
+
+    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
+    arg->cmd = HVMOP_altp2m_set_mem_access_multi;
+    arg->domain = domid;
+    arg->u.set_mem_access_multi.view = view_id;
+    arg->u.set_mem_access_multi.nr = nr;
+
+    if ( xc_hypercall_bounce_pre(xch, pages) ||
+         xc_hypercall_bounce_pre(xch, access) )
+    {
+        PERROR("Could not bounce memory for HVMOP_altp2m_set_mem_access_multi");
+        return -1;
+    }
+
+    set_xen_guest_handle(arg->u.set_mem_access_multi.pfn_list, pages);
+    set_xen_guest_handle(arg->u.set_mem_access_multi.access_list, access);
+
+    rc = xencall2(xch->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
+		  HYPERCALL_BUFFER_AS_ARG(arg));
+
+    xc_hypercall_buffer_free(xch, arg);
+    xc_hypercall_bounce_post(xch, access);
+    xc_hypercall_bounce_post(xch, pages);
+
+    return rc;
+}
+
 int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
                          uint16_t view_id, xen_pfn_t old_gfn,
                          xen_pfn_t new_gfn)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 205b4cb..9b5302a 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -73,6 +73,8 @@
 #include <public/arch-x86/cpuid.h>
 #include <asm/cpuid.h>
 
+#include <compat/hvm/hvm_op.h>
+
 bool_t __read_mostly hvm_enabled;
 
 #ifdef DBG_LEVEL_0
@@ -4451,6 +4453,7 @@ static int do_altp2m_op(
     case HVMOP_altp2m_destroy_p2m:
     case HVMOP_altp2m_switch_p2m:
     case HVMOP_altp2m_set_mem_access:
+    case HVMOP_altp2m_set_mem_access_multi:
     case HVMOP_altp2m_change_gfn:
         break;
     default:
@@ -4568,6 +4571,30 @@ static int do_altp2m_op(
                                     a.u.set_mem_access.view);
         break;
 
+    case HVMOP_altp2m_set_mem_access_multi:
+        if ( a.u.set_mem_access_multi.pad ||
+             a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr )
+        {
+            rc = -EINVAL;
+            break;
+        }
+        rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
+                                      a.u.set_mem_access_multi.access_list,
+                                      a.u.set_mem_access_multi.nr,
+                                      a.u.set_mem_access_multi.opaque,
+                                      MEMOP_CMD_MASK,
+                                      a.u.set_mem_access_multi.view);
+        if ( rc > 0 )
+        {
+            a.u.set_mem_access_multi.opaque = rc;
+            if ( __copy_field_to_guest(guest_handle_cast(arg, xen_hvm_altp2m_op_t), &a, u.set_mem_access_multi.opaque) )
+                rc = -EFAULT;
+            else
+                rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
+                                                   HVMOP_altp2m, arg);
+        }
+        break;
+
     case HVMOP_altp2m_change_gfn:
         if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
             rc = -EINVAL;
@@ -4586,6 +4613,57 @@ static int do_altp2m_op(
     return rc;
 }
 
+static int compat_altp2m_op(
+    XEN_GUEST_HANDLE_PARAM(void) arg)
+{
+    struct compat_hvm_altp2m_op a;
+    union
+    {
+        XEN_GUEST_HANDLE_PARAM(void) hnd;
+        struct xen_hvm_altp2m_op *altp2m_op;
+    } nat;
+
+    if ( !hvm_altp2m_supported() )
+        return -EOPNOTSUPP;
+
+    if ( copy_from_guest(&a, arg, 1) )
+        return -EFAULT;
+
+    if ( a.pad1 || a.pad2 ||
+         (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) )
+        return -EINVAL;
+
+    set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE);
+
+    switch ( a.cmd )
+    {
+        case HVMOP_altp2m_set_mem_access_multi:
+#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_, _s_); \
+            guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list)
+#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_, _s_); \
+            guest_from_compat_handle((_d_)->access_list, (_s_)->access_list)
+            XLAT_hvm_altp2m_set_mem_access_multi(&nat.altp2m_op->u.set_mem_access_multi,
+                    &a.u.set_mem_access_multi);
+#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list
+#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list
+            break;
+        default:
+            return do_altp2m_op(arg);
+    }
+
+    /* Manually fill the common part of the xen_hvm_altp2m_op structure because
+     * the generated XLAT_hvm_altp2m_op macro doesn't correctly handle the
+     * translation of all fields from compat_hvm_altp2m_op to xen_hvm_altp2m_op.
+     */
+    nat.altp2m_op->version  = a.version;
+    nat.altp2m_op->cmd      = a.cmd;
+    nat.altp2m_op->domain   = a.domain;
+    nat.altp2m_op->pad1     = a.pad1;
+    nat.altp2m_op->pad2     = a.pad2;
+
+    return do_altp2m_op(nat.hnd);
+}
+
 static int hvmop_get_mem_type(
     XEN_GUEST_HANDLE_PARAM(xen_hvm_get_mem_type_t) arg)
 {
@@ -4733,7 +4811,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
 
     case HVMOP_altp2m:
-        rc = do_altp2m_op(arg);
+        rc = current->hcall_compat ? compat_altp2m_op(arg) : do_altp2m_op(arg);
         break;
 
     default:
diff --git a/xen/include/Makefile b/xen/include/Makefile
index c90fdee..814b0a8 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -28,6 +28,7 @@ headers-$(CONFIG_X86)     += compat/arch-x86/xen.h
 headers-$(CONFIG_X86)     += compat/arch-x86/xen-$(compat-arch-y).h
 headers-$(CONFIG_X86)     += compat/hvm/hvm_vcpu.h
 headers-$(CONFIG_X86)     += compat/hvm/dm_op.h
+headers-$(CONFIG_X86)     += compat/hvm/hvm_op.h
 headers-y                 += compat/arch-$(compat-arch-y).h compat/pmu.h compat/xlat.h
 headers-$(CONFIG_FLASK)   += compat/xsm/flask_op.h
 
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index 0bdafdf..c12c1af 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -83,6 +83,13 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_pci_link_route_t);
 /* Flushes all VCPU TLBs: @arg must be NULL. */
 #define HVMOP_flush_tlbs          5
 
+/*
+ * hvmmem_type_t should not be defined when generating the corresponding
+ * compat header. This will ensure that the HVMMEM_(*) values are defined
+ * only once.
+ */
+#ifndef XEN_GENERATING_COMPAT_HEADERS
+
 typedef enum {
     HVMMEM_ram_rw,             /* Normal read/write guest RAM */
     HVMMEM_ram_ro,             /* Read-only; writes are discarded */
@@ -102,6 +109,8 @@ typedef enum {
                                   to HVMMEM_ram_rw. */
 } hvmmem_type_t;
 
+#endif /* XEN_GENERATING_COMPAT_HEADERS */
+
 /* Hint from PV drivers for pagetable destruction. */
 #define HVMOP_pagetable_dying        9
 struct xen_hvm_pagetable_dying {
@@ -237,6 +246,23 @@ struct xen_hvm_altp2m_set_mem_access {
 typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
 DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
 
+struct xen_hvm_altp2m_set_mem_access_multi {
+    /* view */
+    uint16_t view;
+    uint16_t pad;
+    /* Number of pages */
+    uint32_t nr;
+    /* Used for continuation purposes */
+    uint64_t opaque;
+    /* List of pfns to set access for */
+    XEN_GUEST_HANDLE(const_uint64) pfn_list;
+    /* Corresponding list of access settings for pfn_list */
+    XEN_GUEST_HANDLE(const_uint8) access_list;
+};
+typedef struct xen_hvm_altp2m_set_mem_access_multi
+    xen_hvm_altp2m_set_mem_access_multi_t;
+DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_multi_t);
+
 struct xen_hvm_altp2m_change_gfn {
     /* view */
     uint16_t view;
@@ -268,15 +294,18 @@ struct xen_hvm_altp2m_op {
 #define HVMOP_altp2m_set_mem_access       7
 /* Change a p2m entry to have a different gfn->mfn mapping */
 #define HVMOP_altp2m_change_gfn           8
+/* Set access for an array of pages */
+#define HVMOP_altp2m_set_mem_access_multi 9
     domid_t domain;
     uint16_t pad1;
     uint32_t pad2;
     union {
-        struct xen_hvm_altp2m_domain_state       domain_state;
-        struct xen_hvm_altp2m_vcpu_enable_notify enable_notify;
-        struct xen_hvm_altp2m_view               view;
-        struct xen_hvm_altp2m_set_mem_access     set_mem_access;
-        struct xen_hvm_altp2m_change_gfn         change_gfn;
+        struct xen_hvm_altp2m_domain_state         domain_state;
+        struct xen_hvm_altp2m_vcpu_enable_notify   enable_notify;
+        struct xen_hvm_altp2m_view                 view;
+        struct xen_hvm_altp2m_set_mem_access       set_mem_access;
+        struct xen_hvm_altp2m_change_gfn           change_gfn;
+        struct xen_hvm_altp2m_set_mem_access_multi set_mem_access_multi;
         uint8_t pad[64];
     } u;
 };
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index 0f17000..5010fcc 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -70,6 +70,7 @@
 ?	dm_op_set_pci_intx_level	hvm/dm_op.h
 ?	dm_op_set_pci_link_route	hvm/dm_op.h
 ?	dm_op_track_dirty_vram		hvm/dm_op.h
+!	hvm_altp2m_set_mem_access_multi	hvm/hvm_op.h
 ?	vcpu_hvm_context		hvm/hvm_vcpu.h
 ?	vcpu_hvm_x86_32			hvm/hvm_vcpu.h
 ?	vcpu_hvm_x86_64			hvm/hvm_vcpu.h
-- 
2.7.4


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

Re: [PATCH v4] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Jan Beulich]

>>> On 09.10.17 at 19:30, <ppircalabu@bitdefender.com> wrote:
> @@ -4568,6 +4571,30 @@ static int do_altp2m_op(
>                                      a.u.set_mem_access.view);
>          break;
>  
> +    case HVMOP_altp2m_set_mem_access_multi:
> +        if ( a.u.set_mem_access_multi.pad ||
> +             a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
> +        rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
> +                                      a.u.set_mem_access_multi.access_list,
> +                                      a.u.set_mem_access_multi.nr,
> +                                      a.u.set_mem_access_multi.opaque,
> +                                      MEMOP_CMD_MASK,

This not being a memop, re-using this value looks arbitrary.
Unless it absolutely has to be this value, please either add a brief
comment saying this just happens to fit your need or use a literal
constant.

> +                                      a.u.set_mem_access_multi.view);
> +        if ( rc > 0 )
> +        {
> +            a.u.set_mem_access_multi.opaque = rc;
> +            if ( __copy_field_to_guest(guest_handle_cast(arg, xen_hvm_altp2m_op_t), &a, u.set_mem_access_multi.opaque) )

Long line.

Also please consider adding a prereq patch changing the function's
parameter to a properly typed handle, which will then make
unnecessary using the not obviously correct cast here. Other
copying back of individual fields in this function could then also be
switched to __copy_field_to_guest().

> @@ -4586,6 +4613,57 @@ static int do_altp2m_op(
>      return rc;
>  }
>  
> +static int compat_altp2m_op(
> +    XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> +    struct compat_hvm_altp2m_op a;
> +    union
> +    {
> +        XEN_GUEST_HANDLE_PARAM(void) hnd;
> +        struct xen_hvm_altp2m_op *altp2m_op;
> +    } nat;
> +
> +    if ( !hvm_altp2m_supported() )
> +        return -EOPNOTSUPP;
> +
> +    if ( copy_from_guest(&a, arg, 1) )
> +        return -EFAULT;
> +
> +    if ( a.pad1 || a.pad2 ||
> +         (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) )
> +        return -EINVAL;
> +
> +    set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE);
> +
> +    switch ( a.cmd )
> +    {
> +        case HVMOP_altp2m_set_mem_access_multi:
> +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_, _s_); \
> +            guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list)
> +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_, _s_); \
> +            guest_from_compat_handle((_d_)->access_list, (_s_)->access_list)
> +            XLAT_hvm_altp2m_set_mem_access_multi(&nat.altp2m_op->u.set_mem_access_multi,
> +                    &a.u.set_mem_access_multi);
> +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list
> +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list
> +            break;
> +        default:
> +            return do_altp2m_op(arg);
> +    }
> +
> +    /* Manually fill the common part of the xen_hvm_altp2m_op structure because

Comment style.

> +     * the generated XLAT_hvm_altp2m_op macro doesn't correctly handle the
> +     * translation of all fields from compat_hvm_altp2m_op to xen_hvm_altp2m_op.
> +     */
> +    nat.altp2m_op->version  = a.version;
> +    nat.altp2m_op->cmd      = a.cmd;
> +    nat.altp2m_op->domain   = a.domain;
> +    nat.altp2m_op->pad1     = a.pad1;
> +    nat.altp2m_op->pad2     = a.pad2;

I specifically asked (and even more than once, I think) that you add
the size (and whatever else) checks we would have here if this wasn't
open coding an XLAT_* macro.

> +    return do_altp2m_op(nat.hnd);

You appear to miss copying back opaque here, in case it was set
in do_altp2m_op().

> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -83,6 +83,13 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_pci_link_route_t);
>  /* Flushes all VCPU TLBs: @arg must be NULL. */
>  #define HVMOP_flush_tlbs          5
>  
> +/*
> + * hvmmem_type_t should not be defined when generating the corresponding
> + * compat header. This will ensure that the HVMMEM_(*) values are defined
> + * only once.

To help readers fully understand the situation, please consider
making this "This will ensure that the improperly named
HVMMEM_(*) values ..."; otherwise it gives the appearance of
there being a bug in the translation scripts.

> @@ -237,6 +246,23 @@ struct xen_hvm_altp2m_set_mem_access {
>  typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
>  
> +struct xen_hvm_altp2m_set_mem_access_multi {
> +    /* view */
> +    uint16_t view;
> +    uint16_t pad;
> +    /* Number of pages */
> +    uint32_t nr;
> +    /* Used for continuation purposes */
> +    uint64_t opaque;
> +    /* List of pfns to set access for */
> +    XEN_GUEST_HANDLE(const_uint64) pfn_list;
> +    /* Corresponding list of access settings for pfn_list */
> +    XEN_GUEST_HANDLE(const_uint8) access_list;
> +};
> +typedef struct xen_hvm_altp2m_set_mem_access_multi
> +    xen_hvm_altp2m_set_mem_access_multi_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_multi_t);

Are typedef and handle actually needed anywhere? Otherwise
please don't add them. Just like recently done for domctl and
sysctl we should even consider cleaning up the others here.

Jan

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

Re: [PATCH v4] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Petre Ovidiu PIRCALABU]

On Ma, 2017-10-10 at 06:28 -0600, Jan Beulich wrote:> > 
> > > 
> > > > 
> > > > a.u.set_mem_access_multi.pfn_list,
> > +                                      a.u.set_mem_access_multi.acc
> > ess_list,
> > +                                      a.u.set_mem_access_multi.nr,
> > +                                      a.u.set_mem_access_multi.opa
> > que,
> > +                                      MEMOP_CMD_MASK,
> This not being a memop, re-using this value looks arbitrary.
> Unless it absolutely has to be this value, please either add a brief
> comment saying this just happens to fit your need or use a literal
> constant.
I will add a comment to clarify the reason for using MEMOP_CMD_MASK:
e.g.
/* MEMOP_CMD_MASK is used here to mirror the way
p2m_set_mem_access_multi() is called for the
XENMEM_access_op_set_access_multi case. */
> 
> > 
> > +                                      a.u.set_mem_access_multi.vie
> > w);
> > +        if ( rc > 0 )
> > +        {
> > +            a.u.set_mem_access_multi.opaque = rc;
> > +            if ( __copy_field_to_guest(guest_handle_cast(arg,
> > xen_hvm_altp2m_op_t), &a, u.set_mem_access_multi.opaque) )
> Long line.
> 
> Also please consider adding a prereq patch changing the function's
> parameter to a properly typed handle, which will then make
> unnecessary using the not obviously correct cast here. Other
> copying back of individual fields in this function could then also be
> switched to __copy_field_to_guest().

Actually, changing the function's parameter to a typed handle could
complicate things a little for compat_altp2m_op. It should be modified
also to use a typed handle (compat_hvm_altp2m_op_t), which in case of
commands other than HVMOP_altp2m_set_mem_access_multi should be casted
to xen_hvm_altp2m_op_t before calling do_altp2m_op.
If the problem was only related to the code clarity, I think the new
implementation will be a little more difficult to follow.
> 
> > 
> > @@ -4586,6 +4613,57 @@ static int do_altp2m_op(
> >      return rc;
> >  }
> >  
> > +static int compat_altp2m_op(
> > +    XEN_GUEST_HANDLE_PARAM(void) arg)
> > +{
> > +    struct compat_hvm_altp2m_op a;
> > +    union
> > +    {
> > +        XEN_GUEST_HANDLE_PARAM(void) hnd;
> > +        struct xen_hvm_altp2m_op *altp2m_op;
> > +    } nat;
> > +
> > +    if ( !hvm_altp2m_supported() )
> > +        return -EOPNOTSUPP;
> > +
> > +    if ( copy_from_gueWill change in the next patch iteration.
> 
st(&a, arg, 1) )
> > +        return -EFAULT;
> > +
> > +    if ( a.pad1 || a.pad2 ||
> > +         (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) )
> > +        return -EINVAL;
> > +
> > +    set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE);
> > +
> > +    switch ( a.cmd )
> > +    {
> > +        case HVMOP_altp2m_set_mem_access_multi:
> > +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_,
> > _s_); \
> > +            guest_from_compat_handle((_d_)->pfn_list, (_s_)-
> > >pfn_list)
> > +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_,
> > _s_); \
> > +            guest_from_compat_handle((_d_)->access_list, (_s_)-
> > >access_list)
> > +            XLAT_hvm_altp2m_set_mem_access_multi(&nat.altp2m_op-
> > >u.set_mem_access_multi,
> > +                    &a.u.set_mem_access_multi);
> > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list
> > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list
> > +            break;
> > +        default:
> > +            return do_altp2m_op(arg);
> > +    }
> > +
> > +    /* Manually fill the common part of the xen_hvm_altp2m_op
> > structure because
> Comment style.
Will change in the next patch iteration.
> 
> > 
> > +     * the generated XLAT_hvm_altp2m_op macro doesn't correctly
> > handle the
> > +     * translation of all fields from compat_hvm_altp2m_op to
> > xen_hvm_altp2m_op.
> > +     */
> > +    nat.altp2m_op->version  = a.version;
> > +    nat.altp2m_op->cmd      = a.cmd;
> > +    nat.altp2m_op->domain   = a.domain;
> > +    nat.altp2m_op->pad1     = a.pad1;
> > +    nat.altp2m_op->pad2     = a.pad2;
> I specifically asked (and even more than once, I think) that you add
> the size (and whatever else) checks we would have here if this wasn't
> open coding an XLAT_* macro.
I ran into a build error while trying to use generated CHECK_ macros. I
will try to add a manual check here.
> 
> > 
> > +    return do_altp2m_op(nat.hnd);
> You appear to miss copying back opaque here, in case it was set
> in do_altp2m_op().
> 
> > 
> > --- a/xen/include/public/hvm/hvm_op.h
> > +++ b/xen/include/public/hvm/hvm_op.h
> > @@ -83,6 +83,13 @@
> > DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_pci_link_route_t);
> >  /* Flushes all VCPU TLBs: @arg must be NULL. */
> >  #define HVMOP_flush_tlbs          5
> >  
> > +/*
> > + * hvmmem_type_t should not be defined when generating the
> > corresponding
> > + * compat header. This will ensure that the HVMMEM_(*) values are
> > defined
> > + * only once.
> To help readers fully understand the situation, please consider
> making this "This will ensure that the improperly named
> HVMMEM_(*) values ..."; otherwise it gives the appearance of
> there being a bug in the translation scripts.
Will change in the next patch iteration.
> 
Will change in the next patch iteration.
> 
> 
> > 
> > @@ -237,6 +246,23 @@ struct xen_hvm_altp2m_set_mem_access {
> >  typedef struct xen_hvm_altp2m_set_mem_access
> > xen_hvm_altp2m_set_mem_access_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
> >  
> > +struct xen_hvm_altp2m_set_mem_access_multi {
> > +    /* view */
> > +    uint16_t view;
> > +    uint16_t pad;
> > +    /* Number of pages */
> > +    uint32_t nr;
> > +    /* Used for continuation purposes */
> > +    uint64_t opaque;
> > +    /* List of pfns to set access for */
> > +    XEN_GUEST_HANDLE(const_uint64) pfn_list;
> > +    /* Corresponding list of access settings for pfn_list */
> > +    XEN_GUEST_HANDLE(const_uint8) access_list;
> > +};
> > +typedef struct xen_hvm_altp2m_set_mem_access_multi
> > +    xen_hvm_altp2m_set_mem_access_multi_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_multi_t);
> Are typedef and handle actually needed anywhere? Otherwise
> please don't add them. Just like recently done for domctl and
> sysctl we should even consider cleaning up the others here.
Will change in the next patch iteration.
> 
> 
> Jan
> 
> 

Many thanks for your support,
Petre
> ________________________
> This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Jan Beulich]

>>> On 10.10.17 at 16:13, <ppircalabu@bitdefender.com> wrote:
> On Ma, 2017-10-10 at 06:28 -0600, Jan Beulich wrote:> > 
>> > > 
>> > > > 
>> > > > a.u.set_mem_access_multi.pfn_list,
>> > +                                      a.u.set_mem_access_multi.acc
>> > ess_list,
>> > +                                      a.u.set_mem_access_multi.nr,
>> > +                                      a.u.set_mem_access_multi.opa
>> > que,
>> > +                                      MEMOP_CMD_MASK,
>> This not being a memop, re-using this value looks arbitrary.
>> Unless it absolutely has to be this value, please either add a brief
>> comment saying this just happens to fit your need or use a literal
>> constant.
> I will add a comment to clarify the reason for using MEMOP_CMD_MASK:
> e.g.
> /* MEMOP_CMD_MASK is used here to mirror the way
> p2m_set_mem_access_multi() is called for the
> XENMEM_access_op_set_access_multi case. */

But that's neither brief nor an actual reason (if at all it is a cosmetic
one). A wider or more narrow mask would do, afaict.

>> > +                                      a.u.set_mem_access_multi.vie
>> > w);
>> > +        if ( rc > 0 )
>> > +        {
>> > +            a.u.set_mem_access_multi.opaque = rc;
>> > +            if ( __copy_field_to_guest(guest_handle_cast(arg,
>> > xen_hvm_altp2m_op_t), &a, u.set_mem_access_multi.opaque) )
>> Long line.
>> 
>> Also please consider adding a prereq patch changing the function's
>> parameter to a properly typed handle, which will then make
>> unnecessary using the not obviously correct cast here. Other
>> copying back of individual fields in this function could then also be
>> switched to __copy_field_to_guest().
> 
> Actually, changing the function's parameter to a typed handle could
> complicate things a little for compat_altp2m_op. It should be modified
> also to use a typed handle (compat_hvm_altp2m_op_t), which in case of
> commands other than HVMOP_altp2m_set_mem_access_multi should be casted
> to xen_hvm_altp2m_op_t before calling do_altp2m_op.
> If the problem was only related to the code clarity, I think the new
> implementation will be a little more difficult to follow.

Hmm, likely true - it would help do_altp2m_op(), but would indeed
look to harm the new ad hoc compat wrapper you add.

>> > @@ -4586,6 +4613,57 @@ static int do_altp2m_op(
>> >      return rc;
>> >  }
>> >  
>> > +static int compat_altp2m_op(
>> > +    XEN_GUEST_HANDLE_PARAM(void) arg)
>> > +{
>> > +    struct compat_hvm_altp2m_op a;
>> > +    union
>> > +    {
>> > +        XEN_GUEST_HANDLE_PARAM(void) hnd;
>> > +        struct xen_hvm_altp2m_op *altp2m_op;
>> > +    } nat;
>> > +
>> > +    if ( !hvm_altp2m_supported() )
>> > +        return -EOPNOTSUPP;
>> > +
>> > +    if ( copy_from_gueWill change in the next patch iteration.
>> 
> st(&a, arg, 1) )

This isn't the first time I see such a misplaced comment. Once
again I can't make sense of it when being placed this way.

Jan


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

Re: [PATCH v4] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Razvan Cojocaru]

On 10/10/2017 05:24 PM, Jan Beulich wrote:
>>>> On 10.10.17 at 16:13, <ppircalabu@bitdefender.com> wrote:
>> On Ma, 2017-10-10 at 06:28 -0600, Jan Beulich wrote:> > 
>>>>>
>>>>>>
>>>>>> a.u.set_mem_access_multi.pfn_list,
>>>> +                                      a.u.set_mem_access_multi.acc
>>>> ess_list,
>>>> +                                      a.u.set_mem_access_multi.nr,
>>>> +                                      a.u.set_mem_access_multi.opa
>>>> que,
>>>> +                                      MEMOP_CMD_MASK,
>>> This not being a memop, re-using this value looks arbitrary.
>>> Unless it absolutely has to be this value, please either add a brief
>>> comment saying this just happens to fit your need or use a literal
>>> constant.
>> I will add a comment to clarify the reason for using MEMOP_CMD_MASK:
>> e.g.
>> /* MEMOP_CMD_MASK is used here to mirror the way
>> p2m_set_mem_access_multi() is called for the
>> XENMEM_access_op_set_access_multi case. */
> 
> But that's neither brief nor an actual reason (if at all it is a cosmetic
> one). A wider or more narrow mask would do, afaict.

Revisiting the p2m_set_mem_access_multi() code, the mask is used here:

447         /* Check for continuation if it's not the last iteration. */
448         if ( nr > ++start && !(start & mask) &&
hypercall_preempt_check() )
449         {
450             rc = start;
451             break;
452         }

If I'm reading the code correctly, MEMOP_CMD_MASK happens to be
0000000000111111, which allows an interruption in the processing loop
every 64th time we go through it.

Now, it does indeed make more sense to see MEMOP_CMD_MASK being used
with XENMEM_access_op_set_access_multi than it does with altp2m (where
we're not dealing with memops). However, indeed almost any mask would do
here (0x1f, for example, or 0x7f, or something else entirely).

The reason I've initially picked MEMOP_CMD_MASK for this patch is that
it had seemed like a reasonable default (currenly made use of with
XENMEM_access_op_set_access_multi). I'm quite likely missing something,
but I don't know what criteria we should use for picking a good value
here, and how to express it. And if we go with the tried and true
MEMOP_CMD_MASK (because it seems to work well), is it not appropriate to
express that intent by using its name in the code?

Should we just use it's value directly (i.e. 0x3f)?


Thanks,
Razvan

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

Re: [PATCH v4] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Petre Ovidiu PIRCALABU]

On Tue, 2017-10-10 at 06:28 -0600, Jan Beulich wrote:
> > > > +typedef struct xen_hvm_altp2m_set_mem_access_multi
> > +    xen_hvm_altp2m_set_mem_access_multi_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_multi_t);
> 
> Are typedef and handle actually needed anywhere? Otherwise
> please don't add them. Just like recently done for domctl and
> sysctl we should even consider cleaning up the others here.
> 
> Jan
All xen_hvm_altp2m_* structs have also defined the typedef and the
handle. I can remove them for xen_hvm_altp2m_set_mem_access_multi but
this way it will not be in sync with the other xen_hvm_altp2m_*
definitions.

Also, regarding the typedef, I have encountered a possible usage when
trying to generate the XLAT macro for xen_hvm_altp2m_op. Using the
existing way of declaring the structure (union of structs) the enum
corresponding to the union members was not generated. Replacing struct
with the corresponding typedef fixed the issue.
The cause is the condition the ./xen/tools/get-fields.sh uses to
generate the enum (if [ "${kind%%;*}" = union ] ). In our case the kind
variable is something like "struct;struct;struct;..;union" and the
condition is valid only if kind starts with "union", hence the enum is
not generated.
Unfortunately my understanding of this script is quite scarce, so I
cannot propose viable a solution.

Personally I would prefer to keep the definition in sync with the other
 xen_hvm_altp2m_* structs for now and hanlde this issue in a separate
patch for applicable for all definitions.

Best regards,
Petre

> 
> ________________________
> This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Jan Beulich]

>>> On 10.10.17 at 20:08, <rcojocaru@bitdefender.com> wrote:
> On 10/10/2017 05:24 PM, Jan Beulich wrote:
>>>>> On 10.10.17 at 16:13, <ppircalabu@bitdefender.com> wrote:
>>> On Ma, 2017-10-10 at 06:28 -0600, Jan Beulich wrote:> > 
>>>>>>
>>>>>>>
>>>>>>> a.u.set_mem_access_multi.pfn_list,
>>>>> +                                      a.u.set_mem_access_multi.acc
>>>>> ess_list,
>>>>> +                                      a.u.set_mem_access_multi.nr,
>>>>> +                                      a.u.set_mem_access_multi.opa
>>>>> que,
>>>>> +                                      MEMOP_CMD_MASK,
>>>> This not being a memop, re-using this value looks arbitrary.
>>>> Unless it absolutely has to be this value, please either add a brief
>>>> comment saying this just happens to fit your need or use a literal
>>>> constant.
>>> I will add a comment to clarify the reason for using MEMOP_CMD_MASK:
>>> e.g.
>>> /* MEMOP_CMD_MASK is used here to mirror the way
>>> p2m_set_mem_access_multi() is called for the
>>> XENMEM_access_op_set_access_multi case. */
>> 
>> But that's neither brief nor an actual reason (if at all it is a cosmetic
>> one). A wider or more narrow mask would do, afaict.
> 
> Revisiting the p2m_set_mem_access_multi() code, the mask is used here:
> 
> 447         /* Check for continuation if it's not the last iteration. */
> 448         if ( nr > ++start && !(start & mask) && hypercall_preempt_check() )
> 449         {
> 450             rc = start;
> 451             break;
> 452         }
> 
> If I'm reading the code correctly, MEMOP_CMD_MASK happens to be
> 0000000000111111, which allows an interruption in the processing loop
> every 64th time we go through it.
> 
> Now, it does indeed make more sense to see MEMOP_CMD_MASK being used
> with XENMEM_access_op_set_access_multi than it does with altp2m (where
> we're not dealing with memops). However, indeed almost any mask would do
> here (0x1f, for example, or 0x7f, or something else entirely).
> 
> The reason I've initially picked MEMOP_CMD_MASK for this patch is that
> it had seemed like a reasonable default (currenly made use of with
> XENMEM_access_op_set_access_multi). I'm quite likely missing something,
> but I don't know what criteria we should use for picking a good value
> here, and how to express it. And if we go with the tried and true
> MEMOP_CMD_MASK (because it seems to work well), is it not appropriate to
> express that intent by using its name in the code?

My point is that MEMOP_CMD_MASK can't be changed, i.e. other
than the value to be used here it is not arbitrary.

> Should we just use it's value directly (i.e. 0x3f)?

That's certainly an option.

Jan


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

Re: [PATCH v4] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Jan Beulich]

>>> On 10.10.17 at 23:00, <ppircalabu@bitdefender.com> wrote:
> On Tue, 2017-10-10 at 06:28 -0600, Jan Beulich wrote:
>> > > > +typedef struct xen_hvm_altp2m_set_mem_access_multi
>> > +    xen_hvm_altp2m_set_mem_access_multi_t;
>> > +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_multi_t);
>> 
>> Are typedef and handle actually needed anywhere? Otherwise
>> please don't add them. Just like recently done for domctl and
>> sysctl we should even consider cleaning up the others here.
>> 
> All xen_hvm_altp2m_* structs have also defined the typedef and the
> handle. I can remove them for xen_hvm_altp2m_set_mem_access_multi but
> this way it will not be in sync with the other xen_hvm_altp2m_*
> definitions.

Please, as frequently asked for elsewhere elsewhere, let's not
spread badness once it was recognized.

> Also, regarding the typedef, I have encountered a possible usage when
> trying to generate the XLAT macro for xen_hvm_altp2m_op. Using the
> existing way of declaring the structure (union of structs) the enum
> corresponding to the union members was not generated. Replacing struct
> with the corresponding typedef fixed the issue.

Now that's a valid argument, if that way less customization is
necessary elsewhere in your patch. But that still wouldn't require
the handle to be declared.

Jan


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