[PATCH 0/5] Improvements with __attribute__((packed))

[PATCH 0/5] Improvements with __attribute__((packed))

[Andrew Cooper]

Due to a lack of suitable define in scope, mce-apei.c ends up accidentally
creating a global unreferenced struct of type 'cper_mce_record', named
'__packed'.

This series is a cleanup of all use of __attribute__((packed)) in Xen.  A
formal define is created in compiler.h, all opencoded uses of the attribute
are updated to use the new define (or dropped if unnecessary in the first
place), and the position of __packed is standardised at the beginning of the
struct rather than the end, so a lack of __packed in scope will result in a
compile error.

Patch 1 drops some redundant uses of __attribute__((packed)), and is verified
by diffing xen-syms.

Patches 2 through 4 make the main changes, and are split up by core component
for the benefit of the maintainers/reviewers.

Patch 5 is only for people wishing to verify my assertions.  The entire
series including patch 5 compiles to identical binaries, proving that the
reverse contents of patch 5 are the only semantic differences across the
series.

This is compile tested on each architecture and functionally tested on x86.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

[PATCH 1/5] xen: Remove redundant __attribute__((packed)) statements

[Andrew Cooper]

All of these structure packing statements are redundant, and was confirmed by
diffing xen-syms with and without the change in place.

Removing struct xgt_desc in its entirety (as it was unreferenced) results in
shuffling the symbol index, but makes no change to the code.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Eddie Dong <eddie.dong@intel.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c       |    5 -----
 xen/arch/x86/oprofile/backtrace.c |    4 ++--
 xen/arch/x86/trace.c              |    6 +++---
 xen/common/trace.c                |    2 +-
 xen/drivers/char/ehci-dbgp.c      |    4 ++--
 xen/include/asm-x86/edd.h         |   30 +++++++++++++++---------------
 6 files changed, 23 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 9ffb4af..4b886e5 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -670,11 +670,6 @@ void vmx_vmcs_exit(struct vcpu *v)
     }
 }
 
-struct xgt_desc {
-    unsigned short size;
-    unsigned long address __attribute__((packed));
-};
-
 static void vmx_set_host_env(struct vcpu *v)
 {
     unsigned int cpu = smp_processor_id();
diff --git a/xen/arch/x86/oprofile/backtrace.c b/xen/arch/x86/oprofile/backtrace.c
index b3ea7f3..354ebff 100644
--- a/xen/arch/x86/oprofile/backtrace.c
+++ b/xen/arch/x86/oprofile/backtrace.c
@@ -18,14 +18,14 @@
 struct frame_head {
     struct frame_head * ebp;
     unsigned long ret;
-} __attribute__((packed));
+};
 typedef struct frame_head frame_head_t;
 DEFINE_XEN_GUEST_HANDLE(frame_head_t);
 
 struct frame_head_32bit {
     uint32_t ebp;
     uint32_t ret;
-} __attribute__((packed));
+};
 typedef struct frame_head_32bit frame_head32_t;
 DEFINE_COMPAT_HANDLE(frame_head32_t);
 
diff --git a/xen/arch/x86/trace.c b/xen/arch/x86/trace.c
index b1804a4..652c4e3 100644
--- a/xen/arch/x86/trace.c
+++ b/xen/arch/x86/trace.c
@@ -127,7 +127,7 @@ void __trace_trap_two_addr(unsigned event, unsigned long va1,
     {
         struct {
             u32 va1, va2;
-        } __attribute__((packed)) d;
+        } d;
         d.va1=va1;
         d.va2=va2;
         __trace_var(event, 1, sizeof(d), &d);
@@ -136,7 +136,7 @@ void __trace_trap_two_addr(unsigned event, unsigned long va1,
     {
         struct {
             unsigned long va1, va2;
-        } __attribute__((packed)) d;
+        } d;
         d.va1=va1;
         d.va2=va2;
         event |= TRC_64_FLAG;
@@ -161,7 +161,7 @@ void __trace_ptwr_emulation(unsigned long addr, l1_pgentry_t npte)
         struct {
             l1_pgentry_t pte;
             u32 addr, eip;
-        } __attribute__((packed)) d;
+        } d;
         d.addr = addr;
         d.eip = eip;
         d.pte = npte;
diff --git a/xen/common/trace.c b/xen/common/trace.c
index 41ddc33..dde662f 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -822,7 +822,7 @@ void __trace_hypercall(uint32_t event, unsigned long op,
     struct {
         uint32_t op;
         uint32_t args[6];
-    } __attribute__((packed)) d;
+    } d;
     uint32_t *a = d.args;
 
 #define APPEND_ARG32(i)                         \
diff --git a/xen/drivers/char/ehci-dbgp.c b/xen/drivers/char/ehci-dbgp.c
index b900d60..61417bd 100644
--- a/xen/drivers/char/ehci-dbgp.c
+++ b/xen/drivers/char/ehci-dbgp.c
@@ -257,7 +257,7 @@ struct usb_ctrlrequest {
     __le16 wValue;
     __le16 wIndex;
     __le16 wLength;
-} __attribute__ ((packed));
+};
 
 /* USB_DT_DEBUG: for special highspeed devices, replacing serial console */
 
@@ -269,7 +269,7 @@ struct usb_debug_descriptor {
     /* bulk endpoints with 8 byte maxpacket */
     u8 bDebugInEndpoint;
     u8 bDebugOutEndpoint;
-} __attribute__((packed));
+};
 
 #define USB_DEBUG_DEVNUM 127
 
diff --git a/xen/include/asm-x86/edd.h b/xen/include/asm-x86/edd.h
index 5544ba5..57d4bff 100644
--- a/xen/include/asm-x86/edd.h
+++ b/xen/include/asm-x86/edd.h
@@ -55,27 +55,27 @@ struct edd_info {
                 u16 base_address;
                 u16 reserved1;
                 u32 reserved2;
-            } __attribute__ ((packed)) isa;
+            } isa;
             struct {
                 u8 bus;
                 u8 slot;
                 u8 function;
                 u8 channel;
                 u32 reserved;
-            } __attribute__ ((packed)) pci;
+            } pci;
             /* pcix is same as pci */
             struct {
                 u64 reserved;
-            } __attribute__ ((packed)) ibnd;
+            } ibnd;
             struct {
                 u64 reserved;
-            } __attribute__ ((packed)) xprs;
+            } xprs;
             struct {
                 u64 reserved;
-            } __attribute__ ((packed)) htpt;
+            } htpt;
             struct {
                 u64 reserved;
-            } __attribute__ ((packed)) unknown;
+            } unknown;
         } interface_path;
         union {
             struct {
@@ -84,7 +84,7 @@ struct edd_info {
                 u16 reserved2;
                 u32 reserved3;
                 u64 reserved4;
-            } __attribute__ ((packed)) ata;
+            } ata;
             struct {
                 u8 device;
                 u8 lun;
@@ -92,7 +92,7 @@ struct edd_info {
                 u8 reserved2;
                 u32 reserved3;
                 u64 reserved4;
-            } __attribute__ ((packed)) atapi;
+            } atapi;
             struct {
                 u16 id;
                 u64 lun;
@@ -102,35 +102,35 @@ struct edd_info {
             struct {
                 u64 serial_number;
                 u64 reserved;
-            } __attribute__ ((packed)) usb;
+            } usb;
             struct {
                 u64 eui;
                 u64 reserved;
-            } __attribute__ ((packed)) i1394;
+            } i1394;
             struct {
                 u64 wwid;
                 u64 lun;
-            } __attribute__ ((packed)) fibre;
+            } fibre;
             struct {
                 u64 identity_tag;
                 u64 reserved;
-            } __attribute__ ((packed)) i2o;
+            } i2o;
             struct {
                 u32 array_number;
                 u32 reserved1;
                 u64 reserved2;
-            } __attribute__ ((packed)) raid;
+            } raid;
             struct {
                 u8 device;
                 u8 reserved1;
                 u16 reserved2;
                 u32 reserved3;
                 u64 reserved4;
-            } __attribute__ ((packed)) sata;
+            } sata;
             struct {
                 u64 reserved1;
                 u64 reserved2;
-            } __attribute__ ((packed)) unknown;
+            } unknown;
         } device_path;
         u8 reserved4;
         u8 checksum;
-- 
1.7.10.4

Re: [PATCH 1/5] xen: Remove redundant __attribute__((packed)) statements

[Jan Beulich]

