Xen devel - Aug 2006 - uint64_aligned_t not compatible across gcc versions

gcc up to 3.3.x doesn''t honor __attribute__((__aligned(..))) on
typedef-s (and
in my opinion this was the correct behavior, as a typedef and its original type
must be fully exchangeable in all their uses). For this reason, I''d
recommend
changing the definition of this type on x86_32 with below patch.

Further looking into how it is being used and therefore into the uses of
XEN_GUEST_HANDLE_64 I am not really clear about the intentions of that
macro and its companions: Reserving a 64-bit slot for a pointer would seem
nice for doing the compatibility layer, but turns out useless if the 32-bit
native implementation doesn''t check the upper half of passed in values,
as
that implies that the 64-bit compatibility layer must not assume these upper
bits are all zero or else it wouldn''t be binary compatible.

Jan

Signed-off-by: Jan Beulich <jbeulich@novell.com>

Index: 2006-08-28/xen/include/public/arch-x86_32.h
==================================================================---
2006-08-28.orig/xen/include/public/arch-x86_32.h	2006-08-28 08:32:38.000000000
+0200
+++ 2006-08-28/xen/include/public/arch-x86_32.h	2006-08-28 16:35:11.000000000
+0200
@@ -29,12 +29,14 @@
 
 /* Structural guest handles introduced in 0x00030201. */
 #if (defined(__XEN__) || defined(__XEN_TOOLS__)) &&
!defined(__ASSEMBLY__)
-typedef uint64_t __attribute__((aligned(8))) uint64_aligned_t;
+#define uint64_aligned_t uint64_t __attribute__((__aligned__(8)))
 #define __DEFINE_XEN_GUEST_HANDLE(name, type)                   \
     typedef struct { type *p; }                                 \
         __guest_handle_ ## name;                                \
-    typedef struct { union { type *p; uint64_aligned_t q; }; }  \
-        __guest_handle_64_ ## name
+    typedef union {                                             \
+        type *p;                                                \
+        uint64_t __attribute__((__aligned__(8))) q;             \
+    } __guest_handle_64_ ## name
 #elif __XEN_INTERFACE_VERSION__ >= 0x00030201
 #define __DEFINE_XEN_GUEST_HANDLE(name, type) \
     typedef struct { type *p; } __guest_handle_ ## name


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

On 28/8/06 3:46 pm, "Jan Beulich" <jbeulich@novell.com> wrote:
> gcc up to 3.3.x doesn''t honor __attribute__((__aligned(..))) on
typedef-s (and
> in my opinion this was the correct behavior, as a typedef and its original
> type
> must be fully exchangeable in all their uses). For this reason,
I''d recommend
> changing the definition of this type on x86_32 with below patch.
Thanks, I did wonder.
> Further looking into how it is being used and therefore into the uses of
> XEN_GUEST_HANDLE_64 I am not really clear about the intentions of that
> macro and its companions: Reserving a 64-bit slot for a pointer would seem
> nice for doing the compatibility layer, but turns out useless if the 32-bit
> native implementation doesn''t check the upper half of passed in
values, as
> that implies that the 64-bit compatibility layer must not assume these
upper
> bits are all zero or else it wouldn''t be binary compatible.
Yes, the aim is to avoid the need for a 32-on-64 shim for the domctl and
sysctl hypercalls. I guess we''ll end up with a semi-automated scheme
for the
other hypercalls anyway, but since we don''t guarantee compatibility on
those
interfaces we may as well make our lives as easy as possible.

Guest handle fields are *only* written by the set_xen_guest_handle() macro
in the tools -- you''ll see that I modified that macro to zap the high
bits
if the field is 8 bytes. This should be sufficient?

 -- Keir



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

>> Further looking into how it is being used and therefore into the uses
of
>> XEN_GUEST_HANDLE_64 I am not really clear about the intentions of that
>> macro and its companions: Reserving a 64-bit slot for a pointer would
seem
>> nice for doing the compatibility layer, but turns out useless if the
32-bit
>> native implementation doesn''t check the upper half of passed
in values, as
>> that implies that the 64-bit compatibility layer must not assume these
upper
>> bits are all zero or else it wouldn''t be binary compatible.
>
>Yes, the aim is to avoid the need for a 32-on-64 shim for the domctl and
>sysctl hypercalls. I guess we''ll end up with a semi-automated
scheme for the
>other hypercalls anyway, but since we don''t guarantee compatibility
on those
>interfaces we may as well make our lives as easy as possible.
>
>Guest handle fields are *only* written by the set_xen_guest_handle() macro
>in the tools -- you''ll see that I modified that macro to zap the
high bits
This doesn''t seem to be that way: I didn''t do an exhaustive
search, but at
least struct sched_poll has a handle member and is being intended to be
called from the kernel. I''m also thinking that rather than inventing
yet
another abstraction, re-using the handle stuff for the few remaining
pointers in the public headers would be more appropriate (which is what
I''m actively doing at this moment).
>if the field is 8 bytes. This should be sufficient?
No, I wouldn''t want to trust the user mode tools in the hypervisor.
Hence I
see a need for checking the high bits in 32-bit native or not reading them in
64-bits compat mode.

Jan

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

On 28/8/06 4:32 pm, "Jan Beulich" <jbeulich@novell.com> wrote:
>> Guest handle fields are *only* written by the set_xen_guest_handle()
macro
>> in the tools -- you''ll see that I modified that macro to zap
the high bits
> 
> This doesn''t seem to be that way: I didn''t do an
exhaustive search, but at
> least struct sched_poll has a handle member and is being intended to be
> called from the kernel.
The kernel also uses set_xen_guest_handle in all cases. This is checked by
the compiler because guest handles are structural types and so cannot be
directly written with a scalar value.
> I''m also thinking that rather than inventing yet
> another abstraction, re-using the handle stuff for the few remaining
> pointers in the public headers would be more appropriate (which is what
> I''m actively doing at this moment).
This isn''t a new abstraction. All the old accessor functions work as
they
always did on the Xen and guest sides of the interface. It''s just a
change
of size and alignment -- an implementation detail from most p.o.v.

Changing the non-handles to be handles is debatable. Those pointers are in
interfaces that need backward API and ABI compatibility. I guess it depends
how much effort it saves in trying to deal with them specially in a script.
>> if the field is 8 bytes. This should be sufficient?
> 
> No, I wouldn''t want to trust the user mode tools in the
hypervisor. Hence I
> see a need for checking the high bits in 32-bit native or not reading them
in
> 64-bits compat mode.
Well, it''s not really an issue of trust imo. In this respect they can
only
hurt themselves by passing a ''dirty'' 64-bit field that fails
validation
checks on 64-bit Xen. And it''s unlikely there''ll be a
programmer error here
because it would be difficult to initialise a guest-handle field without
using the correct macro.

OTOH, extra checking rarely hurts and would be easy to add.

 -- Keir



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

One more point here: The addition of this type and XEN_GUEST_HANDLE_64(),
as it turns out, makes things more complicated/unreliable in the compatibility
stuff rather then helping the situation: Since we need to force 4-byte
alignment on uint64_t (and possible derived types) fields, we have to use
#pragma pack() framing the entire (generated) compatibility headers.
However, #pragma pack() takes precendence over attribute((aligned())), and
hence there is no easy way to force 8-byte alignment on uint64_aligned_t
fields. The only option I currently have is to
(a) exclude sysctl.h and domctl.h from the processing (which is probably
intended, but opens the possibility of unintentionally introducing fields of
types that need translation, which would go undetected at build time), and
(b) assume that no other header uses this type, or depends on the 8-byte
alignment forced on __guest_handle_64_XXX.
Both are maintenance-wise bad in my opinion. Ideas?

Jan

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

