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

[Jean Guyader <jean.guyader@xxxxxxxxx>]

On 19 July 2012 10:54, Attilio Rao <attilio.rao@xxxxxxxxxx> 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.
>>
>>
>
>
> Can you use gcc __attribute__((aligned(64))) for this? Or we try to avoid
> gcc-ism at all?
>

I'm trying to avoid any compiler specific statement. I'll added some
manual padding
to have it explicitly aligned to 64b.

Jean

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel