[PATCH v10] x86/altp2m: support for setting restrictions for an array of pages

[PATCH v10] x86/altp2m: support for setting restrictions for an array of pages

[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_op_t.

Changed since v4:
    * Changed the mask parameter to 0x3F.
    * Split long lines.
    * Added "improperly named HVMMEM_(*)" to the comment explaining the
    XEN_GENERATING_COMPAT_HEADERS guard.
    * Removed typedef and XEN_GUEST_HANDLE for xen_hvm_altp2m_set_mem_access_multi.
    * Copied the "opaque" field to guest in compat_altp2m_op.
    * Added build time test to check if the size of
    xen_hvm_altp2m_set_mem_access_multi at least equal to the size of
    compat_hvm_altp2m_set_mem_access_multi.

Changed since v5:
    * Changed the domid parameter type to uint32_t to match 5b42c82f.
    * Added comment to explain why the 0x3F value was chosen.
    * Fixed switch indentation in compat_altp2m_op.
    * Changed the condition used to check if the opaque field has to
    be copied to the guest.
    * Added CHECK_hvm_altp2m_op and CHECK_hvm_altp2m_set_mem_access_multi.

Changed since v6:
    * Removed trailing semicolon from the definitions of CHECK_hvm_altp2m_op
    and CHECK_hvm_altp2m_set_mem_access_multi.
    * Removed BUILD_BUG_ON check.
    * Added comment describing the reason for manually defining the CHECK_
    macros.
    * Added ASSERT_UNREACHABLE as the default switch label action in
    compat_altp2m_op.
    * Added ASSERT(rc == __HYPERVISOR_hvm_op) to make sure the return
    code was actually sey by hypercall_create_continuation.

Changed since v7:
    * Changed the patch title.

Changed since v8:
    * Use sizeof *var for portability
    * Added "must be set to 0" to opaque's comment
    * Reordered alphabetically the compat headers
    * Added blanks to switch statements at the end of each "case" block
    * Do not return -EINVAL when nr is 0

Changed since v9:
    * Return -EINVAL only if "opaque" is greater than "nr" when handling
    HVMOP_altp2m_set_mem_access_multi.
---
 tools/libxc/include/xenctrl.h   |   3 +
 tools/libxc/xc_altp2m.c         |  41 ++++++++++++
 xen/arch/x86/hvm/hvm.c          | 141 +++++++++++++++++++++++++++++++++++++++-
 xen/include/Makefile            |   3 +-
 xen/include/public/hvm/hvm_op.h |  39 +++++++++--
 xen/include/xlat.lst            |   1 +
 6 files changed, 221 insertions(+), 7 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 666db0b..f171668 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1974,6 +1974,9 @@ int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
 int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid,
                          uint16_t view_id, xen_pfn_t old_gfn,
                          xen_pfn_t new_gfn);
+int xc_altp2m_set_mem_access_multi(xc_interface *handle, uint32_t domid,
+                                   uint16_t view_id, uint8_t *access,
+                                   uint64_t *pages, uint32_t nr);
 
 /** 
  * Mem paging operations.
diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index 07fcd18..0f792b5 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -213,3 +213,44 @@ int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid,
     return rc;
 }
 
+int xc_altp2m_set_mem_access_multi(xc_interface *xch, uint32_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 * sizeof(*access),
+                             XC_HYPERCALL_BUFFER_BOUNCE_IN);
+    DECLARE_HYPERCALL_BOUNCE(pages, nr * sizeof(*pages),
+                             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;
+}
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 28bc7e4..efe140b 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
@@ -4496,8 +4498,10 @@ 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:
         return -EOPNOTSUPP;
     }
@@ -4619,6 +4623,37 @@ 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;
+        }
+
+        /*
+         * The mask was set (arbitrary) to 0x3F to match the value used for
+         * MEMOP, despite the fact there are no encoding limitations for the
+         * start parameter.
+         */
+        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,
+                                      0x3F,
+                                      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;
@@ -4637,6 +4672,110 @@ static int do_altp2m_op(
     return rc;
 }
 