>>> On 12.03.14 at 20:08, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/xen/drivers/char/ehci-dbgp.c
> +++ b/xen/drivers/char/ehci-dbgp.c
> @@ -257,7 +257,7 @@ struct usb_ctrlrequest {
>      __le16 wValue;
>      __le16 wIndex;
>      __le16 wLength;
> -} __attribute__ ((packed));
> +};
>  
>  /* USB_DT_DEBUG: for special highspeed devices, replacing serial console */
>  
> @@ -269,7 +269,7 @@ struct usb_debug_descriptor {
>      /* bulk endpoints with 8 byte maxpacket */
>      u8 bDebugInEndpoint;
>      u8 bDebugOutEndpoint;
> -} __attribute__((packed));
> +};

While correct, I'm not sure we want to drop these ...

> --- a/xen/include/asm-x86/edd.h
> +++ b/xen/include/asm-x86/edd.h
> @@ -55,27 +55,27 @@ struct edd_info {
>                  u16 base_address;
>                  u16 reserved1;
>                  u32 reserved2;
> -            } __attribute__ ((packed)) isa;
> +            } isa;
>              struct {
>                  u8 bus;
>                  u8 slot;
>                  u8 function;
>                  u8 channel;
>                  u32 reserved;
> -            } __attribute__ ((packed)) pci;
> +            } pci;
>              /* pcix is same as pci */
>              struct {
>                  u64 reserved;
> -            } __attribute__ ((packed)) ibnd;
> +            } ibnd;
>              struct {
>                  u64 reserved;
> -            } __attribute__ ((packed)) xprs;
> +            } xprs;
>              struct {
>                  u64 reserved;
> -            } __attribute__ ((packed)) htpt;
> +            } htpt;
>              struct {
>                  u64 reserved;
> -            } __attribute__ ((packed)) unknown;
> +            } unknown;
>          } interface_path;
>          union {
>              struct {
> @@ -84,7 +84,7 @@ struct edd_info {
>                  u16 reserved2;
>                  u32 reserved3;
>                  u64 reserved4;
> -            } __attribute__ ((packed)) ata;
> +            } ata;
>              struct {
>                  u8 device;
>                  u8 lun;
> @@ -92,7 +92,7 @@ struct edd_info {
>                  u8 reserved2;
>                  u32 reserved3;
>                  u64 reserved4;
> -            } __attribute__ ((packed)) atapi;
> +            } atapi;
>              struct {
>                  u16 id;
>                  u64 lun;
> @@ -102,35 +102,35 @@ struct edd_info {
>              struct {
>                  u64 serial_number;
>                  u64 reserved;
> -            } __attribute__ ((packed)) usb;
> +            } usb;
>              struct {
>                  u64 eui;
>                  u64 reserved;
> -            } __attribute__ ((packed)) i1394;
> +            } i1394;
>              struct {
>                  u64 wwid;
>                  u64 lun;
> -            } __attribute__ ((packed)) fibre;
> +            } fibre;
>              struct {
>                  u64 identity_tag;
>                  u64 reserved;
> -            } __attribute__ ((packed)) i2o;
> +            } i2o;
>              struct {
>                  u32 array_number;
>                  u32 reserved1;
>                  u64 reserved2;
> -            } __attribute__ ((packed)) raid;
> +            } raid;
>              struct {
>                  u8 device;
>                  u8 reserved1;
>                  u16 reserved2;
>                  u32 reserved3;
>                  u64 reserved4;
> -            } __attribute__ ((packed)) sata;
> +            } sata;
>              struct {
>                  u64 reserved1;
>                  u64 reserved2;
> -            } __attribute__ ((packed)) unknown;
> +            } unknown;
>          } device_path;
>          u8 reserved4;
>          u8 checksum;

... and these - they're serving a documentation purpose.
Furthermore, in the latter case you (correctly) left the "scsi"
sub-structure unchanged, resulting in an inconsistency with
all other sub-structures.

Jan

Re: [PATCH 1/5] xen: Remove redundant __attribute__((packed)) statements

[Andrew Cooper]

On 13/03/14 08:06, Jan Beulich wrote:
>>>> On 12.03.14 at 20:08, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/drivers/char/ehci-dbgp.c
>> +++ b/xen/drivers/char/ehci-dbgp.c
>> @@ -257,7 +257,7 @@ struct usb_ctrlrequest {
>>      __le16 wValue;
>>      __le16 wIndex;
>>      __le16 wLength;
>> -} __attribute__ ((packed));
>> +};
>>  
>>  /* USB_DT_DEBUG: for special highspeed devices, replacing serial console */
>>  
>> @@ -269,7 +269,7 @@ struct usb_debug_descriptor {
>>      /* bulk endpoints with 8 byte maxpacket */
>>      u8 bDebugInEndpoint;
>>      u8 bDebugOutEndpoint;
>> -} __attribute__((packed));
>> +};
> While correct, I'm not sure we want to drop these ...

Ok

>
>> --- a/xen/include/asm-x86/edd.h
>> +++ b/xen/include/asm-x86/edd.h
>> @@ -55,27 +55,27 @@ struct edd_info {
>>                  u16 base_address;
>>                  u16 reserved1;
>>                  u32 reserved2;
>> -            } __attribute__ ((packed)) isa;
>> +            } isa;
>>              struct {
>>                  u8 bus;
>>                  u8 slot;
>>                  u8 function;
>>                  u8 channel;
>>                  u32 reserved;
>> -            } __attribute__ ((packed)) pci;
>> +            } pci;
>>              /* pcix is same as pci */
>>              struct {
>>                  u64 reserved;
>> -            } __attribute__ ((packed)) ibnd;
>> +            } ibnd;
>>              struct {
>>                  u64 reserved;
>> -            } __attribute__ ((packed)) xprs;
>> +            } xprs;
>>              struct {
>>                  u64 reserved;
>> -            } __attribute__ ((packed)) htpt;
>> +            } htpt;
>>              struct {
>>                  u64 reserved;
>> -            } __attribute__ ((packed)) unknown;
>> +            } unknown;
>>          } interface_path;
>>          union {
>>              struct {
>> @@ -84,7 +84,7 @@ struct edd_info {
>>                  u16 reserved2;
>>                  u32 reserved3;
>>                  u64 reserved4;
>> -            } __attribute__ ((packed)) ata;
>> +            } ata;
>>              struct {
>>                  u8 device;
>>                  u8 lun;
>> @@ -92,7 +92,7 @@ struct edd_info {
>>                  u8 reserved2;
>>                  u32 reserved3;
>>                  u64 reserved4;
>> -            } __attribute__ ((packed)) atapi;
>> +            } atapi;
>>              struct {
>>                  u16 id;
>>                  u64 lun;
>> @@ -102,35 +102,35 @@ struct edd_info {
>>              struct {
>>                  u64 serial_number;
>>                  u64 reserved;
>> -            } __attribute__ ((packed)) usb;
>> +            } usb;
>>              struct {
>>                  u64 eui;
>>                  u64 reserved;
>> -            } __attribute__ ((packed)) i1394;
>> +            } i1394;
>>              struct {
>>                  u64 wwid;
>>                  u64 lun;
>> -            } __attribute__ ((packed)) fibre;
>> +            } fibre;
>>              struct {
>>                  u64 identity_tag;
>>                  u64 reserved;
>> -            } __attribute__ ((packed)) i2o;
>> +            } i2o;
>>              struct {
>>                  u32 array_number;
>>                  u32 reserved1;
>>                  u64 reserved2;
>> -            } __attribute__ ((packed)) raid;
>> +            } raid;
>>              struct {
>>                  u8 device;
>>                  u8 reserved1;
>>                  u16 reserved2;
>>                  u32 reserved3;
>>                  u64 reserved4;
>> -            } __attribute__ ((packed)) sata;
>> +            } sata;
>>              struct {
>>                  u64 reserved1;
>>                  u64 reserved2;
>> -            } __attribute__ ((packed)) unknown;
>> +            } unknown;
>>          } device_path;
>>          u8 reserved4;
>>          u8 checksum;
> ... and these - they're serving a documentation purpose.
> Furthermore, in the latter case you (correctly) left the "scsi"
> sub-structure unchanged, resulting in an inconsistency with
> all other sub-structures.
>
> Jan

As a hardware interface, I will leave the all as-were.

~Andrew

Re: [PATCH 1/5] xen: Remove redundant __attribute__((packed)) statements

[George Dunlap]

