[PATCH 0/4] xen/public: arm: rework set_xen_guest_handle_raw

[PATCH 0/4] xen/public: arm: rework set_xen_guest_handle_raw

[Julien Grall]

Hello all,

The main goal of this series is to rework set_xen_guest_handle_raw which is
not ISO compliant. This is based on the discussion on a previous patch
[1] to switch from typeof to __typeof__.

At the same time, I took the opportunity to clean up some macros used for
guest handle. For each changes see in each patch.

This series is based on the patch sent by Jan Beulich to drop
get_xen_guest_handle [2]. But I wasn't able to apply it cleanly on
unstable, so I've provided a branch with this patch and this series:

git://xenbits.xen.org/people/julieng/xen-unstable.git branch guest-handle-v1

Regards,

[1] http://lists.xen.org/archives/html/xen-devel/2015-10/msg02831.html
[2] http://lists.xen.org/archives/html/xen-devel/2015-10/msg02854.html

Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>

Julien Grall (4):
  xen/public: arm: Clarify the name of guest handle structures
  xen/public: arm: Rework __guest_handle_param*
  xen/public: Don't expose XEN_GUEST_HANDLE_PARAM outside of the
    hypervisor
  xen/public: arm: rework the macro set_xen_guest_handle_raw

 xen/include/asm-arm/guest_access.h | 10 ++++++++++
 xen/include/public/arch-arm.h      | 33 +++++++++++++++++++++++----------
 xen/include/public/arch-x86/xen.h  |  4 ++++
 3 files changed, 37 insertions(+), 10 deletions(-)

-- 
2.1.4

[PATCH 1/4] xen/public: arm: Clarify the name of guest handle structures

[Julien Grall]

Currently it's hard to know which __guest_handle* is associated to a
guest handle or a guest handle param.

Rename the types to match the usage. I.e
    * __guest_handle is renamed to __guest_handle_param as it's used for
    hypercall parameters.
    * __guest_handle_64 is renamed to __guest_handle as it's used for
    guest handle in structure field stored in memory.

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

---

Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 xen/include/public/arch-arm.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 6322548..35839db 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -171,9 +171,9 @@
 #ifndef __ASSEMBLY__
 #define ___DEFINE_XEN_GUEST_HANDLE(name, type)                  \
     typedef union { type *p; unsigned long q; }                 \
-        __guest_handle_ ## name;                                \
+        __guest_handle_param_ ## name;                          \
     typedef union { type *p; uint64_aligned_t q; }              \
-        __guest_handle_64_ ## name;
+        __guest_handle_ ## name;
 
 /*
  * XEN_GUEST_HANDLE represents a guest pointer, when passed as a field
@@ -186,9 +186,9 @@
     ___DEFINE_XEN_GUEST_HANDLE(name, type);   \
     ___DEFINE_XEN_GUEST_HANDLE(const_##name, const type)
 #define DEFINE_XEN_GUEST_HANDLE(name)   __DEFINE_XEN_GUEST_HANDLE(name, name)
-#define __XEN_GUEST_HANDLE(name)        __guest_handle_64_ ## name
+#define __XEN_GUEST_HANDLE(name)        __guest_handle_ ## name
 #define XEN_GUEST_HANDLE(name)          __XEN_GUEST_HANDLE(name)
-#define XEN_GUEST_HANDLE_PARAM(name)    __guest_handle_ ## name
+#define XEN_GUEST_HANDLE_PARAM(name)    __guest_handle_param_ ## name
 #define set_xen_guest_handle_raw(hnd, val)                  \
     do {                                                    \
         typeof(&(hnd)) _sxghr_tmp = &(hnd);                 \
-- 
2.1.4

[PATCH 2/4] xen/public: arm: Rework __guest_handle_param*

[Julien Grall]

__guest_handle_param is used to represent a guest pointer stored pass as
an hypercall parameters. They are the same size as the native register
for the architecture. It will be 32-bit on ARM32 and 64-bit on ARM64.

As the __guest_handle_param will always be the size of a native
pointer, there is no need to have a union with an unsigned long.

Note that unsigned long may be not equivalent to the size of a pointer
on ARM64. It depends whether the software is build using the LP64 or
LLP64 data model. The size of an unsigned long in the latter will be
32-bit.

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

---

Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 xen/include/public/arch-arm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 35839db..477254f 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -170,7 +170,7 @@
 
 #ifndef __ASSEMBLY__
 #define ___DEFINE_XEN_GUEST_HANDLE(name, type)                  \
-    typedef union { type *p; unsigned long q; }                 \
+    typedef struct { type *p; }                                 \
         __guest_handle_param_ ## name;                          \
     typedef union { type *p; uint64_aligned_t q; }              \
         __guest_handle_ ## name;
-- 
2.1.4

[PATCH 3/4] xen/public: Don't expose XEN_GUEST_HANDLE_PARAM outside of the hypervisor

[Julien Grall]

A XEN_GUEST_HANDLE_PARAM is used to represent a guest pointer passed as
an hypercall register. It will always be the size of a native register.

This is only used in Xen for type-safety, the guest will directly pass a
pointer in the hypercall parameter.

Note that for ARM we only hide XEN_GUEST_HANDLE_PARAM. The type
__guest_handle_param_* is still exposed because it would result to
duplicating the macro to create the type. I find this solution more
difficult to read.

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

---

Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 xen/include/public/arch-arm.h     | 4 ++++
 xen/include/public/arch-x86/xen.h | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 477254f..b278bc0 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -188,7 +188,11 @@
 #define DEFINE_XEN_GUEST_HANDLE(name)   __DEFINE_XEN_GUEST_HANDLE(name, name)
 #define __XEN_GUEST_HANDLE(name)        __guest_handle_ ## name
 #define XEN_GUEST_HANDLE(name)          __XEN_GUEST_HANDLE(name)
+
+#ifdef __XEN__ /* Internal only, should never be exposed to the guest */
 #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);                 \
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index 5187560..8036b3c 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -52,7 +52,11 @@
 #define DEFINE_XEN_GUEST_HANDLE(name)   __DEFINE_XEN_GUEST_HANDLE(name, name)
 #define __XEN_GUEST_HANDLE(name)        __guest_handle_ ## name
 #define XEN_GUEST_HANDLE(name)          __XEN_GUEST_HANDLE(name)
+
+#ifdef __XEN__ /* Internal only, should never be exposed to the guest */
 #define XEN_GUEST_HANDLE_PARAM(name)    XEN_GUEST_HANDLE(name)
+#endif
+
 #define set_xen_guest_handle_raw(hnd, val)  do { (hnd).p = val; } while (0)
 #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
 
-- 
2.1.4

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

[Julien Grall]

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@citrix.com>

---

Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 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

Re: [PATCH 0/4] xen/public: arm: rework set_xen_guest_handle_raw

[Jan Beulich]

>>> On 30.10.15 at 19:13, <julien.grall@citrix.com> wrote:
> This series is based on the patch sent by Jan Beulich to drop
> get_xen_guest_handle [2]. But I wasn't able to apply it cleanly on
> unstable, so I've provided a branch with this patch and this series:
> 
> git://xenbits.xen.org/people/julieng/xen-unstable.git branch guest-handle-v1

Odd - the patch still appears to apply fine to current staging.

Jan

Re: [PATCH 3/4] xen/public: Don't expose XEN_GUEST_HANDLE_PARAM outside of the hypervisor

[Jan Beulich]

>>> On 30.10.15 at 19:13, <julien.grall@citrix.com> wrote:
> A XEN_GUEST_HANDLE_PARAM is used to represent a guest pointer passed as
> an hypercall register. It will always be the size of a native register.
> 
> This is only used in Xen for type-safety, the guest will directly pass a
> pointer in the hypercall parameter.
> 
> Note that for ARM we only hide XEN_GUEST_HANDLE_PARAM. The type
> __guest_handle_param_* is still exposed because it would result to
> duplicating the macro to create the type. I find this solution more
> difficult to read.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

With the comment coding style issue (missing full stop) fixed
Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH 0/4] xen/public: arm: rework set_xen_guest_handle_raw

[Julien Grall]

On 02/11/15 13:42, Jan Beulich wrote:
>>>> On 30.10.15 at 19:13, <julien.grall@citrix.com> wrote:
>> This series is based on the patch sent by Jan Beulich to drop
>> get_xen_guest_handle [2]. But I wasn't able to apply it cleanly on
>> unstable, so I've provided a branch with this patch and this series:
>>
>> git://xenbits.xen.org/people/julieng/xen-unstable.git branch guest-handle-v1
> 
> Odd - the patch still appears to apply fine to current staging.

FWIW:

julien@chilopoda:~/works/xen> git am /tmp/\[PATCH\]\ drop\ get_xen_guest_handle\(\).eml
Applying: drop get_xen_guest_handle()
error: patch failed: xen/arch/x86/sysctl.c:31
error: xen/arch/x86/sysctl.c: patch does not apply
error: patch failed: xen/arch/x86/x86_64/cpu_idle.c:21
error: xen/arch/x86/x86_64/cpu_idle.c: patch does not apply
error: patch failed: xen/include/public/arch-arm.h:195
error: xen/include/public/arch-arm.h: patch does not apply
error: patch failed: xen/include/public/arch-x86/xen.h:54
error: xen/include/public/arch-x86/xen.h: patch does not apply
Patch failed at 0001 drop get_xen_guest_handle()
The copy of the patch that failed is found in:
   /home/julien/works/xen/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

-- 
Julien Grall

Re: [PATCH 3/4] xen/public: Don't expose XEN_GUEST_HANDLE_PARAM outside of the hypervisor

[Stefano Stabellini]

On Mon, 2 Nov 2015, Jan Beulich wrote:
> >>> On 30.10.15 at 19:13, <julien.grall@citrix.com> wrote:
> > A XEN_GUEST_HANDLE_PARAM is used to represent a guest pointer passed as
> > an hypercall register. It will always be the size of a native register.
> > 
> > This is only used in Xen for type-safety, the guest will directly pass a
> > pointer in the hypercall parameter.
> > 
> > Note that for ARM we only hide XEN_GUEST_HANDLE_PARAM. The type
> > __guest_handle_param_* is still exposed because it would result to
> > duplicating the macro to create the type. I find this solution more
> > difficult to read.
> > 
> > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> With the comment coding style issue (missing full stop) fixed
> Acked-by: Jan Beulich <jbeulich@suse.com>

Same here

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Re: [PATCH 1/4] xen/public: arm: Clarify the name of guest handle structures

[Stefano Stabellini]

On Fri, 30 Oct 2015, Julien Grall wrote:
> Currently it's hard to know which __guest_handle* is associated to a
> guest handle or a guest handle param.
> 
> Rename the types to match the usage. I.e
>     * __guest_handle is renamed to __guest_handle_param as it's used for
>     hypercall parameters.
>     * __guest_handle_64 is renamed to __guest_handle as it's used for
>     guest handle in structure field stored in memory.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> ---
>  xen/include/public/arch-arm.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 6322548..35839db 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -171,9 +171,9 @@
>  #ifndef __ASSEMBLY__
>  #define ___DEFINE_XEN_GUEST_HANDLE(name, type)                  \
>      typedef union { type *p; unsigned long q; }                 \
> -        __guest_handle_ ## name;                                \
> +        __guest_handle_param_ ## name;                          \
>      typedef union { type *p; uint64_aligned_t q; }              \
> -        __guest_handle_64_ ## name;
> +        __guest_handle_ ## name;
>  
>  /*
>   * XEN_GUEST_HANDLE represents a guest pointer, when passed as a field
> @@ -186,9 +186,9 @@
>      ___DEFINE_XEN_GUEST_HANDLE(name, type);   \
>      ___DEFINE_XEN_GUEST_HANDLE(const_##name, const type)
>  #define DEFINE_XEN_GUEST_HANDLE(name)   __DEFINE_XEN_GUEST_HANDLE(name, name)
> -#define __XEN_GUEST_HANDLE(name)        __guest_handle_64_ ## name
> +#define __XEN_GUEST_HANDLE(name)        __guest_handle_ ## name
>  #define XEN_GUEST_HANDLE(name)          __XEN_GUEST_HANDLE(name)
> -#define XEN_GUEST_HANDLE_PARAM(name)    __guest_handle_ ## name
> +#define XEN_GUEST_HANDLE_PARAM(name)    __guest_handle_param_ ## name
>  #define set_xen_guest_handle_raw(hnd, val)                  \
>      do {                                                    \
>          typeof(&(hnd)) _sxghr_tmp = &(hnd);                 \
> -- 
> 2.1.4
> 

Re: [PATCH 2/4] xen/public: arm: Rework __guest_handle_param*

[Stefano Stabellini]

On Fri, 30 Oct 2015, Julien Grall wrote:
> __guest_handle_param is used to represent a guest pointer stored pass as
> an hypercall parameters. They are the same size as the native register
> for the architecture. It will be 32-bit on ARM32 and 64-bit on ARM64.
> 
> As the __guest_handle_param will always be the size of a native
> pointer, there is no need to have a union with an unsigned long.
> 
> Note that unsigned long may be not equivalent to the size of a pointer
> on ARM64. It depends whether the software is build using the LP64 or
> LLP64 data model. The size of an unsigned long in the latter will be
> 32-bit.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Obviously this is going to break set_xen_guest_handle_raw. I don't think
this cannot be committed separately to the change to
set_xen_guest_handle_raw.


> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> ---
>  xen/include/public/arch-arm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 35839db..477254f 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -170,7 +170,7 @@
>  
>  #ifndef __ASSEMBLY__
>  #define ___DEFINE_XEN_GUEST_HANDLE(name, type)                  \
> -    typedef union { type *p; unsigned long q; }                 \
> +    typedef struct { type *p; }                                 \
>          __guest_handle_param_ ## name;                          \
>      typedef union { type *p; uint64_aligned_t q; }              \
>          __guest_handle_ ## name;
> -- 
> 2.1.4
> 

Re: [PATCH 2/4] xen/public: arm: Rework __guest_handle_param*

[Julien Grall]

Hi Stefano,

On 02/11/15 15:19, Stefano Stabellini wrote:
> On Fri, 30 Oct 2015, Julien Grall wrote:
>> __guest_handle_param is used to represent a guest pointer stored pass as
>> an hypercall parameters. They are the same size as the native register
>> for the architecture. It will be 32-bit on ARM32 and 64-bit on ARM64.
>>
>> As the __guest_handle_param will always be the size of a native
>> pointer, there is no need to have a union with an unsigned long.
>>
>> Note that unsigned long may be not equivalent to the size of a pointer
>> on ARM64. It depends whether the software is build using the LP64 or
>> LLP64 data model. The size of an unsigned long in the latter will be
>> 32-bit.
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> Obviously this is going to break set_xen_guest_handle_raw. I don't think
> this cannot be committed separately to the change to
> set_xen_guest_handle_raw.

Well, all the usage of set_xen_guest_handle_raw within the hypervisor
are in compat and kexec which is not built for ARM.

Furthermore, after this series set_xen_guest_handle_raw won't work
anymore with a guest_handle_param.

Regards,

-- 
Julien Grall

Re: [PATCH 2/4] xen/public: arm: Rework __guest_handle_param*

[Ian Campbell]

On Mon, 2015-11-02 at 15:24 +0000, Julien Grall wrote:
> Hi Stefano,
> 
> On 02/11/15 15:19, Stefano Stabellini wrote:
> > On Fri, 30 Oct 2015, Julien Grall wrote:
> > > __guest_handle_param is used to represent a guest pointer stored pass
> > > as
> > > an hypercall parameters. They are the same size as the native
> > > register
> > > for the architecture. It will be 32-bit on ARM32 and 64-bit on ARM64.
> > > 
> > > As the __guest_handle_param will always be the size of a native
> > > pointer, there is no need to have a union with an unsigned long.
> > > 
> > > Note that unsigned long may be not equivalent to the size of a
> > > pointer
> > > on ARM64. It depends whether the software is build using the LP64 or
> > > LLP64 data model. The size of an unsigned long in the latter will be
> > > 32-bit.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > 
> > Obviously this is going to break set_xen_guest_handle_raw. I don't
> > think
> > this cannot be committed separately to the change to
> > set_xen_guest_handle_raw.
> 
> Well, all the usage of set_xen_guest_handle_raw within the hypervisor
> are in compat and kexec which is not built for ARM.

Shall we drop it from ARM then?

It can always be added back when it is needed (or the code in question
reworked to avoid the need).

> 
> Furthermore, after this series set_xen_guest_handle_raw won't work
> anymore with a guest_handle_param.
> 
> Regards,
> 

Re: [PATCH 2/4] xen/public: arm: Rework __guest_handle_param*

[Julien Grall]

On 02/11/15 15:35, Ian Campbell wrote:
> On Mon, 2015-11-02 at 15:24 +0000, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 02/11/15 15:19, Stefano Stabellini wrote:
>>> On Fri, 30 Oct 2015, Julien Grall wrote:
>>>> __guest_handle_param is used to represent a guest pointer stored pass
>>>> as
>>>> an hypercall parameters. They are the same size as the native
>>>> register
>>>> for the architecture. It will be 32-bit on ARM32 and 64-bit on ARM64.
>>>>
>>>> As the __guest_handle_param will always be the size of a native
>>>> pointer, there is no need to have a union with an unsigned long.
>>>>
>>>> Note that unsigned long may be not equivalent to the size of a
>>>> pointer
>>>> on ARM64. It depends whether the software is build using the LP64 or
>>>> LLP64 data model. The size of an unsigned long in the latter will be
>>>> 32-bit.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>>>
>>> Obviously this is going to break set_xen_guest_handle_raw. I don't
>>> think
>>> this cannot be committed separately to the change to
>>> set_xen_guest_handle_raw.
>>
>> Well, all the usage of set_xen_guest_handle_raw within the hypervisor
>> are in compat and kexec which is not built for ARM.
> 
> Shall we drop it from ARM then?

No. Sorry it wasn't clear on my previous mail, I was only speaking about
the usage of set_xen_guest_handle_raw on a XEN_GUEST_HANDLE_PARAM.

set_xen_guest_handle_raw is used by the guest/toolstack to set a pointer
in a structure. All those usages are done with a GUEST_HANDLE and not a
GUEST_HANDLE_PARAM.

Regards,

-- 
Julien Grall

Re: [PATCH 2/4] xen/public: arm: Rework __guest_handle_param*

[Ian Campbell]

On Mon, 2015-11-02 at 15:39 +0000, Julien Grall wrote:
> On 02/11/15 15:35, Ian Campbell wrote:
> > On Mon, 2015-11-02 at 15:24 +0000, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 02/11/15 15:19, Stefano Stabellini wrote:
> > > > On Fri, 30 Oct 2015, Julien Grall wrote:
> > > > > __guest_handle_param is used to represent a guest pointer stored
> > > > > pass
> > > > > as
> > > > > an hypercall parameters. They are the same size as the native
> > > > > register
> > > > > for the architecture. It will be 32-bit on ARM32 and 64-bit on
> > > > > ARM64.
> > > > > 
> > > > > As the __guest_handle_param will always be the size of a native
> > > > > pointer, there is no need to have a union with an unsigned long.
> > > > > 
> > > > > Note that unsigned long may be not equivalent to the size of a
> > > > > pointer
> > > > > on ARM64. It depends whether the software is build using the LP64
> > > > > or
> > > > > LLP64 data model. The size of an unsigned long in the latter will
> > > > > be
> > > > > 32-bit.
> > > > > 
> > > > > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > > > 
> > > > Obviously this is going to break set_xen_guest_handle_raw. I don't
> > > > think
> > > > this cannot be committed separately to the change to
> > > > set_xen_guest_handle_raw.
> > > 
> > > Well, all the usage of set_xen_guest_handle_raw within the hypervisor
> > > are in compat and kexec which is not built for ARM.
> > 
> > Shall we drop it from ARM then?
> 
> No. Sorry it wasn't clear on my previous mail, I was only speaking about
> the usage of set_xen_guest_handle_raw on a XEN_GUEST_HANDLE_PARAM.
> 
> set_xen_guest_handle_raw is used by the guest/toolstack to set a pointer
> in a structure. All those usages are done with a GUEST_HANDLE and not a
> GUEST_HANDLE_PARAM.

Ah yes, if I'd have thought about it I could have worked that out myself,
sorry.

Ian.

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

[Stefano Stabellini]

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@citrix.com>
> 
> ---
> 
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> ---
>  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)

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

[Jan Beulich]

>>> On 02.11.15 at 16:55, <stefano.stabellini@eu.citrix.com> wrote:
> 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 )

Because __asm__ again is an extension?

Jan

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

[Ian Campbell]

On Mon, 2015-11-02 at 15:55 +0000, Stefano Stabellini wrote:
> 
> > +/*
> > + * 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.

I suppose your objection is to two things:

+        /* Check if the handle is 64-bit (i.e 8-byte) */                \
+        (void) sizeof(struct { int : -!!(sizeof (hnd) != 8); });        \

