[PATCH 0/7] xen: Consolidate asm-*/guest_access.h in xen/guest_access.h

[PATCH 0/7] xen: Consolidate asm-*/guest_access.h in xen/guest_access.h

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Hi all,

A lot of the helpers implemented in asm-*/guest_access.h are implemented
the same way. This series aims to avoid the duplication and implement
them only once in xen/guest_access.h.

Cheers,

Julien Grall (7):
  xen/guest_access: Add missing emacs magics
  xen/arm: kernel: Re-order the includes
  xen/arm: decode: Re-order the includes
  xen/arm: guestcopy: Re-order the includes
  xen: include xen/guest_access.h rather than asm/guest_access.h
  xen/guest_access: Consolidate guest access helpers in
    xen/guest_access.h
  xen/guest_access: Fix coding style in xen/guest_access.h

 xen/arch/arm/decode.c                |   7 +-
 xen/arch/arm/domain.c                |   2 +-
 xen/arch/arm/guest_walk.c            |   3 +-
 xen/arch/arm/guestcopy.c             |   5 +-
 xen/arch/arm/kernel.c                |  12 +--
 xen/arch/arm/vgic-v3-its.c           |   2 +-
 xen/arch/x86/hvm/svm/svm.c           |   2 +-
 xen/arch/x86/hvm/viridian/viridian.c |   2 +-
 xen/arch/x86/hvm/vmx/vmx.c           |   2 +-
 xen/common/libelf/libelf-loader.c    |   2 +-
 xen/include/asm-arm/guest_access.h   | 132 ------------------------
 xen/include/asm-x86/guest_access.h   | 129 ++---------------------
 xen/include/xen/guest_access.h       | 149 +++++++++++++++++++++++++++
 xen/lib/x86/private.h                |   2 +-
 14 files changed, 178 insertions(+), 273 deletions(-)

-- 
2.17.1

[PATCH 1/7] xen/guest_access: Add missing emacs magics

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Add missing emacs magics for xen/guest_access.h and
asm-x86/guest_access.h.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/include/asm-x86/guest_access.h | 8 ++++++++
 xen/include/xen/guest_access.h     | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/xen/include/asm-x86/guest_access.h b/xen/include/asm-x86/guest_access.h
index 0b58f2baee..9ee275d01f 100644
--- a/xen/include/asm-x86/guest_access.h
+++ b/xen/include/asm-x86/guest_access.h
@@ -175,3 +175,11 @@
 })
 
 #endif /* __ASM_X86_GUEST_ACCESS_H__ */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
index 09989df819..ef9aaa3efc 100644
--- a/xen/include/xen/guest_access.h
+++ b/xen/include/xen/guest_access.h
@@ -33,3 +33,11 @@ char *safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf,
                                   size_t size, size_t max_size);
 
 #endif /* __XEN_GUEST_ACCESS_H__ */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.17.1

[PATCH 2/7] xen/arm: kernel: Re-order the includes

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

We usually have xen/ includes first and then asm/. They are also ordered
alphabetically among themselves.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/kernel.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 8eff074836..f95fa392af 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -3,20 +3,20 @@
  *
  * Copyright (C) 2011 Citrix Systems, Inc.
  */
+#include <xen/domain_page.h>
 #include <xen/errno.h>
+#include <xen/gunzip.h>
 #include <xen/init.h>
 #include <xen/lib.h>
+#include <xen/libfdt/libfdt.h>
 #include <xen/mm.h>
-#include <xen/domain_page.h>
 #include <xen/sched.h>
-#include <asm/byteorder.h>
-#include <asm/setup.h>
-#include <xen/libfdt/libfdt.h>
-#include <xen/gunzip.h>
 #include <xen/vmap.h>
 
+#include <asm/byteorder.h>
 #include <asm/guest_access.h>
 #include <asm/kernel.h>
+#include <asm/setup.h>
 
 #define UIMAGE_MAGIC          0x27051956
 #define UIMAGE_NMLEN          32
-- 
2.17.1

[PATCH 3/7] xen/arm: decode: Re-order the includes

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

We usually have xen/ includes first and then asm/. They are also ordered
alphabetically among themselves.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/decode.c | 5 +++--
 xen/arch/arm/kernel.c | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
index 8b1e15d118..144793c8ce 100644
--- a/xen/arch/arm/decode.c
+++ b/xen/arch/arm/decode.c
@@ -17,11 +17,12 @@
  * GNU General Public License for more details.
  */
 
-#include <xen/types.h>
+#include <xen/lib.h>
 #include <xen/sched.h>
+#include <xen/types.h>
+
 #include <asm/current.h>
 #include <asm/guest_access.h>
-#include <xen/lib.h>
 
 #include "decode.h"
 
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index f95fa392af..032923853f 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -5,6 +5,7 @@
  */
 #include <xen/domain_page.h>
 #include <xen/errno.h>
+#include <xen/guest_access.h>
 #include <xen/gunzip.h>
 #include <xen/init.h>
 #include <xen/lib.h>
@@ -14,7 +15,6 @@
 #include <xen/vmap.h>
 
 #include <asm/byteorder.h>
-#include <asm/guest_access.h>
 #include <asm/kernel.h>
 #include <asm/setup.h>
 
-- 
2.17.1

[PATCH 4/7] xen/arm: guestcopy: Re-order the includes

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

We usually have xen/ includes first and then asm/. They are also ordered
alphabetically among themselves.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/guestcopy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
index 7a0f3e9d5f..c8023e2bca 100644
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -1,7 +1,8 @@
-#include <xen/lib.h>
 #include <xen/domain_page.h>
+#include <xen/lib.h>
 #include <xen/mm.h>
 #include <xen/sched.h>
+
 #include <asm/current.h>
 #include <asm/guest_access.h>
 
-- 
2.17.1

[PATCH 5/7] xen: include xen/guest_access.h rather than asm/guest_access.h

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Only a few places are actually including asm/guest_access.h. While this
is fine today, a follow-up patch will want to move most of the helpers
from asm/guest_access.h to xen/guest_access.h.

To prepare the move, everyone should include xen/guest_access.h rather
than asm/guest_access.h.

Interestingly, asm-arm/guest_access.h includes xen/guest_access.h. The
inclusion is now removed as no-one but the latter should include the
former.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/decode.c                |  2 +-
 xen/arch/arm/domain.c                |  2 +-
 xen/arch/arm/guest_walk.c            |  3 ++-
 xen/arch/arm/guestcopy.c             |  2 +-
 xen/arch/arm/vgic-v3-its.c           |  2 +-
 xen/arch/x86/hvm/svm/svm.c           |  2 +-
 xen/arch/x86/hvm/viridian/viridian.c |  2 +-
 xen/arch/x86/hvm/vmx/vmx.c           |  2 +-
 xen/common/libelf/libelf-loader.c    |  2 +-
 xen/include/asm-arm/guest_access.h   |  1 -
 xen/include/asm-x86/guest_access.h   | 22 ++++++++++++----------
 xen/lib/x86/private.h                |  2 +-
 12 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
index 144793c8ce..792c2e92a7 100644
--- a/xen/arch/arm/decode.c
+++ b/xen/arch/arm/decode.c
@@ -17,12 +17,12 @@
  * GNU General Public License for more details.
  */
 
+#include <xen/guest_access.h>
 #include <xen/lib.h>
 #include <xen/sched.h>
 #include <xen/types.h>
 
 #include <asm/current.h>
-#include <asm/guest_access.h>
 
 #include "decode.h"
 
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 2190d908eb..b062c232b6 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -12,6 +12,7 @@
 #include <xen/bitops.h>
 #include <xen/errno.h>
 #include <xen/grant_table.h>
+#include <xen/guest_access.h>
 #include <xen/hypercall.h>
 #include <xen/init.h>
 #include <xen/lib.h>
@@ -26,7 +27,6 @@
 #include <asm/current.h>
 #include <asm/event.h>
 #include <asm/gic.h>
-#include <asm/guest_access.h>
 #include <asm/guest_atomics.h>
 #include <asm/irq.h>
 #include <asm/p2m.h>
diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index a1cdd7f4af..b4496c4c86 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -16,8 +16,9 @@
  */
 
 #include <xen/domain_page.h>
+#include <xen/guest_access.h>
 #include <xen/sched.h>
-#include <asm/guest_access.h>
+
 #include <asm/guest_walk.h>
 #include <asm/short-desc.h>
 
diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
index c8023e2bca..32681606d8 100644
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -1,10 +1,10 @@
 #include <xen/domain_page.h>
+#include <xen/guest_access.h>
 #include <xen/lib.h>
 #include <xen/mm.h>
 #include <xen/sched.h>
 
 #include <asm/current.h>
-#include <asm/guest_access.h>
 
 #define COPY_flush_dcache   (1U << 0)
 #define COPY_from_guest     (0U << 1)
diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 6e153c698d..58d939b85f 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -32,6 +32,7 @@
 #include <xen/bitops.h>
 #include <xen/config.h>
 #include <xen/domain_page.h>
+#include <xen/guest_access.h>
 #include <xen/lib.h>
 #include <xen/init.h>
 #include <xen/softirq.h>
@@ -39,7 +40,6 @@
 #include <xen/sched.h>
 #include <xen/sizes.h>
 #include <asm/current.h>
-#include <asm/guest_access.h>
 #include <asm/mmio.h>
 #include <asm/gic_v3_defs.h>
 #include <asm/gic_v3_its.h>
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 888f504a94..9e14a451eb 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -16,6 +16,7 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <xen/guest_access.h>
 #include <xen/init.h>
 #include <xen/lib.h>
 #include <xen/trace.h>
@@ -34,7 +35,6 @@
 #include <asm/cpufeature.h>
 #include <asm/processor.h>
 #include <asm/amd.h>
-#include <asm/guest_access.h>
 #include <asm/debugreg.h>
 #include <asm/msr.h>
 #include <asm/i387.h>
diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 977c1bc54f..dc7183a546 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -5,12 +5,12 @@
  * Hypervisor Top Level Functional Specification for more information.
  */
 
+#include <xen/guest_access.h>
 #include <xen/sched.h>
 #include <xen/version.h>
 #include <xen/hypercall.h>
 #include <xen/domain_page.h>
 #include <xen/param.h>
-#include <asm/guest_access.h>
 #include <asm/guest/hyperv-tlfs.h>
 #include <asm/paging.h>
 #include <asm/p2m.h>
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 1c398fdb6e..98e9c91ea3 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -15,6 +15,7 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <xen/guest_access.h>
 #include <xen/init.h>
 #include <xen/lib.h>
 #include <xen/param.h>
@@ -31,7 +32,6 @@
 #include <asm/regs.h>
 #include <asm/cpufeature.h>
 #include <asm/processor.h>
-#include <asm/guest_access.h>
 #include <asm/debugreg.h>
 #include <asm/msr.h>
 #include <asm/p2m.h>
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 0f468727d0..629cc0d3e6 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -16,7 +16,7 @@
  */
 
 #ifdef __XEN__
-#include <asm/guest_access.h>
+#include <xen/guest_access.h>
 #endif
 
 #include "libelf-private.h"
diff --git a/xen/include/asm-arm/guest_access.h b/xen/include/asm-arm/guest_access.h
index 4046d50347..93d56868f1 100644
--- a/xen/include/asm-arm/guest_access.h
+++ b/xen/include/asm-arm/guest_access.h
@@ -1,7 +1,6 @@
 #ifndef __ASM_ARM_GUEST_ACCESS_H__
 #define __ASM_ARM_GUEST_ACCESS_H__
 
-#include <xen/guest_access.h>
 #include <xen/errno.h>
 #include <xen/sched.h>
 
diff --git a/xen/include/asm-x86/guest_access.h b/xen/include/asm-x86/guest_access.h
index 9ee275d01f..5c3dfc47b6 100644
--- a/xen/include/asm-x86/guest_access.h
+++ b/xen/include/asm-x86/guest_access.h
@@ -54,22 +54,24 @@
 
 /* Cast a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
 #define guest_handle_to_param(hnd, type) ({                  \
+    typeof((hnd).p) _x = (hnd).p;                            \
+    XEN_GUEST_HANDLE_PARAM(type) _y = { _x };                \
     /* type checking: make sure that the pointers inside     \
      * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
      * the same type, then return hnd */                     \
-    (void)((typeof(&(hnd).p)) 0 ==                           \
-        (typeof(&((XEN_GUEST_HANDLE_PARAM(type)) {}).p)) 0); \
-    (hnd);                                                   \
+    (void)(&_x == &_y.p);                                    \
+    _y;                                                      \
 })
 
 /* Cast a XEN_GUEST_HANDLE_PARAM to XEN_GUEST_HANDLE */
-#define guest_handle_from_param(hnd, type) ({                \
-    /* type checking: make sure that the pointers inside     \
-     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
-     * the same type, then return hnd */                     \
-    (void)((typeof(&(hnd).p)) 0 ==                           \
-        (typeof(&((XEN_GUEST_HANDLE_PARAM(type)) {}).p)) 0); \
-    (hnd);                                                   \
+#define guest_handle_from_param(hnd, type) ({               \
+    typeof((hnd).p) _x = (hnd).p;                           \
+    XEN_GUEST_HANDLE(type) _y = { _x };                     \
+    /* type checking: make sure that the pointers inside    \
+     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of   \
+     * the same type, then return hnd */                    \
+    (void)(&_x == &_y.p);                                   \
+    _y;                                                     \
 })
 
 #define guest_handle_for_field(hnd, type, fld)          \
diff --git a/xen/lib/x86/private.h b/xen/lib/x86/private.h
index b793181464..2d53bd3ced 100644
--- a/xen/lib/x86/private.h
+++ b/xen/lib/x86/private.h
@@ -4,12 +4,12 @@
 #ifdef __XEN__
 
 #include <xen/bitops.h>
+#include <xen/guest_access.h>
 #include <xen/kernel.h>
 #include <xen/lib.h>
 #include <xen/nospec.h>
 #include <xen/types.h>
 
-#include <asm/guest_access.h>
 #include <asm/msr-index.h>
 
 #define copy_to_buffer_offset copy_to_guest_offset
-- 
2.17.1

[PATCH 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Most of the helpers to access guest memory are implemented the same way
on Arm and x86. The only differences are:
    - guest_handle_{from, to}_param(): while on x86 XEN_GUEST_HANDLE()
      and XEN_GUEST_HANDLE_PARAM() are the same, they are not on Arm. It
      is still fine to use the Arm implementation on x86.
    - __clear_guest_offset(): Interestingly the prototype does not match
      between the x86 and Arm. However, the Arm one is bogus. So the x86
      implementation can be used.
    - guest_handle{,_subrange}_okay(): They are validly differing
      because Arm is only supporting auto-translated guest and therefore
      handles are always valid.

While it would be possible to adjust the coding style at the same, this
is left for a follow-up patch so 'diff' can be used to check the
consolidation was done correctly.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/include/asm-arm/guest_access.h | 131 ----------------------------
 xen/include/asm-x86/guest_access.h | 125 --------------------------
 xen/include/xen/guest_access.h     | 135 +++++++++++++++++++++++++++++
 3 files changed, 135 insertions(+), 256 deletions(-)

diff --git a/xen/include/asm-arm/guest_access.h b/xen/include/asm-arm/guest_access.h
index 93d56868f1..53766386d3 100644
--- a/xen/include/asm-arm/guest_access.h
+++ b/xen/include/asm-arm/guest_access.h
@@ -23,102 +23,6 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t ipa, void *buf,
 #define __raw_copy_from_guest raw_copy_from_guest
 #define __raw_clear_guest raw_clear_guest
 
-/* Remainder copied from x86 -- could be common? */
-
-/* Is the guest handle a NULL reference? */
-#define guest_handle_is_null(hnd)        ((hnd).p == NULL)
-
-/* Offset the given guest handle into the array it refers to. */
-#define guest_handle_add_offset(hnd, nr) ((hnd).p += (nr))
-#define guest_handle_subtract_offset(hnd, nr) ((hnd).p -= (nr))
-
-/* Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
- * to the specified type of XEN_GUEST_HANDLE_PARAM. */
-#define guest_handle_cast(hnd, type) ({         \
-    type *_x = (hnd).p;                         \
-    (XEN_GUEST_HANDLE_PARAM(type)) { _x };            \
-})
-
-/* Cast a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
-#define guest_handle_to_param(hnd, type) ({                  \
-    typeof((hnd).p) _x = (hnd).p;                            \
-    XEN_GUEST_HANDLE_PARAM(type) _y = { _x };                \
-    /* type checking: make sure that the pointers inside     \
-     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
-     * the same type, then return hnd */                     \
-    (void)(&_x == &_y.p);                                    \
-    _y;                                                      \
-})
-
-
-/* Cast a XEN_GUEST_HANDLE_PARAM to XEN_GUEST_HANDLE */
-#define guest_handle_from_param(hnd, type) ({               \
-    typeof((hnd).p) _x = (hnd).p;                           \
-    XEN_GUEST_HANDLE(type) _y = { _x };                     \
-    /* type checking: make sure that the pointers inside    \
-     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of   \
-     * the same type, then return hnd */                    \
-    (void)(&_x == &_y.p);                                   \
-    _y;                                                     \
-})
-
-#define guest_handle_for_field(hnd, type, fld)          \
-    ((XEN_GUEST_HANDLE(type)) { &(hnd).p->fld })
-
-#define guest_handle_from_ptr(ptr, type)        \
-    ((XEN_GUEST_HANDLE_PARAM(type)) { (type *)ptr })
-#define const_guest_handle_from_ptr(ptr, type)  \
-    ((XEN_GUEST_HANDLE_PARAM(const_##type)) { (const type *)ptr })
-
-/*
- * Copy an array of objects to guest context via a guest handle,
- * specifying an offset into the guest array.
- *
- * The variable _t is only here to catch at build time whether we are
- * copying back to a const guest handle.
- */
-#define copy_to_guest_offset(hnd, off, ptr, nr) ({      \
-    const typeof(*(ptr)) *_s = (ptr);                   \
-    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
-    void *__maybe_unused _t = (hnd).p;                  \
-    ((void)((hnd).p == (ptr)));                         \
-    raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));  \
-})
-
-/*
- * Clear an array of objects in guest context via a guest handle,
- * specifying an offset into the guest array.
- */
-#define clear_guest_offset(hnd, off, nr) ({    \
-    void *_d = (hnd).p;                        \
-    raw_clear_guest(_d+(off), nr);             \
-})
-
-/*
- * Copy an array of objects from guest context via a guest handle,
- * specifying an offset into the guest array.
- */
-#define copy_from_guest_offset(ptr, hnd, off, nr) ({    \
-    const typeof(*(ptr)) *_s = (hnd).p;                 \
-    typeof(*(ptr)) *_d = (ptr);                         \
-    raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
-})
-
-/* Copy sub-field of a structure to guest context via a guest handle. */
-#define copy_field_to_guest(hnd, ptr, field) ({         \
-    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
-    void *_d = &(hnd).p->field;                         \
-    ((void)(&(hnd).p->field == &(ptr)->field));         \
-    raw_copy_to_guest(_d, _s, sizeof(*_s));             \
-})
-
-/* Copy sub-field of a structure from guest context via a guest handle. */
-#define copy_field_from_guest(ptr, hnd, field) ({       \
-    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
-    typeof(&(ptr)->field) _d = &(ptr)->field;           \
-    raw_copy_from_guest(_d, _s, sizeof(*_d));           \
-})
-
 /*
  * Pre-validate a guest handle.
  * Allows use of faster __copy_* functions.
@@ -127,41 +31,6 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t ipa, void *buf,
 #define guest_handle_okay(hnd, nr) (1)
 #define guest_handle_subrange_okay(hnd, first, last) (1)
 
-/*
- * The variable _t is only here to catch at build time whether we are
- * copying back to a const guest handle.
- */
-#define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
-    const typeof(*(ptr)) *_s = (ptr);                   \
-    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
-    void *__maybe_unused _t = (hnd).p;                  \
-    ((void)((hnd).p == (ptr)));                         \
-    __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
-})
-
-#define __clear_guest_offset(hnd, off, ptr, nr) ({      \
-    __raw_clear_guest(_d+(off), nr);  \
-})
-
-#define __copy_from_guest_offset(ptr, hnd, off, nr) ({  \
-    const typeof(*(ptr)) *_s = (hnd).p;                 \
-    typeof(*(ptr)) *_d = (ptr);                         \
-    __raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
-})
-
-#define __copy_field_to_guest(hnd, ptr, field) ({       \
-    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
-    void *_d = &(hnd).p->field;                         \
-    ((void)(&(hnd).p->field == &(ptr)->field));         \
-    __raw_copy_to_guest(_d, _s, sizeof(*_s));           \
-})
-
-#define __copy_field_from_guest(ptr, hnd, field) ({     \
-    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
-    typeof(&(ptr)->field) _d = &(ptr)->field;           \
-    __raw_copy_from_guest(_d, _s, sizeof(*_d));         \
-})
-
 #endif /* __ASM_ARM_GUEST_ACCESS_H__ */
 /*
  * Local variables:
diff --git a/xen/include/asm-x86/guest_access.h b/xen/include/asm-x86/guest_access.h
index 5c3dfc47b6..08c9fbbc78 100644
--- a/xen/include/asm-x86/guest_access.h
+++ b/xen/include/asm-x86/guest_access.h
@@ -38,95 +38,6 @@
      clear_user_hvm((dst), (len)) :             \
      clear_user((dst), (len)))
 
-/* Is the guest handle a NULL reference? */
-#define guest_handle_is_null(hnd)        ((hnd).p == NULL)
-
-/* Offset the given guest handle into the array it refers to. */
-#define guest_handle_add_offset(hnd, nr) ((hnd).p += (nr))
-#define guest_handle_subtract_offset(hnd, nr) ((hnd).p -= (nr))
-
-/* Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
- * to the specified type of XEN_GUEST_HANDLE_PARAM. */
-#define guest_handle_cast(hnd, type) ({         \
-    type *_x = (hnd).p;                         \
-    (XEN_GUEST_HANDLE_PARAM(type)) { _x };            \
-})
-
-/* Cast a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
-#define guest_handle_to_param(hnd, type) ({                  \
-    typeof((hnd).p) _x = (hnd).p;                            \
-    XEN_GUEST_HANDLE_PARAM(type) _y = { _x };                \
-    /* type checking: make sure that the pointers inside     \
-     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
-     * the same type, then return hnd */                     \
-    (void)(&_x == &_y.p);                                    \
-    _y;                                                      \
-})
-
-/* Cast a XEN_GUEST_HANDLE_PARAM to XEN_GUEST_HANDLE */
-#define guest_handle_from_param(hnd, type) ({               \
-    typeof((hnd).p) _x = (hnd).p;                           \
-    XEN_GUEST_HANDLE(type) _y = { _x };                     \
-    /* type checking: make sure that the pointers inside    \
-     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of   \
-     * the same type, then return hnd */                    \
-    (void)(&_x == &_y.p);                                   \
-    _y;                                                     \
-})
-
-#define guest_handle_for_field(hnd, type, fld)          \
-    ((XEN_GUEST_HANDLE(type)) { &(hnd).p->fld })
-
-#define guest_handle_from_ptr(ptr, type)        \
-    ((XEN_GUEST_HANDLE_PARAM(type)) { (type *)ptr })
-#define const_guest_handle_from_ptr(ptr, type)  \
-    ((XEN_GUEST_HANDLE_PARAM(const_##type)) { (const type *)ptr })
-
-/*
- * Copy an array of objects to guest context via a guest handle,
- * specifying an offset into the guest array.
- *
- * The variable _t is only here to catch at build time whether we are
- * copying back to a const guest handle.
- */
-#define copy_to_guest_offset(hnd, off, ptr, nr) ({      \
-    const typeof(*(ptr)) *_s = (ptr);                   \
-    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
-    void *__maybe_unused _t = (hnd).p;                  \
-    ((void)((hnd).p == (ptr)));                         \
-    raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));  \
-})
-
-/*
- * Copy an array of objects from guest context via a guest handle,
- * specifying an offset into the guest array.
- */
-#define copy_from_guest_offset(ptr, hnd, off, nr) ({    \
-    const typeof(*(ptr)) *_s = (hnd).p;                 \
-    typeof(*(ptr)) *_d = (ptr);                         \
-    raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
-})
-
-#define clear_guest_offset(hnd, off, nr) ({    \
-    void *_d = (hnd).p;                        \
-    raw_clear_guest(_d+(off), nr);             \
-})
-
-/* Copy sub-field of a structure to guest context via a guest handle. */
-#define copy_field_to_guest(hnd, ptr, field) ({         \
-    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
-    void *_d = &(hnd).p->field;                         \
-    ((void)(&(hnd).p->field == &(ptr)->field));         \
-    raw_copy_to_guest(_d, _s, sizeof(*_s));             \
-})
-
-/* Copy sub-field of a structure from guest context via a guest handle. */
-#define copy_field_from_guest(ptr, hnd, field) ({       \
-    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
-    typeof(&(ptr)->field) _d = &(ptr)->field;           \
-    raw_copy_from_guest(_d, _s, sizeof(*_d));           \
-})
-
 /*
  * Pre-validate a guest handle.
  * Allows use of faster __copy_* functions.
@@ -140,42 +51,6 @@
                      (last)-(first)+1,                  \
                      sizeof(*(hnd).p)))
 
-/*
- * The variable _t is only here to catch at build time whether we are
- * copying back to a const guest handle.
- */
-#define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
-    const typeof(*(ptr)) *_s = (ptr);                   \
-    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
-    void *__maybe_unused _t = (hnd).p;                  \
-    ((void)((hnd).p == (ptr)));                         \
-    __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
-})
-
-#define __copy_from_guest_offset(ptr, hnd, off, nr) ({  \
-    const typeof(*(ptr)) *_s = (hnd).p;                 \
-    typeof(*(ptr)) *_d = (ptr);                         \
-    __raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
-})
-
-#define __clear_guest_offset(hnd, off, nr) ({    \
-    void *_d = (hnd).p;                          \
-    __raw_clear_guest(_d+(off), nr);             \
-})
-
-#define __copy_field_to_guest(hnd, ptr, field) ({       \
-    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
-    void *_d = &(hnd).p->field;                         \
-    ((void)(&(hnd).p->field == &(ptr)->field));         \
-    __raw_copy_to_guest(_d, _s, sizeof(*_s));           \
-})
-
-#define __copy_field_from_guest(ptr, hnd, field) ({     \
-    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
-    typeof(&(ptr)->field) _d = &(ptr)->field;           \
-    __raw_copy_from_guest(_d, _s, sizeof(*_d));         \
-})
-
 #endif /* __ASM_X86_GUEST_ACCESS_H__ */
 /*
  * Local variables:
diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
index ef9aaa3efc..f724f995c3 100644
--- a/xen/include/xen/guest_access.h
+++ b/xen/include/xen/guest_access.h
@@ -11,6 +11,99 @@
 #include <xen/types.h>
 #include <public/xen.h>
 
+/* Is the guest handle a NULL reference? */
+#define guest_handle_is_null(hnd)        ((hnd).p == NULL)
+
+/* Offset the given guest handle into the array it refers to. */
+#define guest_handle_add_offset(hnd, nr) ((hnd).p += (nr))
+#define guest_handle_subtract_offset(hnd, nr) ((hnd).p -= (nr))
+
+/* Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
+ * to the specified type of XEN_GUEST_HANDLE_PARAM. */
+#define guest_handle_cast(hnd, type) ({         \
+    type *_x = (hnd).p;                         \
+    (XEN_GUEST_HANDLE_PARAM(type)) { _x };            \
+})
+
+/* Cast a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
+#define guest_handle_to_param(hnd, type) ({                  \
+    typeof((hnd).p) _x = (hnd).p;                            \
+    XEN_GUEST_HANDLE_PARAM(type) _y = { _x };                \
+    /* type checking: make sure that the pointers inside     \
+     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
+     * the same type, then return hnd */                     \
+    (void)(&_x == &_y.p);                                    \
+    _y;                                                      \
+})
+
+/* Cast a XEN_GUEST_HANDLE_PARAM to XEN_GUEST_HANDLE */
+#define guest_handle_from_param(hnd, type) ({               \
+    typeof((hnd).p) _x = (hnd).p;                           \
+    XEN_GUEST_HANDLE(type) _y = { _x };                     \
+    /* type checking: make sure that the pointers inside    \
+     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of   \
+     * the same type, then return hnd */                    \
+    (void)(&_x == &_y.p);                                   \
+    _y;                                                     \
+})
+
+#define guest_handle_for_field(hnd, type, fld)          \
+    ((XEN_GUEST_HANDLE(type)) { &(hnd).p->fld })
+
+#define guest_handle_from_ptr(ptr, type)        \
+    ((XEN_GUEST_HANDLE_PARAM(type)) { (type *)ptr })
+#define const_guest_handle_from_ptr(ptr, type)  \
+    ((XEN_GUEST_HANDLE_PARAM(const_##type)) { (const type *)ptr })
+
+/*
+ * Copy an array of objects to guest context via a guest handle,
+ * specifying an offset into the guest array.
+ *
+ * The variable _t is only here to catch at build time whether we are
+ * copying back to a const guest handle.
+ */
+#define copy_to_guest_offset(hnd, off, ptr, nr) ({      \
+    const typeof(*(ptr)) *_s = (ptr);                   \
+    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
+    void *__maybe_unused _t = (hnd).p;                  \
+    ((void)((hnd).p == (ptr)));                         \
+    raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));  \
+})
+
+/*
+ * Clear an array of objects in guest context via a guest handle,
+ * specifying an offset into the guest array.
+ */
+#define clear_guest_offset(hnd, off, nr) ({    \
+    void *_d = (hnd).p;                        \
+    raw_clear_guest(_d+(off), nr);             \
+})
+
+/*
+ * Copy an array of objects from guest context via a guest handle,
+ * specifying an offset into the guest array.
+ */
+#define copy_from_guest_offset(ptr, hnd, off, nr) ({    \
+    const typeof(*(ptr)) *_s = (hnd).p;                 \
+    typeof(*(ptr)) *_d = (ptr);                         \
+    raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
+})
+
+/* Copy sub-field of a structure to guest context via a guest handle. */
+#define copy_field_to_guest(hnd, ptr, field) ({         \
+    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
+    void *_d = &(hnd).p->field;                         \
+    ((void)(&(hnd).p->field == &(ptr)->field));         \
+    raw_copy_to_guest(_d, _s, sizeof(*_s));             \
+})
+
+/* Copy sub-field of a structure from guest context via a guest handle. */
+#define copy_field_from_guest(ptr, hnd, field) ({       \
+    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
+    typeof(&(ptr)->field) _d = &(ptr)->field;           \
+    raw_copy_from_guest(_d, _s, sizeof(*_d));           \
+})
+
 #define copy_to_guest(hnd, ptr, nr)                     \
     copy_to_guest_offset(hnd, 0, ptr, nr)
 
