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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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?

Please keep in mins that there is an extra uint8_t tacked on the start of this (as this is part of another struct).

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