[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 4/5] xen: Add V4V implementation
On Thu, 19 Jul 2012, Andrew Cooper wrote: > On 19/07/12 10:58, Jean Guyader wrote: > > On 19 July 2012 10:34, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote: > >> On 18/07/12 21:09, Jean Guyader wrote: > >>> On 29 June 2012 11:36, Jan Beulich <JBeulich@xxxxxxxx> wrote: > >>>>>>> On 29.06.12 at 12:03, Jean Guyader <jean.guyader@xxxxxxxxx> wrote: > >>>>> On 29 June 2012 09:33, Jan Beulich <JBeulich@xxxxxxxx> wrote: > >>>>>>>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@xxxxxxxxxx> wrote: > >>>>>>> +typedef struct v4v_ring_id > >>>>>>> +{ > >>>>>>> + struct v4v_addr addr; > >>>>>>> + domid_t partner; > >>>>>>> +} V4V_PACKED v4v_ring_id_t; > >>>>>>> + > >>>>> This structure is really the one that cause trouble. domid_t is 16b > >>>>> and v4v_addr_t is used > >>>>> inside v4v_ring_t. I would like the structure to remind as close as we > >>>>> can from the original version > >>>>> as we already versions in the field. Having explicit padding will make > >>>>> all the structures different > >>>>> which will make much harder to write a driver that will support the > >>>>> two versions of the API. > >>>> Oh, I see, "partner" would end up on a different offset if the > >>>> packed attribute was removed from v4v_addr_t. But that > >>>> could still be solved by making this type a union: > >>>> > >>>> typedef union v4v_ring_id > >>>> { > >>>> struct v4v_addr addr; > >>>> struct { > >>>> uint32_t port; > >>>> domid_t domain; > >>>> domid_t partner; > >>>> } full; > >>>> } v4v_ring_id_t; > >>>> > >>>> That would guarantee binary compatibility. And you could even > >>>> achieve source compatibility for gcc users by making the naming > >>>> of the second structure conditional upon __GNUC__ being > >>>> undefined (or adding a second instance of the same, just > >>>> unnamed structure within a respective #ifdef - that would make > >>>> it possible to write code that can be compiled by both gcc and > >>>> non-gcc, yet existing gcc-only code would need changing). > >>>> > >>>>> Also most all the consumer of those headers will have to rewrite the > >>>>> structure anyway, for instance > >>>>> the Linux kernel have it's own naming convention, macros definitions > >>>>> which are different, etc.. > >>>> Such can usually be done via scripts, so having a fully defined > >>>> public header is still worthwhile. > >>>> > >>> Hi, > >>> > >>> I've been working on this and it work for most of it apart from one case. > >>> Let's take this structure: > >>> > >>> struct a > >>> { > >>> uint64_t a; > >>> uint32_t b; > >> uint32_t _pad0; > >>> uint16_t c; > >>> uint16_t d; > >>> uint32_t e; > >>> uint32_t f; > >>> uint32_t g; > >>> uint8_t h[32]; > >>> uint8_t q[0]; > >>> }; > >> Manually padding so the alignment is the same on 32 and 64 bit is the > >> only way to do this in the public headers, which cant have gcc'isms for > >> compatibility reasons with other compilers. > >> > > The problem isn't with the individual fields (they are all correctly > > aligned) it is > > the the overall structure size which is 64 even so offset of q is 60 > > (and sizeof q > > should be 0). > > > > I think there is no way around it. The structure I have should be > > aligned on 64b anyway. > > > > Thanks, > > Jean > > Ah yes - silly me. I understand your problem now > > struct b > { > uint64_t a; > uint32_t b; > uint16_t c; > uint16_t d; > uint32_t e; > uint32_t f; > uint32_t g; > uint8_t h[32]; > union { uint8_t q[0]; uint32_t _pad; } u; > }; > > This works for me on gcc and gives identical sizeof and offsetof results > on both 32 and 64bit. Yes, but then the access to q becomes b.u.q unless we use anonymous unions that are only available in C11. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |