Re: [Xen-devel] [PATCH] blkif: add indirect descriptors interface to public headers

[Roger Pau Monnà <roger.pau@xxxxxxxxxx>]

On 14/11/13 11:38, Paul Durrant wrote:
>> -----Original Message-----
>> From: Roger Pau Monnà [mailto:roger.pau@xxxxxxxxxx]
>> Sent: 14 November 2013 10:27
>> To: Paul Durrant; Ian Campbell
>> Cc: Konrad Rzeszutek Wilk; xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org);
>> Jan Beulich
>> Subject: Re: [Xen-devel] [PATCH] blkif: add indirect descriptors interface to
>> public headers
>>
>> On 14/11/13 11:14, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Roger Pau Monnà [mailto:roger.pau@xxxxxxxxxx]
>>>> Sent: 14 November 2013 10:06
>>>> To: Paul Durrant; Ian Campbell
>>>> Cc: Konrad Rzeszutek Wilk; xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir
>> (Xen.org);
>>>> Jan Beulich
>>>> Subject: Re: [Xen-devel] [PATCH] blkif: add indirect descriptors interface
>> to
>>>> public headers
>>>>
>>>> On 13/11/13 12:24, Paul Durrant wrote:
>>>>>> -----Original Message-----
>>>>>> From: Ian Campbell
>>>>>> Sent: 13 November 2013 11:11
>>>>>> To: Paul Durrant
>>>>>> Cc: Konrad Rzeszutek Wilk; xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir
>>>> (Xen.org);
>>>>>> Jan Beulich; Roger Pau Monne
>>>>>> Subject: Re: [Xen-devel] [PATCH] blkif: add indirect descriptors
>> interface
>>>> to
>>>>>> public headers
>>>>>>
>>>>>> On Wed, 2013-11-13 at 11:07 +0000, Paul Durrant wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: Ian Campbell
>>>>>>>> Sent: 13 November 2013 09:27
>>>>>>>> To: Paul Durrant
>>>>>>>> Cc: Konrad Rzeszutek Wilk; xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir
>>>>>> (Xen.org);
>>>>>>>> Jan Beulich; Roger Pau Monne
>>>>>>>> Subject: Re: [Xen-devel] [PATCH] blkif: add indirect descriptors
>>>> interface
>>>>>> to
>>>>>>>> public headers
>>>>>>>>
>>>>>>>> On Tue, 2013-11-12 at 15:16 +0000, Paul Durrant wrote:
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Ian Campbell
>>>>>>>>>> Sent: 12 November 2013 14:29
>>>>>>>>>> To: Konrad Rzeszutek Wilk
>>>>>>>>>> Cc: Paul Durrant; xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org);
>> Jan
>>>>>>>> Beulich;
>>>>>>>>>> Roger Pau Monne
>>>>>>>>>> Subject: Re: [Xen-devel] [PATCH] blkif: add indirect descriptors
>>>>>> interface
>>>>>>>> to
>>>>>>>>>> public headers
>>>>>>>>>>
>>>>>>>>>> On Tue, 2013-11-12 at 09:22 -0500, Konrad Rzeszutek Wilk wrote:
>>>>>>>>>>
>>>>>>>>>>>>> +struct blkif_request_indirect {
>>>>>>>>>>>>> +    uint8_t        operation;    /* BLKIF_OP_INDIRECT            
>>>>>>>>>>>>>         */
>>>>>>>>>>>>> +    uint8_t        indirect_op;  /* BLKIF_OP_{READ/WRITE}
>>>>>> */
>>>>>>>>>>>>> +    uint16_t       nr_segments;  /* number of segments
>>>>>> */
>>>>>>>>>>>>
>>>>>>>>>>>> This is going to be a problem. What alignment boundary are you
>>>>>>>>>>> expecting the next field to start on? AFAIK 32-bit gcc will 4-byte
>>>>>>>>>>> align it, 32-bit MSVC will 8-byte align it.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Oh no. I thought that the Linux one had this set correctly, ah it
>> did:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> struct blkif_request_indirect {
>>>>>>>>>>> [...]
>>>>>>>>>>> } __attribute__((__packed__));
>>>>>>>>>>
>>>>>>>>>> That attribute packed isn't allowed in the public interface headers.
>>>>>>>>>>
>>>>>>>>>> Since compilers do differ in their packing, and guests may be using
>>>>>>>>>> various pragmas, it might be useful to write down that for x86
>> these
>>>>>>>>>> headers are to be treated as using the <WHATEVER> ABI (gcc?
>> Some
>>>>>> Intel
>>>>>>>>>> standard?).
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Can we go for types aligned on their size then rather than gcc
>>>>>> brokenness.
>>>>>>>>
>>>>>>>> We should go for some existing well defined ABI spec not make up
>> our
>>>>>>>> own.
>>>>>>>>
>>>>>>>> In effect the x86 ABI has historically been de-facto specified as the
>>>>>>>> gcc ABI.
>>>>>>>>
>>>>>>>
>>>>>>> Since the linux headers seem to hardcode the x64 ABI for this struct,
>>>>>>> do we need to support an x86 variant? After all there's no backwards
>>>>>>> compatibility issue here.
>>>>>>
>>>>>> I am talking about the general case for all xen/include/public headers,
>>>>>> not these structs specifically.
>>>>>>
>>>>>
>>>>> Ah ok. Then yes I guess the x86 gcc ABI has to be the default.
>>>>>
>>>>>> There should be a well specified default for the struct layout. If
>>>>>> particular structs diverge from this (and being consistent across 32-
>>>>>> and 64-bit is a good reason to do so) then suitable padding and perhaps
>>>>>> #ifdefs might be needed.
>>>>>>
>>>>>
>>>>> Yes, agreed. This patch therefore needs to be fixed.
>>>>
>>>> I don't understand why or how this patch should be fixed, the ABI of
>>>> this new structures is defined by the way gcc generates it's layout
>>>> (different on i386 or amd64), it's not pretty, but it's how the blkif
>>>> protocol is defined. Doing something different now just for struct
>>>> blkif_request_indirect seems even worse.
>>>
>>> I don't see where it's defined that the protocol always uses the gcc ABI?
>> And if that's the case then why the need for __attribute__((__packed__)) all
>> over the linux header?
>>
>> AFAIK there's no need for all the __attribute__((__packed__)) in Linux
>> blkif.h header, but it's Linux copy of the header, so it's arguably that
>> Linux can define those as wanted, as long as they have the same layout
>> as the one generated by a pristine copy of blkif.h from the Xen tree (as
>> it is now).
>>
>> __attribute__((__packed__)) should only be needed in blkback in order to
>> define the i386 and amd64 version of those structures and handle
>> correctly requests from an i386 DomU on an amd64 Dom0 for example.
> 
> Yes, agreed. So can we have a comment in the patch stating the ABI and the 
> fact that it's different in x86 and x64 builds and hence frontends need to be 
> careful about correctly setting the xenstore key?

There's already a comment describing the "protocol" xenstore node, and
the possible values are in protocols.h. Adding a comment describing that
the layout of this structures should match the gcc ABI should probably
be set in a more generic header (like xen.h?), since I guess there might
be other cases for this.

From my POV, if we are going to add a comment regarding the ABI, it
should be done in a different patch, since it's not related to the
inclusion of indirect descriptors, it has always been there.


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