@@ -20,6 +113,48 @@
 #define clear_guest(hnd, nr)                            \
     clear_guest_offset(hnd, 0, nr)
 
+/*
+ * The __copy_* functions should only be used after the guest handle has
+ * been pre-validated via guest_handle_okay() and
+ * guest_handle_subrange_okay().
+ */
+
+/*
+ * The variable _t is only here to catch at build time whether we are
+ * copying back to a const guest handle.
+ */
+#define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
+    const typeof(*(ptr)) *_s = (ptr);                   \
+    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
+    void *__maybe_unused _t = (hnd).p;                  \
+    ((void)((hnd).p == (ptr)));                         \
+    __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
+})
+
+#define __clear_guest_offset(hnd, off, nr) ({    \
+    void *_d = (hnd).p;                          \
+    __raw_clear_guest(_d + (off), nr);           \
+})
+
+#define __copy_from_guest_offset(ptr, hnd, off, nr) ({  \
+    const typeof(*(ptr)) *_s = (hnd).p;                 \
+    typeof(*(ptr)) *_d = (ptr);                         \
+    __raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
+})
+
+#define __copy_field_to_guest(hnd, ptr, field) ({       \
+    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
+    void *_d = &(hnd).p->field;                         \
+    ((void)(&(hnd).p->field == &(ptr)->field));         \
+    __raw_copy_to_guest(_d, _s, sizeof(*_s));           \
+})
+
+#define __copy_field_from_guest(ptr, hnd, field) ({     \
+    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
+    typeof(&(ptr)->field) _d = &(ptr)->field;           \
+    __raw_copy_from_guest(_d, _s, sizeof(*_d));         \
+})
+
 #define __copy_to_guest(hnd, ptr, nr)                   \
     __copy_to_guest_offset(hnd, 0, ptr, nr)
 
-- 
2.17.1

[PATCH 7/7] xen/guest_access: Fix coding style in xen/guest_access.h

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

    * Add space before and after operator
    * Align \
    * Format comments

No functional changes expected.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/include/xen/guest_access.h | 40 +++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
index f724f995c3..24f03a84ff 100644
--- a/xen/include/xen/guest_access.h
+++ b/xen/include/xen/guest_access.h
@@ -18,20 +18,24 @@
 #define guest_handle_add_offset(hnd, nr) ((hnd).p += (nr))
 #define guest_handle_subtract_offset(hnd, nr) ((hnd).p -= (nr))
 
-/* Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
- * to the specified type of XEN_GUEST_HANDLE_PARAM. */
+/*
+ * Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
+ * to the specified type of XEN_GUEST_HANDLE_PARAM.
+ */
 #define guest_handle_cast(hnd, type) ({         \
     type *_x = (hnd).p;                         \
-    (XEN_GUEST_HANDLE_PARAM(type)) { _x };            \
+    (XEN_GUEST_HANDLE_PARAM(type)) { _x };      \
 })
 
 /* Cast a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
 #define guest_handle_to_param(hnd, type) ({                  \
     typeof((hnd).p) _x = (hnd).p;                            \
     XEN_GUEST_HANDLE_PARAM(type) _y = { _x };                \
-    /* type checking: make sure that the pointers inside     \
+    /*                                                       \
+     * type checking: make sure that the pointers inside     \
      * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
-     * the same type, then return hnd */                     \
+     * the same type, then return hnd.                       \
+     */                                                      \
     (void)(&_x == &_y.p);                                    \
     _y;                                                      \
 })
@@ -40,9 +44,11 @@
 #define guest_handle_from_param(hnd, type) ({               \
     typeof((hnd).p) _x = (hnd).p;                           \
     XEN_GUEST_HANDLE(type) _y = { _x };                     \
-    /* type checking: make sure that the pointers inside    \
+    /*                                                      \
+     * type checking: make sure that the pointers inside    \
      * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of   \
-     * the same type, then return hnd */                    \
+     * the same type, then return hnd.                      \
+     */                                                     \
     (void)(&_x == &_y.p);                                   \
     _y;                                                     \
 })
@@ -123,12 +129,12 @@
  * The variable _t is only here to catch at build time whether we are
  * copying back to a const guest handle.
  */
-#define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
-    const typeof(*(ptr)) *_s = (ptr);                   \
-    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
-    void *__maybe_unused _t = (hnd).p;                  \
-    ((void)((hnd).p == (ptr)));                         \
-    __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
+#define __copy_to_guest_offset(hnd, off, ptr, nr) ({        \
+    const typeof(*(ptr)) *_s = (ptr);                       \
+    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;              \
+    void *__maybe_unused _t = (hnd).p;                      \
+    ((void)((hnd).p == (ptr)));                             \
+    __raw_copy_to_guest(_d + (off), _s, sizeof(*_s) * (nr));\
 })
 
 #define __clear_guest_offset(hnd, off, nr) ({    \
@@ -136,10 +142,10 @@
     __raw_clear_guest(_d + (off), nr);           \
 })
 
-#define __copy_from_guest_offset(ptr, hnd, off, nr) ({  \
-    const typeof(*(ptr)) *_s = (hnd).p;                 \
-    typeof(*(ptr)) *_d = (ptr);                         \
-    __raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
+#define __copy_from_guest_offset(ptr, hnd, off, nr) ({          \
+    const typeof(*(ptr)) *_s = (hnd).p;                         \
+    typeof(*(ptr)) *_d = (ptr);                                 \
+    __raw_copy_from_guest(_d, _s + (off), sizeof (*_d) * (nr)); \
 })
 
 #define __copy_field_to_guest(hnd, ptr, field) ({       \
-- 
2.17.1

Re: [PATCH 0/7] xen: Consolidate asm-*/guest_access.h in xen/guest_access.h

[Julien Grall]

On 04/04/2020 14:10, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Hi all,
> 
> A lot of the helpers implemented in asm-*/guest_access.h are implemented
> the same way. This series aims to avoid the duplication and implement
> them only once in xen/guest_access.h.

I forgot to mention this is based on "xen/guest_access: Harden 
*copy_to_guest_offset() to prevent const dest operand" [1].

This will also clash with Jan's patch "guestcopy: evaluate 
{,__}copy{,_field}_to_guest*() arguments just once" [2]. I am happy to 
rebase this series on top of it.

Cheers,

[1] https://lore.kernel.org/xen-devel/20200404130613.26428-1-julien@xen.org/
[2] 
https://lore.kernel.org/xen-devel/9918b339-e914-7228-5f8e-86c82090b5bd@suse.com/

> 
> Cheers,
> 
> Julien Grall (7):
>    xen/guest_access: Add missing emacs magics
>    xen/arm: kernel: Re-order the includes
>    xen/arm: decode: Re-order the includes
>    xen/arm: guestcopy: Re-order the includes
>    xen: include xen/guest_access.h rather than asm/guest_access.h
>    xen/guest_access: Consolidate guest access helpers in
>      xen/guest_access.h
>    xen/guest_access: Fix coding style in xen/guest_access.h
> 
>   xen/arch/arm/decode.c                |   7 +-
>   xen/arch/arm/domain.c                |   2 +-
>   xen/arch/arm/guest_walk.c            |   3 +-
>   xen/arch/arm/guestcopy.c             |   5 +-
>   xen/arch/arm/kernel.c                |  12 +--
>   xen/arch/arm/vgic-v3-its.c           |   2 +-
>   xen/arch/x86/hvm/svm/svm.c           |   2 +-
>   xen/arch/x86/hvm/viridian/viridian.c |   2 +-
>   xen/arch/x86/hvm/vmx/vmx.c           |   2 +-
>   xen/common/libelf/libelf-loader.c    |   2 +-
>   xen/include/asm-arm/guest_access.h   | 132 ------------------------
>   xen/include/asm-x86/guest_access.h   | 129 ++---------------------
>   xen/include/xen/guest_access.h       | 149 +++++++++++++++++++++++++++
>   xen/lib/x86/private.h                |   2 +-
>   14 files changed, 178 insertions(+), 273 deletions(-)
> 

-- 
Julien Grall

RE: [PATCH 5/7] xen: include xen/guest_access.h rather than asm/guest_access.h

