| Message ID | 20241101211720.3354111-1-roqueh@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/usb: Use __attribute__((packed)) vs __packed | expand |
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>
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
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
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
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
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 */