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:58
> 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: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
> >> >>
> >>
> 
> 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;

would be 11 bytes long in a 32-bit build and 15 bytes long in a 64-bit build 
(as it's part of a packed struct).

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

Ok. That explains the alignment bit - not at all obvious though.

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