[Paul Durrant]

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 04 April 2020 14:10
> To: xen-devel@lists.xenproject.org
> Cc: julien@xen.org; Julien Grall <jgrall@amazon.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper <andrew.cooper3@citrix.com>; George
> Dunlap <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Jan Beulich
> <jbeulich@suse.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; Paul Durrant
> <paul@xen.org>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>
> Subject: [PATCH 5/7] xen: include xen/guest_access.h rather than asm/guest_access.h
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Only a few places are actually including asm/guest_access.h. While this
> is fine today, a follow-up patch will want to move most of the helpers
> from asm/guest_access.h to xen/guest_access.h.
> 
> To prepare the move, everyone should include xen/guest_access.h rather
> than asm/guest_access.h.
> 
> Interestingly, asm-arm/guest_access.h includes xen/guest_access.h. The
> inclusion is now removed as no-one but the latter should include the
> former.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
>  xen/arch/arm/decode.c                |  2 +-
>  xen/arch/arm/domain.c                |  2 +-
>  xen/arch/arm/guest_walk.c            |  3 ++-
>  xen/arch/arm/guestcopy.c             |  2 +-
>  xen/arch/arm/vgic-v3-its.c           |  2 +-
>  xen/arch/x86/hvm/svm/svm.c           |  2 +-
>  xen/arch/x86/hvm/viridian/viridian.c |  2 +-
>  xen/arch/x86/hvm/vmx/vmx.c           |  2 +-
>  xen/common/libelf/libelf-loader.c    |  2 +-
>  xen/include/asm-arm/guest_access.h   |  1 -
>  xen/include/asm-x86/guest_access.h   | 22 ++++++++++++----------
>  xen/lib/x86/private.h                |  2 +-
>  12 files changed, 23 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
> index 144793c8ce..792c2e92a7 100644
> --- a/xen/arch/arm/decode.c
> +++ b/xen/arch/arm/decode.c
> @@ -17,12 +17,12 @@
>   * GNU General Public License for more details.
>   */
> 
> +#include <xen/guest_access.h>
>  #include <xen/lib.h>
>  #include <xen/sched.h>
>  #include <xen/types.h>
> 
>  #include <asm/current.h>
> -#include <asm/guest_access.h>
> 
>  #include "decode.h"
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 2190d908eb..b062c232b6 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -12,6 +12,7 @@
>  #include <xen/bitops.h>
>  #include <xen/errno.h>
>  #include <xen/grant_table.h>
> +#include <xen/guest_access.h>
>  #include <xen/hypercall.h>
>  #include <xen/init.h>
>  #include <xen/lib.h>
> @@ -26,7 +27,6 @@
>  #include <asm/current.h>
>  #include <asm/event.h>
>  #include <asm/gic.h>
> -#include <asm/guest_access.h>
>  #include <asm/guest_atomics.h>
>  #include <asm/irq.h>
>  #include <asm/p2m.h>
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index a1cdd7f4af..b4496c4c86 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -16,8 +16,9 @@
>   */
> 
>  #include <xen/domain_page.h>
> +#include <xen/guest_access.h>
>  #include <xen/sched.h>
> -#include <asm/guest_access.h>
> +
>  #include <asm/guest_walk.h>
>  #include <asm/short-desc.h>
> 
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index c8023e2bca..32681606d8 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -1,10 +1,10 @@
>  #include <xen/domain_page.h>
> +#include <xen/guest_access.h>
>  #include <xen/lib.h>
>  #include <xen/mm.h>
>  #include <xen/sched.h>
> 
>  #include <asm/current.h>
> -#include <asm/guest_access.h>
> 
>  #define COPY_flush_dcache   (1U << 0)
>  #define COPY_from_guest     (0U << 1)
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 6e153c698d..58d939b85f 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -32,6 +32,7 @@
>  #include <xen/bitops.h>
>  #include <xen/config.h>
>  #include <xen/domain_page.h>
> +#include <xen/guest_access.h>
>  #include <xen/lib.h>
>  #include <xen/init.h>
>  #include <xen/softirq.h>
> @@ -39,7 +40,6 @@
>  #include <xen/sched.h>
>  #include <xen/sizes.h>
>  #include <asm/current.h>
> -#include <asm/guest_access.h>
>  #include <asm/mmio.h>
>  #include <asm/gic_v3_defs.h>
>  #include <asm/gic_v3_its.h>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 888f504a94..9e14a451eb 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -16,6 +16,7 @@
>   * this program; If not, see <http://www.gnu.org/licenses/>.
>   */
> 
> +#include <xen/guest_access.h>
>  #include <xen/init.h>
>  #include <xen/lib.h>
>  #include <xen/trace.h>
> @@ -34,7 +35,6 @@
>  #include <asm/cpufeature.h>
>  #include <asm/processor.h>
>  #include <asm/amd.h>
> -#include <asm/guest_access.h>
>  #include <asm/debugreg.h>
>  #include <asm/msr.h>
>  #include <asm/i387.h>
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
> index 977c1bc54f..dc7183a546 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -5,12 +5,12 @@
>   * Hypervisor Top Level Functional Specification for more information.
>   */
> 
> +#include <xen/guest_access.h>
>  #include <xen/sched.h>
>  #include <xen/version.h>
>  #include <xen/hypercall.h>
>  #include <xen/domain_page.h>
>  #include <xen/param.h>
> -#include <asm/guest_access.h>
>  #include <asm/guest/hyperv-tlfs.h>
>  #include <asm/paging.h>
>  #include <asm/p2m.h>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 1c398fdb6e..98e9c91ea3 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -15,6 +15,7 @@
>   * this program; If not, see <http://www.gnu.org/licenses/>.
>   */
> 
> +#include <xen/guest_access.h>
>  #include <xen/init.h>
>  #include <xen/lib.h>
>  #include <xen/param.h>
> @@ -31,7 +32,6 @@
>  #include <asm/regs.h>
>  #include <asm/cpufeature.h>
>  #include <asm/processor.h>
> -#include <asm/guest_access.h>
>  #include <asm/debugreg.h>
>  #include <asm/msr.h>
>  #include <asm/p2m.h>
> diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
> index 0f468727d0..629cc0d3e6 100644
> --- a/xen/common/libelf/libelf-loader.c
> +++ b/xen/common/libelf/libelf-loader.c
> @@ -16,7 +16,7 @@
>   */
> 
>  #ifdef __XEN__
> -#include <asm/guest_access.h>
> +#include <xen/guest_access.h>
>  #endif
> 
>  #include "libelf-private.h"
> diff --git a/xen/include/asm-arm/guest_access.h b/xen/include/asm-arm/guest_access.h
> index 4046d50347..93d56868f1 100644
> --- a/xen/include/asm-arm/guest_access.h
> +++ b/xen/include/asm-arm/guest_access.h
> @@ -1,7 +1,6 @@
>  #ifndef __ASM_ARM_GUEST_ACCESS_H__
>  #define __ASM_ARM_GUEST_ACCESS_H__
> 
> -#include <xen/guest_access.h>
>  #include <xen/errno.h>
>  #include <xen/sched.h>
> 
> diff --git a/xen/include/asm-x86/guest_access.h b/xen/include/asm-x86/guest_access.h
> index 9ee275d01f..5c3dfc47b6 100644
> --- a/xen/include/asm-x86/guest_access.h
> +++ b/xen/include/asm-x86/guest_access.h
> @@ -54,22 +54,24 @@
> 
>  /* Cast a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
>  #define guest_handle_to_param(hnd, type) ({                  \
> +    typeof((hnd).p) _x = (hnd).p;                            \
> +    XEN_GUEST_HANDLE_PARAM(type) _y = { _x };                \
>      /* type checking: make sure that the pointers inside     \
>       * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
>       * the same type, then return hnd */                     \
> -    (void)((typeof(&(hnd).p)) 0 ==                           \
> -        (typeof(&((XEN_GUEST_HANDLE_PARAM(type)) {}).p)) 0); \
> -    (hnd);                                                   \
> +    (void)(&_x == &_y.p);                                    \
> +    _y;                                                      \
>  })
> 
>  /* Cast a XEN_GUEST_HANDLE_PARAM to XEN_GUEST_HANDLE */
> -#define guest_handle_from_param(hnd, type) ({                \
> -    /* type checking: make sure that the pointers inside     \
> -     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
> -     * the same type, then return hnd */                     \
> -    (void)((typeof(&(hnd).p)) 0 ==                           \
> -        (typeof(&((XEN_GUEST_HANDLE_PARAM(type)) {}).p)) 0); \
> -    (hnd);                                                   \
> +#define guest_handle_from_param(hnd, type) ({               \
> +    typeof((hnd).p) _x = (hnd).p;                           \
> +    XEN_GUEST_HANDLE(type) _y = { _x };                     \
> +    /* type checking: make sure that the pointers inside    \
> +     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of   \
> +     * the same type, then return hnd */                    \
> +    (void)(&_x == &_y.p);                                   \
> +    _y;                                                     \
>  })
> 

The commit comment would have the reader believe that this patch is just some changes in header file inclusion. These last two hunks are something else so I would suggest they get split out into a separate patch.

  Paul

>  #define guest_handle_for_field(hnd, type, fld)          \
> diff --git a/xen/lib/x86/private.h b/xen/lib/x86/private.h
> index b793181464..2d53bd3ced 100644
> --- a/xen/lib/x86/private.h
> +++ b/xen/lib/x86/private.h
> @@ -4,12 +4,12 @@
>  #ifdef __XEN__
> 
>  #include <xen/bitops.h>
> +#include <xen/guest_access.h>
>  #include <xen/kernel.h>
>  #include <xen/lib.h>
>  #include <xen/nospec.h>
>  #include <xen/types.h>
> 
> -#include <asm/guest_access.h>
>  #include <asm/msr-index.h>
> 
>  #define copy_to_buffer_offset copy_to_guest_offset
> --
> 2.17.1

