[Qemu-devel] [PATCH] pc: e820 qemu_cfg tables need to be packed

[Qemu-devel] [PATCH] pc: e820 qemu_cfg tables need to be packed

[Alex Williamson]

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(-)

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;
 

[Qemu-devel] Re: [PATCH] pc: e820 qemu_cfg tables need to be packed

[Jes Sorensen]

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

Re: [Qemu-devel] Re: [PATCH] pc: e820 qemu_cfg tables need to be packed

[Anthony Liguori]

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
>
>    

Re: [Qemu-devel] Re: [PATCH] pc: e820 qemu_cfg tables need to be packed

[Alex Williamson]

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

Re: [Qemu-devel] Re: [PATCH] pc: e820 qemu_cfg tables need to be packed

[Anthony Liguori]

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
>
>    

Re: [Qemu-devel] Re: [PATCH] pc: e820 qemu_cfg tables need to be packed

[Arnd Bergmann]

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

Re: [Qemu-devel] Re: [PATCH] pc: e820 qemu_cfg tables need to be packed

[Alex Williamson]

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

Re: [Qemu-devel] Re: [PATCH] pc: e820 qemu_cfg tables need to be packed

[Arnd Bergmann]

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

Re: [Qemu-devel] Re: [PATCH] pc: e820 qemu_cfg tables need to be packed

[Alex Williamson]

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

[Qemu-devel] [PATCH v2] pc: e820 qemu_cfg tables need to be packed

[Alex Williamson]

We can't let the compiler define the alignment for qemu_cfg data.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 v2: Adjust alignment to help non-x86 hosts per Arnd's suggestion

 0.13-stable candidate

 hw/pc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 69b13bf..0264e3d 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__, __aligned__(4)));
 
 struct e820_table {
     uint32_t count;
     struct e820_entry entry[E820_NR_ENTRIES];
-};
+} __attribute((__packed__, __aligned__(4)));
 
 static struct e820_table e820_table;
 

[Qemu-devel] Re: [PATCH v2] pc: e820 qemu_cfg tables need to be packed

[Arnd Bergmann]

On Friday 15 October 2010, 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>
> ---
> 
>  v2: Adjust alignment to help non-x86 hosts per Arnd's suggestion

Ok, looks good now. Thanks!

	Arnd