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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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