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

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