All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4] x86/altp2m: Add a subop for obtaining the mem access of a page
@ 2018-09-03 15:47 Adrian Pop
  2018-09-19 14:44 ` Wei Liu
  2018-09-19 15:29 ` Tamas K Lengyel
  0 siblings, 2 replies; 7+ messages in thread
From: Adrian Pop @ 2018-09-03 15:47 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, Razvan Cojocaru, George Dunlap,
	Andrew Cooper, Adrian Pop, Ian Jackson, Tim Deegan, Julien Grall,
	Tamas K Lengyel, Jan Beulich, Sergej Proskurin

Currently there is a subop for setting the memaccess of a page, but not
for consulting it.  The new HVMOP_altp2m_get_mem_access adds this
functionality.

Both altp2m get/set mem access functions use the struct
xen_hvm_altp2m_mem_access which has now dropped the `set' part and has
been renamed from xen_hvm_altp2m_set_mem_access.

Signed-off-by: Adrian Pop <apop@bitdefender.com>
---
Changes in v4:
- don't break the stable interface

Changes in v3:
- remove the unrelated helper function
- simplify the locking in p2m_get_mem_access

Changes in v2:
- use the _p2m_get_mem_access helper from p2m_get_mem_access
- minor Arm adjustments
- move out the addition of a memaccess helper function to a separate
  patch in the attempts of making the diff clearer
---
 tools/libxc/include/xenctrl.h   |  3 +++
 tools/libxc/xc_altp2m.c         | 33 +++++++++++++++++++++++++++++++++
 xen/arch/arm/mem_access.c       |  8 ++++++--
 xen/arch/x86/hvm/hvm.c          | 27 +++++++++++++++++++++++++++
 xen/arch/x86/mm/mem_access.c    | 21 +++++++++++++++++++--
 xen/common/mem_access.c         |  2 +-
 xen/include/public/hvm/hvm_op.h | 19 +++++++++++++++++++
 xen/include/public/xen-compat.h |  2 +-
 xen/include/xen/mem_access.h    |  3 ++-
 9 files changed, 111 insertions(+), 7 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index c626984aba..ae298899fc 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1958,6 +1958,9 @@ int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
 int xc_altp2m_set_mem_access_multi(xc_interface *handle, uint32_t domid,
                                    uint16_t view_id, uint8_t *access,
                                    uint64_t *gfns, uint32_t nr);
+int xc_altp2m_get_mem_access(xc_interface *handle, uint32_t domid,
+                             uint16_t view_id, xen_pfn_t gfn,
+                             xenmem_access_t *access);
 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);
diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index ce4a1e4d60..53754ff6d3 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -177,9 +177,15 @@ int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
     arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
     arg->cmd = HVMOP_altp2m_set_mem_access;
     arg->domain = domid;
+#if __XEN_INTERFACE_VERSION__ < 0x00040a00
     arg->u.set_mem_access.view = view_id;
     arg->u.set_mem_access.hvmmem_access = access;
     arg->u.set_mem_access.gfn = gfn;
+#else /* __XEN_INTERFACE_VERSION__ >= 0x00040a00 */
+    arg->u.mem_access.view = view_id;
+    arg->u.mem_access.hvmmem_access = access;
+    arg->u.mem_access.gfn = gfn;
+#endif
 
     rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
 		  HYPERCALL_BUFFER_AS_ARG(arg));
@@ -254,3 +260,30 @@ int xc_altp2m_set_mem_access_multi(xc_interface *xch, uint32_t domid,
 
     return rc;
 }
+
+int xc_altp2m_get_mem_access(xc_interface *handle, uint32_t domid,
+                             uint16_t view_id, xen_pfn_t gfn,
+                             xenmem_access_t *access)
+{
+    int rc;
+    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
+
+    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
+    if ( arg == NULL )
+        return -1;
+
+    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
+    arg->cmd = HVMOP_altp2m_get_mem_access;
+    arg->domain = domid;
+    arg->u.mem_access.view = view_id;
+    arg->u.mem_access.gfn = gfn;
+
+    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
+                 HYPERCALL_BUFFER_AS_ARG(arg));
+
+    if ( !rc )
+        *access = arg->u.mem_access.hvmmem_access;
+
+    xc_hypercall_buffer_free(handle, arg);
+    return rc;
+}
diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
index ba4ec780fd..178bc1a6c1 100644
--- a/xen/arch/arm/mem_access.c
+++ b/xen/arch/arm/mem_access.c
@@ -236,7 +236,7 @@ bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
     if ( !p2m->mem_access_enabled )
         return true;
 
-    rc = p2m_get_mem_access(v->domain, gaddr_to_gfn(gpa), &xma);
+    rc = p2m_get_mem_access(v->domain, gaddr_to_gfn(gpa), &xma, 0);
     if ( rc )
         return true;
 
@@ -441,11 +441,15 @@ long p2m_set_mem_access_multi(struct domain *d,
 }
 
 int p2m_get_mem_access(struct domain *d, gfn_t gfn,
-                       xenmem_access_t *access)
+                       xenmem_access_t *access, unsigned int altp2m_idx)
 {
     int ret;
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
 
+    /* altp2m is not yet implemented on Arm. The altp2m_idx should be 0. */
+    if ( altp2m_idx )
+        return -EINVAL;
+
     p2m_read_lock(p2m);
     ret = __p2m_get_mem_access(d, gfn, access);
     p2m_read_unlock(p2m);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 72c51faecb..460c9f7d4f 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4526,6 +4526,7 @@ static int do_altp2m_op(
     case HVMOP_altp2m_switch_p2m:
     case HVMOP_altp2m_set_mem_access:
     case HVMOP_altp2m_set_mem_access_multi:
+    case HVMOP_altp2m_get_mem_access:
     case HVMOP_altp2m_change_gfn:
         break;
 
@@ -4642,12 +4643,21 @@ static int do_altp2m_op(
         break;
 
     case HVMOP_altp2m_set_mem_access:
+#if __XEN_INTERFACE_VERSION__ < 0x00040a00
         if ( a.u.set_mem_access.pad )
             rc = -EINVAL;
         else
             rc = p2m_set_mem_access(d, _gfn(a.u.set_mem_access.gfn), 1, 0, 0,
                                     a.u.set_mem_access.hvmmem_access,
                                     a.u.set_mem_access.view);
+#else /* __XEN_INTERFACE_VERSION__ >= 0x00040a00 */
+        if ( a.u.mem_access.pad )
+            rc = -EINVAL;
+        else
+            rc = p2m_set_mem_access(d, _gfn(a.u.mem_access.gfn), 1, 0, 0,
+                                    a.u.mem_access.hvmmem_access,
+                                    a.u.mem_access.view);
+#endif
         break;
 
     case HVMOP_altp2m_set_mem_access_multi:
@@ -4683,6 +4693,23 @@ static int do_altp2m_op(
         }
         break;
 
+    case HVMOP_altp2m_get_mem_access:
+        if ( a.u.mem_access.pad )
+            rc = -EINVAL;
+        else
+        {
+            xenmem_access_t access;
+
+            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
+                                    a.u.mem_access.view);
+            if ( !rc )
+            {
+                a.u.mem_access.hvmmem_access = access;
+                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+            }
+        }
+        break;
+
     case HVMOP_altp2m_change_gfn:
         if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
             rc = -EINVAL;
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 84d260ebd8..78abdaed36 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -469,9 +469,26 @@ long p2m_set_mem_access_multi(struct domain *d,
     return rc;
 }
 
-int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
+int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access,
+                       unsigned int altp2m_idx)
 {
-    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    struct p2m_domain *p2m;
+
+    if ( !altp2m_active(d) )
+    {
+        if ( altp2m_idx )
+            return -EINVAL;
+
+        p2m = p2m_get_hostp2m(d);
+    }
+    else
+    {
+        if ( altp2m_idx >= MAX_ALTP2M ||
+             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
+            return -EINVAL;
+
+        p2m = d->arch.altp2m_p2m[altp2m_idx];
+    }
 
     return _p2m_get_mem_access(p2m, gfn, access);
 }
diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c
index 1bf6824442..010e6f8dbf 100644
--- a/xen/common/mem_access.c
+++ b/xen/common/mem_access.c
@@ -99,7 +99,7 @@ int mem_access_memop(unsigned long cmd,
         if ( (mao.pfn > domain_get_maximum_gpfn(d)) && mao.pfn != ~0ull )
             break;
 
-        rc = p2m_get_mem_access(d, _gfn(mao.pfn), &access);
+        rc = p2m_get_mem_access(d, _gfn(mao.pfn), &access, 0);
         if ( rc != 0 )
             break;
 
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index bbba99e5f5..bbb0aa984a 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -234,6 +234,7 @@ struct xen_hvm_altp2m_view {
 typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;
 DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t);
 
+#if __XEN_INTERFACE_VERSION__ < 0x00040a00
 struct xen_hvm_altp2m_set_mem_access {
     /* view */
     uint16_t view;
@@ -245,6 +246,19 @@ 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);
+#endif /* __XEN_INTERFACE_VERSION__ < 0x00040a00 */
+
+struct xen_hvm_altp2m_mem_access {
+    /* view */
+    uint16_t view;
+    /* Memory type */
+    uint16_t hvmmem_access; /* xenmem_access_t */
+    uint32_t pad;
+    /* gfn */
+    uint64_t gfn;
+};
+typedef struct xen_hvm_altp2m_mem_access xen_hvm_altp2m_mem_access_t;
+DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_mem_access_t);
 
 struct xen_hvm_altp2m_set_mem_access_multi {
     /* view */
@@ -296,6 +310,8 @@ struct xen_hvm_altp2m_op {
 #define HVMOP_altp2m_change_gfn           8
 /* Set access for an array of pages */
 #define HVMOP_altp2m_set_mem_access_multi 9
+/* Get the access of a page of memory from a certain view */
+#define HVMOP_altp2m_get_mem_access       10
     domid_t domain;
     uint16_t pad1;
     uint32_t pad2;
@@ -303,7 +319,10 @@ struct xen_hvm_altp2m_op {
         struct xen_hvm_altp2m_domain_state         domain_state;
         struct xen_hvm_altp2m_vcpu_enable_notify   enable_notify;
         struct xen_hvm_altp2m_view                 view;
+#if __XEN_INTERFACE_VERSION__ < 0x00040a00
         struct xen_hvm_altp2m_set_mem_access       set_mem_access;
+#endif /* __XEN_INTERFACE_VERSION__ < 0x00040a00 */
+        struct xen_hvm_altp2m_mem_access           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];
diff --git a/xen/include/public/xen-compat.h b/xen/include/public/xen-compat.h
index b67365340b..fa6ffb72e8 100644
--- a/xen/include/public/xen-compat.h
+++ b/xen/include/public/xen-compat.h
@@ -27,7 +27,7 @@
 #ifndef __XEN_PUBLIC_XEN_COMPAT_H__
 #define __XEN_PUBLIC_XEN_COMPAT_H__
 
-#define __XEN_LATEST_INTERFACE_VERSION__ 0x00040900
+#define __XEN_LATEST_INTERFACE_VERSION__ 0x00040a00
 
 #if defined(__XEN__) || defined(__XEN_TOOLS__)
 /* Xen is built with matching headers and implements the latest interface. */
diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
index 7e95eab81c..7348f81233 100644
--- a/xen/include/xen/mem_access.h
+++ b/xen/include/xen/mem_access.h
@@ -76,7 +76,8 @@ long p2m_set_mem_access_multi(struct domain *d,
  * Get access type for a gfn.
  * If gfn == INVALID_GFN, gets the default access type.
  */
-int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access);
+int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access,
+                       unsigned int altp2m_idx);
 
 #ifdef CONFIG_MEM_ACCESS
 int mem_access_memop(unsigned long cmd,
-- 
2.18.0


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

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

* Re: [PATCH v4] x86/altp2m: Add a subop for obtaining the mem access of a page
  2018-09-03 15:47 [PATCH v4] x86/altp2m: Add a subop for obtaining the mem access of a page Adrian Pop
@ 2018-09-19 14:44 ` Wei Liu
  2018-09-19 14:48   ` Jan Beulich
  2018-09-19 15:29 ` Tamas K Lengyel
  1 sibling, 1 reply; 7+ messages in thread
From: Wei Liu @ 2018-09-19 14:44 UTC (permalink / raw)
  To: Adrian Pop
  Cc: Stefano Stabellini, Wei Liu, Razvan Cojocaru,
	Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
	Tim Deegan, Julien Grall, Tamas K Lengyel, Jan Beulich,
	Sergej Proskurin, xen-devel

On Mon, Sep 03, 2018 at 06:47:34PM +0300, Adrian Pop wrote:
> Currently there is a subop for setting the memaccess of a page, but not
> for consulting it.  The new HVMOP_altp2m_get_mem_access adds this
> functionality.
> 
> Both altp2m get/set mem access functions use the struct
> xen_hvm_altp2m_mem_access which has now dropped the `set' part and has
> been renamed from xen_hvm_altp2m_set_mem_access.
> 
> Signed-off-by: Adrian Pop <apop@bitdefender.com>
> ---
> Changes in v4:
> - don't break the stable interface
> 
> Changes in v3:
> - remove the unrelated helper function
> - simplify the locking in p2m_get_mem_access
> 
> Changes in v2:
> - use the _p2m_get_mem_access helper from p2m_get_mem_access
> - minor Arm adjustments
> - move out the addition of a memaccess helper function to a separate
>   patch in the attempts of making the diff clearer
> ---
>  tools/libxc/include/xenctrl.h   |  3 +++
>  tools/libxc/xc_altp2m.c         | 33 +++++++++++++++++++++++++++++++++
>  xen/arch/arm/mem_access.c       |  8 ++++++--
>  xen/arch/x86/hvm/hvm.c          | 27 +++++++++++++++++++++++++++
>  xen/arch/x86/mm/mem_access.c    | 21 +++++++++++++++++++--
>  xen/common/mem_access.c         |  2 +-
>  xen/include/public/hvm/hvm_op.h | 19 +++++++++++++++++++
>  xen/include/public/xen-compat.h |  2 +-
>  xen/include/xen/mem_access.h    |  3 ++-
>  9 files changed, 111 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index c626984aba..ae298899fc 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1958,6 +1958,9 @@ int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
>  int xc_altp2m_set_mem_access_multi(xc_interface *handle, uint32_t domid,
>                                     uint16_t view_id, uint8_t *access,
>                                     uint64_t *gfns, uint32_t nr);
> +int xc_altp2m_get_mem_access(xc_interface *handle, uint32_t domid,
> +                             uint16_t view_id, xen_pfn_t gfn,
> +                             xenmem_access_t *access);
>  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);
> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
> index ce4a1e4d60..53754ff6d3 100644
> --- a/tools/libxc/xc_altp2m.c
> +++ b/tools/libxc/xc_altp2m.c
> @@ -177,9 +177,15 @@ int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
>      arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
>      arg->cmd = HVMOP_altp2m_set_mem_access;
>      arg->domain = domid;
> +#if __XEN_INTERFACE_VERSION__ < 0x00040a00
>      arg->u.set_mem_access.view = view_id;
>      arg->u.set_mem_access.hvmmem_access = access;
>      arg->u.set_mem_access.gfn = gfn;
> +#else /* __XEN_INTERFACE_VERSION__ >= 0x00040a00 */
> +    arg->u.mem_access.view = view_id;
> +    arg->u.mem_access.hvmmem_access = access;
> +    arg->u.mem_access.gfn = gfn;
> +#endif

You don't need to do this for tools and hypervisor. They are always
built with the latest interface -- see xen-compat.h, which you modified.

The only relevant changes are the ones in hvm_op.h AFAICT.

>  
[...]
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index bbba99e5f5..bbb0aa984a 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -234,6 +234,7 @@ struct xen_hvm_altp2m_view {
>  typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t);
>  
> +#if __XEN_INTERFACE_VERSION__ < 0x00040a00
>  struct xen_hvm_altp2m_set_mem_access {
>      /* view */
>      uint16_t view;
> @@ -245,6 +246,19 @@ 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);
> +#endif /* __XEN_INTERFACE_VERSION__ < 0x00040a00 */

#else here please, otherwise you're adding this new interface to a set
of old interfaces.

> +
> +struct xen_hvm_altp2m_mem_access {
> +    /* view */
> +    uint16_t view;
> +    /* Memory type */
> +    uint16_t hvmmem_access; /* xenmem_access_t */
> +    uint32_t pad;
> +    /* gfn */
> +    uint64_t gfn;
> +};
> +typedef struct xen_hvm_altp2m_mem_access xen_hvm_altp2m_mem_access_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_mem_access_t);
>  
>  struct xen_hvm_altp2m_set_mem_access_multi {

Why not provide xen_hvm_altp2m_mem_access_multi at the same time. That
would save you adding more compatibility cruft later.

Wei.

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

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

* Re: [PATCH v4] x86/altp2m: Add a subop for obtaining the mem access of a page
  2018-09-19 14:44 ` Wei Liu
