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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

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

  Paul

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