[Xen-devel] [PATCH 4/4] xen/public: arm: rework the macro set_xen_guest_handle_raw

[Julien Grall <julien.grall@xxxxxxxxxx>]

The current implementation of set_xen_guest_handle_raw is using the
keyword "typeof" which is not part of C spec.

Furthermore, when the guest handle is defined in ARM32 guest, the
pointer will always be smaller than the handle. Based on the C99 spec
[1] 6.2.6.1#7, the bits that do not correspond to the member written
will take unspecified value.

Finally, based on the defect report #283 [2], the behavior of writing
from one member and reading from another is not clear.

We don't hit the problems for Xen and the toolstack because they are
both built with strict aliasing disabled. However, this may not be true
for any software using those headers.

We want to rewrite the macro set_xen_guest_handle_raw based on the
following constraint:
    1) typeof should not be used
    2) the parameters of the macros should only be interpreted one time
    3) the bits unused by the pointer should be zeroed

So we to have to zeroed the top 32-bit (for ARM32) and set the pointer
in a single expression. This means that we have to use a 64-bit access
via the field '.q' represent an uint64_t. Because of that we loose the
type safety provided by the field '.p' storing the pointer. Two compile
time checks have been added to ensure the validity of the handle and the
data stored.

While we don't provide a generic macro to get the guest pointer from the
handle, Xen obviously uses the guest pointer. As Xen is build with strict
aliasing disabled, we don't have to worry to set and get the value in the
union using different field. To avoid future issue, comments has been
added in the public header and Xen guest access to explain the problem
and why it works.

Note that set_xen_guest_handle_raw won't work with guest handle param.
But this is fine as this kind of handle is only used within the
hypervisor and updating the guest pointer for ARM guest will require
more care. FWIW, the caller of set_xen_guest_handle_raw in the
hypervisor are in compat code and kexec. None of them of build for ARM
today.

Finally while we modify asm-arm/guest_access.h, fix a typo.

[1] http://cs.nyu.edu/courses/spring13/CSCI-GA.2110-001/downloads/C99.pdf
[2] http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_283.htm

Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>

---

Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Cc: Jan Beulich <jbeulich@xxxxxxxx>
Cc: Keir Fraser <keir@xxxxxxx>
Cc: Tim Deegan <tim@xxxxxxx>
---
 xen/include/asm-arm/guest_access.h | 10 ++++++++++
 xen/include/public/arch-arm.h      | 19 ++++++++++++++-----
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/xen/include/asm-arm/guest_access.h 
b/xen/include/asm-arm/guest_access.h
index 5876988..fd2a849 100644
--- a/xen/include/asm-arm/guest_access.h
+++ b/xen/include/asm-arm/guest_access.h
@@ -4,6 +4,16 @@
 #include <xen/guest_access.h>
 #include <xen/errno.h>
 
+/*
+ * Some of the macros within this file are using the field ".p" of the
+ * guest handle.
+ *
+ * While set_xen_guest_handle is always setting the handle using 64-bit
+ * access, the value is retrieved using the field ".p" which is 32-bit
+ * on ARM32 and 64-bit on ARM64. However, it's safe to use as Xen is built
+ * with strict aliasing disabled.
+ */
+
 /* Guests have their own comlete address space */
 #define access_ok(addr,size) (1)
 
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index b278bc0..390f6f3 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -193,11 +193,20 @@
 #define XEN_GUEST_HANDLE_PARAM(name)    __guest_handle_param_ ## name
 #endif
 
-#define set_xen_guest_handle_raw(hnd, val)                  \
-    do {                                                    \
-        typeof(&(hnd)) _sxghr_tmp = &(hnd);                 \
-        _sxghr_tmp->q = 0;                                  \
-        _sxghr_tmp->p = val;                                \
+/*
+ * Macro to set a guest pointer in the handle.
+ *
+ * Note that it's not possible to implement safely a macro to retrieve the
+ * handle unless the guest is built with strict aliasing disabling.
+ * Hence, we don't provide a such macro in the public headers.
+ */
+#define set_xen_guest_handle_raw(hnd, val)                              \
+    do {                                                                \
+        /* Check if the handle is 64-bit (i.e 8-byte) */                \
+        (void) sizeof(struct { int : -!!(sizeof (hnd) != 8); });        \
+        /* Check if the type of val is compatible with the handle */    \
+        (void) sizeof((val) != (hnd).p);                                \
+        (hnd).q = (uint64_t)(uintptr_t)(val);                           \
     } while ( 0 )
 #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel