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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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