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: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.

  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