Re: [PATCH 5/7] xen: include xen/guest_access.h rather than asm/guest_access.h

[Julien Grall]

Hi Paul,

On 06/04/2020 08:40, Paul Durrant wrote:
>> diff --git a/xen/include/asm-x86/guest_access.h b/xen/include/asm-x86/guest_access.h
>> index 9ee275d01f..5c3dfc47b6 100644
>> --- a/xen/include/asm-x86/guest_access.h
>> +++ b/xen/include/asm-x86/guest_access.h
>> @@ -54,22 +54,24 @@
>>
>>   /* Cast a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
>>   #define guest_handle_to_param(hnd, type) ({                  \
>> +    typeof((hnd).p) _x = (hnd).p;                            \
>> +    XEN_GUEST_HANDLE_PARAM(type) _y = { _x };                \
>>       /* type checking: make sure that the pointers inside     \
>>        * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
>>        * the same type, then return hnd */                     \
>> -    (void)((typeof(&(hnd).p)) 0 ==                           \
>> -        (typeof(&((XEN_GUEST_HANDLE_PARAM(type)) {}).p)) 0); \
>> -    (hnd);                                                   \
>> +    (void)(&_x == &_y.p);                                    \
>> +    _y;                                                      \
>>   })
>>
>>   /* Cast a XEN_GUEST_HANDLE_PARAM to XEN_GUEST_HANDLE */
>> -#define guest_handle_from_param(hnd, type) ({                \
>> -    /* type checking: make sure that the pointers inside     \
>> -     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
>> -     * the same type, then return hnd */                     \
>> -    (void)((typeof(&(hnd).p)) 0 ==                           \
>> -        (typeof(&((XEN_GUEST_HANDLE_PARAM(type)) {}).p)) 0); \
>> -    (hnd);                                                   \
>> +#define guest_handle_from_param(hnd, type) ({               \
>> +    typeof((hnd).p) _x = (hnd).p;                           \
>> +    XEN_GUEST_HANDLE(type) _y = { _x };                     \
>> +    /* type checking: make sure that the pointers inside    \
>> +     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of   \
>> +     * the same type, then return hnd */                    \
>> +    (void)(&_x == &_y.p);                                   \
>> +    _y;                                                     \
>>   })
>>
> 
> The commit comment would have the reader believe that this patch is just some changes in header file inclusion. These last two hunks are something else so I would suggest they get split out into a separate patch.

These two chunks were meant to be squashed in patch #6, but I messed up 
the rebase. I will fix on the next version.

Sorry for that.

Cheers,

> 
>    Paul
> 
>>   #define guest_handle_for_field(hnd, type, fld)          \
>> diff --git a/xen/lib/x86/private.h b/xen/lib/x86/private.h
>> index b793181464..2d53bd3ced 100644
>> --- a/xen/lib/x86/private.h
>> +++ b/xen/lib/x86/private.h
>> @@ -4,12 +4,12 @@
>>   #ifdef __XEN__
>>
>>   #include <xen/bitops.h>
>> +#include <xen/guest_access.h>
>>   #include <xen/kernel.h>
>>   #include <xen/lib.h>
>>   #include <xen/nospec.h>
>>   #include <xen/types.h>
>>
>> -#include <asm/guest_access.h>
>>   #include <asm/msr-index.h>
>>
>>   #define copy_to_buffer_offset copy_to_guest_offset
>> --
>> 2.17.1
> 
> 

-- 
Julien Grall

Re: [PATCH 1/7] xen/guest_access: Add missing emacs magics

[Jan Beulich]

On 04.04.2020 15:10, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Add missing emacs magics for xen/guest_access.h and
> asm-x86/guest_access.h.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

I don't think these are "missing"; as per ./CODING_STYLE they're
permitted, but not required (and I continue to question why one
form of such a comment should be preferred over possible other
forms other editors may support). Nevertheless, as this is in
line with what we have elsewhere:

Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Jan Beulich]

On 04.04.2020 15:10, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Most of the helpers to access guest memory are implemented the same way
> on Arm and x86. The only differences are:
>     - guest_handle_{from, to}_param(): while on x86 XEN_GUEST_HANDLE()
>       and XEN_GUEST_HANDLE_PARAM() are the same, they are not on Arm. It
>       is still fine to use the Arm implementation on x86.
>     - __clear_guest_offset(): Interestingly the prototype does not match
>       between the x86 and Arm. However, the Arm one is bogus. So the x86
>       implementation can be used.
>     - guest_handle{,_subrange}_okay(): They are validly differing
>       because Arm is only supporting auto-translated guest and therefore
>       handles are always valid.

While I'm fine in principle with such consolidation, I'm afraid I
really need to ask for some historical background to be added
here. It may very well be that there's a reason for the separation
(likely to be found in the removed ia64 or ppc ports), which may
then provide a hint at why future ports may want to have these
separated. If such reasons exist, I'd prefer to avoid the back and
forth between headers. What we could do in such a case is borrow
Linux'es asm-generic/ concept, and move the "typical"
implementation there. (And of course if there were no noticable
reasons for the split, the change as it is would be fine in
general; saying so without having looked at the details of it,
yet).

Jan

Re: [PATCH 7/7] xen/guest_access: Fix coding style in xen/guest_access.h

[Jan Beulich]

On 04.04.2020 15:10, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
>     * Add space before and after operator
>     * Align \
>     * Format comments

To be honest, despite the reason you give for the split in patch 6,
I'd rather see code movement like this to do formatting adjustments
right away. But if you're convinced the split is better, I'm not
meaning to insist.

Jan

Re: [PATCH 7/7] xen/guest_access: Fix coding style in xen/guest_access.h

[Julien Grall]

On 07/04/2020 09:17, Jan Beulich wrote:
> On 04.04.2020 15:10, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>>      * Add space before and after operator
>>      * Align \
>>      * Format comments
> 
> To be honest, despite the reason you give for the split in patch 6,
> I'd rather see code movement like this to do formatting adjustments
> right away.

Thank you for thinking I can move code and also modify coding style at 
the same time :).

However I know mistakes can be easily made and may not be caught during 
review (possibly leading to an XSA). So I tend to adhere to the KISS 
principle.

> But if you're convinced the split is better, I'm not
> meaning to insist.

I will keep the code as-is unless someone else think it is bad idea.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/7] xen/guest_access: Add missing emacs magics

[Julien Grall]

Hi Jan,

On 07/04/2020 09:05, Jan Beulich wrote:
> On 04.04.2020 15:10, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Add missing emacs magics for xen/guest_access.h and
>> asm-x86/guest_access.h.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> I don't think these are "missing"; as per ./CODING_STYLE they're
> permitted, but not required (and I continue to question why one
> form of such a comment should be preferred over possible other
> forms other editors may support). Nevertheless, as this is in
> line with what we have elsewhere:

I can remove the "missing" words if you prefer.

> 
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thank you!

Cheers,

-- 
Julien Grall

Re: [PATCH 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Julien Grall]

Hi Jan,

On 07/04/2020 09:14, Jan Beulich wrote:
> On 04.04.2020 15:10, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Most of the helpers to access guest memory are implemented the same way
>> on Arm and x86. The only differences are:
>>      - guest_handle_{from, to}_param(): while on x86 XEN_GUEST_HANDLE()
>>        and XEN_GUEST_HANDLE_PARAM() are the same, they are not on Arm. It
>>        is still fine to use the Arm implementation on x86.
>>      - __clear_guest_offset(): Interestingly the prototype does not match
>>        between the x86 and Arm. However, the Arm one is bogus. So the x86
>>        implementation can be used.
>>      - guest_handle{,_subrange}_okay(): They are validly differing
>>        because Arm is only supporting auto-translated guest and therefore
>>        handles are always valid.
> 
> While I'm fine in principle with such consolidation, I'm afraid I
> really need to ask for some historical background to be added
> here. It may very well be that there's a reason for the separation
> (likely to be found in the removed ia64 or ppc ports), which may
> then provide a hint at why future ports may want to have these
> separated. If such reasons exist, I'd prefer to avoid the back and
> forth between headers. What we could do in such a case is borrow
> Linux'es asm-generic/ concept, and move the "typical"
> implementation there. (And of course if there were no noticable
> reasons for the split, the change as it is would be fine in
> general; saying so without having looked at the details of it,
> yet).

Looking at the history, ia64 and ppc used to include a common header 
called xen/xencomm.h from asm/guest_access.h.

This has now disappeared with the removal of the two ports.

Regarding future arch, the fact arm and x86 gives me some confidence we 
are unlikely going to get a new ABI for an arch. Do you see any reason to?

Cheers,

-- 
Julien Grall

Re: [PATCH 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Jan Beulich]

On 09.04.2020 00:05, Julien Grall wrote:
> Hi Jan,
> 
> On 07/04/2020 09:14, Jan Beulich wrote:
>> On 04.04.2020 15:10, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> Most of the helpers to access guest memory are implemented the same way
>>> on Arm and x86. The only differences are:
>>>      - guest_handle_{from, to}_param(): while on x86 XEN_GUEST_HANDLE()
>>>        and XEN_GUEST_HANDLE_PARAM() are the same, they are not on Arm. It
>>>        is still fine to use the Arm implementation on x86.
>>>      - __clear_guest_offset(): Interestingly the prototype does not match
>>>        between the x86 and Arm. However, the Arm one is bogus. So the x86
>>>        implementation can be used.
>>>      - guest_handle{,_subrange}_okay(): They are validly differing
>>>        because Arm is only supporting auto-translated guest and therefore
>>>        handles are always valid.
>>
>> While I'm fine in principle with such consolidation, I'm afraid I
>> really need to ask for some historical background to be added
>> here. It may very well be that there's a reason for the separation
>> (likely to be found in the removed ia64 or ppc ports), which may
>> then provide a hint at why future ports may want to have these
>> separated. If such reasons exist, I'd prefer to avoid the back and
>> forth between headers. What we could do in such a case is borrow
>> Linux'es asm-generic/ concept, and move the "typical"
>> implementation there. (And of course if there were no noticable
>> reasons for the split, the change as it is would be fine in
>> general; saying so without having looked at the details of it,
>> yet).
> 
> Looking at the history, ia64 and ppc used to include a common
> header called xen/xencomm.h from asm/guest_access.h.
> 
> This has now disappeared with the removal of the two ports.
> 
> Regarding future arch, the fact arm and x86 gives me some confidence
> we are unlikely going to get a new ABI for an arch. Do you see any
> reason to?

