Re: [Qemu-devel] [PATCH 0/7] Fix packing for MinGW with -mms-bitfields

[Kevin Wolf]

Am 29.08.2011 21:55, schrieb Stefan Weil:
> Am 29.08.2011 10:34, schrieb TeLeMan:
>> On Mon, Aug 29, 2011 at 13:01, Stefan Weil <address@hidden> wrote:
>>> Am 28.08.2011 23:43, schrieb Blue Swirl:
>>>>
>>>> On Sun, Aug 28, 2011 at 8:43 PM, Stefan Weil <address@hidden> 
>>>> wrote:
>>>>>
>>>>> These patches fix the packing of structures which were affected by
>>>>> the new compiler attribute -mms-bitfields (which is needed for 
>>>>> glib-2.0).
>>>>>
>>>>> I compiled qemu.exe with and without -mms-bitfields and compared
>>>>> the resulting struct alignment using pahole and codiff.
>>>>
>>>> If a structure is only used internally by QEMU (not used in network,
>>>> disk or guest interfaces), changes in padding don't matter. In fact,
>>>> in those cases it may be better to remove the packing, because then
>>>> the fields may be naturally aligned and that gives better performance
>>>> on most architectures. Could you please check if this is the case for
>>>> any of the structs?
>>>
>>> I did this already, but also forward your question to the maintainers.
>>> Here is my result:
>>>
>>> [PATCH 2/7] block/vvfat: Fix packing for w32: needs packing (disk)
>>> [PATCH 3/7] acpi: Fix packing for w32: needs packing (bios interface)
>>> [PATCH 4/7] hpet: Fix packing for w32: needs packing (bios interface)
>>> [PATCH 5/7] usb: Fix packing for w32: needs packing (usb interface)
>>> [PATCH 6/7] virtio: Fix packing for w32: needs packing? (guest 
>>> interface?)
>>> [PATCH 7/7] slirp: Fix packing for w32: needs packing (network interface)
>>>
>>> All those struct statements need the pack attribute (otherwise the code
>>> would have to be rewritten which is of course always possible).
>> gesn_cdb in atapi.c, VMDK4Header in vmdk.c and many structures in
>> bt.h need be fixed too.
> 
> Oops, you are right. Obviously I missed all anonymous structs:
> codiff simply ignores them, and pahole must be called with
> flags -a -A to show them. Who invented packing of structs?
> 
> Comparing the output of pahole -a -A is less elegant than using
> codiff, but shows the structs which you mentioned.
> 
> I suggest to apply my patch series first because it fixes
> the most important bugs in networking. The remaining
> bugs are in code which is used less often. They will be
> fixed by a second patch series which replaces all remaining
> packed attributes.

Shouldn't we have a look at every packed structure instead of just
fixing what we notice as broken in the x86 emulator binary with one
given configuration? I think if there is a QEMU_PACKED, we should use it
consistently, or is there a reason not to do so?

Kevin