+DEFINE_XEN_GUEST_HANDLE(compat_hvm_altp2m_op_t);
+
+/*
+ * Manually define the CHECK_ macros for hvm_altp2m_op and
+ * hvm_altp2m_set_mem_access_multi as the generated versions can't handle
+ * correctly the translation of all fields from compat_(*) to xen_(*).
+ */
+#ifndef CHECK_hvm_altp2m_op
+#define CHECK_hvm_altp2m_op \
+    CHECK_SIZE_(struct, hvm_altp2m_op); \
+    CHECK_FIELD_(struct, hvm_altp2m_op, version); \
+    CHECK_FIELD_(struct, hvm_altp2m_op, cmd); \
+    CHECK_FIELD_(struct, hvm_altp2m_op, domain); \
+    CHECK_FIELD_(struct, hvm_altp2m_op, pad1); \
+    CHECK_FIELD_(struct, hvm_altp2m_op, pad2)
+#endif /* CHECK_hvm_altp2m_op */
+
+#ifndef CHECK_hvm_altp2m_set_mem_access_multi
+#define CHECK_hvm_altp2m_set_mem_access_multi \
+    CHECK_FIELD_(struct, hvm_altp2m_set_mem_access_multi, view); \
+    CHECK_FIELD_(struct, hvm_altp2m_set_mem_access_multi, pad); \
+    CHECK_FIELD_(struct, hvm_altp2m_set_mem_access_multi, nr); \
+    CHECK_FIELD_(struct, hvm_altp2m_set_mem_access_multi, opaque)
+#endif /* CHECK_hvm_altp2m_set_mem_access_multi */
+
+CHECK_hvm_altp2m_op;
+CHECK_hvm_altp2m_set_mem_access_multi;
+
+static int compat_altp2m_op(
+    XEN_GUEST_HANDLE_PARAM(void) arg)
+{
+    int rc = 0;
+    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. */
+    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;
+
+    rc = do_altp2m_op(nat.hnd);
+
+    switch ( a.cmd )
+    {
+    case HVMOP_altp2m_set_mem_access_multi:
+        /*
+         * The return code can be positive only if it is the return value
+         * of hypercall_create_continuation. In this case, the opaque value
+         * must be copied back to the guest.
+         */
+        if ( rc > 0 )
+        {
+            ASSERT(rc == __HYPERVISOR_hvm_op);
+            a.u.set_mem_access_multi.opaque =
+                nat.altp2m_op->u.set_mem_access_multi.opaque;
+            if ( __copy_field_to_guest(guest_handle_cast(arg,
+                                                         compat_hvm_altp2m_op_t),
+                                       &a, u.set_mem_access_multi.opaque) )
+                rc = -EFAULT;
+        }
+        break;
+
+    default:
+        ASSERT_UNREACHABLE();
+    }
+
+    return rc;
+}
+
 static int hvmop_get_mem_type(
     XEN_GUEST_HANDLE_PARAM(xen_hvm_get_mem_type_t) arg)
 {
@@ -4784,7 +4923,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 1299b19..5e9220c 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -26,8 +26,9 @@ headers-$(CONFIG_X86)     += compat/arch-x86/pmu.h
 headers-$(CONFIG_X86)     += compat/arch-x86/xen-mca.h
 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-$(CONFIG_X86)     += compat/hvm/hvm_vcpu.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..bbba99e 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 improperly named 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.
+     * Must be set to zero upon initial invocation.
+     */
+    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;
+};
+
 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 4346cbe..e3fb0c1 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -71,6 +71,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.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v10] x86/altp2m: support for setting restrictions for an array of pages

[Jan Beulich]

>>> On 13.12.17 at 15:22, <ppircalabu@bitdefender.com> wrote:
> 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.

I've just stumbled across this 3 months old patch. All my comments
code wise have been addressed, so I'm not going to object to this
going in. However, the permissions issue alluded to above is what
makes me refrain from giving an ack for it; it'll need Andrew's or
George's ack (plus a tool stack maintainer's) to go in. (Please
remember that it's generally the submitter of a patch to ping people
for missing acks.)

Jan


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

Re: [PATCH v10] x86/altp2m: support for setting restrictions for an array of pages

[Wei Liu]

On Wed, Dec 13, 2017 at 04:22:20PM +0200, Petre Pircalabu wrote:
>  /** 
>   * Mem paging operations.
> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
> index 07fcd18..0f792b5 100644
> --- a/tools/libxc/xc_altp2m.c
> +++ b/tools/libxc/xc_altp2m.c
> @@ -213,3 +213,44 @@ int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid,
>      return rc;
>  }
>  
> +int xc_altp2m_set_mem_access_multi(xc_interface *xch, uint32_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 * sizeof(*access),
> +                             XC_HYPERCALL_BUFFER_BOUNCE_IN);
> +    DECLARE_HYPERCALL_BOUNCE(pages, nr * sizeof(*pages),
> +                             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));

Tabs here.

With this fixed, libxc bits:

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


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

[PATCH v10] x86/altp2m: support for setting restrictions for an array of pages

[Razvan Cojocaru]

On 03/13/2018 07:44 PM, Wei Liu wrote:
> On Wed, Dec 13, 2017 at 04:22:20PM +0200, Petre Pircalabu wrote:
> [...]
> Tabs here.
> 
> With this fixed, libxc bits:
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

We seem to have (again) arrived at an impasse with this patch.

To summarize, the original designers of altp2m have not replied anything
to repeated requests for comments, we've briefly discussed the issue at
the Xen Developer Summit last year, and it was concluded that, since we
do have a valid use case, we carry on with whatever design most fit with
the single-page setting xc_altp2m_set_mem_access() (which is a HVMOP).

Previously, Andrew has argued for the operation to be a HVMOP (in
keeping with the original design), while Jan has pointed out that since
this is only currently useful as a DOMCTL it would perhaps better be
implemented as one (which I personally also prefer, as it gets rid of
the very ugly compat code).

In the process of negotiating this, concerns have been voiced about the
possibility of restricting altp2m operations from either dom0 or the
guest. This is addressed, IMHO, by Tamas' patches:

https://patchwork.kernel.org/patch/9661873/

The situation is now this: Wei's obviously acked it; Jan agrees to let
it in, pending acks from Andrew or George.

We're happy to address any clear objections. But it would be a shame to
lose all this work we've all been doing for the better part of an year,
as I'm sure we all agree.

Please let us know how to proceed.


Thanks,
Razvan

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

Re: [PATCH v10] x86/altp2m: support for setting restrictions for an array of pages

[George Dunlap]

On 03/30/2018 07:34 AM, Razvan Cojocaru wrote:
> On 03/13/2018 07:44 PM, Wei Liu wrote:
>> On Wed, Dec 13, 2017 at 04:22:20PM +0200, Petre Pircalabu wrote:
>> [...]
>> Tabs here.
>>
>> With this fixed, libxc bits:
>>
>> Acked-by: Wei Liu <wei.liu2@citrix.com>
> 
> We seem to have (again) arrived at an impasse with this patch.
> 
> To summarize, the original designers of altp2m have not replied anything
> to repeated requests for comments, we've briefly discussed the issue at
> the Xen Developer Summit last year, and it was concluded that, since we
> do have a valid use case, we carry on with whatever design most fit with
> the single-page setting xc_altp2m_set_mem_access() (which is a HVMOP).
> 
> Previously, Andrew has argued for the operation to be a HVMOP (in
> keeping with the original design), while Jan has pointed out that since
> this is only currently useful as a DOMCTL it would perhaps better be
> implemented as one (which I personally also prefer, as it gets rid of
> the very ugly compat code).
> 
> In the process of negotiating this, concerns have been voiced about the
> possibility of restricting altp2m operations from either dom0 or the
> guest. This is addressed, IMHO, by Tamas' patches:
> 
> https://patchwork.kernel.org/patch/9661873/
> 
> The situation is now this: Wei's obviously acked it; Jan agrees to let
> it in, pending acks from Andrew or George.
> 
> We're happy to address any clear objections. But it would be a shame to
> lose all this work we've all been doing for the better part of an year,
> as I'm sure we all agree.
> 
> Please let us know how to proceed.

Hey Razvan,

Not sure if you were on the list for the x86 community call, but this
probably would have been a good item to put on the agenda to discuss
last month -- the purpose of the call is to try to work through just
these sorts of impasse.

In response to v8, I said:  "FWIW the approach looks good to me here.
You mainly need an x86 maintainer's ack and a toolstack maintainer's
ack (of which I am neither)."

I'm not a maintainer of `xen/arch/x86/hvm/hvm.c` -- that's Andy and Jan.

OTOH, altp2m is a p2m feature, so arguably it should be primarily my
responsibility.

Anyway, I'll take a look at it today; if I can give a Reviewed-by, then
maybe we can negotiate for it to go in with just that (with my committer
hat on on), or for Andy to Ack it based mostly on my R-b.

 -George


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

Re: [PATCH v10] x86/altp2m: support for setting restrictions for an array of pages

[Razvan Cojocaru]

On 03/30/2018 01:03 PM, George Dunlap wrote:
> On 03/30/2018 07:34 AM, Razvan Cojocaru wrote:
>> On 03/13/2018 07:44 PM, Wei Liu wrote:
>>> On Wed, Dec 13, 2017 at 04:22:20PM +0200, Petre Pircalabu wrote:
>>> [...]
>>> Tabs here.
>>>
>>> With this fixed, libxc bits:
>>>
>>> Acked-by: Wei Liu <wei.liu2@citrix.com>
>>
>> We seem to have (again) arrived at an impasse with this patch.
>>
>> To summarize, the original designers of altp2m have not replied anything
>> to repeated requests for comments, we've briefly discussed the issue at
>> the Xen Developer Summit last year, and it was concluded that, since we
>> do have a valid use case, we carry on with whatever design most fit with
>> the single-page setting xc_altp2m_set_mem_access() (which is a HVMOP).
>>
>> Previously, Andrew has argued for the operation to be a HVMOP (in
>> keeping with the original design), while Jan has pointed out that since
>> this is only currently useful as a DOMCTL it would perhaps better be
>> implemented as one (which I personally also prefer, as it gets rid of
>> the very ugly compat code).
>>
>> In the process of negotiating this, concerns have been voiced about the
>> possibility of restricting altp2m operations from either dom0 or the
>> guest. This is addressed, IMHO, by Tamas' patches:
>>
>> https://patchwork.kernel.org/patch/9661873/
>>
>> The situation is now this: Wei's obviously acked it; Jan agrees to let
>> it in, pending acks from Andrew or George.
>>
>> We're happy to address any clear objections. But it would be a shame to
>> lose all this work we've all been doing for the better part of an year,
>> as I'm sure we all agree.
>>
>> Please let us know how to proceed.
> 
> Hey Razvan,
> 
> Not sure if you were on the list for the x86 community call, but this
> probably would have been a good item to put on the agenda to discuss
> last month -- the purpose of the call is to try to work through just
> these sorts of impasse.
> 
> In response to v8, I said:  "FWIW the approach looks good to me here.
> You mainly need an x86 maintainer's ack and a toolstack maintainer's
> ack (of which I am neither)."
> 
> I'm not a maintainer of `xen/arch/x86/hvm/hvm.c` -- that's Andy and Jan.
> 
> OTOH, altp2m is a p2m feature, so arguably it should be primarily my
> responsibility.
> 
> Anyway, I'll take a look at it today; if I can give a Reviewed-by, then
> maybe we can negotiate for it to go in with just that (with my committer
> hat on on), or for Andy to Ack it based mostly on my R-b.

Thank you for your help! And sorry if I've mistakenly named you here,
quite possibly I've misunderstood Jan's earlier reply.

Of course, please let us know if there's anything else we should do.

I wasn't on the list by email address for the last x86 community call,
though I did reply that I'm also partial to GoToMeeting on xen-devel -
the agenda seemed to me to had been already set, and to include larger
items than a single floating patch. With apologies for the off-topic,
we're also very interested in Intel's SPP.


Thanks,
Razvan

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

Re: [PATCH v10] x86/altp2m: support for setting restrictions for an array of pages

[George Dunlap]

On 03/30/2018 11:14 AM, Razvan Cojocaru wrote:
> On 03/30/2018 01:03 PM, George Dunlap wrote:
>> On 03/30/2018 07:34 AM, Razvan Cojocaru wrote:
>>> On 03/13/2018 07:44 PM, Wei Liu wrote:
>>>> On Wed, Dec 13, 2017 at 04:22:20PM +0200, Petre Pircalabu wrote:
>>>> [...]
>>>> Tabs here.
>>>>
>>>> With this fixed, libxc bits:
>>>>
>>>> Acked-by: Wei Liu <wei.liu2@citrix.com>
>>>
>>> We seem to have (again) arrived at an impasse with this patch.
>>>
>>> To summarize, the original designers of altp2m have not replied anything
>>> to repeated requests for comments, we've briefly discussed the issue at
>>> the Xen Developer Summit last year, and it was concluded that, since we
>>> do have a valid use case, we carry on with whatever design most fit with
>>> the single-page setting xc_altp2m_set_mem_access() (which is a HVMOP).
>>>
>>> Previously, Andrew has argued for the operation to be a HVMOP (in
>>> keeping with the original design), while Jan has pointed out that since
>>> this is only currently useful as a DOMCTL it would perhaps better be
>>> implemented as one (which I personally also prefer, as it gets rid of
>>> the very ugly compat code).
>>>
>>> In the process of negotiating this, concerns have been voiced about the
>>> possibility of restricting altp2m operations from either dom0 or the
>>> guest. This is addressed, IMHO, by Tamas' patches:
>>>
>>> https://patchwork.kernel.org/patch/9661873/
>>>
>>> The situation is now this: Wei's obviously acked it; Jan agrees to let
>>> it in, pending acks from Andrew or George.
>>>
>>> We're happy to address any clear objections. But it would be a shame to
>>> lose all this work we've all been doing for the better part of an year,
>>> as I'm sure we all agree.
>>>
>>> Please let us know how to proceed.
>>
>> Hey Razvan,
>>
>> Not sure if you were on the list for the x86 community call, but this
>> probably would have been a good item to put on the agenda to discuss
>> last month -- the purpose of the call is to try to work through just
>> these sorts of impasse.
>>
>> In response to v8, I said:  "FWIW the approach looks good to me here.
>> You mainly need an x86 maintainer's ack and a toolstack maintainer's
>> ack (of which I am neither)."
>>
>> I'm not a maintainer of `xen/arch/x86/hvm/hvm.c` -- that's Andy and Jan.
>>
>> OTOH, altp2m is a p2m feature, so arguably it should be primarily my
>> responsibility.
>>
>> Anyway, I'll take a look at it today; if I can give a Reviewed-by, then
>> maybe we can negotiate for it to go in with just that (with my committer
>> hat on on), or for Andy to Ack it based mostly on my R-b.
> 
> Thank you for your help! And sorry if I've mistakenly named you here,
> quite possibly I've misunderstood Jan's earlier reply.

No, you were clearly taking a lead from Jan -- I'm not sure exactly what
he had in mind by naming me (i.e., with the strict rules for who has to
approve what).  I was just explaining why it didn't already have an R-b
from me. :-)

I'm not sure why Andy hasn't Acked it; maybe it slipped through the cracks.

> Of course, please let us know if there's anything else we should do.
> 
> I wasn't on the list by email address for the last x86 community call,
> though I did reply that I'm also partial to GoToMeeting on xen-devel -
> the agenda seemed to me to had been already set, and to include larger
> items than a single floating patch. With apologies for the off-topic,
> we're also very interested in Intel's SPP.

Oh, I guess that was probably Tamas who asked to be put on the list. FYI
the first agenda Lars sent had a big "[DISCUSS THINGS HERE]" hole
(although not in so many words), and he asked people to submit series
that needed to be discussed in that section.  As it was, only Wei and I
submitted anything; my list included a number of Intel's series,
including SPP, which were lurking in my inbox; but this patch had
dropped off my radar for reasons mentioned above.  You should be able to
add topics to the agenda even at the beginning of the meeting if you
want, but they'll probably be put at the end, and we only got about
halfway through last time; so submitting them earlier would probably be
a better strategy. :-)

 -George

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

Re: [PATCH v10] x86/altp2m: support for setting restrictions for an array of pages

[George Dunlap]

On Wed, Dec 13, 2017 at 2:22 PM, Petre Pircalabu
<ppircalabu@bitdefender.com> wrote:
> 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.

This mostly looks good to me, with a couple of nitpicks...

> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 666db0b..f171668 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1974,6 +1974,9 @@ int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
>  int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid,
>                           uint16_t view_id, xen_pfn_t old_gfn,
>                           xen_pfn_t new_gfn);
> +int xc_altp2m_set_mem_access_multi(xc_interface *handle, uint32_t domid,
> +                                   uint16_t view_id, uint8_t *access,
> +                                   uint64_t *pages, uint32_t nr);

Two minor things:

* It seems like it would make sense to put this directly under the
non-multi version of this call (even though that does put it out of
order with the command number)

* 'Pages' is ambiguous here, as it could be interpreted to mean Linux
virtual pages rather than gfn.  Is there a reason not to call this
argument 'gfns' (as in the other xc call) or 'pfn_list' (as in the
hypercall)?

(And sorry if this has been covered before; I did do a quick look over
the history and didn't notice anything.)

> @@ -4619,6 +4623,37 @@ 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;
> +        }
> +
> +        /*
> +         * The mask was set (arbitrary) to 0x3F to match the value used for
> +         * MEMOP, despite the fact there are no encoding limitations for the
> +         * start parameter.
> +         */

