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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 30 Oct 2015, Julien Grall wrote:
> 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 )

Honestly I would be OK with having a typeof in the public headers to
avoid this code, which is much harder to follow. Why don't we do
something like the following:


diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 9a96401..e676ffb 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -189,11 +189,12 @@
 #define __XEN_GUEST_HANDLE(name)        __guest_handle_64_ ## name
 #define XEN_GUEST_HANDLE(name)          __XEN_GUEST_HANDLE(name)
 #define XEN_GUEST_HANDLE_PARAM(name)    __guest_handle_ ## name
+#define barrier()     __asm__ __volatile__("": : :"memory")
 #define set_xen_guest_handle_raw(hnd, val)                  \
     do {                                                    \
-        typeof(&(hnd)) _sxghr_tmp = &(hnd);                 \
-        _sxghr_tmp->q = 0;                                  \
-        _sxghr_tmp->p = val;                                \
+        *((uint64_aligned_t *)&(hnd)) = 0;                  \
+        barrier();                                          \
+        (hnd).p = val;                                      \
     } while ( 0 )
 #ifdef __XEN_TOOLS__
 #define get_xen_guest_handle(val, hnd)  do { val = (hnd).p; } while (0)

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