[PATCH v2 0/5] Improvements to the use of __attribute__((packed))

[PATCH v2 0/5] Improvements to the use of __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 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 makes all the changes which have a material effect on Xen.

Patches 2 through 5 provide no functional change, but provide consistency with
the use of __attribute__((packed)) across the code.  They can be verified as
noops by diffing the resulting xen-syms.

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

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

[PATCH v2 1/5] xen/misc: Functional cleanup for __attribute__((packed)) changes

[Andrew Cooper]

This is to separate the functional changes from the noop consistency changes.

* Pack struct cper_mce_record rather than creating a structure named __packed
* Remove unreferenced struct xgt_desc
* Use two u16's rather than two u32 16-bit bitfields

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Eddie Dong <eddie.dong@intel.com>
---
 xen/arch/x86/cpu/mcheck/mce-apei.c |    2 +-
 xen/arch/x86/hvm/vmx/vmcs.c        |    5 -----
 xen/common/trace.c                 |    2 +-
 3 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce-apei.c b/xen/arch/x86/cpu/mcheck/mce-apei.c
index 3370341..f9168b0 100644
--- a/xen/arch/x86/cpu/mcheck/mce-apei.c
+++ b/xen/arch/x86/cpu/mcheck/mce-apei.c
@@ -53,7 +53,7 @@ struct cper_mce_record {
 	struct cper_record_header hdr;
 	struct cper_section_descriptor sec_hdr;
 	struct mce mce;
-} __packed;
+} __attribute__((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 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/common/trace.c b/xen/common/trace.c
index 41ddc33..73ba57c 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -643,7 +643,7 @@ static inline void insert_lost_records(struct t_buf *buf)
 {
     struct {
         u32 lost_records;
-        u32 did:16, vid:16;
+        u16 did, vid;
         u64 first_tsc;
     } __attribute__((packed)) ed;
 
-- 
1.7.10.4

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

[Andrew Cooper]

This introduced a formal define in compiler.h, and is otherwise manual
shuffling of __attribute__((packed)) statements to __packed at the head of the
structure.

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

diff --git a/xen/common/trace.c b/xen/common/trace.c
index 73ba57c..1814165 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;
         u16 did, vid;
         u64 first_tsc;
-    } __attribute__((packed)) ed;
+    } ed;
 
     ed.vid = current->vcpu_id;
     ed.did = current->domain->domain_id;
@@ -819,10 +819,10 @@ unlock:
 void __trace_hypercall(uint32_t event, unsigned long op,
                        const unsigned long *args)
 {
-    struct {
+    struct __packed {
         uint32_t op;
         uint32_t args[6];
-    } __attribute__((packed)) d;
+    } d;
     uint32_t *a = d.args;
 
 #define APPEND_ARG32(i)                         \
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/drivers/char/ehci-dbgp.c b/xen/drivers/char/ehci-dbgp.c
index b900d60..3feeafe 100644
--- a/xen/drivers/char/ehci-dbgp.c
+++ b/xen/drivers/char/ehci-dbgp.c
@@ -251,25 +251,25 @@ struct ehci_dbg_port {
  * For most devices, interfaces don't coordinate with each other, so
  * such requests may be made at any time.
  */
-struct usb_ctrlrequest {
+struct __packed usb_ctrlrequest {
     u8 bRequestType;
     u8 bRequest;
     __le16 wValue;
     __le16 wIndex;
     __le16 wLength;
-} __attribute__ ((packed));
+};
 
 /* USB_DT_DEBUG: for special highspeed devices, replacing serial console */
 
 #define USB_DT_DEBUG    0x0a
 
-struct usb_debug_descriptor {
+struct __packed usb_debug_descriptor {
     u8 bLength;
     u8 bDescriptorType;
     /* bulk endpoints with 8 byte maxpacket */
     u8 bDebugInEndpoint;
     u8 bDebugOutEndpoint;
-} __attribute__((packed));
+};
 
 #define USB_DEBUG_DEVNUM 127
 
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

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

[Andrew Cooper]

This is almost all manual shuffling of __attribute__((packed)) statements to
__packed at the head of the structure.

The only different change is in tboot.h, removing some now-redundant ifdefs.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: 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/oprofile/backtrace.c  |    8 ++--
 xen/arch/x86/trace.c               |   28 ++++++-------
 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          |   76 ++++++++++++++++++------------------
 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, 116 insertions(+), 120 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 f9168b0..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;
-} __attribute__((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/oprofile/backtrace.c b/xen/arch/x86/oprofile/backtrace.c
index b3ea7f3..94bd24c 100644
--- a/xen/arch/x86/oprofile/backtrace.c
+++ b/xen/arch/x86/oprofile/backtrace.c
@@ -15,17 +15,17 @@
 #include <xen/xenoprof.h>
 #include <xen/guest_access.h>
 
-struct frame_head {
+struct __packed 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 {
+struct __packed 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..64e9ad0 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;
@@ -125,18 +125,18 @@ void __trace_trap_two_addr(unsigned event, unsigned long va1,
 {
     if ( is_pv_32on64_vcpu(current) )
     {
-        struct {
+        struct __packed {
             u32 va1, va2;
-        } __attribute__((packed)) d;
+        } d;
         d.va1=va1;
         d.va2=va2;
         __trace_var(event, 1, sizeof(d), &d);
     }
     else
     {
-        struct {
+        struct __packed {
             unsigned long va1, va2;
-        } __attribute__((packed)) d;
+        } d;
         d.va1=va1;
         d.va2=va2;
         event |= TRC_64_FLAG;
@@ -158,10 +158,10 @@ void __trace_ptwr_emulation(unsigned long addr, l1_pgentry_t npte)
 
     if ( is_pv_32on64_vcpu(current) )
     {
-        struct {
+        struct __packed {
             l1_pgentry_t pte;
             u32 addr, eip;
-        } __attribute__((packed)) d;
+        } d;
         d.addr = addr;
         d.eip = eip;
         d.pte = npte;
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 5544ba5..afaa237 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;
@@ -51,97 +51,97 @@ struct edd_info {
         u8 host_bus_type[4];
         u8 interface_type[8];
         union {
-            struct {
+            struct __packed {
                 u16 base_address;
                 u16 reserved1;
                 u32 reserved2;
-            } __attribute__ ((packed)) isa;
-            struct {
+            } isa;
+            struct __packed {
                 u8 bus;
                 u8 slot;
                 u8 function;
                 u8 channel;
                 u32 reserved;
-            } __attribute__ ((packed)) pci;
+            } pci;
             /* pcix is same as pci */
-            struct {
+            struct __packed {
                 u64 reserved;
-            } __attribute__ ((packed)) ibnd;
-            struct {
+            } ibnd;
+            struct __packed {
                 u64 reserved;
-            } __attribute__ ((packed)) xprs;
-            struct {
+            } xprs;
+            struct __packed {
                 u64 reserved;
-            } __attribute__ ((packed)) htpt;
-            struct {
+            } htpt;
+            struct __packed {
                 u64 reserved;
-            } __attribute__ ((packed)) unknown;
+            } unknown;
         } interface_path;
         union {
-            struct {
+            struct __packed {
                 u8 device;
                 u8 reserved1;
                 u16 reserved2;
                 u32 reserved3;
                 u64 reserved4;
-            } __attribute__ ((packed)) ata;
-            struct {
+            } ata;
+            struct __packed {
                 u8 device;
                 u8 lun;
                 u8 reserved1;
                 u8 reserved2;
                 u32 reserved3;
                 u64 reserved4;
-            } __attribute__ ((packed)) atapi;
-            struct {
+            } atapi;
+            struct __packed {
                 u16 id;
                 u64 lun;
                 u16 reserved1;
                 u32 reserved2;
-            } __attribute__ ((packed)) scsi;
-            struct {
+            } scsi;
+            struct __packed {
                 u64 serial_number;
                 u64 reserved;
-            } __attribute__ ((packed)) usb;
-            struct {
+            } usb;
+            struct __packed {
                 u64 eui;
                 u64 reserved;
-            } __attribute__ ((packed)) i1394;
-            struct {
+            } i1394;
+            struct __packed {
                 u64 wwid;
                 u64 lun;
-            } __attribute__ ((packed)) fibre;
-            struct {
+            } fibre;
+            struct __packed {
                 u64 identity_tag;
                 u64 reserved;
-            } __attribute__ ((packed)) i2o;
-            struct {
+            } i2o;
+            struct __packed {
                 u32 array_number;
                 u32 reserved1;
                 u64 reserved2;
-            } __attribute__ ((packed)) raid;
-            struct {
+            } raid;
+            struct __packed {
                 u8 device;
                 u8 reserved1;
                 u16 reserved2;
                 u32 reserved3;
                 u64 reserved4;
-            } __attribute__ ((packed)) sata;
-            struct {
+            } sata;
+            struct __packed {
                 u64 reserved1;
                 u64 reserved2;
-            } __attribute__ ((packed)) unknown;
+            } unknown;
         } 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

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

[Andrew Cooper]

This is all mechanical shuffling.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: 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

[PATCH v2 5/5] xen/x86_emulate: Shuffle use of __attribute__((packed))

[Andrew Cooper]

Also include #defines for the test code to allow compilation.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

---

I have mixed thoughts about this, which is why it is explicit separated from
the rest of the series.  On the one hand, consistency is good but on the
other, it makes x86_emulate harder to use as a dropin in other code.
---
 tools/tests/x86_emulator/test_x86_emulator.c |    2 ++
 tools/tests/x86_emulator/x86_emulate.c       |    2 ++
 xen/arch/x86/x86_emulate/x86_emulate.h       |    8 ++++----
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c
index 7404ee3..17b674b 100644
--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -7,6 +7,8 @@
 #include <xen/xen.h>
 #include <sys/mman.h>
 
+#define __packed __attribute__((packed))
+
 #include "x86_emulate/x86_emulate.h"
 #include "blowfish.h"
 
diff --git a/tools/tests/x86_emulator/x86_emulate.c b/tools/tests/x86_emulator/x86_emulate.c
index b157ade..ef9bfe9 100644
--- a/tools/tests/x86_emulator/x86_emulate.c
+++ b/tools/tests/x86_emulator/x86_emulate.c
@@ -14,5 +14,7 @@ typedef bool bool_t;
 #define cpu_has_amd_erratum(nr) 0
 #define mark_regs_dirty(r) ((void)(r))
 
+#define __packed __attribute__((packed))
+
 #include "x86_emulate/x86_emulate.h"
 #include "x86_emulate/x86_emulate.c"
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().
-- 
1.7.10.4

Re: [PATCH v2 5/5] xen/x86_emulate: Shuffle use of __attribute__((packed))

[Keir Fraser]

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

Andrew Cooper wrote:
>
> Also include #defines for the test code to allow compilation.
>
> Signed-off-by: Andrew Cooper<andrew.cooper3@citrix.com>
> CC: Keir Fraser<keir@xen.org>
> CC: Jan Beulich<JBeulich@suse.com>
> CC: Ian Campbell<Ian.Campbell@citrix.com>
> CC: Ian Jackson<Ian.Jackson@eu.citrix.com>
>
> ---
>
> I have mixed thoughts about this, which is why it is explicit 
> separated from
> the rest of the series. On the one hand, consistency is good but on the
> other, it makes x86_emulate harder to use as a dropin in other code.


We already have a bunch of external definitions that must be provided 
for x86_emulate.c. This merely adds another, but which is required for 
x86_emulate.h. So it's perfectly fine by me.

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

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

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

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

Re: [PATCH v2 1/5] xen/misc: Functional cleanup for __attribute__((packed)) changes

[Jan Beulich]

>>> On 13.03.14 at 16:04, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/cpu/mcheck/mce-apei.c
> +++ b/xen/arch/x86/cpu/mcheck/mce-apei.c
> @@ -53,7 +53,7 @@ struct cper_mce_record {
>  	struct cper_record_header hdr;
>  	struct cper_section_descriptor sec_hdr;
>  	struct mce mce;
> -} __packed;
> +} __attribute__((packed));
>  /* Reset to default packing */
>  #pragma pack()

I'm sorry for paying attention only now, but you apparently didn't
notice this either: The #pragma pack() around that structure was
the apparent attempt to deal with __packed "not functioning". We
should get rig of these pragmas at the same time I think.

Jan

Re: [PATCH v2 1/5] xen/misc: Functional cleanup for __attribute__((packed)) changes

[Andrew Cooper]

On 13/03/14 16:22, Jan Beulich wrote:
>>>> On 13.03.14 at 16:04, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/cpu/mcheck/mce-apei.c
>> +++ b/xen/arch/x86/cpu/mcheck/mce-apei.c
>> @@ -53,7 +53,7 @@ struct cper_mce_record {
>>  	struct cper_record_header hdr;
>>  	struct cper_section_descriptor sec_hdr;
>>  	struct mce mce;
>> -} __packed;
>> +} __attribute__((packed));
>>  /* Reset to default packing */
>>  #pragma pack()
> I'm sorry for paying attention only now, but you apparently didn't
> notice this either: The #pragma pack() around that structure was
> the apparent attempt to deal with __packed "not functioning". We
> should get rig of these pragmas at the same time I think.
>
> Jan
>

I did notice that, but was hoping to ignore it for the time being (as
this __packed series has become rather longer than the half hour task I
planned it to be)

I shall do a new following patch which replaces the use of #pragma
pack() in Xen, which doesn't look too difficult.

~Andrew