Mailing List Archive

[RESEND][PATCH v2 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h
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.

In the past, the ia64 and ppc64 port use a different model to access
guest parameter. They have been long gone now.

Given Xen currently only support 2 archictures, it is too soon to have a
directory asm-generic as it is not possible to differentiate it with the
existing directory xen/. If/When there is a 3rd port, we can decide to
create the new directory if that new port decide to use a different way
to access guest parameter.

For now, consolidate it in xen/guest_access.h.

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>

---
Changes in v2:
- Expand the commit message explaining why asm-generic is not
created.
---
xen/include/asm-arm/guest_access.h | 114 ---------------------------
xen/include/asm-x86/guest_access.h | 108 --------------------------
xen/include/xen/guest_access.h | 119 +++++++++++++++++++++++++++++
3 files changed, 119 insertions(+), 222 deletions(-)

diff --git a/xen/include/asm-arm/guest_access.h b/xen/include/asm-arm/guest_access.h
index b9a89c495527..53766386d3d8 100644
--- a/xen/include/asm-arm/guest_access.h
+++ b/xen/include/asm-arm/guest_access.h
@@ -23,88 +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 }; \
-})
-
-/* Convert 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; \
-})
-
-#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.
- */
-#define copy_to_guest_offset(hnd, off, ptr, nr) ({ \
- const typeof(*(ptr)) *_s = (ptr); \
- char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \
- /* Check that the handle is not for a const type */ \
- void *__maybe_unused _t = (hnd).p; \
- (void)((hnd).p == _s); \
- 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 == _s); \
- 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.
@@ -113,38 +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)

-#define __copy_to_guest_offset(hnd, off, ptr, nr) ({ \
- const typeof(*(ptr)) *_s = (ptr); \
- char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \
- /* Check that the handle is not for a const type */ \
- void *__maybe_unused _t = (hnd).p; \
- (void)((hnd).p == _s); \
- __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 == _s); \
- __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 3ffde205f6a1..08c9fbbc78e1 100644
--- a/xen/include/asm-x86/guest_access.h
+++ b/xen/include/asm-x86/guest_access.h
@@ -38,81 +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 }; \
-})
-
-/* Convert a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
-#define guest_handle_to_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_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.
- */
-#define copy_to_guest_offset(hnd, off, ptr, nr) ({ \
- const typeof(*(ptr)) *_s = (ptr); \
- char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \
- /* Check that the handle is not for a const type */ \
- void *__maybe_unused _t = (hnd).p; \
- (void)((hnd).p == _s); \
- 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 == _s); \
- 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.
@@ -126,39 +51,6 @@
(last)-(first)+1, \
sizeof(*(hnd).p)))