and

+        /* Check if the type of val is compatible with the handle */    \
+        (void) sizeof((val) != (hnd).p);                                \

The first is really just an open coding of BUILD_BUG_ON, I suppose for some
reason BUILD_BUG_ON cannot just be used here (I assume because this is
itself a macro).

Personally I think a comment referring back to BUILD_BUG_ON e.g.:
    /* BUILD_BUG_ON(sizeof(hnd) != 8); Cannot use real B_B_O in a macro */
would be sufficient.

For the second I think the comparison of two pointers in this as a macro
type safety check is a common enough idiom that it should be understood.
But I wouldn't object to a more explicit comment explaining this, or
explaining that sizeof is necessary to not evaluate hnd a second time in
the macro.

On the second though, Julien I think it needs to be (&val) since you need
to compare the pointers to the types to trigger the compiler's "comparing
distinct pointer types" warning/error. Also given this new usage I think it
would be worth renaming p and q to something less opaque, value and
type_check or something would be fine IMHO.

>  Why don't we do something like the following:

Apart from Jan's comment about __asm__ and a question I have about whether
it isn't even needed, how certain are you that this doesn't violate any of
the C aliasing rules etc?

BTW, Julien, I think it would be fine to also make this macro differ for
arm32 and arm64, since the arm64 variant would then surely be simpler and
the arm32 one might (or might not) be.

> 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@lists.xen.org
http://lists.xen.org/xen-devel

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

[Julien Grall]

Hi,

On 03/11/15 12:35, Ian Campbell wrote:
> On Mon, 2015-11-02 at 15:55 +0000, Stefano Stabellini wrote:
>>
>>> +/*
>>> + * 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.
> 
> I suppose your objection is to two things:
> 
> +        /* Check if the handle is 64-bit (i.e 8-byte) */                \
> +        (void) sizeof(struct { int : -!!(sizeof (hnd) != 8); });        \
> 
> and
> 
> +        /* Check if the type of val is compatible with the handle */    \
> +        (void) sizeof((val) != (hnd).p);                                \
> 
> The first is really just an open coding of BUILD_BUG_ON, I suppose for some
> reason BUILD_BUG_ON cannot just be used here (I assume because this is
> itself a macro).
> 
> Personally I think a comment referring back to BUILD_BUG_ON e.g.:
>     /* BUILD_BUG_ON(sizeof(hnd) != 8); Cannot use real B_B_O in a macro */
> would be sufficient.

You could use BUILD_BUG_ON in a macro, but this is part of the public
interface and I don't think we should require the guest/toolstack to
provide a BUILD_BUG_ON macro.

> For the second I think the comparison of two pointers in this as a macro
> type safety check is a common enough idiom that it should be understood.
> But I wouldn't object to a more explicit comment explaining this, or
> explaining that sizeof is necessary to not evaluate hnd a second time in
> the macro.

Will do.

> On the second though, Julien I think it needs to be (&val) since you need
> to compare the pointers to the types to trigger the compiler's "comparing
> distinct pointer types" warning/error.

No, val is already a pointer to the type (see the previous implementation).

> Also given this new usage I think it
> would be worth renaming p and q to something less opaque, value and
> type_check or something would be fine IMHO.

I guess you mean replacing "p" by "type_check" and "q" by value. If so,
I disagree with that because we have to use the actual "p" within Xen in
order to get the guest pointer and have type safety. It would be odd to
return "type_check".

I didn't change the names because I wasn't able to find better ones that
could fit for the 2 usages.