This comment isn't actually very enlightening if you're not already
intimately familiar with the code; it took me at least 10 minutes of
grepping around to figure out what this was about.

What about this:

"Unlike XENMEM_access_op_set_access_multi, we don't need any bits of
the 'continuation' counter to be zero (to stash a command in).
However, 0x40 is a good 'stride' to make sure
that we make a reasonable amount of forward progress before yielding,
so use a mask of 0x3F here."

Everything else looks good to me.

 -George

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

Re: [PATCH v10] x86/altp2m: support for setting restrictions for an array of pages

[Razvan Cojocaru]

On 03/30/2018 02:16 PM, George Dunlap wrote:
> On Wed, Dec 13, 2017 at 2:22 PM, Petre Pircalabu
> <ppircalabu@bitdefender.com> wrote:
>> 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.
> 
> This mostly looks good to me, with a couple of nitpicks...
> 
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index 666db0b..f171668 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -1974,6 +1974,9 @@ int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
>>  int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid,
>>                           uint16_t view_id, xen_pfn_t old_gfn,
>>                           xen_pfn_t new_gfn);
>> +int xc_altp2m_set_mem_access_multi(xc_interface *handle, uint32_t domid,
>> +                                   uint16_t view_id, uint8_t *access,
>> +                                   uint64_t *pages, uint32_t nr);
> 
> Two minor things:
> 
> * It seems like it would make sense to put this directly under the
> non-multi version of this call (even though that does put it out of
> order with the command number)

