From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from outbound2-sin-R.bigfish.com (outbound-sin.frontbridge.com [207.46.51.80]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "*.bigfish.com", Issuer "*.bigfish.com" (not verified)) by ozlabs.org (Postfix) with ESMTP id 6AC4967CFC for ; Fri, 8 Dec 2006 09:01:18 +1100 (EST) Message-ID: <45788F20.8030804@am.sony.com> Date: Thu, 07 Dec 2006 14:01:04 -0800 From: Geoff Levand MIME-Version: 1.0 To: Geert Uytterhoeven Subject: Re: [PATCH] ps3: add vuart support References: <45775D89.7030904@am.sony.com> <200612071937.22607.arnd@arndb.de> <457879BE.9000704@am.sony.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Cc: linuxppc-dev@ozlabs.org, Paul Mackerras , Arnd Bergmann List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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