> 
>>  Why don't we do something like the following:
> 
> Apart from Jan's comment about __asm__ and a question I have about whether
> it isn't even needed, how certain are you that this doesn't violate any of
> the C aliasing rules etc?
> 
> BTW, Julien, I think it would be fine to also make this macro differ for
> arm32 and arm64, since the arm64 variant would then surely be simpler and
> the arm32 one might (or might not) be.

I agree that the macro could be simpler (only a single line). However I
didn't want to differ because there is no advantage other than have a
good looking code for the arm64 bits. It's just add extra code to take care.

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

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.

In that case, with set_xen_guest_handle_raw implemented as:

> +#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)

will get_xen_guest_handle, which access (hnd).p, have undefined behaviour?

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

[Ian Campbell]

On Tue, 2015-11-03 at 14:01 +0000, Julien Grall wrote:
> Hi,
> 
> On 03/11/15 12:35, Ian Campbell wrote:
> > On Mon, 2015-11-02 at 15:55 +0000, Stefano Stabellini wrote:
> > > 
> > > > +/*
> > > > + * 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.
> > 
> > I suppose your objection is to two things:
> > 
> > +        /* Check if the handle is 64-bit (i.e 8-byte)
> > */                \
> > +        (void) sizeof(struct { int : -!!(sizeof (hnd) != 8);
> > });        \
> > 
> > and
> > 
> > +        /* Check if the type of val is compatible with the handle
> > */    \
> > +        (void) sizeof((val) !=
> > (hnd).p);                                \
> > 
> > The first is really just an open coding of BUILD_BUG_ON, I suppose for
> > some
> > reason BUILD_BUG_ON cannot just be used here (I assume because this is
> > itself a macro).
> > 
> > Personally I think a comment referring back to BUILD_BUG_ON e.g.:
> >     /* BUILD_BUG_ON(sizeof(hnd) != 8); Cannot use real B_B_O in a macro
> > */
> > would be sufficient.
> 
> You could use BUILD_BUG_ON in a macro, but this is part of the public
> interface and I don't think we should require the guest/toolstack to
> provide a BUILD_BUG_ON macro.

Ah, yes, that makes more sense.

we could:
#ifdef(__XEN__)
#define XEN_BUILD_BUG_ON(x) BUILD_BUG_ON(x)
#elif !defined(XEN_BUILD_BUG_ON)
#define XEN_BUILD_BUG_ON(x)
#endif
and using XEN_BUILD_BUG_ON in the macro

So, __XEN__ builds get the check and users can opt in by providing
XEN_BUILD_BUG_ON if they want. If they don't then they don't get the check.

Or maybe we could just omit this from the public API by one or both of a)
adding an explicit 8 byte type to the union purely to force the size and/or
b) adding something to the non-public Xen side which consumes such
structures. After all any struct in the Xen API ought to be used somewhere
I suppose.

> > On the second though, Julien I think it needs to be (&val) since you
> > need
> > to compare the pointers to the types to trigger the compiler's
> > "comparing
> > distinct pointer types" warning/error.
> 
> No, val is already a pointer to the type (see the previous
> implementation).

Of course, I somehow convinced myself otherwise despite thinking it was
odd...

> > Also given this new usage I think it
> > would be worth renaming p and q to something less opaque, value and
> > type_check or something would be fine IMHO.
> 
> I guess you mean replacing "p" by "type_check" and "q" by value. If so,
> I disagree with that because we have to use the actual "p" within Xen in
> order to get the guest pointer and have type safety. It would be odd to
> return "type_check".

Ah, yes. I was thinking the read was of q not p. Having the Xen side use
type check would indeed be odd (even considering it is indirect via the
access stuff most of the time).

Perhaps the uint could be "bits" or "raw" and the actual pointer ptr, or
tbh "p" probably suffices in this case.

> I didn't change the names because I wasn't able to find better ones that
> could fit for the 2 usages.
> 
> > 
> > >  Why don't we do something like the following:
> > 
> > Apart from Jan's comment about __asm__ and a question I have about
> > whether
> > it isn't even needed, how certain are you that this doesn't violate any
> > of
> > the C aliasing rules etc?
> > 
> > BTW, Julien, I think it would be fine to also make this macro differ
> > for
> > arm32 and arm64, since the arm64 variant would then surely be simpler
> > and
> > the arm32 one might (or might not) be.
> 
> I agree that the macro could be simpler (only a single line). However I
> didn't want to differ because there is no advantage other than have a
> good looking code for the arm64 bits. It's just add extra code to take
> care.

Would the arm32 case be simplified since it would be a regular struct with
4 bytes padding and 4 bytes pointer, which can both be easily set in the
macro. If that works then the simplicity would seem to justify having the
two subarches use different cases here.

This looses out on the arm32 hypervisor sanity checking that the padding
bytes are 0 (as required by the ABI) but TBH I haven't checked that the
current version has that property either.

Ian.

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

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

[Julien Grall]

On 03/11/15 14:18, Stefano Stabellini wrote:
>> +#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)
> 
> will get_xen_guest_handle, which access (hnd).p, have undefined behaviour?

get_xen_guest_handle has been removed in staging.

Regards,

-- 
Julien Grall

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

[Julien Grall]

Hi,


On 03/11/15 14:34, Ian Campbell wrote:
> On Tue, 2015-11-03 at 14:01 +0000, Julien Grall wrote:
>> On 03/11/15 12:35, Ian Campbell wrote:
>>> On Mon, 2015-11-02 at 15:55 +0000, Stefano Stabellini wrote:
>>>>
>>>>> +/*
>>>>> + * 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.
>>>
>>> I suppose your objection is to two things:
>>>
>>> +        /* Check if the handle is 64-bit (i.e 8-byte)
>>> */                \
>>> +        (void) sizeof(struct { int : -!!(sizeof (hnd) != 8);
>>> });        \
>>>
>>> and
>>>
>>> +        /* Check if the type of val is compatible with the handle
>>> */    \
>>> +        (void) sizeof((val) !=
>>> (hnd).p);                                \
>>>
>>> The first is really just an open coding of BUILD_BUG_ON, I suppose for
>>> some
>>> reason BUILD_BUG_ON cannot just be used here (I assume because this is
>>> itself a macro).
>>>
>>> Personally I think a comment referring back to BUILD_BUG_ON e.g.:
>>>     /* BUILD_BUG_ON(sizeof(hnd) != 8); Cannot use real B_B_O in a macro
>>> */
>>> would be sufficient.
>>
>> You could use BUILD_BUG_ON in a macro, but this is part of the public
>> interface and I don't think we should require the guest/toolstack to
>> provide a BUILD_BUG_ON macro.
> 
> Ah, yes, that makes more sense.
> 
> we could:
> #ifdef(__XEN__)
> #define XEN_BUILD_BUG_ON(x) BUILD_BUG_ON(x)
> #elif !defined(XEN_BUILD_BUG_ON)
> #define XEN_BUILD_BUG_ON(x)
> #endif
> and using XEN_BUILD_BUG_ON in the macro
> 
> So, __XEN__ builds get the check and users can opt in by providing
> XEN_BUILD_BUG_ON if they want. If they don't then they don't get the check.

I wouldn't let the user the choice to disable build-time check. There is
no harm to open-code them as I did today and avoid possible issue in the
code later.

> Or maybe we could just omit this from the public API by one or both of a)
> adding an explicit 8 byte type to the union purely to force the size and/or

This is already done in the current version:

    typedef union { type *p; uint64_aligned_t q; }              \
        __guest_handle_64_ ## name;

However I don't see how this ensure that the caller of
set_xen_raw_guest_handle will effectively hnd as a pointer to an 8-byte
placeholder.


> b) adding something to the non-public Xen side which consumes such
> structures. After all any struct in the Xen API ought to be used somewhere
> I suppose.
> 

[...]

>>> Also given this new usage I think it
>>> would be worth renaming p and q to something less opaque, value and
>>> type_check or something would be fine IMHO.
>>
>> I guess you mean replacing "p" by "type_check" and "q" by value. If so,
>> I disagree with that because we have to use the actual "p" within Xen in
>> order to get the guest pointer and have type safety. It would be odd to
>> return "type_check".
> 
> Ah, yes. I was thinking the read was of q not p. Having the Xen side use
> type check would indeed be odd (even considering it is indirect via the
> access stuff most of the time).
> 
> Perhaps the uint could be "bits" or "raw" and the actual pointer ptr, or
> tbh "p" probably suffices in this case.
> 
>> I didn't change the names because I wasn't able to find better ones that
>> could fit for the 2 usages.
>>
>>>
>>>>  Why don't we do something like the following:
>>>
>>> Apart from Jan's comment about __asm__ and a question I have about
>>> whether
>>> it isn't even needed, how certain are you that this doesn't violate any
>>> of
>>> the C aliasing rules etc?
>>>
>>> BTW, Julien, I think it would be fine to also make this macro differ
>>> for
>>> arm32 and arm64, since the arm64 variant would then surely be simpler
>>> and
>>> the arm32 one might (or might not) be.
>>
>> I agree that the macro could be simpler (only a single line). However I
>> didn't want to differ because there is no advantage other than have a
>> good looking code for the arm64 bits. It's just add extra code to take
>> care.
> 
> Would the arm32 case be simplified since it would be a regular struct with
> 4 bytes padding and 4 bytes pointer, which can both be easily set in the
> macro. If that works then the simplicity would seem to justify having the
> two subarches use different cases here.

One of my first idea was to use a structure with 2 fields (4 bytes
padding + 4 bytes pointer) on arm32. But it's not possible to assign in
a single expression all the fields as we lack of the type within the macro:

test.c:14:9: error: expected expression before ‘{’ token
     b = { .a = 0, .b = 0 };

> This looses out on the arm32 hypervisor sanity checking that the padding
> bytes are 0 (as required by the ABI) but TBH I haven't checked that the
> current version has that property either.

It's done during the assignation by the compiler:

(hnd).q = (uint64_t)(uintptr_t)(val);

Regards,

-- 
Julien Grall

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

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

[Ian Campbell]

On Wed, 2015-11-04 at 11:17 +0000, Julien Grall wrote:
> Hi,
> 
> 
> On 03/11/15 14:34, Ian Campbell wrote:
> > On Tue, 2015-11-03 at 14:01 +0000, Julien Grall wrote:
> > > On 03/11/15 12:35, Ian Campbell wrote:
> > > > On Mon, 2015-11-02 at 15:55 +0000, Stefano Stabellini wrote:
> > > > > 
> > > > > > +/*
> > > > > > + * 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.
> > > > 
> > > > I suppose your objection is to two things:
> > > > 
> > > > +        /* Check if the handle is 64-bit (i.e 8-byte)
> > > > */                \
> > > > +        (void) sizeof(struct { int : -!!(sizeof (hnd) != 8);
> > > > });        \
> > > > 
> > > > and
> > > > 
> > > > +        /* Check if the type of val is compatible with the handle
> > > > */    \
> > > > +        (void) sizeof((val) !=
> > > > (hnd).p);                                \
> > > > 
> > > > The first is really just an open coding of BUILD_BUG_ON, I suppose
> > > > for
> > > > some
> > > > reason BUILD_BUG_ON cannot just be used here (I assume because this
> > > > is
> > > > itself a macro).
> > > > 
> > > > Personally I think a comment referring back to BUILD_BUG_ON e.g.:
> > > >     /* BUILD_BUG_ON(sizeof(hnd) != 8); Cannot use real B_B_O in a
> > > > macro
> > > > */
> > > > would be sufficient.
> > > 
> > > You could use BUILD_BUG_ON in a macro, but this is part of the public
> > > interface and I don't think we should require the guest/toolstack to
> > > provide a BUILD_BUG_ON macro.
> > 
> > Ah, yes, that makes more sense.
> > 
> > we could:
> > #ifdef(__XEN__)
> > #define XEN_BUILD_BUG_ON(x) BUILD_BUG_ON(x)
> > #elif !defined(XEN_BUILD_BUG_ON)
> > #define XEN_BUILD_BUG_ON(x)
> > #endif
> > and using XEN_BUILD_BUG_ON in the macro
> > 
> > So, __XEN__ builds get the check and users can opt in by providing
> > XEN_BUILD_BUG_ON if they want. If they don't then they don't get the
> > check.
> 
> I wouldn't let the user the choice to disable build-time check.

Up to you. Note that "the user" here would potentially include
xen.git/tools, and I would expect them to want to define it.

Also...

>  There is
> no harm to open-code them as I did today and avoid possible issue in the
> code later.

... there is always a downside to open coding. If you don't want to expose
the ability to define a BUG_ON to the application then just drop that #elif
from the chain.

> > Or maybe we could just omit this from the public API by one or both of
> > a)
> > adding an explicit 8 byte type to the union purely to force the size
> > and/or
> 
> This is already done in the current version:
> 
>     typedef union { type *p; uint64_aligned_t q; }              \
>         __guest_handle_64_ ## name;
> 
> However I don't see how this ensure that the caller of
> set_xen_raw_guest_handle will effectively hnd as a pointer to an 8-byte
> placeholder.

Not sure I follow. If hnd isn't a suitable struct xen guest handle then
other things will fail. If a user is passing a non struct xen_guest_handle
to this which happens to contain the same fields then more fool them, and
if it happens to be 8 bytes anyway your check won't catch that.

> > Would the arm32 case be simplified since it would be a regular struct
> > with
> > 4 bytes padding and 4 bytes pointer, which can both be easily set in
> > the
> > macro. If that works then the simplicity would seem to justify having
> > the
> > two subarches use different cases here.
> 
> One of my first idea was to use a structure with 2 fields (4 bytes
> padding + 4 bytes pointer) on arm32. But it's not possible to assign in
> a single expression all the fields as we lack of the type within the
> macro:
> 
> test.c:14:9: error: expected expression before ‘{’ token
>      b = { .a = 0, .b = 0 };

Right, that's a shame.

> 
> > This looses out on the arm32 hypervisor sanity checking that the padding
> > bytes are 0 (as required by the ABI) but TBH I haven't checked that the
> > current version has that property either.
> 
> It's done during the assignation by the compiler:
> 
> (hnd).q = (uint64_t)(uintptr_t)(val);

I meant on the reading side.

Ian.

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

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

[Julien Grall]

On 04/11/15 11:27, Ian Campbell wrote:
> On Wed, 2015-11-04 at 11:17 +0000, Julien Grall wrote:
>>>
>>> we could:
>>> #ifdef(__XEN__)
>>> #define XEN_BUILD_BUG_ON(x) BUILD_BUG_ON(x)
>>> #elif !defined(XEN_BUILD_BUG_ON)
>>> #define XEN_BUILD_BUG_ON(x)
>>> #endif
>>> and using XEN_BUILD_BUG_ON in the macro
>>>
>>> So, __XEN__ builds get the check and users can opt in by providing
>>> XEN_BUILD_BUG_ON if they want. If they don't then they don't get the
>>> check.
>>
>> I wouldn't let the user the choice to disable build-time check.
> 
> Up to you. Note that "the user" here would potentially include
> xen.git/tools, and I would expect them to want to define it.
> 
> Also...
> 
>>  There is
>> no harm to open-code them as I did today and avoid possible issue in the
>> code later.
> 
> ... there is always a downside to open coding. If you don't want to expose
> the ability to define a BUG_ON to the application then just drop that #elif
> from the chain.

Good point. I will give a look.

> 
>>> Or maybe we could just omit this from the public API by one or both of
>>> a)
>>> adding an explicit 8 byte type to the union purely to force the size
>>> and/or
>>
>> This is already done in the current version:
>>
>>     typedef union { type *p; uint64_aligned_t q; }              \
>>         __guest_handle_64_ ## name;
>>
>> However I don't see how this ensure that the caller of
>> set_xen_raw_guest_handle will effectively hnd as a pointer to an 8-byte
>> placeholder.
> 
> Not sure I follow. If hnd isn't a suitable struct xen guest handle then
> other things will fail. If a user is passing a non struct xen_guest_handle
> to this which happens to contain the same fields then more fool them, and
> if it happens to be 8 bytes anyway your check won't catch that.

With the 2 checks in set_xen_raw_guest_handle we catch most of the
problem. They both ensure that the handle is 8-byte and the pointer is
valid. However we don't check that the padding is at the beginning of
the structure.

It's better than what we have today as we don't even check that the
handle is 8-byte.

[...]

>>> This looses out on the arm32 hypervisor sanity checking that the padding
>>> bytes are 0 (as required by the ABI) but TBH I haven't checked that the
>>> current version has that property either.
>>
>> It's done during the assignation by the compiler:
>>
>> (hnd).q = (uint64_t)(uintptr_t)(val);
> 
> I meant on the reading side.

It's the responsibility of the caller to zero the padding. There is
nothing to do on the reading side, the hypervisor will use "p" which
will be the size of the natural pointer.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Wed, 2015-11-04 at 11:40 +0000, Julien Grall wrote:
> > Not sure I follow. If hnd isn't a suitable struct xen guest handle then
> > other things will fail. If a user is passing a non struct
> > xen_guest_handle
> > to this which happens to contain the same fields then more fool them,
> > and
> > if it happens to be 8 bytes anyway your check won't catch that.
> 
> With the 2 checks in set_xen_raw_guest_handle we catch most of the
> problem. They both ensure that the handle is 8-byte and the pointer is
> valid. However we don't check that the padding is at the beginning of
> the structure.
> 
> It's better than what we have today as we don't even check that the
> handle is 8-byte.

I'm not sure I'm all that worried about callers constructing their own
guest handle structs and getting it wrong.

> [...]
> 
> > > > This looses out on the arm32 hypervisor sanity checking that the
> > > > padding
> > > > bytes are 0 (as required by the ABI) but TBH I haven't checked that
> > > > the
> > > > current version has that property either.
> > > 
> > > It's done during the assignation by the compiler:
> > > 
> > > (hnd).q = (uint64_t)(uintptr_t)(val);
> > 
> > I meant on the reading side.
> 
> It's the responsibility of the caller to zero the padding. There is
> nothing to do on the reading side, the hypervisor will use "p" which
> will be the size of the natural pointer.

For a 32-bit Xen the check would be that a guest was not inadvertently
violating this rule, such a guest would crash if it was run on a 64-bit
hypervisor (which would see the non-zero padding as part of the pointer),
by rejecting such cases on 32-bit Xen we avoid such guests becoming
established and therefore presenting a case for us to relax this rule in
one way or another.

This is the same reason as check_multicall_32bit_clean() exists. multicall
is special only in that it was pretty easy to know where to add that check.

Ian.

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

[Julien Grall]

On 04/11/15 14:29, Ian Campbell wrote:
>>>>> This looses out on the arm32 hypervisor sanity checking that the
>>>>> padding
>>>>> bytes are 0 (as required by the ABI) but TBH I haven't checked that
>>>>> the
>>>>> current version has that property either.
>>>>
>>>> It's done during the assignation by the compiler:
>>>>
>>>> (hnd).q = (uint64_t)(uintptr_t)(val);
>>>
>>> I meant on the reading side.
>>
>> It's the responsibility of the caller to zero the padding. There is
>> nothing to do on the reading side, the hypervisor will use "p" which
>> will be the size of the natural pointer.
> 
> For a 32-bit Xen the check would be that a guest was not inadvertently
> violating this rule, such a guest would crash if it was run on a 64-bit
> hypervisor (which would see the non-zero padding as part of the pointer),
> by rejecting such cases on 32-bit Xen we avoid such guests becoming
> established and therefore presenting a case for us to relax this rule in
> one way or another.

It would add overhead each time we want to copy to/from the guest memory.

TBH, I don't think it's our business to check if the guest properly
filling out the structure. It could happen only when the guest decides
to implement it's own set_guest_handle_macro.

As long as we don't crash the hypervisor it's fine.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Wed, 2015-11-04 at 14:42 +0000, Julien Grall wrote:
> On 04/11/15 14:29, Ian Campbell wrote:
> > > > > > This looses out on the arm32 hypervisor sanity checking that
> > > > > > the
> > > > > > padding
> > > > > > bytes are 0 (as required by the ABI) but TBH I haven't checked
> > > > > > that
> > > > > > the
> > > > > > current version has that property either.
> > > > > 
> > > > > It's done during the assignation by the compiler:
> > > > > 
> > > > > (hnd).q = (uint64_t)(uintptr_t)(val);
> > > > 
> > > > I meant on the reading side.
> > > 
> > > It's the responsibility of the caller to zero the padding. There is
> > > nothing to do on the reading side, the hypervisor will use "p" which
> > > will be the size of the natural pointer.
> > 
> > For a 32-bit Xen the check would be that a guest was not inadvertently
> > violating this rule, such a guest would crash if it was run on a 64-bit
> > hypervisor (which would see the non-zero padding as part of the
> > pointer),
> > by rejecting such cases on 32-bit Xen we avoid such guests becoming
> > established and therefore presenting a case for us to relax this rule
> > in
> > one way or another.
> 
> It would add overhead each time we want to copy to/from the guest memory.

I was simply commenting that the approach taken here might be removing such
a check _if_ it exists today, and as I said up front I'm not sure we have
such a check right now or not.

I'm not suggesting you add one as a new check, just that _if_ it already
exists then it being removed needs to be considered.

Ian.

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

[Stefano Stabellini]

On Wed, 4 Nov 2015, Julien Grall wrote:
> On 04/11/15 11:27, Ian Campbell wrote:
> > On Wed, 2015-11-04 at 11:17 +0000, Julien Grall wrote:
> >>>
> >>> we could:
> >>> #ifdef(__XEN__)
> >>> #define XEN_BUILD_BUG_ON(x) BUILD_BUG_ON(x)
> >>> #elif !defined(XEN_BUILD_BUG_ON)
> >>> #define XEN_BUILD_BUG_ON(x)
> >>> #endif
> >>> and using XEN_BUILD_BUG_ON in the macro
> >>>
> >>> So, __XEN__ builds get the check and users can opt in by providing
> >>> XEN_BUILD_BUG_ON if they want. If they don't then they don't get the
> >>> check.
> >>
> >> I wouldn't let the user the choice to disable build-time check.
> > 
> > Up to you. Note that "the user" here would potentially include
> > xen.git/tools, and I would expect them to want to define it.
> > 
> > Also...
> > 
> >>  There is
> >> no harm to open-code them as I did today and avoid possible issue in the
> >> code later.
> > 
> > ... there is always a downside to open coding. If you don't want to expose
> > the ability to define a BUG_ON to the application then just drop that #elif
> > from the chain.
> 
> Good point. I will give a look.
> 
> > 
> >>> Or maybe we could just omit this from the public API by one or both of
> >>> a)
> >>> adding an explicit 8 byte type to the union purely to force the size
> >>> and/or
> >>
> >> This is already done in the current version:
> >>
> >>     typedef union { type *p; uint64_aligned_t q; }              \
> >>         __guest_handle_64_ ## name;
> >>
> >> However I don't see how this ensure that the caller of
> >> set_xen_raw_guest_handle will effectively hnd as a pointer to an 8-byte
> >> placeholder.
> > 
> > Not sure I follow. If hnd isn't a suitable struct xen guest handle then
> > other things will fail. If a user is passing a non struct xen_guest_handle
> > to this which happens to contain the same fields then more fool them, and
> > if it happens to be 8 bytes anyway your check won't catch that.
> 
> With the 2 checks in set_xen_raw_guest_handle we catch most of the
> problem. They both ensure that the handle is 8-byte and the pointer is
> valid. However we don't check that the padding is at the beginning of
> the structure.
> 
> It's better than what we have today as we don't even check that the
> handle is 8-byte.
> 
> [...]
> 
> >>> This looses out on the arm32 hypervisor sanity checking that the padding
> >>> bytes are 0 (as required by the ABI) but TBH I haven't checked that the
> >>> current version has that property either.
> >>
> >> It's done during the assignation by the compiler:
> >>
> >> (hnd).q = (uint64_t)(uintptr_t)(val);
> > 
> > I meant on the reading side.
> 
> It's the responsibility of the caller to zero the padding. There is
> nothing to do on the reading side, the hypervisor will use "p" which
> will be the size of the natural pointer.

Sorry to be pedantic, but why is this safe given that previously you
wrote:

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

Looks like it might be better to remove the union completely?

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

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 4/4] xen/public: arm: rework the macro set_xen_guest_handle_raw"):
> Finally, based on the defect report #283 [2], the behavior of writing
> from one member and reading from another is not clear.

Because the hypervisor is compiled with -fno-strict-aliasing.

