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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 19/07/12 10:58, Jean Guyader wrote:
> On 19 July 2012 10:34, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 18/07/12 21:09, Jean Guyader wrote:
>>> On 29 June 2012 11:36, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 29.06.12 at 12:03, Jean Guyader <jean.guyader@gmail.com> wrote:
>>>>> On 29 June 2012 09:33, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@citrix.com> 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.

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com