>OTOH, extra checking rarely hurts and would be easy to add.
Here my take at it. This actually also replaces the patch sent under
the same subject yesterday, as I meanwhile realized there''s a simpler
way to achieve the desired effect. It also converts the 64-bit store
to a 32-bit one, as only the upper 32 bits need clearing.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

Index: 2006-08-28/xen/include/public/arch-x86_32.h
==================================================================---
2006-08-28.orig/xen/include/public/arch-x86_32.h	2006-08-28 08:32:38.000000000
+0200
+++ 2006-08-28/xen/include/public/arch-x86_32.h	2006-08-29 11:15:44.000000000
+0200
@@ -29,11 +29,11 @@
 
 /* Structural guest handles introduced in 0x00030201. */
 #if (defined(__XEN__) || defined(__XEN_TOOLS__)) &&
!defined(__ASSEMBLY__)
-typedef uint64_t __attribute__((aligned(8))) uint64_aligned_t;
+#define uint64_aligned_t uint64_t __attribute__((__aligned__(8)))
 #define __DEFINE_XEN_GUEST_HANDLE(name, type)                   \
     typedef struct { type *p; }                                 \
         __guest_handle_ ## name;                                \
-    typedef struct { union { type *p; uint64_aligned_t q; }; }  \
+    typedef struct { type *p __attribute__((__aligned__(8))); } \
         __guest_handle_64_ ## name
 #elif __XEN_INTERFACE_VERSION__ >= 0x00030201
 #define __DEFINE_XEN_GUEST_HANDLE(name, type) \
@@ -49,7 +49,7 @@ typedef uint64_t __attribute__((aligned(
 #ifdef __XEN_TOOLS__
 #define get_xen_guest_handle(val, hnd)  do { val = (hnd).p; } while (0)
 #define set_xen_guest_handle(hnd, val)                      \
-    do { if ( sizeof(hnd) == 8 ) *(uint64_t *)&(hnd) = 0;   \
+    do { if ( sizeof(hnd) == 8 ) (&(hnd).p)[1] = NULL;      \
          (hnd).p = val;                                     \
     } while ( 0 )
 #else
Index: 2006-08-28/xen/include/asm-x86/guest_access.h
==================================================================---
2006-08-28.orig/xen/include/asm-x86/guest_access.h	2006-08-07 09:07:03.000000000
+0200
+++ 2006-08-28/xen/include/asm-x86/guest_access.h	2006-08-29 11:52:57.000000000
+0200
@@ -17,6 +17,14 @@
 /* Offset the given guest handle into the array it refers to. */
 #define guest_handle_add_offset(hnd, nr) ((hnd).p += (nr))
 
+#if defined(__i386__)
+#define __guest_handle_okay(hnd)                \
+    (sizeof(hnd) == sizeof((hnd).p) ||          \
+     (&(hnd).p)[1] == NULL)
+#elif defined(__x86_64__)
+#define __guest_handle_okay(hnd) ((void)(hnd), 1)
+#endif
+
 /* Cast a guest handle to the specified type of handle. */
 #define guest_handle_cast(hnd, type) ({         \
     type *_x = (hnd).p;                         \
@@ -33,9 +41,11 @@
 #define copy_to_guest_offset(hnd, off, ptr, nr) ({      \
     const typeof(ptr) _x = (hnd).p;                     \
     const typeof(ptr) _y = (ptr);                       \
+    __guest_handle_okay(hnd) ?                          \
     hvm_guest(current) ?                                \
     copy_to_user_hvm(_x+(off), _y, sizeof(*_x)*(nr)) :  \
-    copy_to_user(_x+(off), _y, sizeof(*_x)*(nr));       \
+    copy_to_user(_x+(off), _y, sizeof(*_x)*(nr)) :      \
+    sizeof(*_x) * (nr);                                 \
 })
 
 /*
@@ -45,27 +55,33 @@
 #define copy_from_guest_offset(ptr, hnd, off, nr) ({    \
     const typeof(ptr) _x = (hnd).p;                     \
     const typeof(ptr) _y = (ptr);                       \
+    __guest_handle_okay(hnd) ?                          \
     hvm_guest(current) ?                                \
     copy_from_user_hvm(_y, _x+(off), sizeof(*_x)*(nr)) :\
-    copy_from_user(_y, _x+(off), sizeof(*_x)*(nr));     \
+    copy_from_user(_y, _x+(off), sizeof(*_x)*(nr)) :    \
+    sizeof(*_x) * (nr);                                 \
 })
 
 /* Copy sub-field of a structure to guest context via a guest handle. */
 #define copy_field_to_guest(hnd, ptr, field) ({         \
     const typeof(&(ptr)->field) _x = &(hnd).p->field;   \
     const typeof(&(ptr)->field) _y = &(ptr)->field;     \
+    __guest_handle_okay(hnd) ?                          \
     hvm_guest(current) ?                                \
     copy_to_user_hvm(_x, _y, sizeof(*_x)) :             \
-    copy_to_user(_x, _y, sizeof(*_x));                  \
+    copy_to_user(_x, _y, sizeof(*_x)) :                 \
+    sizeof(*_x);                                        \
 })
 
 /* Copy sub-field of a structure from guest context via a guest handle. */
 #define copy_field_from_guest(ptr, hnd, field) ({       \
     const typeof(&(ptr)->field) _x = &(hnd).p->field;   \
     const typeof(&(ptr)->field) _y = &(ptr)->field;     \
