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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.03.14 at 20:08, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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


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