Not a problem. We'll move it.

> * 'Pages' is ambiguous here, as it could be interpreted to mean Linux
> virtual pages rather than gfn.  Is there a reason not to call this
> argument 'gfns' (as in the other xc call) or 'pfn_list' (as in the
> hypercall)?

No, we'll rename it to 'gfns'.

> (And sorry if this has been covered before; I did do a quick look over
> the history and didn't notice anything.)
> 
>> @@ -4619,6 +4623,37 @@ 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;
>> +        }
>> +
>> +        /*
>> +         * The mask was set (arbitrary) to 0x3F to match the value used for
>> +         * MEMOP, despite the fact there are no encoding limitations for the
>> +         * start parameter.
>> +         */
> 
> This comment isn't actually very enlightening if you're not already
> intimately familiar with the code; it took me at least 10 minutes of
> grepping around to figure out what this was about.
> 
> What about this:
> 
> "Unlike XENMEM_access_op_set_access_multi, we don't need any bits of
> the 'continuation' counter to be zero (to stash a command in).
> However, 0x40 is a good 'stride' to make sure
> that we make a reasonable amount of forward progress before yielding,
> so use a mask of 0x3F here."

We have no objection to the change (you're right, the original is quite
terse).


Thanks,
Razvan

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

Re: [PATCH v10] x86/altp2m: support for setting restrictions for an array of pages