+    __guest_handle_okay(hnd) ?                          \
     hvm_guest(current) ?                                \
     copy_from_user_hvm(_y, _x, sizeof(*_x)) :           \
-    copy_from_user(_y, _x, sizeof(*_x));                \
+    copy_from_user(_y, _x, sizeof(*_x)) :               \
+    sizeof(*_x);                                        \
 })
 
 /*
@@ -73,7 +89,9 @@
  * Allows use of faster __copy_* functions.
  */
 #define guest_handle_okay(hnd, nr)                      \
-    (hvm_guest(current) || array_access_ok((hnd).p, (nr), sizeof(*(hnd).p)))
+    (__guest_handle_okay(hnd) &&                        \
+     (hvm_guest(current) ||                             \
+      array_access_ok((hnd).p, (nr), sizeof(*(hnd).p))))
 
 #define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
     const typeof(ptr) _x = (hnd).p;                     \



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

On Tue, Aug 29, 2006 at 12:13:13PM +0200, Jan Beulich wrote:
> Here my take at it. This actually also replaces the patch sent under
> the same subject yesterday, as I meanwhile realized there''s a
simpler
> way to achieve the desired effect. It also converts the 64-bit store
> to a 32-bit one, as only the upper 32 bits need clearing.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> 
> Index: 2006-08-28/xen/include/public/arch-x86_32.h
> ==================================================================> ---
2006-08-28.orig/xen/include/public/arch-x86_32.h	2006-08-28 08:32:38.000000000
+0200
> +++ 2006-08-28/xen/include/public/arch-x86_32.h	2006-08-29
11:15:44.000000000 +0200
> @@ -29,11 +29,11 @@
>  
>  /* Structural guest handles introduced in 0x00030201. */
>  #if (defined(__XEN__) || defined(__XEN_TOOLS__)) &&
!defined(__ASSEMBLY__)
> -typedef uint64_t __attribute__((aligned(8))) uint64_aligned_t;
> +#define uint64_aligned_t uint64_t __attribute__((__aligned__(8)))
This is GCC-specific, in a public header. At the very least it needs to
be conditional on gcc being used (#define __xen_aligned or whatever)...

regards
john

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

>>> John Levon <levon@movementarian.org> 29.08.06 14:38
>>>
>On Tue, Aug 29, 2006 at 12:13:13PM +0200, Jan Beulich wrote:
>
>> Here my take at it. This actually also replaces the patch sent under
>> the same subject yesterday, as I meanwhile realized there''s a
simpler
>> way to achieve the desired effect. It also converts the 64-bit store
>> to a 32-bit one, as only the upper 32 bits need clearing.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>> 
>> Index: 2006-08-28/xen/include/public/arch-x86_32.h
>>
==================================================================>> ---
2006-08-28.orig/xen/include/public/arch-x86_32.h	2006-08-28 08:32:38.000000000
+0200
>> +++ 2006-08-28/xen/include/public/arch-x86_32.h	2006-08-29
11:15:44.000000000 +0200
>> @@ -29,11 +29,11 @@
>>  
>>  /* Structural guest handles introduced in 0x00030201. */
>>  #if (defined(__XEN__) || defined(__XEN_TOOLS__)) &&
!defined(__ASSEMBLY__)
>> -typedef uint64_t __attribute__((aligned(8))) uint64_aligned_t;
>> +#define uint64_aligned_t uint64_t __attribute__((__aligned__(8)))
>
>This is GCC-specific, in a public header. At the very least it needs to
>be conditional on gcc being used (#define __xen_aligned or whatever)...
That is a comment regarding the original change, then. I still agree this
should be abstracted out in some way - the question is how, since I
guess users of other compilers would need to provide input.

Jan

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

On 29/8/06 9:49 am, "Jan Beulich" <jbeulich@novell.com> wrote:
> One more point here: The addition of this type and XEN_GUEST_HANDLE_64(),
> as it turns out, makes things more complicated/unreliable in the
compatibility
> stuff rather then helping the situation: Since we need to force 4-byte
> alignment on uint64_t (and possible derived types) fields, we have to use
> #pragma pack() framing the entire (generated) compatibility headers.
> However, #pragma pack() takes precendence over attribute((aligned())), and
> hence there is no easy way to force 8-byte alignment on uint64_aligned_t
> fields.
Depending how smart the script is, if it tracks required offset of fields in
the structure definitions it is creating, it could insert explicit padding
to force the required alignment.

As for ensuring sysctl/domctl alignments are same for 32- and 64-bit builds,
we''d anyway like a script that would generate a C program that would
print
field offsets/sizes for all public structures anyway (so that we can be sure
that none change and break the ABI). If we had that, we could compile the C
program -m32 and -m64 for sysctl.h and domctl.h and ensure the outputs are
identical. I think this would be an ideal solution -- but it does assume
existence of a tool that doesn''t exist. :-)

Alternative is to say ''screw it'' and just treat the
sysctl/domctl headers
like any other, and remove the explicit alignment stuff before we fork
3.0.3. Domctl in particular is a *big* interface though, so it''d be
nice to
avoid needing to generate (much) compat code for it.

 -- Keir
> (a) exclude sysctl.h and domctl.h from the processing (which is probably
> intended, but opens the possibility of unintentionally introducing fields
of
> types that need translation, which would go undetected at build time), and
> (b) assume that no other header uses this type, or depends on the 8-byte
> alignment forced on __guest_handle_64_XXX.
> Both are maintenance-wise bad in my opinion. Ideas?



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

On 29/8/06 1:46 pm, "Jan Beulich" <jbeulich@novell.com> wrote:
>>>  /* Structural guest handles introduced in 0x00030201. */
>>>  #if (defined(__XEN__) || defined(__XEN_TOOLS__)) &&
!defined(__ASSEMBLY__)
>>> -typedef uint64_t __attribute__((aligned(8))) uint64_aligned_t;
>>> +#define uint64_aligned_t uint64_t __attribute__((__aligned__(8)))
>> 
>> This is GCC-specific, in a public header. At the very least it needs to
>> be conditional on gcc being used (#define __xen_aligned or whatever)...
> 
> That is a comment regarding the original change, then. I still agree this
> should be abstracted out in some way - the question is how, since I
> guess users of other compilers would need to provide input.
I''ll admit there''s still the question of whether this is
worthwhile for just
these two hypercalls in the first place. Jan: do you think much code will be
saved by explicit alignment for domctl/sysctl, or do you think we''re
just as
well to remove uint64_aligned_t and XEN_GUEST_HANDLE_64, and do compat shims
for domctl/sysctl just as we are for all other hypercalls?

 -- Keir



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

>>> Keir Fraser <Keir.Fraser@cl.cam.ac.uk> 30.08.06 18:18
>>>
>On 29/8/06 9:49 am, "Jan Beulich" <jbeulich@novell.com>
wrote:
>
>> One more point here: The addition of this type and
XEN_GUEST_HANDLE_64(),
>> as it turns out, makes things more complicated/unreliable in the
compatibility
>> stuff rather then helping the situation: Since we need to force 4-byte
>> alignment on uint64_t (and possible derived types) fields, we have to
use
>> #pragma pack() framing the entire (generated) compatibility headers.
>> However, #pragma pack() takes precendence over attribute((aligned())),
and
>> hence there is no easy way to force 8-byte alignment on
uint64_aligned_t
>> fields.
>
>Depending how smart the script is, if it tracks required offset of fields in
>the structure definitions it is creating, it could insert explicit padding
>to force the required alignment.
In my opinion that would be too much smartness (and too much complexity
to maintain) for a compatibility solution.
>As for ensuring sysctl/domctl alignments are same for 32- and 64-bit builds,
>we''d anyway like a script that would generate a C program that
would print
>field offsets/sizes for all public structures anyway (so that we can be sure
>that none change and break the ABI). If we had that, we could compile the C
>program -m32 and -m64 for sysctl.h and domctl.h and ensure the outputs are
>identical. I think this would be an ideal solution -- but it does assume
>existence of a tool that doesn''t exist. :-)
I like that, and would be willing to try to derive such a mechanism from the
scripts I''m already having to deal with the public headers (once that
larger
piece of work is [mostly] done).
>Alternative is to say ''screw it'' and just treat the
sysctl/domctl headers
>like any other, and remove the explicit alignment stuff before we fork
>3.0.3. Domctl in particular is a *big* interface though, so it''d be
nice to
>avoid needing to generate (much) compat code for it.
Agreed.

Jan

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

>I''ll admit there''s still the question of whether this is
worthwhile for just
>these two hypercalls in the first place. Jan: do you think much code will be
>saved by explicit alignment for domctl/sysctl, or do you think
we''re just as
>well to remove uint64_aligned_t and XEN_GUEST_HANDLE_64, and do compat shims
>for domctl/sysctl just as we are for all other hypercalls?
Depends on what you mean by code saving - source code or binary size.
The former shouldn''t be too much (a simple function with mostly auto-
generated body per translated (sub-)structure), the latter might be
significant (a recompiled version of any non-translated hypercall). Which
variant to use for sysctl and domctl I haven''t even started to think
about
yet.

But as said in the other mail - it would seem to me that overall it''d
be
better to not have this construct.

Jan

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

On 31/8/06 7:41 am, "Jan Beulich" <jbeulich@novell.com> wrote:
> I like that, and would be willing to try to derive such a mechanism from
> the
> scripts I''m already having to deal with the public headers (once
that
> larger
> piece of work is [mostly] done).
Okay, following your advice I''ll get rid of the explicit alignment/size
stuff from domctl/sysctl. But it *would* still be nice to have the script
that prints field offsets/sizes -- for the originally stated purpose of
ensuring that we don''t accidentally break ABI compatibility. If it
isn''t too
much hassle, it would be great if you could bend your scripts to that
purpose.

 Thanks,
 Keir
>> Alternative is to say ''screw it'' and just treat the
sysctl/domctl
> headers
>> like any other, and remove the explicit alignment stuff before we fork
>> 3.0.3. Domctl in particular is a *big* interface though, so
it''d be
> nice to
>> avoid needing to generate (much) compat code for it.
> 
> Agreed.


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