Re: [Xen-devel] [PATCH 4/5] xen: Add V4V implementation

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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