[Jan Beulich]

>>> On 30.03.18 at 12:32, <george.dunlap@citrix.com> wrote:
> On 03/30/2018 11:14 AM, Razvan Cojocaru wrote:
>> On 03/30/2018 01:03 PM, George Dunlap wrote:
>>> In response to v8, I said:  "FWIW the approach looks good to me here.
>>> You mainly need an x86 maintainer's ack and a toolstack maintainer's
>>> ack (of which I am neither)."
>>>
>>> I'm not a maintainer of `xen/arch/x86/hvm/hvm.c` -- that's Andy and Jan.
>>>
>>> OTOH, altp2m is a p2m feature, so arguably it should be primarily my
>>> responsibility.
>>>
>>> Anyway, I'll take a look at it today; if I can give a Reviewed-by, then
>>> maybe we can negotiate for it to go in with just that (with my committer
>>> hat on on), or for Andy to Ack it based mostly on my R-b.
>> 
>> Thank you for your help! And sorry if I've mistakenly named you here,
>> quite possibly I've misunderstood Jan's earlier reply.
> 
> No, you were clearly taking a lead from Jan -- I'm not sure exactly what
> he had in mind by naming me (i.e., with the strict rules for who has to
> approve what).  I was just explaining why it didn't already have an R-b
> from me. :-)

Well, I had you in mind with the topic (altp2m) in mind, alongside
the strict file based rules which would limit acks to Andrew and me.

Jan


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