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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@xxxxxxxxxx]
> Sent: 14 November 2013 16:34
> To: Paul Durrant; Roger Pau Monne; Ian Campbell
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org); Jan Beulich
> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect descriptors interface to
> public headers
> 
> Paul Durrant <Paul.Durrant@xxxxxxxxxx> wrote:
> >> -----Original Message-----
> >> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@xxxxxxxxxx]
> >> Sent: 14 November 2013 16:24
> >> To: Paul Durrant; Roger Pau Monne; Ian Campbell
> >> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org); Jan Beulich
> >> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect descriptors
> >interface to
> >> public headers
> >>
> >> Paul Durrant <Paul.Durrant@xxxxxxxxxx> 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?
> >>
> >> 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
> >>
> 

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