@ 2018-09-19 14:48   ` Jan Beulich
  2018-09-19 14:51     ` Wei Liu
  0 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2018-09-19 14:48 UTC (permalink / raw)
  To: Wei Liu
  Cc: Stefano Stabellini, Razvan Cojocaru, Adrian Pop, George Dunlap,
	Andrew Cooper, Konrad Rzeszutek Wilk, Ian Jackson, Tim Deegan,
	Julien Grall, Tamas K Lengyel, Sergej Proskurin, xen-devel

>>> On 19.09.18 at 16:44, <wei.liu2@citrix.com> wrote:
> On Mon, Sep 03, 2018 at 06:47:34PM +0300, Adrian Pop wrote:
>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.h
>> @@ -234,6 +234,7 @@ struct xen_hvm_altp2m_view {
>>  typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t);
>>  
>> +#if __XEN_INTERFACE_VERSION__ < 0x00040a00
>>  struct xen_hvm_altp2m_set_mem_access {
>>      /* view */
>>      uint16_t view;
>> @@ -245,6 +246,19 @@ 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);
>> +#endif /* __XEN_INTERFACE_VERSION__ < 0x00040a00 */
> 
> #else here please, otherwise you're adding this new interface to a set
> of old interfaces.

I don't think we do so elsewhere, so I'd prefer if we didn't do so
here either. By unconditionally exposing the new interface we
allow people to morph their code in steps, rather than all in one
go at the point they bump their __XEN_INTERFACE_VERSION__
setting.

