All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V5] x86/altp2m: Add a subop for obtaining the mem access of a page
@ 2018-09-26  7:34 Razvan Cojocaru
  2018-09-26 11:39 ` Razvan Cojocaru
  0 siblings, 1 reply; 9+ messages in thread
From: Razvan Cojocaru @ 2018-09-26  7:34 UTC (permalink / raw)
  To: xen-devel
  Cc: tamas, wei.liu2, Razvan Cojocaru, konrad.wilk, George.Dunlap,
	andrew.cooper3, Adrian Pop, tim, julien.grall, sstabellini,
	jbeulich, ian.jackson

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>
Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

---
Changes since V4:
 - Replaced the "if ( altp2m_idx ) return -EINVAL;" test in
   p2m_get_mem_access() with "ASSERT(altp2m_idx == 0);" for ARM.
 - Removed XEN_INTERFACE_VERSION #ifdeffery from the toolstack and
   hypervisor (only kept them in hvm_op.h).
 - Renamed hvmmem_access to simply access (on Tamas' request) in
   xen_hvm_altp2m_set_mem_access and xen_hvm_altp2m_mem_access.
---
 tools/libxc/include/xenctrl.h   |  3 +++
 tools/libxc/xc_altp2m.c         | 33 ++++++++++++++++++++++++++++++---
 xen/arch/arm/mem_access.c       |  7 +++++--
 xen/arch/x86/hvm/hvm.c          | 30 ++++++++++++++++++++++++------
 xen/arch/x86/mm/mem_access.c    | 21 +++++++++++++++++++--
 xen/common/mem_access.c         |  2 +-
 xen/include/public/hvm/hvm_op.h | 21 ++++++++++++++++++++-
 xen/include/public/xen-compat.h |  2 +-
 xen/include/xen/mem_access.h    |  3 ++-
 9 files changed, 105 insertions(+), 17 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index dad96a9..618f3cb 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1949,6 +1949,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 be5bfd2..844b9f1 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -226,9 +226,9 @@ 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;
-    arg->u.set_mem_access.view = view_id;
-    arg->u.set_mem_access.hvmmem_access = access;
-    arg->u.set_mem_access.gfn = gfn;
+    arg->u.mem_access.view = view_id;
+    arg->u.mem_access.access = access;
+    arg->u.mem_access.gfn = gfn;
 
     rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
 		  HYPERCALL_BUFFER_AS_ARG(arg));
@@ -303,3 +303,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.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 ba4ec78..653d960 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,14 @@ 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. */
+    ASSERT(altp2m_idx == 0);
+
     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 51fc3ec..8a9abf3 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4473,6 +4473,7 @@ static int do_altp2m_op(
     case HVMOP_altp2m_get_suppress_ve:
     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;
 
@@ -4600,8 +4601,8 @@ static int do_altp2m_op(
             rc = -EINVAL;
         else
         {
-            gfn_t gfn = _gfn(a.u.set_mem_access.gfn);
-            unsigned int altp2m_idx = a.u.set_mem_access.view;
+            gfn_t gfn = _gfn(a.u.mem_access.gfn);
+            unsigned int altp2m_idx = a.u.mem_access.view;
             bool suppress_ve = a.u.suppress_ve.suppress_ve;
 
             rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx);
@@ -4627,12 +4628,12 @@ static int do_altp2m_op(
         break;
 
     case HVMOP_altp2m_set_mem_access:
-        if ( a.u.set_mem_access.pad )
+        if ( a.u.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);
+            rc = p2m_set_mem_access(d, _gfn(a.u.mem_access.gfn), 1, 0, 0,
+                                    a.u.mem_access.access,
+                                    a.u.mem_access.view);
         break;
 
     case HVMOP_altp2m_set_mem_access_multi:
@@ -4668,6 +4669,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.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 3d50fe0..67b4a1d 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -486,9 +486,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 1bf6824..010e6f8 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 cf00cad..5878a25 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -242,17 +242,31 @@ 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;
     /* Memory type */
-    uint16_t hvmmem_access; /* xenmem_access_t */
+    uint16_t access; /* xenmem_access_t */
     uint32_t pad;
     /* gfn */
     uint64_t gfn;
 };
 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 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 */
@@ -308,6 +322,8 @@ struct xen_hvm_altp2m_op {
 #define HVMOP_altp2m_set_suppress_ve      10
 /* Get the "Suppress #VE" bit of a page */
 #define HVMOP_altp2m_get_suppress_ve      11
+/* Get the access of a page of memory from a certain view */
+#define HVMOP_altp2m_get_mem_access       12
     domid_t domain;
     uint16_t pad1;
     uint32_t pad2;
@@ -315,7 +331,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;
         struct xen_hvm_altp2m_suppress_ve          suppress_ve;
diff --git a/xen/include/public/xen-compat.h b/xen/include/public/xen-compat.h
index b673653..fa6ffb7 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 28cab67..e4d2450 100644
--- a/xen/include/xen/mem_access.h
+++ b/xen/include/xen/mem_access.h
@@ -82,7 +82,8 @@ int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
  * 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.7.4


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

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

* [PATCH V5] x86/altp2m: Add a subop for obtaining the mem access of a page
  2018-09-26  7:34 [PATCH V5] x86/altp2m: Add a subop for obtaining the mem access of a page Razvan Cojocaru
@ 2018-09-26 11:39 ` Razvan Cojocaru
  2018-09-26 12:26   ` Razvan Cojocaru
  0 siblings, 1 reply; 9+ messages in thread
From: Razvan Cojocaru @ 2018-09-26 11:39 UTC (permalink / raw)
  To: xen-devel, wei.liu2
  Cc: tamas, konrad.wilk, george.dunlap, andrew.cooper3, Adrian Pop,
	tim, julien.grall, sstabellini, jbeulich, ian.jackson

On 9/26/18 10:34 AM, Razvan Cojocaru 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>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

Sorry, I've just found that this change will break the build of
tools/firmware/xen-dir/xen-root/xen/arch/x86/mm/mem_access.c, because -
while it's building just fine when doing so for the hypervisor (i.e.
"make" in xen/), building the tools somehow doesn't take into account
CONFIG_HVM (output of "make dist"):

gcc -m64 -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall
-Wstrict-prototypes -Wdeclaration-after-statement
-Wno-unused-but-set-variable -Wno-unused-local-typedefs   -O2
-fomit-frame-pointer -nostdinc -fno-builtin -fno-common -Werror
-Wredundant-decls -Wno-pointer-arith -Wvla -pipe -D__XEN__ -include
/home/red/work/pristine/xen.git/tools/firmware/xen-dir/xen-root/xen/include/xen/config.h
'-D__OBJECT_FILE__="mem_access.o"' -Wa,--strip-local-absolute -g -MMD
-MF ./.mem_access.o.d
-I/home/red/work/pristine/xen.git/tools/firmware/xen-dir/xen-root/xen/include
-I/home/red/work/pristine/xen.git/tools/firmware/xen-dir/xen-root/xen/include/asm-x86/mach-generic
-I/home/red/work/pristine/xen.git/tools/firmware/xen-dir/xen-root/xen/include/asm-x86/mach-default
-DXEN_IMG_OFFSET=0x200000 '-D__OBJECT_LABEL__=arch$x86$mm$mem_access.o'
-msoft-float -fno-stack-protector -fno-exceptions -Wnested-externs
-DHAVE_AS_VMX -DHAVE_AS_SSE4_2 -DHAVE_AS_EPT -DHAVE_AS_RDRAND
-DHAVE_AS_FSGSBASE -DHAVE_AS_RDSEED -U__OBJECT_LABEL__
-DHAVE_AS_QUOTED_SYM '-D__OBJECT_LABEL__=arch/x86/mm/mem_access.o'
-DHAVE_AS_INVPCID -DHAVE_AS_NEGATIVE_TRUE  -mno-red-zone -fpic
-fno-asynchronous-unwind-tables -mno-sse -mskip-rax-setup
-DGCC_HAS_VISIBILITY_ATTRIBUTE -mindirect-branch=thunk-extern
-mindirect-branch-register -DCONFIG_INDIRECT_THUNK
-Wa,-I/home/red/work/pristine/xen.git/tools/firmware/xen-dir/xen-root/xen/include
-c mem_access.c -o mem_access.o
mem_access.c: In function ‘p2m_get_mem_access’:
mem_access.c:504:21: error: ‘struct arch_domain’ has no member named
‘altp2m_eptp’
              d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
                     ^
mem_access.c:507:22: error: ‘struct arch_domain’ has no member named
‘altp2m_p2m’
         p2m = d->arch.altp2m_p2m[altp2m_idx];
                      ^

Wei, thoughts on how this would be best approached?


Thanks,
Razvan

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

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

* Re: [PATCH V5] x86/altp2m: Add a subop for obtaining the mem access of a page
  2018-09-26 11:39 ` Razvan Cojocaru