Well, there surely had be a reason for ia64 and ppc to use a
different approach. I don't see why a new port may not also want
to go that route instead of the x86/Arm one.

Jan

Re: [PATCH 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Julien Grall]

Hi,

On 09/04/2020 07:30, Jan Beulich wrote:
> On 09.04.2020 00:05, Julien Grall wrote:
>> Hi Jan,
>>
>> On 07/04/2020 09:14, Jan Beulich wrote:
>>> On 04.04.2020 15:10, Julien Grall wrote:
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> Most of the helpers to access guest memory are implemented the same way
>>>> on Arm and x86. The only differences are:
>>>>       - guest_handle_{from, to}_param(): while on x86 XEN_GUEST_HANDLE()
>>>>         and XEN_GUEST_HANDLE_PARAM() are the same, they are not on Arm. It
>>>>         is still fine to use the Arm implementation on x86.
>>>>       - __clear_guest_offset(): Interestingly the prototype does not match
>>>>         between the x86 and Arm. However, the Arm one is bogus. So the x86
>>>>         implementation can be used.
>>>>       - guest_handle{,_subrange}_okay(): They are validly differing
>>>>         because Arm is only supporting auto-translated guest and therefore
>>>>         handles are always valid.
>>>
>>> While I'm fine in principle with such consolidation, I'm afraid I
>>> really need to ask for some historical background to be added
>>> here. It may very well be that there's a reason for the separation
>>> (likely to be found in the removed ia64 or ppc ports), which may
>>> then provide a hint at why future ports may want to have these
>>> separated. If such reasons exist, I'd prefer to avoid the back and
>>> forth between headers. What we could do in such a case is borrow
>>> Linux'es asm-generic/ concept, and move the "typical"
>>> implementation there. (And of course if there were no noticable
>>> reasons for the split, the change as it is would be fine in
>>> general; saying so without having looked at the details of it,
>>> yet).
>>
>> Looking at the history, ia64 and ppc used to include a common
>> header called xen/xencomm.h from asm/guest_access.h.
>>
>> This has now disappeared with the removal of the two ports.
>>
>> Regarding future arch, the fact arm and x86 gives me some confidence
>> we are unlikely going to get a new ABI for an arch. Do you see any
>> reason to?
> 
> Well, there surely had be a reason for ia64 and ppc to use a
> different approach. 

This was introduced way before my time in Xen. AFAICT, you were already 
part of the community when 'xencomm' were alive.

There are not much information about it only nor in the commits. So do 
you mind sharing a bit more what 'xencomm' meant to be and why it wasn't 
introduced on x86?

> I don't see why a new port may not also want
> to go that route instead of the x86/Arm one.
I could accept that someone would want to reinvent a new ABI from 
scratch for just an hypothetical new arch. However it would be quite an 
effort to reinvent XEN_GUEST_HANDLE(). The chance is RISC-V is only 
going to re-use what Arm did as Arm already did with x86.

I would like to avoid to introduce a new directory asm-generic with just 
one header in it. Maybe you have some other headers in mind?

Cheers,

-- 
Julien Grall

Re: [PATCH 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Jan Beulich]

On 09.04.2020 10:01, Julien Grall wrote:
> Hi,
> 
> On 09/04/2020 07:30, Jan Beulich wrote:
>> On 09.04.2020 00:05, Julien Grall wrote:
>>> Hi Jan,
>>>
>>> On 07/04/2020 09:14, Jan Beulich wrote:
>>>> On 04.04.2020 15:10, Julien Grall wrote:
>>>>> From: Julien Grall <jgrall@amazon.com>
>>>>>
>>>>> Most of the helpers to access guest memory are implemented the same way
>>>>> on Arm and x86. The only differences are:
>>>>>       - guest_handle_{from, to}_param(): while on x86 XEN_GUEST_HANDLE()
>>>>>         and XEN_GUEST_HANDLE_PARAM() are the same, they are not on Arm. It
>>>>>         is still fine to use the Arm implementation on x86.
>>>>>       - __clear_guest_offset(): Interestingly the prototype does not match
>>>>>         between the x86 and Arm. However, the Arm one is bogus. So the x86
>>>>>         implementation can be used.
>>>>>       - guest_handle{,_subrange}_okay(): They are validly differing
>>>>>         because Arm is only supporting auto-translated guest and therefore
>>>>>         handles are always valid.
>>>>
>>>> While I'm fine in principle with such consolidation, I'm afraid I
>>>> really need to ask for some historical background to be added
>>>> here. It may very well be that there's a reason for the separation
>>>> (likely to be found in the removed ia64 or ppc ports), which may
>>>> then provide a hint at why future ports may want to have these
>>>> separated. If such reasons exist, I'd prefer to avoid the back and
>>>> forth between headers. What we could do in such a case is borrow
>>>> Linux'es asm-generic/ concept, and move the "typical"
>>>> implementation there. (And of course if there were no noticable
>>>> reasons for the split, the change as it is would be fine in
>>>> general; saying so without having looked at the details of it,
>>>> yet).
>>>
>>> Looking at the history, ia64 and ppc used to include a common
>>> header called xen/xencomm.h from asm/guest_access.h.
>>>
>>> This has now disappeared with the removal of the two ports.
>>>
>>> Regarding future arch, the fact arm and x86 gives me some confidence
>>> we are unlikely going to get a new ABI for an arch. Do you see any
>>> reason to?
>>
>> Well, there surely had be a reason for ia64 and ppc to use a
>> different approach. 
> 
> This was introduced way before my time in Xen. AFAICT, you were
> already part of the community when 'xencomm' were alive.

I was, but I was never actively involved in ia64 or ppc work.

> There are not much information about it only nor in the commits.
> So do you mind sharing a bit more what 'xencomm' meant to be
> and why it wasn't introduced on x86?

If I had details, I would have provided at least some hints already.

>> I don't see why a new port may not also want
>> to go that route instead of the x86/Arm one.
> I could accept that someone would want to reinvent a new ABI
> from scratch for just an hypothetical new arch. However it would
> be quite an effort to reinvent XEN_GUEST_HANDLE(). The chance is
> RISC-V is only going to re-use what Arm did as Arm already did
> with x86.
> 
> I would like to avoid to introduce a new directory asm-generic
> with just one header in it. Maybe you have some other headers in
> mind?

I recall having wondered a few times whether we shouldn't use this
concept elsewhere. One case iirc was bitops stuff. Looking over
the Linux ones, some atomic and barrier fallback implementations
may also sensibly live there, and there are likely more.

Jan

Re: [PATCH 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Julien Grall]

On 09/04/2020 09:06, Jan Beulich wrote:
> On 09.04.2020 10:01, Julien Grall wrote:
>> Hi,
>>
>> On 09/04/2020 07:30, Jan Beulich wrote:
>>> On 09.04.2020 00:05, Julien Grall wrote:
>>> I don't see why a new port may not also want
>>> to go that route instead of the x86/Arm one.
>> I could accept that someone would want to reinvent a new ABI
>> from scratch for just an hypothetical new arch. However it would
>> be quite an effort to reinvent XEN_GUEST_HANDLE(). The chance is
>> RISC-V is only going to re-use what Arm did as Arm already did
>> with x86.
>>
>> I would like to avoid to introduce a new directory asm-generic
>> with just one header in it. Maybe you have some other headers in
>> mind?
> 
> I recall having wondered a few times whether we shouldn't use this
> concept elsewhere. One case iirc was bitops stuff. Looking over
> the Linux ones, some atomic and barrier fallback implementations
> may also sensibly live there, and there are likely more.

In theory it makes sense but, in the current state of Xen, 'asm-generic' 
means they are common to Arm and x86. This is basically the same 
definition as for the directory 'xen'. So how do you draw a line which 
files go where?

To be honest, I don't think we can draw a line without a 3rd 
architecture. So I would recommend to wait until then to create an 
asm-generic directory.

Meanwhile, I still think the consolidation in xen/ is useful as it makes 
easier to maintain. It is also going to make easier for RISCv (or a new 
arch) as they don't have to worry about duplication (if any).

In the event they decide to purse their own route, then it is not going 
to be a massive pain to move part of xen/guest_access.h in 
asm-generic/guest_access.h and include the latter from {xen, asm} 
/guest_access.h.

Cheers,


-- 
Julien Grall

Re: [PATCH 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Julien Grall]

Hi,

Gentle ping. Any comments on the direction to take?

On 09/04/2020 10:28, Julien Grall wrote:
> 
> 
> On 09/04/2020 09:06, Jan Beulich wrote:
>> On 09.04.2020 10:01, Julien Grall wrote:
>>> Hi,
>>>
>>> On 09/04/2020 07:30, Jan Beulich wrote:
>>>> On 09.04.2020 00:05, Julien Grall wrote:
>>>> I don't see why a new port may not also want
>>>> to go that route instead of the x86/Arm one.
>>> I could accept that someone would want to reinvent a new ABI
>>> from scratch for just an hypothetical new arch. However it would
>>> be quite an effort to reinvent XEN_GUEST_HANDLE(). The chance is
>>> RISC-V is only going to re-use what Arm did as Arm already did
>>> with x86.
>>>
>>> I would like to avoid to introduce a new directory asm-generic
>>> with just one header in it. Maybe you have some other headers in
>>> mind?
>>
>> I recall having wondered a few times whether we shouldn't use this
>> concept elsewhere. One case iirc was bitops stuff. Looking over
>> the Linux ones, some atomic and barrier fallback implementations
>> may also sensibly live there, and there are likely more.
> 
> In theory it makes sense but, in the current state of Xen, 'asm-generic' 
> means they are common to Arm and x86. This is basically the same 
> definition as for the directory 'xen'. So how do you draw a line which 
> files go where?
> 
> To be honest, I don't think we can draw a line without a 3rd 
> architecture. So I would recommend to wait until then to create an 
> asm-generic directory.
> 
> Meanwhile, I still think the consolidation in xen/ is useful as it makes 
> easier to maintain. It is also going to make easier for RISCv (or a new 
> arch) as they don't have to worry about duplication (if any).
> 
> In the event they decide to purse their own route, then it is not going 
> to be a massive pain to move part of xen/guest_access.h in 
> asm-generic/guest_access.h and include the latter from {xen, asm} 
> /guest_access.h.