Jan



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

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

* Re: [PATCH v4] x86/altp2m: Add a subop for obtaining the mem access of a page
  2018-09-19 14:48   ` Jan Beulich
@ 2018-09-19 14:51     ` Wei Liu
  0 siblings, 0 replies; 7+ messages in thread
From: Wei Liu @ 2018-09-19 14:51 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, Wei Liu, Razvan Cojocaru, Adrian Pop,
	George Dunlap, Andrew Cooper, Konrad Rzeszutek Wilk, Ian Jackson,
	Tim Deegan, Julien Grall, Tamas K Lengyel, Sergej Proskurin,
	xen-devel

On Wed, Sep 19, 2018 at 08:48:54AM -0600, Jan Beulich wrote:
> >>> On 19.09.18 at 16:44, <wei.liu2@citrix.com> wrote:
> > On Mon, Sep 03, 2018 at 06:47:34PM +0300, Adrian Pop wrote:
> >> --- a/xen/include/public/hvm/hvm_op.h
> >> +++ b/xen/include/public/hvm/hvm_op.h
> >> @@ -234,6 +234,7 @@ struct xen_hvm_altp2m_view {
> >>  typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;
> >>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t);
> >>  
> >> +#if __XEN_INTERFACE_VERSION__ < 0x00040a00
> >>  struct xen_hvm_altp2m_set_mem_access {
> >>      /* view */
> >>      uint16_t view;
> >> @@ -245,6 +246,19 @@ 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);
> >> +#endif /* __XEN_INTERFACE_VERSION__ < 0x00040a00 */
> > 
> > #else here please, otherwise you're adding this new interface to a set
> > of old interfaces.
> 
> I don't think we do so elsewhere, so I'd prefer if we didn't do so
> here either. By unconditionally exposing the new interface we
> allow people to morph their code in steps, rather than all in one
> go at the point they bump their __XEN_INTERFACE_VERSION__
> setting.
> 