On 03/12/2014 07:08 PM, Andrew Cooper wrote:
> All of these structure packing statements are redundant, and was confirmed by
> diffing xen-syms with and without the change in place.
>
> Removing struct xgt_desc in its entirety (as it was unreferenced) results in
> shuffling the symbol index, but makes no change to the code.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I guess I'm becoming a crotchety old maintainer; at least re the tracing 
code, I don't see how the cost -- your initial effort in figuring out 
which structures need packing, the cost of review time, and the risk of 
regression, no matter how small -- is worth the benefit.  It seems to me 
like having all the tracing structures tagged with "__packed" will 
remind people to think about packing when they're writing or modifying 
structures.

But if Jan and/or Keir think it's a good idea, and are satisfied that 
it's correct, I won't oppose it.

  -George

[PATCH 2/5] xen/common: Cleanup use of __attribute__((packed))

[Andrew Cooper]

And standardise on having the statement at the head of the struct/union rather
than at the tail.

This introduced a formal define in compiler.h, and contains some mechanical
shuffling in common code.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/common/trace.c         |    4 ++--
 xen/common/unlzma.c        |    4 ++--
 xen/include/xen/compat.h   |    2 +-
 xen/include/xen/compiler.h |    2 ++
 4 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/xen/common/trace.c b/xen/common/trace.c
