Re: [PATCH] hw/usb: Use __attribute__((packed)) vs __packed

[Paolo Bonzini <pbonzini@redhat.com>]

On 11/21/24 11:24, Thomas Huth wrote:
> On 21/11/2024 11.01, Michael Tokarev wrote:
>> 04.11.2024 10:37, Thomas Huth wrote:
>>> On 01/11/2024 22.17, Roque Arcudia Hernandez wrote:
>>>> __packed is non standard and is not present in clang-cl.
>>>> __attribute__((packed)) has the same semantics.
>>>>
>>>> Signed-off-by: Erwin Jansen <jansene@google.com>
>>>> Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
>>>> ---
>>>>   include/hw/usb/dwc2-regs.h | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/hw/usb/dwc2-regs.h b/include/hw/usb/dwc2-regs.h
>>>> index 523b112c5e..b8b4266543 100644
>>>> --- a/include/hw/usb/dwc2-regs.h
>>>> +++ b/include/hw/usb/dwc2-regs.h
>>>> @@ -838,7 +838,7 @@
>>>>   struct dwc2_dma_desc {
>>>>           uint32_t status;
>>>>           uint32_t buf;
>>>> -} __packed;
>>>> +} QEMU_PACKED;
>>>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>
>>> Actually, the struct only consists of two 32-bit values, so I doubt 
>>> that the "packed" is needed here at all. Maybe we could even simply 
>>> remove it?
>> To me it is important to mark structures as packed if it is
>> important for them to have strict layout like in this case,
>> even if de-facto it does not change the actual layout.  It's
>> just like an annotation saying this structure can be used
>> on wire or somesuch.
> 
> Well, it can have an impact on your binary, too. On architectures that 
> don't support unaligned memory accesses, the compiler has to generate 
> code that reads the values of packed structures in a more cumbersome 
> way. So if you used "packed" though it's not really necessary (i.e. 
> everything is naturally aligned, and you can be sure that the starting 
> address is also properly aligned), the compiler generates worse code 
> than necessary in that case.
Well, in this case the struct is downright unused. :)  But we can't know 
if it is naturally aligned or not in guest memory, so it is advisable to 
keep the QEMU_PACKED.

Paolo