-#define __copy_to_guest_offset(hnd, off, ptr, nr) ({ \
- const typeof(*(ptr)) *_s = (ptr); \
- char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \
- /* Check that the handle is not for a const type */ \
- void *__maybe_unused _t = (hnd).p; \
- (void)((hnd).p == _s); \
- __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 == _s); \
- __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 ef9aaa3efcfe..4957b8d1f2b8 100644
--- a/xen/include/xen/guest_access.h
+++ b/xen/include/xen/guest_access.h
@@ -11,6 +11,86 @@
#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; \
+})
+
+#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.
+ */
+#define copy_to_guest_offset(hnd, off, ptr, nr) ({ \
+ const typeof(*(ptr)) *_s = (ptr); \
+ char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \
+ /* Check that the handle is not for a const type */ \
+ void *__maybe_unused _t = (hnd).p; \
+ (void)((hnd).p == _s); \
+ 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 == _s); \
+ 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 +100,45 @@
#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().
+ */
+
+#define __copy_to_guest_offset(hnd, off, ptr, nr) ({ \
+ const typeof(*(ptr)) *_s = (ptr); \
+ char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \
+ /* Check that the handle is not for a const type */ \
+ void *__maybe_unused _t = (hnd).p; \
+ (void)((hnd).p == _s); \
+ __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 == _s); \
+ __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
Re: [RESEND][PATCH v2 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h [ In reply to ]
On Thu, 30 Jul 2020, 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()

It is actually just guest_handle_to_param() ?


> 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.
>
> In the past, the ia64 and ppc64 port use a different model to access
> guest parameter. They have been long gone now.
>
> Given Xen currently only support 2 archictures, it is too soon to have a
> directory asm-generic as it is not possible to differentiate it with the
> existing directory xen/. If/When there is a 3rd port, we can decide to
> create the new directory if that new port decide to use a different way
> to access guest parameter.
>
> For now, consolidate it in xen/guest_access.h.
>
> 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>

Looks good to me

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Changes in v2:
> - Expand the commit message explaining why asm-generic is not
> created.
> ---
> xen/include/asm-arm/guest_access.h | 114 ---------------------------
> xen/include/asm-x86/guest_access.h | 108 --------------------------
> xen/include/xen/guest_access.h | 119 +++++++++++++++++++++++++++++
> 3 files changed, 119 insertions(+), 222 deletions(-)
>
> diff --git a/xen/include/asm-arm/guest_access.h b/xen/include/asm-arm/guest_access.h
> index b9a89c495527..53766386d3d8 100644
> --- a/xen/include/asm-arm/guest_access.h
> +++ b/xen/include/asm-arm/guest_access.h
> @@ -23,88 +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 }; \
> -})
> -
> -/* Convert 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; \
> -})
> -
> -#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.
> - */
> -#define copy_to_guest_offset(hnd, off, ptr, nr) ({ \
> - const typeof(*(ptr)) *_s = (ptr); \
> - char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \
> - /* Check that the handle is not for a const type */ \
> - void *__maybe_unused _t = (hnd).p; \
> - (void)((hnd).p == _s); \
> - 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 == _s); \
> - 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.
> @@ -113,38 +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)
>
> -#define __copy_to_guest_offset(hnd, off, ptr, nr) ({ \
> - const typeof(*(ptr)) *_s = (ptr); \
> - char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \
> - /* Check that the handle is not for a const type */ \
> - void *__maybe_unused _t = (hnd).p; \
> - (void)((hnd).p == _s); \
> - __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 == _s); \
> - __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 3ffde205f6a1..08c9fbbc78e1 100644
> --- a/xen/include/asm-x86/guest_access.h
> +++ b/xen/include/asm-x86/guest_access.h
> @@ -38,81 +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 }; \
> -})
> -
> -/* Convert a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
> -#define guest_handle_to_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_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.
> - */
> -#define copy_to_guest_offset(hnd, off, ptr, nr) ({ \
> - const typeof(*(ptr)) *_s = (ptr); \
> - char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \
> - /* Check that the handle is not for a const type */ \
> - void *__maybe_unused _t = (hnd).p; \
> - (void)((hnd).p == _s); \
> - 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 == _s); \
> - 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.
> @@ -126,39 +51,6 @@
> (last)-(first)+1, \
> sizeof(*(hnd).p)))
>
> -#define __copy_to_guest_offset(hnd, off, ptr, nr) ({ \
> - const typeof(*(ptr)) *_s = (ptr); \
> - char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \
> - /* Check that the handle is not for a const type */ \
> - void *__maybe_unused _t = (hnd).p; \
> - (void)((hnd).p == _s); \
> - __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 == _s); \
> - __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 ef9aaa3efcfe..4957b8d1f2b8 100644
> --- a/xen/include/xen/guest_access.h
> +++ b/xen/include/xen/guest_access.h
> @@ -11,6 +11,86 @@
> #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; \
> +})
> +
> +#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.
> + */
> +#define copy_to_guest_offset(hnd, off, ptr, nr) ({ \
> + const typeof(*(ptr)) *_s = (ptr); \
> + char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \
> + /* Check that the handle is not for a const type */ \
> + void *__maybe_unused _t = (hnd).p; \
> + (void)((hnd).p == _s); \
> + 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 == _s); \
> + 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 +100,45 @@
> #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().
> + */
> +
> +#define __copy_to_guest_offset(hnd, off, ptr, nr) ({ \
> + const typeof(*(ptr)) *_s = (ptr); \
> + char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \
> + /* Check that the handle is not for a const type */ \
> + void *__maybe_unused _t = (hnd).p; \
> + (void)((hnd).p == _s); \
> + __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 == _s); \
> + __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
>
Re: [RESEND][PATCH v2 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h [ In reply to ]
On 30.07.2020 20:18, 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.

Is the description stale? I don't think there's any guest_handle_from_param()
anymore.

> - __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.
>
> In the past, the ia64 and ppc64 port use a different model to access
> guest parameter. They have been long gone now.
>
> Given Xen currently only support 2 archictures, it is too soon to have a
> directory asm-generic as it is not possible to differentiate it with the
> existing directory xen/. If/When there is a 3rd port, we can decide to
> create the new directory if that new port decide to use a different way
> to access guest parameter.
>
> For now, consolidate it in xen/guest_access.h.
>
> 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>

Apart from the above
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan
Re: [RESEND][PATCH v2 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h [ In reply to ]
Hi Stefano,

On 30/07/2020 20:37, Stefano Stabellini wrote:
> On Thu, 30 Jul 2020, 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()
>
> It is actually just guest_handle_to_param() ?

Yes. I forgot Jan recently removed the helper in commit dd5520f9df05
"x86: adjustments to guest handle treatment". I will update the commit
message.

>
>
>> 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.
>>
>> In the past, the ia64 and ppc64 port use a different model to access
>> guest parameter. They have been long gone now.
>>
>> Given Xen currently only support 2 archictures, it is too soon to have a
>> directory asm-generic as it is not possible to differentiate it with the
>> existing directory xen/. If/When there is a 3rd port, we can decide to
>> create the new directory if that new port decide to use a different way
>> to access guest parameter.
>>
>> For now, consolidate it in xen/guest_access.h.
>>
>> 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>
>
> Looks good to me
>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Thank you!

Cheers,

--
Julien Grall
Re: [RESEND][PATCH v2 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h [ In reply to ]
Hi Jan,

On 31/07/2020 12:45, Jan Beulich wrote:
> On 30.07.2020 20:18, 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.
>
> Is the description stale? I don't think there's any guest_handle_from_param()
> anymore.

Yes, I forgot you removed it. I will update the commit message.

>
>> - __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.
>>
>> In the past, the ia64 and ppc64 port use a different model to access
>> guest parameter. They have been long gone now.
>>
>> Given Xen currently only support 2 archictures, it is too soon to have a
>> directory asm-generic as it is not possible to differentiate it with the
>> existing directory xen/. If/When there is a 3rd port, we can decide to
>> create the new directory if that new port decide to use a different way
>> to access guest parameter.
>>
>> For now, consolidate it in xen/guest_access.h.
>>
>> 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>
>
> Apart from the above
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks!

Cheers,

--
Julien Grall