@ 2018-09-26 12:26   ` Razvan Cojocaru
  2018-09-26 13:20     ` Jan Beulich
  2018-09-28  9:04     ` Wei Liu
  0 siblings, 2 replies; 9+ messages in thread
From: Razvan Cojocaru @ 2018-09-26 12:26 UTC (permalink / raw)
  To: xen-devel, wei.liu2
  Cc: tamas, konrad.wilk, george.dunlap, andrew.cooper3, Adrian Pop,
	tim, julien.grall, sstabellini, jbeulich, ian.jackson

On 9/26/18 2:39 PM, Razvan Cojocaru wrote:
> On 9/26/18 10:34 AM, Razvan Cojocaru 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>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> 
> Sorry, I've just found that this change will break the build of
> tools/firmware/xen-dir/xen-root/xen/arch/x86/mm/mem_access.c, because -
> while it's building just fine when doing so for the hypervisor (i.e.
> "make" in xen/), building the tools somehow doesn't take into account
> CONFIG_HVM (output of "make dist"):
> 
> gcc -m64 -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall
> -Wstrict-prototypes -Wdeclaration-after-statement
> -Wno-unused-but-set-variable -Wno-unused-local-typedefs   -O2
> -fomit-frame-pointer -nostdinc -fno-builtin -fno-common -Werror
> -Wredundant-decls -Wno-pointer-arith -Wvla -pipe -D__XEN__ -include
> /home/red/work/pristine/xen.git/tools/firmware/xen-dir/xen-root/xen/include/xen/config.h
> '-D__OBJECT_FILE__="mem_access.o"' -Wa,--strip-local-absolute -g -MMD
> -MF ./.mem_access.o.d
> -I/home/red/work/pristine/xen.git/tools/firmware/xen-dir/xen-root/xen/include
> -I/home/red/work/pristine/xen.git/tools/firmware/xen-dir/xen-root/xen/include/asm-x86/mach-generic
> -I/home/red/work/pristine/xen.git/tools/firmware/xen-dir/xen-root/xen/include/asm-x86/mach-default
> -DXEN_IMG_OFFSET=0x200000 '-D__OBJECT_LABEL__=arch$x86$mm$mem_access.o'
> -msoft-float -fno-stack-protector -fno-exceptions -Wnested-externs
> -DHAVE_AS_VMX -DHAVE_AS_SSE4_2 -DHAVE_AS_EPT -DHAVE_AS_RDRAND
> -DHAVE_AS_FSGSBASE -DHAVE_AS_RDSEED -U__OBJECT_LABEL__
> -DHAVE_AS_QUOTED_SYM '-D__OBJECT_LABEL__=arch/x86/mm/mem_access.o'
> -DHAVE_AS_INVPCID -DHAVE_AS_NEGATIVE_TRUE  -mno-red-zone -fpic
> -fno-asynchronous-unwind-tables -mno-sse -mskip-rax-setup
> -DGCC_HAS_VISIBILITY_ATTRIBUTE -mindirect-branch=thunk-extern
> -mindirect-branch-register -DCONFIG_INDIRECT_THUNK
> -Wa,-I/home/red/work/pristine/xen.git/tools/firmware/xen-dir/xen-root/xen/include
> -c mem_access.c -o mem_access.o
> mem_access.c: In function ‘p2m_get_mem_access’:
> mem_access.c:504:21: error: ‘struct arch_domain’ has no member named
> ‘altp2m_eptp’
>               d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
>                      ^
> mem_access.c:507:22: error: ‘struct arch_domain’ has no member named
> ‘altp2m_p2m’
>          p2m = d->arch.altp2m_p2m[altp2m_idx];
>                       ^
> 
> Wei, thoughts on how this would be best approached?

To clarify the question, I'll of course do this:

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 67b4a1d..2b5a621 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -489,14 +489,13 @@ long p2m_set_mem_access_multi(struct domain *d,
 int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t
*access,
                        unsigned int altp2m_idx)
 {
-    struct p2m_domain *p2m;
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);

+#ifdef CONFIG_HVM
     if ( !altp2m_active(d) )
     {
         if ( altp2m_idx )
             return -EINVAL;
-
-        p2m = p2m_get_hostp2m(d);
     }
     else
     {
@@ -506,6 +505,9 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn,
xenmem_access_t *access,

         p2m = d->arch.altp2m_p2m[altp2m_idx];
     }
+#else
+    ASSERT(!altp2m_idx);
+#endif

     return _p2m_get_mem_access(p2m, gfn, access);
 }

but is it OK that the hypervisor builds with a set of flags that
includes CONFIG_HVM and the firmware code with a set that doesn't?


Thanks,
Razvan

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

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

* Re: [PATCH V5] x86/altp2m: Add a subop for obtaining the mem access of a page
  2018-09-26 12:26   ` Razvan Cojocaru
