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: 15 November 2013 08:02
> To: Paul Durrant; Konrad Rzeszutek Wilk; Ian Campbell
> Cc: 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 18:13, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@xxxxxxxxxx]
> >> Sent: 14 November 2013 16:58
> >> 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: Konrad Rzeszutek Wilk [mailto:konrad.wilk@xxxxxxxxxx]
> >>>> Sent: 14 November 2013 16:34
> >>>> 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: 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.
> >>>
> >>> How can this possibly be the same between 32-bit and 64-bit builds
> >>> (unless CONFIG_X86_64 is defined in both cases)? And the fact that
> >>> nr_segments won't be naturally aligned is pretty bad too.
> >>>
> >>> struct blkif_request_indirect {
> >>>        uint8_t        indirect_op;
> >>>        uint16_t       nr_segments;
> >>> #ifdef CONFIG_X86_64
> >>> uint32_t       _pad1;        /* offsetof(blkif_...,u.indirect.id) == 8
> >>> */
> >>> #endif
> >>>        uint64_t       id;
> >>>        blkif_sector_t sector_number;
> >>>        blkif_vdev_t   handle;
> >>>        uint16_t       _pad2;
> >>>   grant_ref_t
> >> indirect_grefs[BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST];
> >>> #ifdef CONFIG_X86_64
> >>>        uint32_t      _pad3;         /* make it 64 byte aligned */
> >>> #else
> >>>        uint64_t      _pad3;         /* make it 64 byte aligned */
> >>> #endif
> >>> } __attribute__((__packed__));
> >>>
> >>>
> >>>  Paul
> >>>
> >>>>>
> >>>>>  Paul
> >>>>>
> >>>>>> I don't understand what the xenstore key has to do with this?
> >>>>>>>
> >>>>>>>  Paul
> >>>>>>
> >>>>
> >>
> >> I must be really thick here but I am not seeing it. Could you explain to me
> >> exactly why we would not get the same size?
> >>
> >
> > Maybe I'm misunderstanding this but by my reading this section:
> >
> > uint8_t        indirect_op;
> > uint16_t       nr_segments;
> > #ifdef CONFIG_X86_64
> > uint32_t       _pad1;        /* offsetof(blkif_...,u.indirect.id) == 8 */
> > #endif
> > uint64_t       id;
> 
> This is done so that:
> 
> offsetof(struct blkif_request, id) == offsetof(struct blkif_request_discard, 
> id)
> == offsetof(struct blkif_request_indirect, id)
> 
> It's the same that's already done for struct blkif_request_discard.

But, as I understand it, you could remove the pad and all the packed attributes 
and achieve the same effect - hence the patch that you submitted to Xen. So, we 
just need a definition of what the ABIs mean. How about tacking on the 
following patch?

diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
index b9b9d98..b6e2b08 100644
--- a/xen/include/public/io/blkif.h
+++ b/xen/include/public/io/blkif.h
@@ -254,7 +254,7 @@
  *      Default Value:  XEN_IO_PROTO_ABI_NATIVE
  *
  *      The machine ABI rules governing the format of all ring request and
- *      response structures.
+ *      response structures. See public/io/protocols.h for more information.
  *
  * ring-page-order
  *      Values:         <uint32_t>
diff --git a/xen/include/public/io/protocols.h b/xen/include/public/io/protocols
index 80b196b..1a3f038 100644
--- a/xen/include/public/io/protocols.h
+++ b/xen/include/public/io/protocols.h
@@ -23,6 +23,10 @@
 #ifndef __XEN_PROTOCOLS_H__
 #define __XEN_PROTOCOLS_H__

+/*
+ * These ABIs are the ones used by GCC. Note that this means 8 byte
+ * values are assumed to be only 4 byte aligned when using x86_32.
+ */
 #define XEN_IO_PROTO_ABI_X86_32     "x86_32-abi"
 #define XEN_IO_PROTO_ABI_X86_64     "x86_64-abi"
 #define XEN_IO_PROTO_ABI_ARM        "arm-abi"

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