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

["Roger Pau Monné" <roger.pau@citrix.com>]

On 14/11/13 18:13, Paul Durrant wrote:
>> -----Original Message-----
>> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
>> Sent: 14 November 2013 16:58
>> To: Paul Durrant; Roger Pau Monne; Ian Campbell
>> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Jan Beulich
>> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect descriptors interface to
>> public headers
>>
>> Paul Durrant <Paul.Durrant@citrix.com> wrote:
>>>> -----Original Message-----
>>>> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
>>>> Sent: 14 November 2013 16:34
>>>> To: Paul Durrant; Roger Pau Monne; Ian Campbell
>>>> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Jan Beulich
>>>> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect descriptors
>>> interface to
>>>> public headers
>>>>
>>>> Paul Durrant <Paul.Durrant@citrix.com> wrote:
>>>>>> -----Original Message-----
>>>>>> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
>>>>>> Sent: 14 November 2013 16:24
>>>>>> To: Paul Durrant; Roger Pau Monne; Ian Campbell
>>>>>> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Jan Beulich
>>>>>> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect descriptors
>>>>> interface to
>>>>>> public headers
>>>>>>
>>>>>> Paul Durrant <Paul.Durrant@citrix.com> wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: Roger Pau Monné [mailto:roger.pau@citrix.com]
>>>>>>>> Sent: 14 November 2013 10:27
>>>>>>>> To: Paul Durrant; Ian Campbell
>>>>>>>> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org; 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@citrix.com]
>>>>>>>>>> Sent: 14 November 2013 10:06
>>>>>>>>>> To: Paul Durrant; Ian Campbell
>>>>>>>>>> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org;
>>> 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@lists.xenproject.org;
>>>>> 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@lists.xenproject.org;
>>>>>>> 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@lists.xenproject.org; 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?
>>>>>>
>>>>>> Thr layout and size of the structure should be the same size on 32
>>>>> and 64 bit
>>>>>> builds.
>>>>>>
>>>>>
>>>>> As the header stands, that is not true.
>>>>
>>>> Which one? The one in Linux or the non-existing one in Xen repo for
>>> which
>>>> this patch was adding?
>>>>
>>>> If it is the Linux one which of the fields is  messed up? The whole
>>> struct
>>>> (including the extra uint8 cmd) should be exactly 64 bytes.
>>>>
>>>> I am pretty sure we double checked that.
>>>
>>> How can this possibly be the same between 32-bit and 64-bit builds
>>> (unless CONFIG_X86_64 is defined in both cases)? And the fact that
>>> nr_segments won't be naturally aligned is pretty bad too.
>>>
>>> struct blkif_request_indirect {
>>>        uint8_t        indirect_op;
>>>        uint16_t       nr_segments;
>>> #ifdef CONFIG_X86_64
>>> uint32_t       _pad1;        /* offsetof(blkif_...,u.indirect.id) == 8
>>> */
>>> #endif
>>>        uint64_t       id;
>>>        blkif_sector_t sector_number;
>>>        blkif_vdev_t   handle;
>>>        uint16_t       _pad2;
>>>   grant_ref_t
>> indirect_grefs[BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST];
>>> #ifdef CONFIG_X86_64
>>>        uint32_t      _pad3;         /* make it 64 byte aligned */
>>> #else
>>>        uint64_t      _pad3;         /* make it 64 byte aligned */
>>> #endif
>>> } __attribute__((__packed__));
>>>
>>>
>>>  Paul
>>>
>>>>>
>>>>>  Paul
>>>>>
>>>>>> I don't understand what the xenstore key has to do with this?
>>>>>>>
>>>>>>>  Paul
>>>>>>
>>>>
>>
>> I must be really thick here but I am not seeing it. Could you explain to me
>> exactly why we would not get the same size?
>>
> 
> Maybe I'm misunderstanding this but by my reading this section:
> 
> uint8_t        indirect_op;
> uint16_t       nr_segments;
> #ifdef CONFIG_X86_64
> uint32_t       _pad1;        /* offsetof(blkif_...,u.indirect.id) == 8 */
> #endif
> uint64_t       id;

This is done so that:

offsetof(struct blkif_request, id) == offsetof(struct blkif_request_discard, id) == offsetof(struct blkif_request_indirect, id)

It's the same that's already done for struct blkif_request_discard.


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