hw/usb: Use __attribute__((packed)) vs __packed

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

Posted by Roque Arcudia Hernandez 3 weeks ago

__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;
 
 /* Host Mode DMA descriptor status quadlet */
 
-- 
2.47.0.163.g1226f6d8fa-goog

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

Posted by Thomas Huth 2 weeks, 5 days ago

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?

  Thomas

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

Posted by Michael Tokarev 2 days, 7 hours ago

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.

/mjt

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

Posted by Thomas Huth 2 days, 7 hours ago

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.

  Thomas

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

Posted by Paolo Bonzini 2 days, 6 hours ago

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

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

Posted by Pierrick Bouvier 3 weeks ago

On 11/1/24 14: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;
>   
>   /* Host Mode DMA descriptor status quadlet */
>   

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>