Ian.

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

[Ian Campbell]

On Wed, 2015-11-04 at 15:19 +0000, Stefano Stabellini wrote:
> On Wed, 4 Nov 2015, Julien Grall wrote:
> > On 04/11/15 11:27, Ian Campbell wrote:
> > > On Wed, 2015-11-04 at 11:17 +0000, Julien Grall wrote:
> > > > > 
> > > > > we could:
> > > > > #ifdef(__XEN__)
> > > > > #define XEN_BUILD_BUG_ON(x) BUILD_BUG_ON(x)
> > > > > #elif !defined(XEN_BUILD_BUG_ON)
> > > > > #define XEN_BUILD_BUG_ON(x)
> > > > > #endif
> > > > > and using XEN_BUILD_BUG_ON in the macro
> > > > > 
> > > > > So, __XEN__ builds get the check and users can opt in by
> > > > > providing
> > > > > XEN_BUILD_BUG_ON if they want. If they don't then they don't get
> > > > > the
> > > > > check.
> > > > 
> > > > I wouldn't let the user the choice to disable build-time check.
> > > 
> > > Up to you. Note that "the user" here would potentially include
> > > xen.git/tools, and I would expect them to want to define it.
> > > 
> > > Also...
> > > 
> > > >  There is
> > > > no harm to open-code them as I did today and avoid possible issue
> > > > in the
> > > > code later.
> > > 
> > > ... there is always a downside to open coding. If you don't want to
> > > expose
> > > the ability to define a BUG_ON to the application then just drop that
> > > #elif
> > > from the chain.
> > 
> > Good point. I will give a look.
> > 
> > > 
> > > > > Or maybe we could just omit this from the public API by one or
> > > > > both of
> > > > > a)
> > > > > adding an explicit 8 byte type to the union purely to force the
> > > > > size
> > > > > and/or
> > > > 
> > > > This is already done in the current version:
> > > > 
> > > >     typedef union { type *p; uint64_aligned_t q; }              \
> > > >         __guest_handle_64_ ## name;
> > > > 
> > > > However I don't see how this ensure that the caller of
> > > > set_xen_raw_guest_handle will effectively hnd as a pointer to an 8-
> > > > byte
> > > > placeholder.
> > > 
> > > Not sure I follow. If hnd isn't a suitable struct xen guest handle
> > > then
> > > other things will fail. If a user is passing a non struct
> > > xen_guest_handle
> > > to this which happens to contain the same fields then more fool them,
> > > and
> > > if it happens to be 8 bytes anyway your check won't catch that.
> > 
> > With the 2 checks in set_xen_raw_guest_handle we catch most of the
> > problem. They both ensure that the handle is 8-byte and the pointer is
> > valid. However we don't check that the padding is at the beginning of
> > the structure.
> > 
> > It's better than what we have today as we don't even check that the
> > handle is 8-byte.
> > 
> > [...]
> > 
> > > > > This looses out on the arm32 hypervisor sanity checking that the
> > > > > padding
> > > > > bytes are 0 (as required by the ABI) but TBH I haven't checked
> > > > > that the
> > > > > current version has that property either.
> > > > 
> > > > It's done during the assignation by the compiler:
> > > > 
> > > > (hnd).q = (uint64_t)(uintptr_t)(val);
> > > 
> > > I meant on the reading side.
> > 
> > It's the responsibility of the caller to zero the padding. There is
> > nothing to do on the reading side, the hypervisor will use "p" which
> > will be the size of the natural pointer.
> 
> Sorry to be pedantic, but why is this safe given that previously you
> wrote:
> 
> Finally, based on the defect report #283 [2], the behavior of writing
> from one member and reading from another is not clear.

The writer via one is the guest and reader via the other is the hypervisor,
so no matter what they are certainly different compilation units, even in
the face of whole program optimisations.

The concerning issue is that if the compiler can observe you writing to
both halves of the union then it can either omit the first write or dive
off into deep undefined behaviour territory.

> Looks like it might be better to remove the union completely?

If that were possible I think someone would have suggested a scheme which
achieves it within the other constraints.

Ian.

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

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

[Ian Jackson]

Ian Jackson writes ("Re: [Xen-devel] [PATCH 4/4] xen/public: arm: rework the macro set_xen_guest_handle_raw"):
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 4/4] xen/public: arm: rework the macro set_xen_guest_handle_raw"):
> > Finally, based on the defect report #283 [2], the behavior of writing
> > from one member and reading from another is not clear.
> 
> Because the hypervisor is compiled with -fno-strict-aliasing.

Incidentally, it is _necessary_ to write to the uint64_t last.  C99
6.2.6.1(5) says that if you write to the 32-bit pointer, the upper
bits (corresponding to the uint64 but not part of the pointer) take on
unspecified values.

So writing the 32-bit pointer always invalidates the top bits, so if
you write to the uint64_t first, the 32-bit write makes the 64-bit
write a dead store.

Ian.

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

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 4/4] xen/public: arm: rework the macro set_xen_guest_handle_raw"):
> The writer via one is the guest and reader via the other is the hypervisor,
> so no matter what they are certainly different compilation units, even in
> the face of whole program optimisations.

The question of them being different `compilation units' (YM
translation units) is irrelevant I think.

> The concerning issue is that if the compiler can observe you writing to
> both halves of the union then it can either omit the first write or dive
> off into deep undefined behaviour territory.

If the compiler can see you write to p, it is allowed to assume that
all subsequent readers will read the object as typeof(p).  Reading
typeof(p) does not read the padding.  Therefore the compiler is
allowed to `prove' that the padding is a dead store, and remove the
write to the padding.

This applies even if the compiler can't see the code which is doing
the reading.

Ian.

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

[Ian Campbell]

On Wed, 2015-11-04 at 15:46 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 4/4] xen/public: arm: rework
> the macro set_xen_guest_handle_raw"):
> > The writer via one is the guest and reader via the other is the
> > hypervisor,
> > so no matter what they are certainly different compilation units, even
> > in
> > the face of whole program optimisations.
> 
> The question of them being different `compilation units' (YM
> translation units) is irrelevant I think.
> 
> > The concerning issue is that if the compiler can observe you writing to
> > both halves of the union then it can either omit the first write or
> > dive
> > off into deep undefined behaviour territory.
> 
> If the compiler can see you write to p, it is allowed to assume that
> all subsequent readers will read the object as typeof(p).  Reading
> typeof(p) does not read the padding.  Therefore the compiler is
> allowed to `prove' that the padding is a dead store, and remove the
> write to the padding.
> 
> This applies even if the compiler can't see the code which is doing
> the reading.

Ah, yes :-(

Ian.

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

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

[Ian Jackson]

Julien Grall writes ("Re: [PATCH 4/4] xen/public: arm: rework the macro set_xen_guest_handle_raw"):
> On 03/11/15 14:18, Stefano Stabellini wrote:
> >> +#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)

I hate to throw yet more variables into this, but I had a discussion
with some C experts in the pub about this problem.  Further thought
(including a red herring where someone suggested that memcpy would
"launder" the types) results in this proposal:


union xen_guest_handle_TYPE {
  TYPE *p;
  uint64_t align;
};

struct xen_guest_handle__pointer {
  uint8_t p0,p1,p2,p3;
};

/*
 * void set_xen_guest_handle_raw(xen_guest_handle_TYPE *hnd, TYPE *val);
 */
#define set_xen_guest_handle_raw(hnd, val)
  do {
     void *copy = (val);
     struct xen_guest_handle__pointer *src = &(hnd);
     struct xen_guest_handle__pointer *dst = &(xen_xgh_copy);
     dst->p0 = src->p0;
     dst->p1 = src->p1;
     dst->p2 = src->p2;
     dst->p3 = src->p3;
     dst[1]->p0 = 0;
     dst[1]->p1 = 0;
     dst[1]->p2 = 0;
     dst[1]->p3 = 0;
     sizeof((hnd).p == (val)); /* typecheck */
  }

#define get_xen_guest_handle(hnd) ((hnd).p)


The above is legal for the following reasons (C99 6.5):

Considering the accesses to src and dst.  These are legal according to
(7) point 5.  (Accesses via a character type are always legal.)

After the above macro has been used to set, what is the effective type
of hnd for a subsequent get_xen_guest_handle ?

If hnd is an object with a declared type (ie, an actual variable,
function parameter, or whatever, rather than from malloc), then the
effective type is that of hnd, so the effective type is the union, and
the effective type of hnd.p is TYPE*.

If hnd is not a declared object, then the effective type might be set
by one of the stores in (6): store through an non-character lvalue;
copy via memcpy or memmove; copy as `an array of character type'.

But we store only through character lvalues.  We do not store with
memcpy or memmove.  We do not copy as an array, but as a struct.

So none of the rules defining the effective type apply.  We are left
with the fallback: the effective type of the subsequent read is the
type used for access.

So a subsequent get's read of hnd.p is legal.


With luck the compiler's optimiser will eliminate the temporaries and
aggregate the byte-wide stores.


In the actual macro all of the struct and union fields and all of the
temporary variables should be prefixed with `xen_xgh_' or some such,
in case the calling code has (arguably foolishly) #defined src or dst
or p or something.

Ian.

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

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH 4/4] xen/public: arm: rework the macro set_xen_guest_handle_raw"):
>      void *copy = (val);
>      struct xen_guest_handle__pointer *src = &(hnd);
>      struct xen_guest_handle__pointer *dst = &(xen_xgh_copy);