@ 2018-09-26 13:20     ` Jan Beulich
  2018-09-26 13:27       ` Razvan Cojocaru
  2018-09-28  9:04     ` Wei Liu
  1 sibling, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2018-09-26 13:20 UTC (permalink / raw)
  To: Razvan Cojocaru
  Cc: Stefano Stabellini, Wei Liu, Adrian Pop, George Dunlap,
	Andrew Cooper, Konrad Rzeszutek Wilk, Ian Jackson, Tim Deegan,
	Julien Grall, Tamas K Lengyel, xen-devel

>>> On 26.09.18 at 14:26, <rcojocaru@bitdefender.com> wrote:
> To clarify the question, I'll of course do this:
> 
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 67b4a1d..2b5a621 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -489,14 +489,13 @@ long p2m_set_mem_access_multi(struct domain *d,
>  int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t
> *access,
>                         unsigned int altp2m_idx)
>  {
> -    struct p2m_domain *p2m;
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> 
> +#ifdef CONFIG_HVM
>      if ( !altp2m_active(d) )
>      {
>          if ( altp2m_idx )
>              return -EINVAL;
> -
> -        p2m = p2m_get_hostp2m(d);
>      }
>      else
>      {
> @@ -506,6 +505,9 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn,
> xenmem_access_t *access,
> 
>          p2m = d->arch.altp2m_p2m[altp2m_idx];
>      }
> +#else
> +    ASSERT(!altp2m_idx);
> +#endif
> 
>      return _p2m_get_mem_access(p2m, gfn, access);
>  }
> 
> but is it OK that the hypervisor builds with a set of flags that
> includes CONFIG_HVM and the firmware code with a set that doesn't?

Is this perhaps simply (so far unnoticed) fallout from Wei's CONFIG_HVM-
disabling work? Or insufficient re-basing of your change on top of his
work? The shim now builds with HVM=n, while the hypervisor (unless
you've overridden the default) uses HVM=y.

Jan



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

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

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

On 9/26/18 4:20 PM, Jan Beulich wrote:
>>>> On 26.09.18 at 14:26, <rcojocaru@bitdefender.com> wrote:
>> To clarify the question, I'll of course do this:
>>
>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>> index 67b4a1d..2b5a621 100644
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -489,14 +489,13 @@ long p2m_set_mem_access_multi(struct domain *d,
>>  int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t
>> *access,
>>                         unsigned int altp2m_idx)
>>  {
>> -    struct p2m_domain *p2m;
>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>
>> +#ifdef CONFIG_HVM
>>      if ( !altp2m_active(d) )
>>      {
>>          if ( altp2m_idx )
>>              return -EINVAL;
>> -
>> -        p2m = p2m_get_hostp2m(d);
>>      }
>>      else
>>      {
>> @@ -506,6 +505,9 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn,
>> xenmem_access_t *access,
>>
>>          p2m = d->arch.altp2m_p2m[altp2m_idx];
>>      }
>> +#else
>> +    ASSERT(!altp2m_idx);
>> +#endif
>>
>>      return _p2m_get_mem_access(p2m, gfn, access);
>>  }
>>
>> but is it OK that the hypervisor builds with a set of flags that
>> includes CONFIG_HVM and the firmware code with a set that doesn't?
> 
> Is this perhaps simply (so far unnoticed) fallout from Wei's CONFIG_HVM-
> disabling work? Or insufficient re-basing of your change on top of his
> work? The shim now builds with HVM=n, while the hypervisor (unless
> you've overridden the default) uses HVM=y.

I believe I'm up-to-date:

$ git pull --rebase origin staging
From git://xenbits.xenproject.org/xen
 * branch            staging    -> FETCH_HEAD
Current branch altp2m-work is up to date.

I've also ran "make clean", "make distclean", "configure" - again, and
"make dist" one more time, with the same results (mem_access.c won't
compile in the shim).

In any case, I'll resend a corrected (as above) version tomorrow, to
hopefully get more comments on the current version in the meantime.


Thanks,
Razvan

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

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

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

>>> On 26.09.18 at 15:27, <rcojocaru@bitdefender.com> wrote:
> On 9/26/18 4:20 PM, Jan Beulich wrote:
>>>>> On 26.09.18 at 14:26, <rcojocaru@bitdefender.com> wrote:
>>> To clarify the question, I'll of course do this:
>>>
>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>>> index 67b4a1d..2b5a621 100644
>>> --- a/xen/arch/x86/mm/mem_access.c
>>> +++ b/xen/arch/x86/mm/mem_access.c
>>> @@ -489,14 +489,13 @@ long p2m_set_mem_access_multi(struct domain *d,
>>>  int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t
>>> *access,
>>>                         unsigned int altp2m_idx)
>>>  {
>>> -    struct p2m_domain *p2m;
>>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>
>>> +#ifdef CONFIG_HVM
>>>      if ( !altp2m_active(d) )
>>>      {
>>>          if ( altp2m_idx )
>>>              return -EINVAL;
>>> -
>>> -        p2m = p2m_get_hostp2m(d);
>>>      }
>>>      else
>>>      {
>>> @@ -506,6 +505,9 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn,
>>> xenmem_access_t *access,
>>>
>>>          p2m = d->arch.altp2m_p2m[altp2m_idx];
>>>      }
>>> +#else
>>> +    ASSERT(!altp2m_idx);
>>> +#endif
>>>
>>>      return _p2m_get_mem_access(p2m, gfn, access);
>>>  }
>>>
>>> but is it OK that the hypervisor builds with a set of flags that
>>> includes CONFIG_HVM and the firmware code with a set that doesn't?
>> 
>> Is this perhaps simply (so far unnoticed) fallout from Wei's CONFIG_HVM-
>> disabling work? Or insufficient re-basing of your change on top of his
>> work? The shim now builds with HVM=n, while the hypervisor (unless
>> you've overridden the default) uses HVM=y.
> 
> I believe I'm up-to-date:
> 
> $ git pull --rebase origin staging
> From git://xenbits.xenproject.org/xen
>  * branch            staging    -> FETCH_HEAD
> Current branch altp2m-work is up to date.
> 
> I've also ran "make clean", "make distclean", "configure" - again, and
> "make dist" one more time, with the same results (mem_access.c won't
> compile in the shim).

I didn't imply you're on an outdated tree, but rather that you may not
have done all changes necessary while re-basing your change over
upstream commits.

Jan



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

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

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

On 9/26/18 4:39 PM, Jan Beulich wrote:
>>>> On 26.09.18 at 15:27, <rcojocaru@bitdefender.com> wrote:
>> On 9/26/18 4:20 PM, Jan Beulich wrote:
>>>>>> On 26.09.18 at 14:26, <rcojocaru@bitdefender.com> wrote:
>>>> To clarify the question, I'll of course do this:
>>>>
>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>>>> index 67b4a1d..2b5a621 100644
>>>> --- a/xen/arch/x86/mm/mem_access.c
>>>> +++ b/xen/arch/x86/mm/mem_access.c
>>>> @@ -489,14 +489,13 @@ long p2m_set_mem_access_multi(struct domain *d,
>>>>  int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t
>>>> *access,
>>>>                         unsigned int altp2m_idx)
>>>>  {
>>>> -    struct p2m_domain *p2m;
>>>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>>
>>>> +#ifdef CONFIG_HVM
>>>>      if ( !altp2m_active(d) )
>>>>      {
>>>>          if ( altp2m_idx )
>>>>              return -EINVAL;
>>>> -
>>>> -        p2m = p2m_get_hostp2m(d);
>>>>      }
>>>>      else
>>>>      {
>>>> @@ -506,6 +505,9 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn,
>>>> xenmem_access_t *access,
>>>>
>>>>          p2m = d->arch.altp2m_p2m[altp2m_idx];
>>>>      }
>>>> +#else
>>>> +    ASSERT(!altp2m_idx);
>>>> +#endif
>>>>
>>>>      return _p2m_get_mem_access(p2m, gfn, access);
>>>>  }
>>>>
>>>> but is it OK that the hypervisor builds with a set of flags that
>>>> includes CONFIG_HVM and the firmware code with a set that doesn't?
>>>
>>> Is this perhaps simply (so far unnoticed) fallout from Wei's CONFIG_HVM-
>>> disabling work? Or insufficient re-basing of your change on top of his
>>> work? The shim now builds with HVM=n, while the hypervisor (unless
>>> you've overridden the default) uses HVM=y.
>>
>> I believe I'm up-to-date:
>>
>> $ git pull --rebase origin staging
>> From git://xenbits.xenproject.org/xen
>>  * branch            staging    -> FETCH_HEAD
>> Current branch altp2m-work is up to date.
>>
>> I've also ran "make clean", "make distclean", "configure" - again, and
>> "make dist" one more time, with the same results (mem_access.c won't
>> compile in the shim).
> 
> I didn't imply you're on an outdated tree, but rather that you may not
> have done all changes necessary while re-basing your change over
> upstream commits.

Other than the above #ifdef-ery, I don't think I've missed anything
else, no. I've also now done an full introspection test with the patch
and everything seems to behave the way it's supposed to.


Thanks,
Razvan

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

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

* Re: [PATCH V5] x86/altp2m: Add a subop for obtaining the mem access of a page
  2018-09-26 12:26   ` Razvan Cojocaru
  2018-09-26 13:20     ` Jan Beulich
@ 2018-09-28  9:04     ` Wei Liu
  2018-09-28  9:16       ` Razvan Cojocaru
  1 sibling, 1 reply; 9+ messages in thread
From: Wei Liu @ 2018-09-28  9:04 UTC (permalink / raw)
  To: Razvan Cojocaru
  Cc: tamas, wei.liu2, konrad.wilk, george.dunlap, andrew.cooper3,
	Adrian Pop, tim, julien.grall, sstabellini, jbeulich, xen-devel,
	ian.jackson

On Wed, Sep 26, 2018 at 03:26:20PM +0300, Razvan Cojocaru wrote:
> but is it OK that the hypervisor builds with a set of flags that
> includes CONFIG_HVM and the firmware code with a set that doesn't?

To answer this question: yes, it is OK to do that. And that's deliberate
for the shim because it doesn't need any HVM interfaces. Jan has given
the reason why your build failed in his replies.

This build failure just exposed how much we took for granted some code
is always there. :)

Wei.

> 
> 
> Thanks,
> Razvan

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

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

* Re: [PATCH V5] x86/altp2m: Add a subop for obtaining the mem access of a page
  2018-09-28  9:04     ` Wei Liu
