* [PATCH] hw/usb: Use __attribute__((packed)) vs __packed @ 2024-11-01 21:17 Roque Arcudia Hernandez 2024-11-01 21:32 ` Pierrick Bouvier 2024-11-04 7:37 ` Thomas Huth 0 siblings, 2 replies; 6+ messages in thread From: Roque Arcudia Hernandez @ 2024-11-01 21:17 UTC (permalink / raw) To: thuth, richard.henderson, jansene, mjt Cc: qemu-devel, Roque Arcudia Hernandez __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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/usb: Use __attribute__((packed)) vs __packed 2024-11-01 21:17 [PATCH] hw/usb: Use __attribute__((packed)) vs __packed Roque Arcudia Hernandez @ 2024-11-01 21:32 ` Pierrick Bouvier 2024-11-04 7:37 ` Thomas Huth 1 sibling, 0 replies; 6+ messages in thread From: Pierrick Bouvier @ 2024-11-01 21:32 UTC (permalink / raw) To: Roque Arcudia Hernandez, thuth, richard.henderson, jansene, mjt Cc: qemu-devel 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/usb: Use __attribute__((packed)) vs __packed 2024-11-01 21:17 [PATCH] hw/usb: Use __attribute__((packed)) vs __packed Roque Arcudia Hernandez 2024-11-01 21:32 ` Pierrick Bouvier @ 2024-11-04 7:37 ` Thomas Huth 2024-11-21 10:01 ` Michael Tokarev 1 sibling, 1 reply; 6+ messages in thread From: Thomas Huth @ 2024-11-04 7:37 UTC (permalink / raw) To: Roque Arcudia Hernandez, richard.henderson, jansene, mjt Cc: qemu-devel, Gerd Hoffmann, QEMU Trivial 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/usb: Use __attribute__((packed)) vs __packed 2024-11-04 7:37 ` Thomas Huth @ 2024-11-21 10:01 ` Michael Tokarev 2024-11-21 10:24 ` Thomas Huth 0 siblings, 1 reply; 6+ messages in thread From: Michael Tokarev @ 2024-11-21 10:01 UTC (permalink / raw) To: Thomas Huth, Roque Arcudia Hernandez, richard.henderson, jansene Cc: qemu-devel, Gerd Hoffmann, QEMU Trivial 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/usb: Use __attribute__((packed)) vs __packed 2024-11-21 10:01 ` Michael Tokarev @ 2024-11-21 10:24 ` Thomas Huth 2024-11-21 11:23 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Thomas Huth @ 2024-11-21 10:24 UTC (permalink / raw) To: Michael Tokarev, Roque Arcudia Hernandez, richard.henderson, jansene Cc: qemu-devel, Gerd Hoffmann, QEMU Trivial 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/usb: Use __attribute__((packed)) vs __packed 2024-11-21 10:24 ` Thomas Huth @ 2024-11-21 11:23 ` Paolo Bonzini 0 siblings, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2024-11-21 11:23 UTC (permalink / raw) To: Thomas Huth, Michael Tokarev, Roque Arcudia Hernandez, richard.henderson, jansene Cc: qemu-devel, Gerd Hoffmann, QEMU Trivial 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-11-21 11:25 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-11-01 21:17 [PATCH] hw/usb: Use __attribute__((packed)) vs __packed Roque Arcudia Hernandez 2024-11-01 21:32 ` Pierrick Bouvier 2024-11-04 7:37 ` Thomas Huth 2024-11-21 10:01 ` Michael Tokarev 2024-11-21 10:24 ` Thomas Huth 2024-11-21 11:23 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).