Re: [PATCH] ps3: add vuart support

[Geoff Levand <geoffrey.levand@am.sony.com>]

Geert Uytterhoeven wrote:
> On Thu, 7 Dec 2006, Geoff Levand wrote:
>> Arnd Bergmann wrote:
>> > On Thursday 07 December 2006 01:17, Geoff Levand wrote:
>> >> Adds support for the PS3 virtual UART (vuart).  The vuart provides a
>> >> bi-directional byte stream data link between logical partitions.
>> > 
>> > Looks good now, just a few minor comments:
>> > 
>> >> +struct ports_bmp
>> >> +{
>> >> +	u64 status;
>> >> +	u64 unused[3];
>> >> +} __attribute__ ((packed));
>> > 
>> > Do you really want to have this structure packed? It will produce
>> > better code if it isn't.
>> 
>> Yes, it is a little obscure in the code, but the address of the ports_bmp
>> instance is passed to ps3_alloc_vuart_irq(), which in turn calls
>> lv1_configure_virtual_uart_irq().  The HV (actually the vuart service
>> in the policy module) uses this as a 256 bit bitmap to indicate the
>> interrupt status of the ports.  The system supports 256 ports, but
>> currently just two ports, 1 and 3, are used, so I took advantage of
>> that to simplify the code and just used the first word of the bitmap.
> 
> I think Arnd meant that the `__attribute__ ((packed))' is useless for structs
> containing u64 members only. The natural alignment of 64-bit values is on a
> 64-bit boundary, so there won't be padding anyway.
> 
> IIRC, someone (davem?) told me a long time ago gcc _always_ uses byte-accesses
> when accessing members in a struct with `__attribute__ ((packed))', since the
> members may be unaligned. Even when the values are perfectly aligned. So it's
> better not to use `__attribute__ ((packed))' unless it's really needed.
> 
> Why is there a comment about `Needs 4 byte alignment.'? The way it's used, it
> will always be 8-byte aligned anyway.


After some discussion on IRC I understand that my comment above is in error.

The packed attribute forces both the structure members and the structure instance
to have 1-byte alignment, so it will not have an 8-byte alignment, but a 1-byte.
In the usage here though, the ports_bmp instance follows an aligned long, so it
by chance starts at 8-byte aligned address.

The 4-byte alignment is a requirement of the HV, so I just noted that in
the comment.  I'll change the comment to be more clear.

-Geoff