I have my src and dst the wrong way round, and have used the a
variable name from a previous draft.  This should be:

>      struct xen_guest_handle__pointer *src = &(copy);
>      struct xen_guest_handle__pointer *dst = &(hnd);

Ian.

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

[Jan Beulich]

>>> On 04.11.15 at 17:22, <Ian.Jackson@eu.citrix.com> wrote:
> Julien Grall writes ("Re: [PATCH 4/4] xen/public: arm: rework the macro 
> set_xen_guest_handle_raw"):
>> On 03/11/15 14:18, Stefano Stabellini wrote:
>> >> +#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)
> 
> I hate to throw yet more variables into this, but I had a discussion
> with some C experts in the pub about this problem.  Further thought
> (including a red herring where someone suggested that memcpy would
> "launder" the types) results in this proposal:
> 
> 
> union xen_guest_handle_TYPE {
>   TYPE *p;
>   uint64_t align;
> };
> 
> struct xen_guest_handle__pointer {
>   uint8_t p0,p1,p2,p3;
> };
> 
> /*
>  * void set_xen_guest_handle_raw(xen_guest_handle_TYPE *hnd, TYPE *val);
>  */
> #define set_xen_guest_handle_raw(hnd, val)
>   do {
>      void *copy = (val);
>      struct xen_guest_handle__pointer *src = &(hnd);
>      struct xen_guest_handle__pointer *dst = &(xen_xgh_copy);
>      dst->p0 = src->p0;
>      dst->p1 = src->p1;
>      dst->p2 = src->p2;
>      dst->p3 = src->p3;
>      dst[1]->p0 = 0;
>      dst[1]->p1 = 0;
>      dst[1]->p2 = 0;
>      dst[1]->p3 = 0;
>      sizeof((hnd).p == (val)); /* typecheck */
>   }
> 
> #define get_xen_guest_handle(hnd) ((hnd).p)
> 
> 
> The above is legal for the following reasons (C99 6.5):
> 
> Considering the accesses to src and dst.  These are legal according to
> (7) point 5.  (Accesses via a character type are always legal.)
> 
> After the above macro has been used to set, what is the effective type
> of hnd for a subsequent get_xen_guest_handle ?

All quite interesting, but pretty moot with there not being any
get_xen_guest_handle() anymore.

Jan

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

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 4/4] xen/public: arm: rework the macro set_xen_guest_handle_raw"):
> All quite interesting, but pretty moot with there not being any
> get_xen_guest_handle() anymore.

If we don't provide a get_xen_guest_handle, a kernel developer will be
sorely tempted to make one.

If they do and they write it in the obvious way, then the compiler
could `deduce' that the top part of the uint64_t store was dead, and
eliminate it.

Ian.

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

[Julien Grall]

On 04/11/15 16:50, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH 4/4] xen/public: arm: rework the macro set_xen_guest_handle_raw"):
>> All quite interesting, but pretty moot with there not being any
>> get_xen_guest_handle() anymore.
> 
> If we don't provide a get_xen_guest_handle, a kernel developer will be
> sorely tempted to make one.
> 
> If they do and they write it in the obvious way, then the compiler
> could `deduce' that the top part of the uint64_t store was dead, and
> eliminate it.

The developers could also decide to rewrite the
set_xen_raw_guest_handle/get_xen_guest_handle in an obvious way because
they think ours it's too complicate... See the Linux version.

TBH, we can't protect ourself for a developer writing it own macro and
don't think about the big picture. The comment in the header pretty much
explain the constraint. If it doesn't read, well it's not our business.

Regards,

-- 
Julien Grall

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

[Jan Beulich]

>>> On 04.11.15 at 17:50, <Ian.Jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH 4/4] xen/public: arm: rework the macro 
> set_xen_guest_handle_raw"):
>> All quite interesting, but pretty moot with there not being any
>> get_xen_guest_handle() anymore.
> 
> If we don't provide a get_xen_guest_handle, a kernel developer will be
> sorely tempted to make one.

What use would it be to them? Kernels only write handles, they
shouldn't have a need for reading them.

Jan

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

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 4/4] xen/public: arm: rework the macro set_xen_guest_handle_raw"):
> On 04.11.15 at 17:50, <Ian.Jackson@eu.citrix.com> wrote:
> > If we don't provide a get_xen_guest_handle, a kernel developer will be
> > sorely tempted to make one.
> 
> What use would it be to them? Kernels only write handles, they
> shouldn't have a need for reading them.

I foresee situations where a kernel might like to update a proposed
hypercall argument structure in place, which might involve reading the
handles.

Ian.

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

[Jan Beulich]

>>> On 04.11.15 at 18:06, <Ian.Jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH 4/4] xen/public: arm: rework the macro 
> set_xen_guest_handle_raw"):
>> On 04.11.15 at 17:50, <Ian.Jackson@eu.citrix.com> wrote:
>> > If we don't provide a get_xen_guest_handle, a kernel developer will be
>> > sorely tempted to make one.
>> 
>> What use would it be to them? Kernels only write handles, they
>> shouldn't have a need for reading them.
> 
> I foresee situations where a kernel might like to update a proposed
> hypercall argument structure in place, which might involve reading the
> handles.

I guess you think of e.g. the privcmd filtering done in XenServer, but
I think this is an odd thing for a kernel to do: Down to the final actual
hypercall invocation, it should deal with pointers, not handles.
Filtering should either be done prior to reaching that layer (obviously
not an option for privcmd, but that layer is guarded against issues
with the compiler doing the wrong thing afaict), or would better be
left to the hypervisor (said filtering in XenServer could likely be moved
into the hypervisor, with a flag added to the hypercall number
indicating whether to invoke the filtering, which the privcmd layer
then would set unconditionally).

Jan

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

[Ian Campbell]

On Wed, 2015-11-04 at 16:50 +0000, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH 4/4] xen/public: arm: rework the
> macro set_xen_guest_handle_raw"):
> > All quite interesting, but pretty moot with there not being any
> > get_xen_guest_handle() anymore.
> 
> If we don't provide a get_xen_guest_handle, a kernel developer will be
> sorely tempted to make one.
> 
> If they do and they write it in the obvious way, then the compiler
> could `deduce' that the top part of the uint64_t store was dead, and
> eliminate it.

I realise that "correct use of unions" here is orthogonal to "using gcc
extensions", but trying to achieve the former while avoiding the later
is having a multiplicative effect on the complexity of the resulting
macros, so:...

Realistically any OS which did try and implement this would probably
not try and jump through such hoops in order to do it in pure ANSI-C
but would instead just use whatever extension their compiler provided
to make it easier to write such macros (such as typeof).

Perhaps we should follow the lead of xen/include/public/io/ring.h here
and provide a clear, readable and correct version under #if __GNUC__
(which our own tools build would use) using the extensions and an
alternative ANSI-C only one which might have short comings e.g. in
efficiency, maybe evaluating the argument more than once, etc but not
in correctness.

Realistically IMHO the #else case could be "#error you need to
implement this carefully" but I suspect others will not be comfortable
with that approach.

Ian.

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