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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

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

  Paul


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