@ 2018-09-28  9:16       ` Razvan Cojocaru
  0 siblings, 0 replies; 9+ messages in thread
From: Razvan Cojocaru @ 2018-09-28  9:16 UTC (permalink / raw)
  To: Wei Liu
  Cc: tamas, konrad.wilk, george.dunlap, andrew.cooper3, Adrian Pop,
	tim, julien.grall, sstabellini, jbeulich, xen-devel, ian.jackson

On 9/28/18 12:04 PM, Wei Liu wrote:
> On Wed, Sep 26, 2018 at 03:26:20PM +0300, Razvan Cojocaru wrote:
>> but is it OK that the hypervisor builds with a set of flags that
>> includes CONFIG_HVM and the firmware code with a set that doesn't?
> 
> To answer this question: yes, it is OK to do that. And that's deliberate
> for the shim because it doesn't need any HVM interfaces. Jan has given
> the reason why your build failed in his replies.
> 
> This build failure just exposed how much we took for granted some code
> is always there. :)

I understand. Thanks for the reply! In that case, V6 should be correct
in that respect, so when you have the time please consider that version
for review.


Thanks,
Razvan

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

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

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

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-26  7:34 [PATCH V5] x86/altp2m: Add a subop for obtaining the mem access of a page Razvan Cojocaru
2018-09-26 11:39 ` Razvan Cojocaru
2018-09-26 12:26   ` Razvan Cojocaru
2018-09-26 13:20     ` Jan Beulich
2018-09-26 13:27       ` Razvan Cojocaru
2018-09-26 13:39         ` Jan Beulich
2018-09-26 14:01           ` Razvan Cojocaru
2018-09-28  9:04     ` Wei Liu
2018-09-28  9:16       ` Razvan Cojocaru

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.