| Message ID | 20101014183249.23510.29196.stgit@s20.home |
|---|---|
| State | New |
| Headers | show |
On 10/14/10 20:33, Alex Williamson wrote: > We can't let the compiler define the alignment for qemu_cfg data. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > > 0.13 stable candidate? ACK I would say so. Jes
On 10/14/2010 02:44 PM, Jes Sorensen wrote: > On 10/14/10 20:33, Alex Williamson wrote: > >> We can't let the compiler define the alignment for qemu_cfg data. >> >> Signed-off-by: Alex Williamson<alex.williamson@redhat.com> >> --- >> >> 0.13 stable candidate? >> > ACK I would say so. > fw_cfg interfaces are somewhat difficult to rationalize about for compatibility. 0.13.0 is tagged already so it's too late to pull it in there. If we say we don't care about compatibility at the fw_cfg level, then it doesn't matter if we pull it into stable-0.13. If we do care, then this is an ABI breaker. I don't know that the answer is obvious to me. Regards, Anthony Liguori > Jes > >
On Thu, 2010-10-14 at 14:48 -0500, Anthony Liguori wrote: > On 10/14/2010 02:44 PM, Jes Sorensen wrote: > > On 10/14/10 20:33, Alex Williamson wrote: > > > >> We can't let the compiler define the alignment for qemu_cfg data. > >> > >> Signed-off-by: Alex Williamson<alex.williamson@redhat.com> > >> --- > >> > >> 0.13 stable candidate? > >> > > ACK I would say so. > > > > fw_cfg interfaces are somewhat difficult to rationalize about for > compatibility. > > 0.13.0 is tagged already so it's too late to pull it in there. If we > say we don't care about compatibility at the fw_cfg level, then it > doesn't matter if we pull it into stable-0.13. If we do care, then this > is an ABI breaker. If it works anywhere (I assume it works on 32bit), then it's only because it happened to get the alignment right. This just makes 64bit hosts get it right too. I don't see any compatibility issues, non-packed + 64bit = broken. Thanks, Alex
On 10/14/2010 02:58 PM, Alex Williamson wrote: > On Thu, 2010-10-14 at 14:48 -0500, Anthony Liguori wrote: > >> On 10/14/2010 02:44 PM, Jes Sorensen wrote: >> >>> On 10/14/10 20:33, Alex Williamson wrote: >>> >>> >>>> We can't let the compiler define the alignment for qemu_cfg data. >>>> >>>> Signed-off-by: Alex Williamson<alex.williamson@redhat.com> >>>> --- >>>> >>>> 0.13 stable candidate? >>>> >>>> >>> ACK I would say so. >>> >>> >> fw_cfg interfaces are somewhat difficult to rationalize about for >> compatibility. >> >> 0.13.0 is tagged already so it's too late to pull it in there. If we >> say we don't care about compatibility at the fw_cfg level, then it >> doesn't matter if we pull it into stable-0.13. If we do care, then this >> is an ABI breaker. >> > If it works anywhere (I assume it works on 32bit), then it's only > because it happened to get the alignment right. This just makes 64bit > hosts get it right too. I don't see any compatibility issues, > non-packed + 64bit = broken. Thanks, > Ok, I'll buy that argument :-) Regards, Anthony Liguori > Alex > >
On Thursday 14 October 2010 21:58:08 Alex Williamson wrote: > If it works anywhere (I assume it works on 32bit), then it's only > because it happened to get the alignment right. This just makes 64bit > hosts get it right too. I don't see any compatibility issues, > non-packed + 64bit = broken. Thanks, I would actually assume that only x86-32 hosts got it right, because all 32 bit hosts I've seen other than x86 also define 8 byte alignment for uint64_t. You might however consider making it __attribute((__packed__, __aligned__(4))) instead of just packed, because otherwise you make the alignment one byte, which is not only different from what it used to be on x86-32 but also will cause inefficient compiler outpout on platforms that don't have unaligned word accesses in hardware. Arnd
On Thu, 2010-10-14 at 22:20 +0200, Arnd Bergmann wrote: > On Thursday 14 October 2010 21:58:08 Alex Williamson wrote: > > If it works anywhere (I assume it works on 32bit), then it's only > > because it happened to get the alignment right. This just makes 64bit > > hosts get it right too. I don't see any compatibility issues, > > non-packed + 64bit = broken. Thanks, > > I would actually assume that only x86-32 hosts got it right, because > all 32 bit hosts I've seen other than x86 also define 8 byte alignment > for uint64_t. > > You might however consider making it > > __attribute((__packed__, __aligned__(4))) > > instead of just packed, because otherwise you make the alignment one byte, > which is not only different from what it used to be on x86-32 but also > will cause inefficient compiler outpout on platforms that don't have unaligned > word accesses in hardware. The structs in question only contain 4 & 8 byte elements, so there shouldn't be any change on x86-32 using one-byte aligned packing. AFAIK, e820 is x86-only, so we don't need to worry about breaking anyone else. Performance isn't much of a consideration for this type of interface since it's only used pre-boot. In fact, the channel between qemu and the bios is only one byte wide, so wider alignment can cost extra emulated I/O accesses. Thanks, Alex
On Thursday 14 October 2010 22:59:04 Alex Williamson wrote: > The structs in question only contain 4 & 8 byte elements, so there > shouldn't be any change on x86-32 using one-byte aligned packing. I'm talking about the alignment of the structure, not the members within the structure. The data structure should be compatible, but not accesses to it. > AFAIK, e820 is x86-only, so we don't need to worry about breaking anyone > else. You can use qemu to emulate an x86 pc on anything... > Performance isn't much of a consideration for this type of > interface since it's only used pre-boot. In fact, the channel between > qemu and the bios is only one byte wide, so wider alignment can cost > extra emulated I/O accesses. Right, the data gets passed as bytes, so it hardly matters in the end. Still the e820_add_entry assigns data to the struct members, which it either does using byte accesses and shifts or a multiple 32 bit assignment. Just because using a one byte alignment technically results in correct output doesn't make it the right solution. I don't care about the few cycles of execution time or the few bytes you waste in this particular case, but you are setting a wrong example by using smaller alignment than necessary. Arnd
On Thu, 2010-10-14 at 23:19 +0200, Arnd Bergmann wrote: > On Thursday 14 October 2010 22:59:04 Alex Williamson wrote: > > The structs in question only contain 4 & 8 byte elements, so there > > shouldn't be any change on x86-32 using one-byte aligned packing. > > I'm talking about the alignment of the structure, not the members > within the structure. The data structure should be compatible, but > not accesses to it. > > > AFAIK, e820 is x86-only, so we don't need to worry about breaking anyone > > else. > > You can use qemu to emulate an x86 pc on anything... You're right, I wasn't thinking about non-x86 emulating x86. I'll send a v2 with your suggestion. Thanks, Alex
diff --git a/hw/pc.c b/hw/pc.c index 69b13bf..90839bd 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -75,12 +75,12 @@ struct e820_entry { uint64_t address; uint64_t length; uint32_t type; -}; +} __attribute__((__packed__)); struct e820_table { uint32_t count; struct e820_entry entry[E820_NR_ENTRIES]; -}; +} __attribute__((__packed__)); static struct e820_table e820_table;
We can't let the compiler define the alignment for qemu_cfg data. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- 0.13 stable candidate? hw/pc.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)