Cheers,

-- 
Julien Grall

Re: [PATCH 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Jan Beulich]

On 29.04.2020 16:04, Julien Grall wrote:
> Gentle ping. Any comments on the direction to take?

Just to avoid misunderstanding - while the mail was sent with me as
the only one on the To list, I don't think I've been meant, as I've
voiced my opinion. I assume you rather meant to ping others to chime
in?

Jan

> On 09/04/2020 10:28, Julien Grall wrote:
>>
>>
>> On 09/04/2020 09:06, Jan Beulich wrote:
>>> On 09.04.2020 10:01, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 09/04/2020 07:30, Jan Beulich wrote:
>>>>> On 09.04.2020 00:05, Julien Grall wrote:
>>>>> I don't see why a new port may not also want
>>>>> to go that route instead of the x86/Arm one.
>>>> I could accept that someone would want to reinvent a new ABI
>>>> from scratch for just an hypothetical new arch. However it would
>>>> be quite an effort to reinvent XEN_GUEST_HANDLE(). The chance is
>>>> RISC-V is only going to re-use what Arm did as Arm already did
>>>> with x86.
>>>>
>>>> I would like to avoid to introduce a new directory asm-generic
>>>> with just one header in it. Maybe you have some other headers in
>>>> mind?
>>>
>>> I recall having wondered a few times whether we shouldn't use this
>>> concept elsewhere. One case iirc was bitops stuff. Looking over
>>> the Linux ones, some atomic and barrier fallback implementations
>>> may also sensibly live there, and there are likely more.
>>
>> In theory it makes sense but, in the current state of Xen, 'asm-generic' means they are common to Arm and x86. This is basically the same definition as for the directory 'xen'. So how do you draw a line which files go where?
>>
>> To be honest, I don't think we can draw a line without a 3rd architecture. So I would recommend to wait until then to create an asm-generic directory.
>>
>> Meanwhile, I still think the consolidation in xen/ is useful as it makes easier to maintain. It is also going to make easier for RISCv (or a new arch) as they don't have to worry about duplication (if any).
>>
>> In the event they decide to purse their own route, then it is not going to be a massive pain to move part of xen/guest_access.h in asm-generic/guest_access.h and include the latter from {xen, asm} /guest_access.h.
> 
> Cheers,
> 

Re: [PATCH 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Julien Grall]

Hi,

On 29/04/2020 15:07, Jan Beulich wrote:
> On 29.04.2020 16:04, Julien Grall wrote:
>> Gentle ping. Any comments on the direction to take?
> 
> Just to avoid misunderstanding - while the mail was sent with me as
> the only one on the To list, I don't think I've been meant, as I've
> voiced my opinion. I assume you rather meant to ping others to chime
> in?

I barely pay attention to CC vs TO. If I am on the list of recipients of 
an e-mail, then I will at least have a glance.

This e-mail is directed to you specifically and also the others. While 
you may have voiced some of your opinion already, you haven't replied 
back how you would decide whether an header should be added in xen or 
asm-generic.

So can you please have another and explain how the line can be drawn 
with just two architectures in place.

Cheers,

-- 
Julien Grall

Re: [PATCH 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Jan Beulich]

On 29.04.2020 16:13, Julien Grall wrote:
> So can you please have another and explain how the line can be drawn with just two architectures in place.

There are abstract considerations that can be used to draw the
line, as well as knowledge of people on architectures Xen doesn't
run on, but where one can - with such knowledge - extrapolate how
it would want to be implemented.

I don't think the question at this point is where to draw the
line, but whether to have asm-generic/ in the first place.

Jan

Re: [PATCH 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Julien Grall]

On 29/04/2020 15:54, Jan Beulich wrote:
> On 29.04.2020 16:13, Julien Grall wrote:
>> So can you please have another and explain how the line can be drawn with just two architectures in place.
> 
> There are abstract considerations that can be used to draw the
> line, as well as knowledge of people on architectures Xen doesn't
> run on, but where one can - with such knowledge - extrapolate how
> it would want to be implemented.
 >
> I don't think the question at this point is where to draw the
> line, but whether to have asm-generic/ in the first place.

Well the two come together. You can't add a new directory with no clear 
view how this is going to be used.

At the moment, this would result at best bikeshedding because 
developpers may have a different opinion on how a new architecture would 
be implemented in Xen.

If you have a 3rd architectures then it would be easier to argue the 
header should be added in asm-generic/ or xen/:
    - asm-generic/ should be used if 2 of the architectures are using 
the same interface
    - xen/ should be if the 3 architectures are using the same interface

Cheers,

-- 
Julien Grall

Re: [PATCH 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Julien Grall]

Ping 2.

On 29/04/2020 15:04, Julien Grall wrote:
> Hi,
> 
> Gentle ping. Any comments on the direction to take?
> 
> On 09/04/2020 10:28, Julien Grall wrote:
>>
>>
>> On 09/04/2020 09:06, Jan Beulich wrote:
>>> On 09.04.2020 10:01, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 09/04/2020 07:30, Jan Beulich wrote:
>>>>> On 09.04.2020 00:05, Julien Grall wrote:
>>>>> I don't see why a new port may not also want
>>>>> to go that route instead of the x86/Arm one.
>>>> I could accept that someone would want to reinvent a new ABI
>>>> from scratch for just an hypothetical new arch. However it would
>>>> be quite an effort to reinvent XEN_GUEST_HANDLE(). The chance is
>>>> RISC-V is only going to re-use what Arm did as Arm already did
>>>> with x86.
>>>>
>>>> I would like to avoid to introduce a new directory asm-generic
>>>> with just one header in it. Maybe you have some other headers in
>>>> mind?
>>>
>>> I recall having wondered a few times whether we shouldn't use this
>>> concept elsewhere. One case iirc was bitops stuff. Looking over
>>> the Linux ones, some atomic and barrier fallback implementations
>>> may also sensibly live there, and there are likely more.
>>
>> In theory it makes sense but, in the current state of Xen, 
>> 'asm-generic' means they are common to Arm and x86. This is basically 
>> the same definition as for the directory 'xen'. So how do you draw a 
>> line which files go where?
>>
>> To be honest, I don't think we can draw a line without a 3rd 
>> architecture. So I would recommend to wait until then to create an 
>> asm-generic directory.
>>
>> Meanwhile, I still think the consolidation in xen/ is useful as it 
>> makes easier to maintain. It is also going to make easier for RISCv 
>> (or a new arch) as they don't have to worry about duplication (if any).
>>
>> In the event they decide to purse their own route, then it is not 
>> going to be a massive pain to move part of xen/guest_access.h in 
>> asm-generic/guest_access.h and include the latter from {xen, asm} 
>> /guest_access.h.
> 
> Cheers,
> 

-- 
Julien Grall

Re: [PATCH 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Ian Jackson]

Hi.  My attention was drawn to this thread.

As I understand it, everyone is agreed that deduplicating the
implementation is good (I also agree).  The debate is only between:

1. Put it in xen/ until an arch comes along that needs something
  different, at which point maybe introduce an asm-generic-style
  thing with default implementations.

2. Say, now, that this is a default implementation and it should go in
   asm-generic.

My starting point is that Julien, as the primary author of this
cleanup, should be given leeway on a matter of taste like this.
(There are as I understand it no wider implications.)

Also, ISTM that it can be argued that introducing a new abstraction is
an additional piece of work.  Doing that is certainly not hampered by
Julien's change.  So that would be another reason to take Julien's
patch as-is.

On the merits, I don't have anything to add to the arguments already
presented.  I am considerably more persuaded by Julien's arguments
than Jan's.

So on all levels I think this commit should go in, unless there are
other concerns that have not been discussed here ?

Thanks,
Ian.

Re: [PATCH 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Julien Grall]

Hi Ian,

On 19/05/2020 16:05, Ian Jackson wrote:
> Hi.  My attention was drawn to this thread.
> 
> As I understand it, everyone is agreed that deduplicating the
> implementation is good (I also agree).  The debate is only between:

Thank you for stepping in!

> 
> 1. Put it in xen/ until an arch comes along that needs something
>    different, at which point maybe introduce an asm-generic-style
>    thing with default implementations.
> 
> 2. Say, now, that this is a default implementation and it should go in
>     asm-generic.
> 
> My starting point is that Julien, as the primary author of this
> cleanup, should be given leeway on a matter of taste like this.
> (There are as I understand it no wider implications.)
> 
> Also, ISTM that it can be argued that introducing a new abstraction is
> an additional piece of work.  Doing that is certainly not hampered by
> Julien's change.  So that would be another reason to take Julien's
> patch as-is.
> 
> On the merits, I don't have anything to add to the arguments already
> presented.  I am considerably more persuaded by Julien's arguments
> than Jan's.
> 
> So on all levels I think this commit should go in, unless there are
> other concerns that have not been discussed here ?
The major blocker is where the common header lives. The rest are small 
comments I should address in the next version.

I will send a new version (probably post freeze) to address those comments.

Cheers,

-- 
Julien Grall