Okay, I'm not too fussed. Your argument sounds reasonable.

Wei.

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

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

* Re: [PATCH v4] x86/altp2m: Add a subop for obtaining the mem access of a page
  2018-09-03 15:47 [PATCH v4] x86/altp2m: Add a subop for obtaining the mem access of a page Adrian Pop
  2018-09-19 14:44 ` Wei Liu
@ 2018-09-19 15:29 ` Tamas K Lengyel
  2018-09-25 15:03   ` Razvan Cojocaru
  1 sibling, 1 reply; 7+ messages in thread
From: Tamas K Lengyel @ 2018-09-19 15:29 UTC (permalink / raw)
  To: Adrian Pop
  Cc: Stefano Stabellini, Wei Liu, Razvan Cojocaru,
	Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
	Tim Deegan, Julien Grall, Jan Beulich, Sergej Proskurin,
	Xen-devel

On Mon, Sep 3, 2018 at 9:47 AM Adrian Pop <apop@bitdefender.com> wrote:
>
> Currently there is a subop for setting the memaccess of a page, but not
> for consulting it.  The new HVMOP_altp2m_get_mem_access adds this
> functionality.
>
> Both altp2m get/set mem access functions use the struct
> xen_hvm_altp2m_mem_access which has now dropped the `set' part and has
> been renamed from xen_hvm_altp2m_set_mem_access.
>
> Signed-off-by: Adrian Pop <apop@bitdefender.com>
> ---
> Changes in v4:
> - don't break the stable interface
>
> Changes in v3:
> - remove the unrelated helper function
> - simplify the locking in p2m_get_mem_access
>
> Changes in v2:
> - use the _p2m_get_mem_access helper from p2m_get_mem_access
> - minor Arm adjustments
> - move out the addition of a memaccess helper function to a separate
>   patch in the attempts of making the diff clearer
> ---
>  tools/libxc/include/xenctrl.h   |  3 +++
>  tools/libxc/xc_altp2m.c         | 33 +++++++++++++++++++++++++++++++++
>  xen/arch/arm/mem_access.c       |  8 ++++++--
>  xen/arch/x86/hvm/hvm.c          | 27 +++++++++++++++++++++++++++
>  xen/arch/x86/mm/mem_access.c    | 21 +++++++++++++++++++--
>  xen/common/mem_access.c         |  2 +-
>  xen/include/public/hvm/hvm_op.h | 19 +++++++++++++++++++
>  xen/include/public/xen-compat.h |  2 +-
>  xen/include/xen/mem_access.h    |  3 ++-
>  9 files changed, 111 insertions(+), 7 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index c626984aba..ae298899fc 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1958,6 +1958,9 @@ int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
>  int xc_altp2m_set_mem_access_multi(xc_interface *handle, uint32_t domid,
>                                     uint16_t view_id, uint8_t *access,
>                                     uint64_t *gfns, uint32_t nr);
> +int xc_altp2m_get_mem_access(xc_interface *handle, uint32_t domid,
> +                             uint16_t view_id, xen_pfn_t gfn,
> +                             xenmem_access_t *access);
>  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);
> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
> index ce4a1e4d60..53754ff6d3 100644
> --- a/tools/libxc/xc_altp2m.c
> +++ b/tools/libxc/xc_altp2m.c
> @@ -177,9 +177,15 @@ int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
>      arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
>      arg->cmd = HVMOP_altp2m_set_mem_access;
>      arg->domain = domid;
> +#if __XEN_INTERFACE_VERSION__ < 0x00040a00
>      arg->u.set_mem_access.view = view_id;
>      arg->u.set_mem_access.hvmmem_access = access;
>      arg->u.set_mem_access.gfn = gfn;
> +#else /* __XEN_INTERFACE_VERSION__ >= 0x00040a00 */
> +    arg->u.mem_access.view = view_id;
> +    arg->u.mem_access.hvmmem_access = access;
> +    arg->u.mem_access.gfn = gfn;
> +#endif
>
>      rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
>                   HYPERCALL_BUFFER_AS_ARG(arg));
> @@ -254,3 +260,30 @@ int xc_altp2m_set_mem_access_multi(xc_interface *xch, uint32_t domid,
>
>      return rc;
>  }
> +
> +int xc_altp2m_get_mem_access(xc_interface *handle, uint32_t domid,
> +                             uint16_t view_id, xen_pfn_t gfn,
> +                             xenmem_access_t *access)
> +{
> +    int rc;
> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
> +
> +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
> +    if ( arg == NULL )
> +        return -1;
> +
> +    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
> +    arg->cmd = HVMOP_altp2m_get_mem_access;
> +    arg->domain = domid;
> +    arg->u.mem_access.view = view_id;
> +    arg->u.mem_access.gfn = gfn;
> +
> +    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
> +                 HYPERCALL_BUFFER_AS_ARG(arg));
> +
> +    if ( !rc )
> +        *access = arg->u.mem_access.hvmmem_access;
> +
> +    xc_hypercall_buffer_free(handle, arg);
> +    return rc;
> +}
> diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> index ba4ec780fd..178bc1a6c1 100644
> --- a/xen/arch/arm/mem_access.c
> +++ b/xen/arch/arm/mem_access.c
> @@ -236,7 +236,7 @@ bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
>      if ( !p2m->mem_access_enabled )
>          return true;
>
> -    rc = p2m_get_mem_access(v->domain, gaddr_to_gfn(gpa), &xma);
> +    rc = p2m_get_mem_access(v->domain, gaddr_to_gfn(gpa), &xma, 0);
>      if ( rc )
>          return true;
>
> @@ -441,11 +441,15 @@ long p2m_set_mem_access_multi(struct domain *d,
>  }
>
>  int p2m_get_mem_access(struct domain *d, gfn_t gfn,
> -                       xenmem_access_t *access)
> +                       xenmem_access_t *access, unsigned int altp2m_idx)
>  {
>      int ret;
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>
> +    /* altp2m is not yet implemented on Arm. The altp2m_idx should be 0. */

Just use ASSERT here.

> +    if ( altp2m_idx )
> +        return -EINVAL;
> +
>      p2m_read_lock(p2m);
>      ret = __p2m_get_mem_access(d, gfn, access);
>      p2m_read_unlock(p2m);
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 72c51faecb..460c9f7d4f 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4526,6 +4526,7 @@ static int do_altp2m_op(
>      case HVMOP_altp2m_switch_p2m:
>      case HVMOP_altp2m_set_mem_access:
>      case HVMOP_altp2m_set_mem_access_multi:
> +    case HVMOP_altp2m_get_mem_access:
>      case HVMOP_altp2m_change_gfn:
>          break;
>
> @@ -4642,12 +4643,21 @@ static int do_altp2m_op(
>          break;
>
>      case HVMOP_altp2m_set_mem_access:
> +#if __XEN_INTERFACE_VERSION__ < 0x00040a00
>          if ( a.u.set_mem_access.pad )
>              rc = -EINVAL;
>          else
>              rc = p2m_set_mem_access(d, _gfn(a.u.set_mem_access.gfn), 1, 0, 0,
>                                      a.u.set_mem_access.hvmmem_access,
>                                      a.u.set_mem_access.view);
> +#else /* __XEN_INTERFACE_VERSION__ >= 0x00040a00 */
> +        if ( a.u.mem_access.pad )
> +            rc = -EINVAL;
> +        else
> +            rc = p2m_set_mem_access(d, _gfn(a.u.mem_access.gfn), 1, 0, 0,
> +                                    a.u.mem_access.hvmmem_access,
> +                                    a.u.mem_access.view);
> +#endif
>          break;
>
>      case HVMOP_altp2m_set_mem_access_multi:
> @@ -4683,6 +4693,23 @@ static int do_altp2m_op(
>          }
>          break;
>
> +    case HVMOP_altp2m_get_mem_access:
> +        if ( a.u.mem_access.pad )
> +            rc = -EINVAL;
> +        else
> +        {
> +            xenmem_access_t access;
> +
> +            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
> +                                    a.u.mem_access.view);
> +            if ( !rc )
> +            {
> +                a.u.mem_access.hvmmem_access = access;
> +                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> +            }
> +        }
> +        break;
> +
>      case HVMOP_altp2m_change_gfn:
>          if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
>              rc = -EINVAL;
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 84d260ebd8..78abdaed36 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -469,9 +469,26 @@ long p2m_set_mem_access_multi(struct domain *d,
>      return rc;
>  }
>
> -int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
> +int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access,
> +                       unsigned int altp2m_idx)
>  {
> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *p2m;
> +
> +    if ( !altp2m_active(d) )
> +    {
> +        if ( altp2m_idx )
> +            return -EINVAL;
> +
> +        p2m = p2m_get_hostp2m(d);
> +    }
> +    else
> +    {
> +        if ( altp2m_idx >= MAX_ALTP2M ||
> +             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
> +            return -EINVAL;
> +
> +        p2m = d->arch.altp2m_p2m[altp2m_idx];
> +    }
>
>      return _p2m_get_mem_access(p2m, gfn, access);
>  }
> diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c
> index 1bf6824442..010e6f8dbf 100644
> --- a/xen/common/mem_access.c
> +++ b/xen/common/mem_access.c
> @@ -99,7 +99,7 @@ int mem_access_memop(unsigned long cmd,
>          if ( (mao.pfn > domain_get_maximum_gpfn(d)) && mao.pfn != ~0ull )
>              break;
>
> -        rc = p2m_get_mem_access(d, _gfn(mao.pfn), &access);
> +        rc = p2m_get_mem_access(d, _gfn(mao.pfn), &access, 0);
>          if ( rc != 0 )
>              break;
>
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index bbba99e5f5..bbb0aa984a 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -234,6 +234,7 @@ struct xen_hvm_altp2m_view {
>  typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t);
>
> +#if __XEN_INTERFACE_VERSION__ < 0x00040a00
>  struct xen_hvm_altp2m_set_mem_access {
>      /* view */
>      uint16_t view;
> @@ -245,6 +246,19 @@ 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);
> +#endif /* __XEN_INTERFACE_VERSION__ < 0x00040a00 */
> +
> +struct xen_hvm_altp2m_mem_access {
> +    /* view */
> +    uint16_t view;
> +    /* Memory type */
> +    uint16_t hvmmem_access; /* xenmem_access_t */

A structure name with "mem_access" having a variable name
"hvmmem_access" and a comment saying "xenmem_access". This is
confusing. I understand that you copy/pasted this from the existing op
but it doesn't look good. Perhaps fix both if we are touching it?
Also, in public/memory.h the width of access is uint8_t so I'm not
sure why the discrepancy.

> +    uint32_t pad;
> +    /* gfn */
> +    uint64_t gfn;
> +};
> +typedef struct xen_hvm_altp2m_mem_access xen_hvm_altp2m_mem_access_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_mem_access_t);
>
>  struct xen_hvm_altp2m_set_mem_access_multi {
>      /* view */
> @@ -296,6 +310,8 @@ struct xen_hvm_altp2m_op {
>  #define HVMOP_altp2m_change_gfn           8
>  /* Set access for an array of pages */
>  #define HVMOP_altp2m_set_mem_access_multi 9
> +/* Get the access of a page of memory from a certain view */
> +#define HVMOP_altp2m_get_mem_access       10
>      domid_t domain;
>      uint16_t pad1;
>      uint32_t pad2;
> @@ -303,7 +319,10 @@ struct xen_hvm_altp2m_op {
>          struct xen_hvm_altp2m_domain_state         domain_state;
>          struct xen_hvm_altp2m_vcpu_enable_notify   enable_notify;
>          struct xen_hvm_altp2m_view                 view;
> +#if __XEN_INTERFACE_VERSION__ < 0x00040a00
>          struct xen_hvm_altp2m_set_mem_access       set_mem_access;
> +#endif /* __XEN_INTERFACE_VERSION__ < 0x00040a00 */
> +        struct xen_hvm_altp2m_mem_access           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];
> diff --git a/xen/include/public/xen-compat.h b/xen/include/public/xen-compat.h
> index b67365340b..fa6ffb72e8 100644
> --- a/xen/include/public/xen-compat.h
> +++ b/xen/include/public/xen-compat.h
> @@ -27,7 +27,7 @@
>  #ifndef __XEN_PUBLIC_XEN_COMPAT_H__
>  #define __XEN_PUBLIC_XEN_COMPAT_H__
>
> -#define __XEN_LATEST_INTERFACE_VERSION__ 0x00040900
> +#define __XEN_LATEST_INTERFACE_VERSION__ 0x00040a00
>
>  #if defined(__XEN__) || defined(__XEN_TOOLS__)
>  /* Xen is built with matching headers and implements the latest interface. */
> diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
> index 7e95eab81c..7348f81233 100644
> --- a/xen/include/xen/mem_access.h
> +++ b/xen/include/xen/mem_access.h
> @@ -76,7 +76,8 @@ long p2m_set_mem_access_multi(struct domain *d,
>   * Get access type for a gfn.
>   * If gfn == INVALID_GFN, gets the default access type.
>   */
> -int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access);
> +int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access,
> +                       unsigned int altp2m_idx);
>
>  #ifdef CONFIG_MEM_ACCESS
>  int mem_access_memop(unsigned long cmd,
> --
> 2.18.0

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

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

* Re: [PATCH v4] x86/altp2m: Add a subop for obtaining the mem access of a page
  2018-09-19 15:29 ` Tamas K Lengyel
@ 2018-09-25 15:03   ` Razvan Cojocaru
  2018-09-25 16:36     ` Tamas K Lengyel
  0 siblings, 1 reply; 7+ messages in thread
From: Razvan Cojocaru @ 2018-09-25 15:03 UTC (permalink / raw)
  To: Tamas K Lengyel, Adrian Pop
  Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
	George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
	Julien Grall, Jan Beulich, Sergej Proskurin, Xen-devel

On 9/19/18 6:29 PM, Tamas K Lengyel wrote:
> On Mon, Sep 3, 2018 at 9:47 AM Adrian Pop <apop@bitdefender.com> wrote:
>> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
>> index bbba99e5f5..bbb0aa984a 100644
>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.h
>> @@ -234,6 +234,7 @@ struct xen_hvm_altp2m_view {
>>  typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t);
>>
>> +#if __XEN_INTERFACE_VERSION__ < 0x00040a00
>>  struct xen_hvm_altp2m_set_mem_access {
>>      /* view */
>>      uint16_t view;
>> @@ -245,6 +246,19 @@ 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);
>> +#endif /* __XEN_INTERFACE_VERSION__ < 0x00040a00 */
>> +
>> +struct xen_hvm_altp2m_mem_access {
>> +    /* view */
>> +    uint16_t view;
>> +    /* Memory type */
>> +    uint16_t hvmmem_access; /* xenmem_access_t */
> 
> A structure name with "mem_access" having a variable name
> "hvmmem_access" and a comment saying "xenmem_access". This is
> confusing. I understand that you copy/pasted this from the existing op
> but it doesn't look good. Perhaps fix both if we are touching it?
> Also, in public/memory.h the width of access is uint8_t so I'm not
> sure why the discrepancy.

The uint16_t has been probably chosen to be that because it simplifies
padding. I can rename "hvmmem_access" to simply "access". Should I just
remove the comment, or do you think something else is more appropriate?


Thanks,
Razvan

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

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

* Re: [PATCH v4] x86/altp2m: Add a subop for obtaining the mem access of a page
  2018-09-25 15:03   ` Razvan Cojocaru
@ 2018-09-25 16:36     ` Tamas K Lengyel
  0 siblings, 0 replies; 7+ messages in thread
From: Tamas K Lengyel @ 2018-09-25 16:36 UTC (permalink / raw)
  To: Razvan Cojocaru
  Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
	George Dunlap, Andrew Cooper, Adrian Pop, Ian Jackson,
	Tim Deegan, Julien Grall, Jan Beulich, Sergej Proskurin,
	Xen-devel

On Tue, Sep 25, 2018 at 9:04 AM Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
>
> On 9/19/18 6:29 PM, Tamas K Lengyel wrote:
> > On Mon, Sep 3, 2018 at 9:47 AM Adrian Pop <apop@bitdefender.com> wrote:
> >> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> >> index bbba99e5f5..bbb0aa984a 100644
> >> --- a/xen/include/public/hvm/hvm_op.h
> >> +++ b/xen/include/public/hvm/hvm_op.h
> >> @@ -234,6 +234,7 @@ struct xen_hvm_altp2m_view {
> >>  typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;
> >>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t);
> >>
> >> +#if __XEN_INTERFACE_VERSION__ < 0x00040a00
> >>  struct xen_hvm_altp2m_set_mem_access {
> >>      /* view */
> >>      uint16_t view;
> >> @@ -245,6 +246,19 @@ 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);
> >> +#endif /* __XEN_INTERFACE_VERSION__ < 0x00040a00 */
> >> +
> >> +struct xen_hvm_altp2m_mem_access {
> >> +    /* view */
> >> +    uint16_t view;
> >> +    /* Memory type */
> >> +    uint16_t hvmmem_access; /* xenmem_access_t */
> >
> > A structure name with "mem_access" having a variable name
> > "hvmmem_access" and a comment saying "xenmem_access". This is
> > confusing. I understand that you copy/pasted this from the existing op
> > but it doesn't look good. Perhaps fix both if we are touching it?
> > Also, in public/memory.h the width of access is uint8_t so I'm not
> > sure why the discrepancy.
>
> The uint16_t has been probably chosen to be that because it simplifies
> padding. I can rename "hvmmem_access" to simply "access". Should I just
> remove the comment, or do you think something else is more appropriate?

Just renaming the variable should suffice, the comment is helpful to
explain what values go in the field.

Thanks,
Tamas

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

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

end of thread, other threads:[~2018-09-25 16:37 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-03 15:47 [PATCH v4] x86/altp2m: Add a subop for obtaining the mem access of a page Adrian Pop
2018-09-19 14:44 ` Wei Liu
2018-09-19 14:48   ` Jan Beulich
2018-09-19 14:51     ` Wei Liu
2018-09-19 15:29 ` Tamas K Lengyel
2018-09-25 15:03   ` Razvan Cojocaru
2018-09-25 16:36     ` Tamas K Lengyel

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.