index dde662f..fa5c6ae 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -641,11 +641,11 @@ static inline void insert_wrap_record(struct t_buf *buf,
 
 static inline void insert_lost_records(struct t_buf *buf)
 {
-    struct {
+    struct __packed {
         u32 lost_records;
         u32 did:16, vid:16;
         u64 first_tsc;
-    } __attribute__((packed)) ed;
+    } ed;
 
     ed.vid = current->vcpu_id;
     ed.did = current->domain->domain_id;
diff --git a/xen/common/unlzma.c b/xen/common/unlzma.c
index 4d04330..103d2df 100644
--- a/xen/common/unlzma.c
+++ b/xen/common/unlzma.c
@@ -214,11 +214,11 @@ rc_bit_tree_decode(struct rc *rc, uint16_t *p, int num_levels, int *symbol)
  */
 
 
-struct lzma_header {
+struct __packed lzma_header {
 	uint8_t pos;
 	uint32_t dict_size;
 	uint64_t dst_size;
-} __attribute__ ((packed)) ;
+};
 
 
 #define LZMA_BASE_SIZE 1846
diff --git a/xen/include/xen/compat.h b/xen/include/xen/compat.h
index ca60699..d58aede 100644
--- a/xen/include/xen/compat.h
+++ b/xen/include/xen/compat.h
@@ -14,7 +14,7 @@
 #define __DEFINE_COMPAT_HANDLE(name, type) \
     typedef struct { \
         compat_ptr_t c; \
-        type *_[0] __attribute__((__packed__)); \
+        type *_[0] __packed; \
     } __compat_handle_ ## name
 
 #define DEFINE_COMPAT_HANDLE(name) \
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index c80398d..6e07990 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -16,6 +16,8 @@
 
 #define noreturn      __attribute__((noreturn))
 
+#define __packed      __attribute__((packed))
+
 #if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__ < 5))
 #define unreachable() do {} while (1)
 #else
-- 
1.7.10.4

Re: [PATCH 2/5] xen/common: Cleanup use of __attribute__((packed))

[Jan Beulich]

>>> On 12.03.14 at 20:08, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/xen/common/trace.c
> +++ b/xen/common/trace.c
> @@ -641,11 +641,11 @@ static inline void insert_wrap_record(struct t_buf 
> *buf,
>  
>  static inline void insert_lost_records(struct t_buf *buf)
>  {
> -    struct {
> +    struct __packed {
>          u32 lost_records;
>          u32 did:16, vid:16;
>          u64 first_tsc;
> -    } __attribute__((packed)) ed;
> +    } ed;

So why did you not strip this one in the previous patch?

Jan

Re: [PATCH 2/5] xen/common: Cleanup use of __attribute__((packed))

[Andrew Cooper]

On 13/03/14 08:15, Jan Beulich wrote:
>>>> On 12.03.14 at 20:08, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/common/trace.c
>> +++ b/xen/common/trace.c
>> @@ -641,11 +641,11 @@ static inline void insert_wrap_record(struct t_buf 
>> *buf,
>>  
>>  static inline void insert_lost_records(struct t_buf *buf)
>>  {
>> -    struct {
>> +    struct __packed {
>>          u32 lost_records;
>>          u32 did:16, vid:16;
>>          u64 first_tsc;
>> -    } __attribute__((packed)) ed;
>> +    } ed;
> So why did you not strip this one in the previous patch?
>
> Jan
>

My reading of a recent C spec draft would indicate that the compiler is
perfectly at liberty to expand these :16 bitfields up 32 bits each, if
it feels like doing so.

For peace of mind, I left all structures with bitfields with their
__packed attributes.

~Andrew

Re: [PATCH 2/5] xen/common: Cleanup use of __attribute__((packed))

[Jan Beulich]

>>> On 13.03.14 at 11:22, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 13/03/14 08:15, Jan Beulich wrote:
>>>>> On 12.03.14 at 20:08, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/common/trace.c
>>> +++ b/xen/common/trace.c
>>> @@ -641,11 +641,11 @@ static inline void insert_wrap_record(struct t_buf 
>>> *buf,
>>>  
>>>  static inline void insert_lost_records(struct t_buf *buf)
>>>  {
>>> -    struct {
>>> +    struct __packed {
>>>          u32 lost_records;
>>>          u32 did:16, vid:16;
>>>          u64 first_tsc;
>>> -    } __attribute__((packed)) ed;
>>> +    } ed;
>> So why did you not strip this one in the previous patch?
> 
> My reading of a recent C spec draft would indicate that the compiler is
> perfectly at liberty to expand these :16 bitfields up 32 bits each, if
> it feels like doing so.

Which would then be better addressed by changing them both
to u16, dropping the bit fields altogether.

But I don't think the liberty given to a compiler is that wide: "An
implementation may allocate any addressable storage unit large
enough to hold a bitfield. If enough space remains, a bit-field
that immediately follows another bit-field in a structure shall be
packed into adjacent bits of the same unit."

I.e. the compiler has basically two choices: Use a 2-byte storage
unit for each of them, or use a 4-byte storage unit and put them
both in there. The end result is the same. What you're concerned
about can only happen when crossing storage unit boundaries.

Jan

Re: [PATCH 2/5] xen/common: Cleanup use of __attribute__((packed))

[Andrew Cooper]

On 13/03/14 10:40, Jan Beulich wrote:
>>>> On 13.03.14 at 11:22, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 13/03/14 08:15, Jan Beulich wrote:
>>>>>> On 12.03.14 at 20:08, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/common/trace.c
>>>> +++ b/xen/common/trace.c
>>>> @@ -641,11 +641,11 @@ static inline void insert_wrap_record(struct t_buf 
>>>> *buf,
>>>>  
>>>>  static inline void insert_lost_records(struct t_buf *buf)
>>>>  {
>>>> -    struct {
>>>> +    struct __packed {
>>>>          u32 lost_records;
>>>>          u32 did:16, vid:16;
>>>>          u64 first_tsc;
>>>> -    } __attribute__((packed)) ed;
>>>> +    } ed;
>>> So why did you not strip this one in the previous patch?
>> My reading of a recent C spec draft would indicate that the compiler is
>> perfectly at liberty to expand these :16 bitfields up 32 bits each, if
>> it feels like doing so.
> Which would then be better addressed by changing them both
> to u16, dropping the bit fields altogether.
>
> But I don't think the liberty given to a compiler is that wide: "An
> implementation may allocate any addressable storage unit large
> enough to hold a bitfield. If enough space remains, a bit-field
> that immediately follows another bit-field in a structure shall be
> packed into adjacent bits of the same unit."
>
> I.e. the compiler has basically two choices: Use a 2-byte storage
> unit for each of them, or use a 4-byte storage unit and put them
> both in there. The end result is the same. What you're concerned
> about can only happen when crossing storage unit boundaries.
>
> Jan
>

Ok - I will switch them to u16 and drop the __packed.

~Andrew

[PATCH 3/5] xen/x86: Cleanup use of __attribute__((packed))

[Andrew Cooper]

And standardise on having the statement at the head of the struct/union rather
than at the tail.

Almost all of this is mechanical shuffling.  The two interesting places are
tboot.h (removing some now-redundant ifdefs) and mce-apei.c (removing an
unreferenced global object with the name '__packed')

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Christoph Egger <chegger@amazon.de>
CC: Liu Jinsong <jinsong.liu@intel.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Eddie Dong <eddie.dong@intel.com>
CC: Joseph Cihula <joseph.cihula@intel.com>
CC: Gang Wei <gang.wei@intel.com>
CC: Shane Wang <shane.wang@intel.com>
---
 xen/arch/x86/bzimage.c                 |    4 ++--
 xen/arch/x86/cpu/mcheck/mce-apei.c     |    4 ++--
 xen/arch/x86/dmi_scan.c                |    8 ++++----
 xen/arch/x86/microcode_amd.c           |    8 ++++----
 xen/arch/x86/mm/shadow/multi.c         |   16 ++++++++--------
 xen/arch/x86/trace.c                   |   16 ++++++++--------
 xen/arch/x86/x86_emulate/x86_emulate.h |    8 ++++----
 xen/include/asm-x86/apicdef.h          |    4 ++--
 xen/include/asm-x86/desc.h             |    4 ++--
 xen/include/asm-x86/e820.h             |    4 ++--
 xen/include/asm-x86/edd.h              |   16 ++++++++--------
 xen/include/asm-x86/hvm/svm/vmcb.h     |   24 ++++++++++++------------
 xen/include/asm-x86/hvm/vmx/vmx.h      |    4 ++--
 xen/include/asm-x86/i387.h             |    4 ++--
 xen/include/asm-x86/io_apic.h          |   20 ++++++++++----------
 xen/include/asm-x86/msi.h              |    8 ++++----
 xen/include/asm-x86/processor.h        |    4 ++--
 xen/include/asm-x86/tboot.h            |    4 ----
 xen/include/asm-x86/xstate.h           |    4 ++--
 19 files changed, 80 insertions(+), 84 deletions(-)

diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c
index d72b832..c86c39e 100644
--- a/xen/arch/x86/bzimage.c
+++ b/xen/arch/x86/bzimage.c
@@ -146,7 +146,7 @@ static __init int perform_gunzip(char *output, char *image, unsigned long image_
     return rc;
 }
 
-struct setup_header {
+struct __packed setup_header {
         uint8_t         _pad0[0x1f1];           /* skip uninteresting stuff */
         uint8_t         setup_sects;
         uint16_t        root_flags;
@@ -183,7 +183,7 @@ struct setup_header {
         uint64_t        hardware_subarch_data;
         uint32_t        payload_offset;
         uint32_t        payload_length;
-    } __attribute__((packed));
+    };
 
 static __init int bzimage_check(struct setup_header *hdr, unsigned long len)
 {
diff --git a/xen/arch/x86/cpu/mcheck/mce-apei.c b/xen/arch/x86/cpu/mcheck/mce-apei.c
index 3370341..3bf760a 100644
--- a/xen/arch/x86/cpu/mcheck/mce-apei.c
+++ b/xen/arch/x86/cpu/mcheck/mce-apei.c
@@ -49,11 +49,11 @@
  * CPER specification (in UEFI specification 2.3 appendix N) requires
  * byte-packed.
  */
-struct cper_mce_record {
+struct __packed cper_mce_record {
 	struct cper_record_header hdr;
 	struct cper_section_descriptor sec_hdr;
 	struct mce mce;
-} __packed;
+};
 /* Reset to default packing */
 #pragma pack()
 
diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c
index 9d4348b..500133a 100644
--- a/xen/arch/x86/dmi_scan.c
+++ b/xen/arch/x86/dmi_scan.c
@@ -18,16 +18,16 @@
 #define memcpy_fromio    memcpy
 #define alloc_bootmem(l) xmalloc_bytes(l)
 
-struct dmi_eps {
+struct __packed dmi_eps {
 	char anchor[5];			/* "_DMI_" */
 	u8 checksum;
 	u16 size;
 	u32 address;
 	u16 num_structures;
 	u8 revision;
-} __attribute__((packed));
+};
 
-struct smbios_eps {
+struct __packed smbios_eps {
 	char anchor[4];			/* "_SM_" */
 	u8 checksum;
 	u8 length;
@@ -36,7 +36,7 @@ struct smbios_eps {
 	u8 revision;
 	u8 _rsrvd_[5];
 	struct dmi_eps dmi;
-} __attribute__((packed));
+};
 
 struct dmi_header
 {
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index 3014245..b227173 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -27,15 +27,15 @@
 #include <asm/microcode.h>
 #include <asm/hvm/svm/svm.h>
 
-struct equiv_cpu_entry {
+struct __packed equiv_cpu_entry {
     uint32_t installed_cpu;
     uint32_t fixed_errata_mask;
     uint32_t fixed_errata_compare;
     uint16_t equiv_cpu;
     uint16_t reserved;
-} __attribute__((packed));
+};
 
-struct microcode_header_amd {
+struct __packed microcode_header_amd {
     uint32_t data_code;
     uint32_t patch_id;
     uint8_t  mc_patch_data_id[2];
@@ -50,7 +50,7 @@ struct microcode_header_amd {
     uint8_t  bios_api_rev;
     uint8_t  reserved1[3];
     uint32_t match_reg[8];
-} __attribute__((packed));
+};
 
 #define UCODE_MAGIC                0x00414d44
 #define UCODE_EQUIV_CPU_TABLE_TYPE 0x00000000
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 3c1b25b..9dfa345 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -2700,13 +2700,13 @@ static inline void trace_shadow_fixup(guest_l1e_t gl1e,
 {
     if ( tb_init_done )
     {
-        struct {
+        struct __packed {
             /* for PAE, guest_l1e may be 64 while guest_va may be 32;
                so put it first for alignment sake. */
             guest_l1e_t gl1e;
             guest_va_t va;
             u32 flags;
-        } __attribute__((packed)) d;
+        } d;
         u32 event;
 
         event = TRC_SHADOW_FIXUP | ((GUEST_PAGING_LEVELS-2)<<8);
@@ -2724,13 +2724,13 @@ static inline void trace_not_shadow_fault(guest_l1e_t gl1e,
 {
     if ( tb_init_done )
     {
-        struct {
+        struct __packed {
             /* for PAE, guest_l1e may be 64 while guest_va may be 32;
                so put it first for alignment sake. */
             guest_l1e_t gl1e;
             guest_va_t va;
             u32 flags;
-        } __attribute__((packed)) d;
+        } d;
         u32 event;
 
         event = TRC_SHADOW_NOT_SHADOW | ((GUEST_PAGING_LEVELS-2)<<8);
@@ -2749,7 +2749,7 @@ static inline void trace_shadow_emulate_other(u32 event,
 {
     if ( tb_init_done )
     {
-        struct {
+        struct __packed {
             /* for PAE, guest_l1e may be 64 while guest_va may be 32;
                so put it first for alignment sake. */
 #if GUEST_PAGING_LEVELS == 2
@@ -2758,7 +2758,7 @@ static inline void trace_shadow_emulate_other(u32 event,
             u64 gfn;
 #endif
             guest_va_t va;
-        } __attribute__((packed)) d;
+        } d;
 
         event |= ((GUEST_PAGING_LEVELS-2)<<8);
 
@@ -2779,13 +2779,13 @@ static inline void trace_shadow_emulate(guest_l1e_t gl1e, unsigned long va)
 {
     if ( tb_init_done )
     {
-        struct {
+        struct __packed {
             /* for PAE, guest_l1e may be 64 while guest_va may be 32;
                so put it first for alignment sake. */
             guest_l1e_t gl1e, write_val;
             guest_va_t va;
             unsigned flags:29, emulation_count:3;
-        } __attribute__((packed)) d;
+        } d;
         u32 event;
 
         event = TRC_SHADOW_EMULATE | ((GUEST_PAGING_LEVELS-2)<<8);
diff --git a/xen/arch/x86/trace.c b/xen/arch/x86/trace.c
index 652c4e3..4549e58 100644
--- a/xen/arch/x86/trace.c
+++ b/xen/arch/x86/trace.c
@@ -38,12 +38,12 @@ void __trace_pv_trap(int trapnr, unsigned long eip,
 {
     if ( is_pv_32on64_vcpu(current) )
     {
-        struct {
+        struct __packed {
             unsigned eip:32,
                 trapnr:15,
                 use_error_code:1,
                 error_code:16;
-        } __attribute__((packed)) d;
+        } d;
 
         d.eip = eip;
         d.trapnr = trapnr;
@@ -54,12 +54,12 @@ void __trace_pv_trap(int trapnr, unsigned long eip,
     }
     else
     {
-        struct {
+        struct __packed {
             unsigned long eip;
             unsigned trapnr:15,
                 use_error_code:1,
                 error_code:16;
-        } __attribute__((packed)) d;
+        } d;
         unsigned event;
 
         d.eip = eip;
@@ -79,9 +79,9 @@ void __trace_pv_page_fault(unsigned long addr, unsigned error_code)
 
     if ( is_pv_32on64_vcpu(current) )
     {
-        struct {
+        struct __packed {
             u32 eip, addr, error_code;
-        } __attribute__((packed)) d;
+        } d;
 
         d.eip = eip;
         d.addr = addr;
@@ -91,10 +91,10 @@ void __trace_pv_page_fault(unsigned long addr, unsigned error_code)
     }
     else
     {
-        struct {
+        struct __packed {
             unsigned long eip, addr;
             u32 error_code;
-        } __attribute__((packed)) d;
+        } d;
         unsigned event;
 
         d.eip = eip;
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index 85bc4bc..107addf 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -55,7 +55,7 @@ enum x86_segment {
  * Attribute for segment selector. This is a copy of bit 40:47 & 52:55 of the
  * segment descriptor. It happens to match the format of an AMD SVM VMCB.
  */
-typedef union segment_attributes {
+typedef union __packed segment_attributes {
     uint16_t bytes;
     struct
     {
@@ -69,18 +69,18 @@ typedef union segment_attributes {
         uint16_t g:   1;    /* 11; Bit 55 */
         uint16_t pad: 4;
     } fields;
-} __attribute__ ((packed)) segment_attributes_t;
+} segment_attributes_t;
 
 /*
  * Full state of a segment register (visible and hidden portions).
  * Again, this happens to match the format of an AMD SVM VMCB.
  */
-struct segment_register {
+struct __packed segment_register {
     uint16_t   sel;
     segment_attributes_t attr;
     uint32_t   limit;
     uint64_t   base;
-} __attribute__ ((packed));
+};
 
 /*
  * Return codes from state-accessor functions and from x86_emulate().
diff --git a/xen/include/asm-x86/apicdef.h b/xen/include/asm-x86/apicdef.h
index 2bdb3df..d66c8ea 100644
--- a/xen/include/asm-x86/apicdef.h
+++ b/xen/include/asm-x86/apicdef.h
@@ -142,7 +142,7 @@
 #define lapic ((volatile struct local_apic *)APIC_BASE)
 
 #ifndef __ASSEMBLY__
-struct local_apic {
+struct __packed local_apic {
 
 /*000*/	struct { u32 __reserved[4]; } __reserved_01;
 
@@ -388,7 +388,7 @@ struct local_apic {
 
 /*3F0*/	struct { u32 __reserved[4]; } __reserved_20;
 
-} __attribute__ ((packed));
+};
 #endif /* !__ASSEMBLY__ */
 
 #undef u32
diff --git a/xen/include/asm-x86/desc.h b/xen/include/asm-x86/desc.h
index 354b889..4edb834 100644
--- a/xen/include/asm-x86/desc.h
+++ b/xen/include/asm-x86/desc.h
@@ -181,10 +181,10 @@ do {                                                     \
         (((u32)(addr) & 0x00FF0000U) >> 16);             \
 } while (0)
 
-struct desc_ptr {
+struct __packed desc_ptr {
 	unsigned short limit;
 	unsigned long base;
-} __attribute__((__packed__)) ;
+};
 
 extern struct desc_struct boot_cpu_gdt_table[];
 DECLARE_PER_CPU(struct desc_struct *, gdt_table);
diff --git a/xen/include/asm-x86/e820.h b/xen/include/asm-x86/e820.h
index 0fd81f6..08b413d 100644
--- a/xen/include/asm-x86/e820.h
+++ b/xen/include/asm-x86/e820.h
@@ -10,11 +10,11 @@
 #define E820_NVS          4
 #define E820_UNUSABLE     5
 
-struct e820entry {
+struct __packed e820entry {
     uint64_t addr;
     uint64_t size;
     uint32_t type;
-} __attribute__((packed));
+};
 
 #define E820MAX	128
 
diff --git a/xen/include/asm-x86/edd.h b/xen/include/asm-x86/edd.h
index 57d4bff..e061912 100644
--- a/xen/include/asm-x86/edd.h
+++ b/xen/include/asm-x86/edd.h
@@ -25,7 +25,7 @@
 
 #ifndef __ASSEMBLY__
 
-struct edd_info {
+struct __packed edd_info {
     /* Int13, Fn48: Check Extensions Present. */
     u8 device;                   /* %dl: device */
     u8 version;                  /* %ah: major version */
@@ -35,7 +35,7 @@ struct edd_info {
     u8 legacy_max_head;          /* %dh: maximum head number */
     u8 legacy_sectors_per_track; /* %cl[5:0]: maximum sector number */
     /* Int13, Fn41: Get Device Parameters (as filled into %ds:%esi). */
-    struct edd_device_params {
+    struct __packed edd_device_params {
         u16 length;
         u16 info_flags;
         u32 num_default_cylinders;
@@ -93,12 +93,12 @@ struct edd_info {
                 u32 reserved3;
                 u64 reserved4;
             } atapi;
-            struct {
+            struct __packed {
                 u16 id;
                 u64 lun;
                 u16 reserved1;
                 u32 reserved2;
-            } __attribute__ ((packed)) scsi;
+            } scsi;
             struct {
                 u64 serial_number;
                 u64 reserved;
@@ -134,14 +134,14 @@ struct edd_info {
         } device_path;
         u8 reserved4;
         u8 checksum;
-    } __attribute__ ((packed)) edd_device_params;
-} __attribute__ ((packed));
+    }  edd_device_params;
+};
 
-struct mbr_signature {
+struct __packed mbr_signature {
     u8 device;
     u8 pad[3];
     u32 signature;
-} __attribute__ ((packed));
+};
 
 /* These all reside in the boot trampoline. Access via bootsym(). */
 extern struct mbr_signature boot_mbr_signature[];
diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h
index ade4bb2..9b0c789 100644
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -309,7 +309,7 @@ enum VMEXIT_EXITCODE
 /* Definition of segment state is borrowed by the generic HVM code. */
 typedef struct segment_register svm_segment_register_t;
 
-typedef union 
+typedef union __packed
 {
     u64 bytes;
     struct 
@@ -321,9 +321,9 @@ typedef union
         u64 v:         1;
         u64 errorcode:32;
     } fields;
-} __attribute__ ((packed)) eventinj_t;
+} eventinj_t;
 
-typedef union 
+typedef union __packed
 {
     u64 bytes;
     struct 
@@ -339,9 +339,9 @@ typedef union
         u64 vector:       8;
         u64 rsvd3:       24;
     } fields;
-} __attribute__ ((packed)) vintr_t;
+} vintr_t;
 
-typedef union
+typedef union __packed
 {
     u64 bytes;
     struct 
@@ -356,18 +356,18 @@ typedef union
         u64 rsv1: 9;
         u64 port: 16;
     } fields;
-} __attribute__ ((packed)) ioio_info_t;
+} ioio_info_t;
 
-typedef union
+typedef union __packed
 {
     u64 bytes;
     struct
     {
         u64 enable:1;
     } fields;
-} __attribute__ ((packed)) lbrctrl_t;
+} lbrctrl_t;
 
-typedef union
+typedef union __packed
 {
     uint32_t bytes;
     struct
@@ -397,12 +397,12 @@ typedef union
         uint32_t lbr: 1;
         uint32_t resv: 21;
     } fields;
-} __attribute__ ((packed)) vmcbcleanbits_t;
+} vmcbcleanbits_t;
 
 #define IOPM_SIZE   (12 * 1024)
 #define MSRPM_SIZE  (8  * 1024)
 
-struct vmcb_struct {
+struct __packed vmcb_struct {
     u32 _cr_intercepts;         /* offset 0x00 - cleanbit 0 */
     u32 _dr_intercepts;         /* offset 0x04 - cleanbit 0 */
     u32 _exception_intercepts;  /* offset 0x08 - cleanbit 0 */
@@ -487,7 +487,7 @@ struct vmcb_struct {
     u64 _lastintfromip;         /* cleanbit 10 */
     u64 _lastinttoip;           /* cleanbit 10 */
     u64 res16[301];
-} __attribute__ ((packed));
+};
 
 struct svm_domain {
 };
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 827c97e..ac4380a 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -421,11 +421,11 @@ static inline void __invept(unsigned long type, u64 eptp, u64 gpa)
 
 static inline void __invvpid(unsigned long type, u16 vpid, u64 gva)
 {
-    struct {
+    struct __packed {
         u64 vpid:16;
         u64 rsvd:48;
         u64 gva;
-    } __attribute__ ((packed)) operand = {vpid, 0, gva};
+    }  operand = {vpid, 0, gva};
 
     /* Fix up #UD exceptions which occur when TLBs are flushed before VMXON. */
     asm volatile ( "1: "
diff --git a/xen/include/asm-x86/i387.h b/xen/include/asm-x86/i387.h
index 1f5fe50..38dbcae 100644
--- a/xen/include/asm-x86/i387.h
+++ b/xen/include/asm-x86/i387.h
@@ -28,11 +28,11 @@ struct ix87_state {
         uint32_t fdp;
         uint16_t fds, _res6;
     } env;
-    struct ix87_reg {
+    struct __packed ix87_reg {
         uint64_t mantissa;
         uint16_t exponent:15;
         uint16_t sign:1;
-    } __attribute__((__packed__)) r[8];
+    } r[8];
 };
 
 void vcpu_restore_fpu_eager(struct vcpu *v);
diff --git a/xen/include/asm-x86/io_apic.h b/xen/include/asm-x86/io_apic.h
index 6d90628..b318a36 100644
--- a/xen/include/asm-x86/io_apic.h
+++ b/xen/include/asm-x86/io_apic.h
@@ -36,41 +36,41 @@
  */
 union IO_APIC_reg_00 {
 	u32	raw;
-	struct {
+	struct __packed {
 		u32	__reserved_2	: 14,
 			LTS		:  1,
 			delivery_type	:  1,
 			__reserved_1	:  8,
 			ID		:  8;
-	} __attribute__ ((packed)) bits;
+	} bits;
 };
 
 union IO_APIC_reg_01 {
 	u32	raw;
-	struct {
+	struct __packed {
 		u32	version		:  8,
 			__reserved_2	:  7,
 			PRQ		:  1,
 			entries		:  8,
 			__reserved_1	:  8;
-	} __attribute__ ((packed)) bits;
+	} bits;
 };
 
 union IO_APIC_reg_02 {
 	u32	raw;
-	struct {
+	struct __packed {
 		u32	__reserved_2	: 24,
 			arbitration	:  4,
 			__reserved_1	:  4;
-	} __attribute__ ((packed)) bits;
+	} bits;
 };
 
 union IO_APIC_reg_03 {
 	u32	raw;
-	struct {
+	struct __packed {
 		u32	boot_DT		:  1,
 			__reserved_1	: 31;
-	} __attribute__ ((packed)) bits;
+	} bits;
 };
 
 /*
@@ -90,7 +90,7 @@ enum ioapic_irq_destination_types {
 	dest_ExtINT = 7
 };
 
-struct IO_APIC_route_entry {
+struct __packed IO_APIC_route_entry {
 	__u32	vector		:  8,
 		delivery_mode	:  3,	/* 000: FIXED
 					 * 001: lowest prio
@@ -119,7 +119,7 @@ struct IO_APIC_route_entry {
 			__u32 dest32;
 	} dest;
 
-} __attribute__ ((packed));
+};
 
 /*
  * MP-BIOS irq configuration table structures:
diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
index 89a9266..1dd0d8c 100644
--- a/xen/include/asm-x86/msi.h
+++ b/xen/include/asm-x86/msi.h
@@ -168,7 +168,7 @@ int msi_free_irq(struct msi_desc *entry);
  * MSI Defined Data Structures
  */
 
-struct msg_data {
+struct __packed msg_data {
 #if defined(__LITTLE_ENDIAN_BITFIELD)
 	__u32	vector		:  8;
 	__u32	delivery_mode	:  3;	/* 000b: FIXED | 001b: lowest prior */
@@ -186,9 +186,9 @@ struct msg_data {
 #else
 #error "Bitfield endianness not defined! Check your byteorder.h"
 #endif
-} __attribute__ ((packed));
+};
 
-struct msg_address {
+struct __packed msg_address {
 	union {
 		struct {
 #if defined(__LITTLE_ENDIAN_BITFIELD)
@@ -212,7 +212,7 @@ struct msg_address {
        		__u32  value;
 	}lo_address;
 	__u32 	hi_address;
-} __attribute__ ((packed));
+};
 
 #define MAX_MSIX_TABLE_ENTRIES  (PCI_MSIX_FLAGS_QSIZE + 1)
 #define MAX_MSIX_TABLE_PAGES    PFN_UP(MAX_MSIX_TABLE_ENTRIES * \
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 498d8a7..f5e9eda 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -412,7 +412,7 @@ static always_inline void __mwait(unsigned long eax, unsigned long ecx)
 #define IOBMP_BYTES             8192
 #define IOBMP_INVALID_OFFSET    0x8000
 
-struct tss_struct {
+struct __packed __cacheline_aligned tss_struct {
     unsigned short	back_link,__blh;
     union { u64 rsp0, esp0; };
     union { u64 rsp1, esp1; };
@@ -426,7 +426,7 @@ struct tss_struct {
     u16 bitmap;
     /* Pads the TSS to be cacheline-aligned (total size is 0x80). */
     u8 __cacheline_filler[24];
-} __cacheline_aligned __attribute__((packed));
+};
 
 #define IST_NONE 0UL
 #define IST_DF   1UL
diff --git a/xen/include/asm-x86/tboot.h b/xen/include/asm-x86/tboot.h
index e77d1c0..d242862 100644
--- a/xen/include/asm-x86/tboot.h
+++ b/xen/include/asm-x86/tboot.h
@@ -39,10 +39,6 @@
 
 #include <xen/acpi.h>
 
-#ifndef __packed
-#define __packed   __attribute__ ((packed))
-#endif
-
 typedef struct __packed {
   uint32_t    data1;
   uint16_t    data2;
diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
index de5711e..1a92ac3 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -42,7 +42,7 @@
 extern u64 xfeature_mask;
 
 /* extended state save area */
-struct xsave_struct
+struct __packed __attribute__((aligned (64))) xsave_struct
 {
     union {                                  /* FPU/MMX, SSE */
         char x[512];
@@ -73,7 +73,7 @@ struct xsave_struct
 
     struct { char x[XSTATE_YMM_SIZE]; } ymm; /* YMM */
     char   data[];                           /* Future new states */
-} __attribute__ ((packed, aligned (64)));
+};
 
 /* extended state operations */
 bool_t __must_check set_xcr0(u64 xfeatures);
-- 
1.7.10.4

Re: [PATCH 3/5] xen/x86: Cleanup use of __attribute__((packed))

[Jan Beulich]

>>> On 12.03.14 at 20:08, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -27,15 +27,15 @@
>  #include <asm/microcode.h>
>  #include <asm/hvm/svm/svm.h>
>  
> -struct equiv_cpu_entry {
> +struct __packed equiv_cpu_entry {
>      uint32_t installed_cpu;
>      uint32_t fixed_errata_mask;
>      uint32_t fixed_errata_compare;
>      uint16_t equiv_cpu;
>      uint16_t reserved;
> -} __attribute__((packed));
> +};

This too seems to be a case of an unneeded use of the attribute.

> -struct microcode_header_amd {
> +struct __packed microcode_header_amd {
>      uint32_t data_code;
>      uint32_t patch_id;
>      uint8_t  mc_patch_data_id[2];
> @@ -50,7 +50,7 @@ struct microcode_header_amd {
>      uint8_t  bios_api_rev;
>      uint8_t  reserved1[3];
>      uint32_t match_reg[8];
> -} __attribute__((packed));
> +};

As does this one.

> --- a/xen/arch/x86/trace.c
> +++ b/xen/arch/x86/trace.c
> @@ -38,12 +38,12 @@ void __trace_pv_trap(int trapnr, unsigned long eip,
>  {
>      if ( is_pv_32on64_vcpu(current) )
>      {
> -        struct {
> +        struct __packed {
>              unsigned eip:32,
>                  trapnr:15,
>                  use_error_code:1,
>                  error_code:16;
> -        } __attribute__((packed)) d;
> +        } d;

And this.

> @@ -79,9 +79,9 @@ void __trace_pv_page_fault(unsigned long addr, unsigned error_code)
>  
>      if ( is_pv_32on64_vcpu(current) )
>      {
> -        struct {
> +        struct __packed {
>              u32 eip, addr, error_code;
> -        } __attribute__((packed)) d;
> +        } d;

Again.

> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -55,7 +55,7 @@ enum x86_segment {
>   * Attribute for segment selector. This is a copy of bit 40:47 & 52:55 of the
>   * segment descriptor. It happens to match the format of an AMD SVM VMCB.
>   */
> -typedef union segment_attributes {
> +typedef union __packed segment_attributes {

Did you check that tools/tests/x86_emulator/ still builds with this
change? I don't think it would, and I do think an open coded use
here is acceptable.

But then again I can't see why the attribute would be needed here
in the first place.

> @@ -69,18 +69,18 @@ typedef union segment_attributes {
>          uint16_t g:   1;    /* 11; Bit 55 */
>          uint16_t pad: 4;
>      } fields;
> -} __attribute__ ((packed)) segment_attributes_t;
> +} segment_attributes_t;
>  
>  /*
>   * Full state of a segment register (visible and hidden portions).
>   * Again, this happens to match the format of an AMD SVM VMCB.
>   */
> -struct segment_register {
> +struct __packed segment_register {
>      uint16_t   sel;
>      segment_attributes_t attr;
>      uint32_t   limit;
>      uint64_t   base;
> -} __attribute__ ((packed));
> +};

Same for this one.

> --- a/xen/include/asm-x86/apicdef.h
> +++ b/xen/include/asm-x86/apicdef.h
> @@ -142,7 +142,7 @@
>  #define lapic ((volatile struct local_apic *)APIC_BASE)
>  
>  #ifndef __ASSEMBLY__
> -struct local_apic {
> +struct __packed local_apic {

Another case of unnecessary attribute. You really need to settle on
one of two models: Have the attribute in place (even if redundant)
for definitions describing external interfaces (hardware/firmware), or
unconditionally omit them when redundant. I.e. keep it here _and_
e.g. in edd.h, or drop it here, there, and wherever else. I'm not going
to repeat respective remarks further down.

Jan

Re: [PATCH 3/5] xen/x86: Cleanup use of __attribute__((packed))

[Andrew Cooper]

On 13/03/14 08:32, Jan Beulich wrote:
>>>> On 12.03.14 at 20:08, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/microcode_amd.c
>> +++ b/xen/arch/x86/microcode_amd.c
>> @@ -27,15 +27,15 @@
>>  #include <asm/microcode.h>
>>  #include <asm/hvm/svm/svm.h>
>>  
>> -struct equiv_cpu_entry {
>> +struct __packed equiv_cpu_entry {
>>      uint32_t installed_cpu;
>>      uint32_t fixed_errata_mask;
>>      uint32_t fixed_errata_compare;
>>      uint16_t equiv_cpu;
>>      uint16_t reserved;
>> -} __attribute__((packed));
>> +};
> This too seems to be a case of an unneeded use of the attribute.

So it is, and it is just a Xen internal structure.

>
>> -struct microcode_header_amd {
>> +struct __packed microcode_header_amd {
>>      uint32_t data_code;
>>      uint32_t patch_id;
>>      uint8_t  mc_patch_data_id[2];
>> @@ -50,7 +50,7 @@ struct microcode_header_amd {
>>      uint8_t  bios_api_rev;
>>      uint8_t  reserved1[3];
>>      uint32_t match_reg[8];
>> -} __attribute__((packed));
>> +};
> As does this one.

It would appear so, although as a hardware description, it should keep
its __packed.

>
>> --- a/xen/arch/x86/trace.c
>> +++ b/xen/arch/x86/trace.c
>> @@ -38,12 +38,12 @@ void __trace_pv_trap(int trapnr, unsigned long eip,
>>  {
>>      if ( is_pv_32on64_vcpu(current) )
>>      {
>> -        struct {
>> +        struct __packed {
>>              unsigned eip:32,
>>                  trapnr:15,
>>                  use_error_code:1,
>>                  error_code:16;
>> -        } __attribute__((packed)) d;
>> +        } d;
> And this.
>
>> @@ -79,9 +79,9 @@ void __trace_pv_page_fault(unsigned long addr, unsigned error_code)
>>  
>>      if ( is_pv_32on64_vcpu(current) )
>>      {
>> -        struct {
>> +        struct __packed {
>>              u32 eip, addr, error_code;
>> -        } __attribute__((packed)) d;
>> +        } d;
> Again.

This one is awkward, because of the way the compiler overlaps the two
struct d's in this function on the stack.

This one is safe to change, but results in different alignment on the
stack, causing a diff between the two binaries when comparing.  It is
probably ok to drop and just note the diff.

>
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>> @@ -55,7 +55,7 @@ enum x86_segment {
>>   * Attribute for segment selector. This is a copy of bit 40:47 & 52:55 of the
>>   * segment descriptor. It happens to match the format of an AMD SVM VMCB.
>>   */
>> -typedef union segment_attributes {
>> +typedef union __packed segment_attributes {
> Did you check that tools/tests/x86_emulator/ still builds with this
> change? I don't think it would, and I do think an open coded use
> here is acceptable.

I didn't check the build.  I will do that in the future.

>
> But then again I can't see why the attribute would be needed here
> in the first place.

As indicated, I was overly hesitant with bitfields, but perhaps overly.


>
>> @@ -69,18 +69,18 @@ typedef union segment_attributes {
>>          uint16_t g:   1;    /* 11; Bit 55 */
>>          uint16_t pad: 4;
>>      } fields;
>> -} __attribute__ ((packed)) segment_attributes_t;
>> +} segment_attributes_t;
>>  
>>  /*
>>   * Full state of a segment register (visible and hidden portions).
>>   * Again, this happens to match the format of an AMD SVM VMCB.
>>   */
>> -struct segment_register {
>> +struct __packed segment_register {
>>      uint16_t   sel;
>>      segment_attributes_t attr;
>>      uint32_t   limit;
>>      uint64_t   base;
>> -} __attribute__ ((packed));
>> +};
> Same for this one.
>
>> --- a/xen/include/asm-x86/apicdef.h
>> +++ b/xen/include/asm-x86/apicdef.h
>> @@ -142,7 +142,7 @@
>>  #define lapic ((volatile struct local_apic *)APIC_BASE)
>>  
>>  #ifndef __ASSEMBLY__
>> -struct local_apic {
>> +struct __packed local_apic {
> Another case of unnecessary attribute. You really need to settle on
> one of two models: Have the attribute in place (even if redundant)
> for definitions describing external interfaces (hardware/firmware), or
> unconditionally omit them when redundant. I.e. keep it here _and_
> e.g. in edd.h, or drop it here, there, and wherever else. I'm not going
> to repeat respective remarks further down.
>
> Jan
>

Alright - All hardware interfaces can keep their redundant __packed,
which also serves partly as documentation.

One interesting question George raises is whether the trace record
format should be count as well?  It is just a software interface, but
mixing __packed on different record structures seems a little inconsistent.

~Andrew

Re: [PATCH 3/5] xen/x86: Cleanup use of __attribute__((packed))

[Jan Beulich]

>>> On 13.03.14 at 12:00, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 13/03/14 08:32, Jan Beulich wrote:
>> But then again I can't see why the attribute would be needed here
>> in the first place.
> 
> As indicated, I was overly hesitant with bitfields, but perhaps overly.

Since that's the only thing we care of, perhaps add a
BUILD_BUG_ON(sizeof(union segment_attributes) != 2) somewhere
where it doesn't conflict with the tools/tests/ use, but the type is in
scope.

> Alright - All hardware interfaces can keep their redundant __packed,
> which also serves partly as documentation.
> 
> One interesting question George raises is whether the trace record
> format should be count as well?  It is just a software interface, but
> mixing __packed on different record structures seems a little inconsistent.

And I agree with that - it is an interface specification (even if
scattered around awkwardly) after all.

Jan

Re: [PATCH 3/5] xen/x86: Cleanup use of __attribute__((packed))

[Keir Fraser]

[-- Attachment #1.1: Type: text/plain, Size: 1313 bytes --]

On Thu, Mar 13, 2014 at 11:13 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 13.03.14 at 12:00, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> > On 13/03/14 08:32, Jan Beulich wrote:
> >> But then again I can't see why the attribute would be needed here
> >> in the first place.
> >
> > As indicated, I was overly hesitant with bitfields, but perhaps overly.
>
> Since that's the only thing we care of, perhaps add a
> BUILD_BUG_ON(sizeof(union segment_attributes) != 2) somewhere
> where it doesn't conflict with the tools/tests/ use, but the type is in
> scope.
>
>
I don't mind having __packed attribute used in x86_emulate.h, and then have
tools/tests/ have a proper wrapper round that header file, as it already
has a wrapper round the .c file.

Really most of the suggestions here are acceptable to me however.

 -- Keir


> > Alright - All hardware interfaces can keep their redundant __packed,
> > which also serves partly as documentation.
> >
> > One interesting question George raises is whether the trace record
> > format should be count as well?  It is just a software interface, but
> > mixing __packed on different record structures seems a little
> inconsistent.
>
> And I agree with that - it is an interface specification (even if
> scattered around awkwardly) after all.
>
> Jan
>
>

[-- Attachment #1.2: Type: text/html, Size: 2134 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

[PATCH 4/5] xen/arm: Cleanup use of __attribute__((packed))

[Andrew Cooper]

And standardise on having the statement at the head of the struct/union rather
than at the tail.  This is all mechanical shuffling.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/include/asm-arm/page.h |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index e00be9e..6fe7fc5 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -94,7 +94,7 @@
  * bits that's not in use in a given node type can be used as
  * extra software-defined bits. */
 
-typedef struct {
+typedef struct __packed {
     /* These are used in all kinds of entry. */
     unsigned long valid:1;      /* Valid mapping */
     unsigned long table:1;      /* == 1 in 4k map entries too */
@@ -126,11 +126,11 @@ typedef struct {
     unsigned long xnt:1;        /* eXecute-Never */
     unsigned long apt:2;        /* Access Permissions */
     unsigned long nst:1;        /* Not-Secure */
-} __attribute__((__packed__)) lpae_pt_t;
+} lpae_pt_t;
 
 /* The p2m tables have almost the same layout, but some of the permission
  * and cache-control bits are laid out differently (or missing) */
-typedef struct {
+typedef struct __packed {
     /* These are used in all kinds of entry. */
     unsigned long valid:1;      /* Valid mapping */
     unsigned long table:1;      /* == 1 in 4k map entries too */
@@ -156,13 +156,13 @@ typedef struct {
     unsigned long type:4;       /* Ignore by hardware. Used to store p2m types */
 
     unsigned long sbz1:5;
-} __attribute__((__packed__)) lpae_p2m_t;
+} lpae_p2m_t;
 
 /*
  * Walk is the common bits of p2m and pt entries which are needed to
  * simply walk the table (e.g. for debug).
  */
-typedef struct {
+typedef struct __packed {
     /* These are used in all kinds of entry. */
     unsigned long valid:1;      /* Valid mapping */
     unsigned long table:1;      /* == 1 in 4k map entries too */
@@ -173,7 +173,7 @@ typedef struct {
     unsigned long base:28;      /* Base address of block or next table */
 
     unsigned long pad1:24;
-} __attribute__((__packed__)) lpae_walk_t;
+} lpae_walk_t;
 
 typedef union {
     uint64_t bits;
-- 
1.7.10.4

Re: [PATCH 4/5] xen/arm: Cleanup use of __attribute__((packed))

[Jan Beulich]

>>> On 12.03.14 at 20:08, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> And standardise on having the statement at the head of the struct/union 
> rather
> than at the tail.  This is all mechanical shuffling.

Yet none of the attributes seem really necessary here.

Jan

Re: [PATCH 4/5] xen/arm: Cleanup use of __attribute__((packed))

[Ian Campbell]

On Thu, 2014-03-13 at 08:34 +0000, Jan Beulich wrote:
> >>> On 12.03.14 at 20:08, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> > And standardise on having the statement at the head of the struct/union 
> > rather
> > than at the tail.  This is all mechanical shuffling.
> 
> Yet none of the attributes seem really necessary here.

These structs mirror the actual hardware PTE layouts, although it should
make no difference in practice isn't __packed quite a useful indicator
for this?

Ian.

Re: [PATCH 4/5] xen/arm: Cleanup use of __attribute__((packed))

[Tim Deegan]

At 09:55 +0000 on 13 Mar (1394700902), Ian Campbell wrote:
> On Thu, 2014-03-13 at 08:34 +0000, Jan Beulich wrote:
> > >>> On 12.03.14 at 20:08, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> > > And standardise on having the statement at the head of the struct/union 
> > > rather
> > > than at the tail.  This is all mechanical shuffling.
> > 
> > Yet none of the attributes seem really necessary here.
> 
> These structs mirror the actual hardware PTE layouts, although it should
> make no difference in practice isn't __packed quite a useful indicator
> for this?

+1.  I think that __packed on things where the actual bit layout is
important is fine, even if it's not strictly necessary.

Cheers,

Tim.

Re: [PATCH 4/5] xen/arm: Cleanup use of __attribute__((packed))

[Ian Campbell]

On Thu, 2014-03-13 at 10:59 +0100, Tim Deegan wrote:
> At 09:55 +0000 on 13 Mar (1394700902), Ian Campbell wrote:
> > On Thu, 2014-03-13 at 08:34 +0000, Jan Beulich wrote:
> > > >>> On 12.03.14 at 20:08, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> > > > And standardise on having the statement at the head of the struct/union 
> > > > rather
> > > > than at the tail.  This is all mechanical shuffling.
> > > 
> > > Yet none of the attributes seem really necessary here.
> > 
> > These structs mirror the actual hardware PTE layouts, although it should
> > make no difference in practice isn't __packed quite a useful indicator
> > for this?
> 
> +1.  I think that __packed on things where the actual bit layout is
> important is fine, even if it's not strictly necessary.

Therefore for the original patch:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.

Re: [PATCH 4/5] xen/arm: Cleanup use of __attribute__((packed))

[Jan Beulich]

>>> On 13.03.14 at 10:55, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2014-03-13 at 08:34 +0000, Jan Beulich wrote:
>> >>> On 12.03.14 at 20:08, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> > And standardise on having the statement at the head of the struct/union 
>> > rather
>> > than at the tail.  This is all mechanical shuffling.
>> 
>> Yet none of the attributes seem really necessary here.
> 
> These structs mirror the actual hardware PTE layouts, although it should
> make no difference in practice isn't __packed quite a useful indicator
> for this?

Yes - see my respective comments elsewhere on this series. All
I'm asking Andrew for is to get this done consistently: Either
retain the attribute on everything defining interfaces to lower
level entities (hardware/firmware), or drop them everywhere
where they don't serve a purpose beyond documentation.

Jan

[PATCH 5/5] DO NOT APPLY: for verification purposes only

[Andrew Cooper]

---
 xen/arch/x86/cpu/mcheck/mce-apei.c |    5 +++--
 xen/arch/x86/hvm/vmx/vmcs.c        |    5 +++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce-apei.c b/xen/arch/x86/cpu/mcheck/mce-apei.c
index 3bf760a..6182d6b 100644
--- a/xen/arch/x86/cpu/mcheck/mce-apei.c
+++ b/xen/arch/x86/cpu/mcheck/mce-apei.c
@@ -49,11 +49,12 @@
  * CPER specification (in UEFI specification 2.3 appendix N) requires
  * byte-packed.
  */
-struct __packed cper_mce_record {
+#undef __packed
+struct cper_mce_record {
 	struct cper_record_header hdr;
 	struct cper_section_descriptor sec_hdr;
 	struct mce mce;
-};
+} __packed;
 /* Reset to default packing */
 #pragma pack()
 
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 4b886e5..9ffb4af 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -670,6 +670,11 @@ void vmx_vmcs_exit(struct vcpu *v)
     }
 }
 
+struct xgt_desc {
+    unsigned short size;
+    unsigned long address __attribute__((packed));
+};
+
 static void vmx_set_host_env(struct vcpu *v)
 {
     unsigned int cpu = smp_processor_id();
-- 
1.7.10.4

Re: [PATCH 0/5] Improvements with __attribute__((packed))

[Keir Fraser]

[-- Attachment #1.1: Type: text/plain, Size: 1397 bytes --]

Andrew Cooper wrote:
>
> Due to a lack of suitable define in scope, mce-apei.c ends up accidentally
> creating a global unreferenced struct of type 'cper_mce_record', named
> '__packed'.
>
> This series is a cleanup of all use of __attribute__((packed)) in Xen. A
> formal define is created in compiler.h, all opencoded uses of the 
> attribute
> are updated to use the new define (or dropped if unnecessary in the first
> place), and the position of __packed is standardised at the beginning 
> of the
> struct rather than the end, so a lack of __packed in scope will result 
> in a
> compile error.
>
> Patch 1 drops some redundant uses of __attribute__((packed)), and is 
> verified
> by diffing xen-syms.
>
> Patches 2 through 4 make the main changes, and are split up by core 
> component
> for the benefit of the maintainers/reviewers.
>
> Patch 5 is only for people wishing to verify my assertions. The entire
> series including patch 5 compiles to identical binaries, proving that the
> reverse contents of patch 5 are the only semantic differences across the
> series.
>
> This is compile tested on each architecture and functionally tested on 
> x86.
>
> Signed-off-by: Andrew Cooper<andrew.cooper3@citrix.com>


Acked-by: Keir Fraser <keir@xen.org>

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

[-- Attachment #1.2: Type: text/html, Size: 1656 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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