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

Indirect descriptors introduce a new block operation
(BLKIF_OP_INDIRECT) that passes grant references instead of segments
in the request. This grant references are filled with arrays of
blkif_request_segment_aligned, this way we can send more segments in a
request.

This interface is already implemented in Linux >= 3.11.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/include/public/io/blkif.h |   51 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
index b9b9d98..84eb7fd 100644
--- a/xen/include/public/io/blkif.h
+++ b/xen/include/public/io/blkif.h
@@ -468,6 +468,30 @@
 #define BLKIF_OP_DISCARD           5
 
 /*
+ * Recognized if "feature-max-indirect-segments" in present in the
backend
+ * xenbus info. The "feature-max-indirect-segments" node contains the
maximum
+ * number of segments allowed by the backend per request. If the node is
+ * present, the frontend might use blkif_request_indirect structs in order to
+ * issue requests with more than BLKIF_MAX_SEGMENTS_PER_REQUEST (11). The
+ * maximum number of indirect segments is fixed by the backend, but the
+ * frontend can issue requests with any number of indirect segments as long as
+ * it's less than the number provided by the backend. The indirect_grefs
field
+ * in blkif_request_indirect should be filled by the frontend with the
+ * grant references of the pages that are holding the indirect segments.
+ * This pages are filled with an array of blkif_request_segment_aligned
+ * that hold the information about the segments. The number of indirect
+ * pages to use is determined by the maximum number of segments
+ * an indirect request contains. Every indirect page can contain a maximum
+ * of 512 segments (PAGE_SIZE/sizeof(blkif_request_segment_aligned)),
+ * so to calculate the number of indirect pages to use we have to do
+ * ceil(indirect_segments/512).
+ *
+ * If a backend does not recognize BLKIF_OP_INDIRECT, it should *not*
+ * create the "feature-max-indirect-segments" node!
+ */
+#define BLKIF_OP_INDIRECT          6
+
+/*
  * Maximum scatter/gather segments per request.
  * This is carefully chosen so that sizeof(blkif_ring_t) <= PAGE_SIZE.
  * NB. This could be 12 if the ring indexes weren't stored in the same
page.
@@ -475,6 +499,11 @@
 #define BLKIF_MAX_SEGMENTS_PER_REQUEST 11
 
 /*
+ * Maximum number of indirect pages to use per request.
+ */
+#define BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST 8
+
+/*
  * NB. first_sect and last_sect in blkif_request_segment, as well as
  * sector_number in blkif_request, are always expressed in 512-byte units.
  * However they must be properly aligned to the real sector size of the
@@ -517,6 +546,28 @@ struct blkif_request_discard {
 };
 typedef struct blkif_request_discard blkif_request_discard_t;
 
+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                   */
+    uint64_t       id;           /* private guest value, echoed in resp  */
+    blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
+    blkif_vdev_t   handle;       /* same as for read/write requests      */
+    grant_ref_t    indirect_grefs[BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST];
+#ifdef __i386__
+    uint64_t       pad;          /* Make it 64 byte aligned on i386      */
+#endif
+};
+typedef struct blkif_request_indirect blkif_request_indirect_t;
+
+struct blkif_request_segment_aligned {
+    grant_ref_t gref;            /* reference to I/O buffer frame        */
+    /* @first_sect: first sector in frame to transfer (inclusive).   */
+    /* @last_sect: last sector in frame to transfer (inclusive).     */
+    uint8_t     first_sect, last_sect;
+    uint16_t    _pad; /* padding to make it 8 bytes, so it's cache-aligned
*/
+};
+
 struct blkif_response {
     uint64_t        id;              /* copied from request */
     uint8_t         operation;       /* copied from request */
-- 
1.7.7.5 (Apple Git-26)


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

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Roger Pau Monne
> Sent: 12 November 2013 10:37
> To: xen-devel@lists.xenproject.org
> Cc: Keir (Xen.org); Jan Beulich; Roger Pau Monne
> Subject: [Xen-devel] [PATCH] blkif: add indirect descriptors interface to
> public headers
> 
> Indirect descriptors introduce a new block operation
> (BLKIF_OP_INDIRECT) that passes grant references instead of segments
> in the request. This grant references are filled with arrays of
> blkif_request_segment_aligned, this way we can send more segments in a
> request.
> 
> This interface is already implemented in Linux >= 3.11.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/include/public/io/blkif.h |   51
> +++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 51 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> index b9b9d98..84eb7fd 100644
> --- a/xen/include/public/io/blkif.h
> +++ b/xen/include/public/io/blkif.h
> @@ -468,6 +468,30 @@
>  #define BLKIF_OP_DISCARD           5
> 
>  /*
> + * Recognized if "feature-max-indirect-segments" in present in
the backend
> + * xenbus info. The "feature-max-indirect-segments" node
contains the
> maximum
> + * number of segments allowed by the backend per request. If the node is
> + * present, the frontend might use blkif_request_indirect structs in order
to
> + * issue requests with more than BLKIF_MAX_SEGMENTS_PER_REQUEST
> (11). The
> + * maximum number of indirect segments is fixed by the backend, but the
> + * frontend can issue requests with any number of indirect segments as
> long as
> + * it's less than the number provided by the backend. The
indirect_grefs
> field
> + * in blkif_request_indirect should be filled by the frontend with the
> + * grant references of the pages that are holding the indirect segments.
> + * This pages are filled with an array of blkif_request_segment_aligned
> + * that hold the information about the segments. The number of indirect
> + * pages to use is determined by the maximum number of segments
> + * an indirect request contains. Every indirect page can contain a maximum
> + * of 512 segments (PAGE_SIZE/sizeof(blkif_request_segment_aligned)),
> + * so to calculate the number of indirect pages to use we have to do
> + * ceil(indirect_segments/512).
> + *
> + * If a backend does not recognize BLKIF_OP_INDIRECT, it should *not*
> + * create the "feature-max-indirect-segments" node!
> + */
> +#define BLKIF_OP_INDIRECT          6
> +
> +/*
>   * Maximum scatter/gather segments per request.
>   * This is carefully chosen so that sizeof(blkif_ring_t) <= PAGE_SIZE.
>   * NB. This could be 12 if the ring indexes weren't stored in the same
page.
> @@ -475,6 +499,11 @@
>  #define BLKIF_MAX_SEGMENTS_PER_REQUEST 11
> 
>  /*
> + * Maximum number of indirect pages to use per request.
> + */
> +#define BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST 8
> +
> +/*
>   * NB. first_sect and last_sect in blkif_request_segment, as well as
>   * sector_number in blkif_request, are always expressed in 512-byte units.
>   * However they must be properly aligned to the real sector size of the
> @@ -517,6 +546,28 @@ struct blkif_request_discard {
>  };
>  typedef struct blkif_request_discard blkif_request_discard_t;
> 
> +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.

  Paul
> +    uint64_t       id;           /* private guest value, echoed in resp 
*/
> +    blkif_sector_t sector_number;/* start sector idx on disk (r/w only) 
*/
> +    blkif_vdev_t   handle;       /* same as for read/write requests     
*/
> +    grant_ref_t
> indirect_grefs[BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST];
> +#ifdef __i386__
> +    uint64_t       pad;          /* Make it 64 byte aligned on i386     
*/
> +#endif
> +};
> +typedef struct blkif_request_indirect blkif_request_indirect_t;
> +
> +struct blkif_request_segment_aligned {
> +    grant_ref_t gref;            /* reference to I/O buffer frame       
*/
> +    /* @first_sect: first sector in frame to transfer (inclusive).   */
> +    /* @last_sect: last sector in frame to transfer (inclusive).     */
> +    uint8_t     first_sect, last_sect;
> +    uint16_t    _pad; /* padding to make it 8 bytes, so it's
cache-aligned */
> +};
> +
>  struct blkif_response {
>      uint64_t        id;              /* copied from request */
>      uint8_t         operation;       /* copied from request */
> --
> 1.7.7.5 (Apple Git-26)
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On 12/11/13 13:46, Paul Durrant wrote:
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
>> bounces@lists.xen.org] On Behalf Of Roger Pau Monne
>> Sent: 12 November 2013 10:37
>> To: xen-devel@lists.xenproject.org
>> Cc: Keir (Xen.org); Jan Beulich; Roger Pau Monne
>> Subject: [Xen-devel] [PATCH] blkif: add indirect descriptors interface
to
>> public headers
>>
>> Indirect descriptors introduce a new block operation
>> (BLKIF_OP_INDIRECT) that passes grant references instead of segments
>> in the request. This grant references are filled with arrays of
>> blkif_request_segment_aligned, this way we can send more segments in a
>> request.
>>
>> This interface is already implemented in Linux >= 3.11.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Keir Fraser <keir@xen.org>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> ---
>>  xen/include/public/io/blkif.h |   51
>> +++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 51 insertions(+), 0 deletions(-)
>>
>> diff --git a/xen/include/public/io/blkif.h
b/xen/include/public/io/blkif.h
>> index b9b9d98..84eb7fd 100644
>> --- a/xen/include/public/io/blkif.h
>> +++ b/xen/include/public/io/blkif.h
>> @@ -468,6 +468,30 @@
>>  #define BLKIF_OP_DISCARD           5
>>
>>  /*
>> + * Recognized if "feature-max-indirect-segments" in present
in the backend
>> + * xenbus info. The "feature-max-indirect-segments" node
contains the
>> maximum
>> + * number of segments allowed by the backend per request. If the node
is
>> + * present, the frontend might use blkif_request_indirect structs in
order to
>> + * issue requests with more than BLKIF_MAX_SEGMENTS_PER_REQUEST
>> (11). The
>> + * maximum number of indirect segments is fixed by the backend, but
the
>> + * frontend can issue requests with any number of indirect segments as
>> long as
>> + * it's less than the number provided by the backend. The
indirect_grefs
>> field
>> + * in blkif_request_indirect should be filled by the frontend with the
>> + * grant references of the pages that are holding the indirect
segments.
>> + * This pages are filled with an array of
blkif_request_segment_aligned
>> + * that hold the information about the segments. The number of
indirect
>> + * pages to use is determined by the maximum number of segments
>> + * an indirect request contains. Every indirect page can contain a
maximum
>> + * of 512 segments (PAGE_SIZE/sizeof(blkif_request_segment_aligned)),
>> + * so to calculate the number of indirect pages to use we have to do
>> + * ceil(indirect_segments/512).
>> + *
>> + * If a backend does not recognize BLKIF_OP_INDIRECT, it should *not*
>> + * create the "feature-max-indirect-segments" node!
>> + */
>> +#define BLKIF_OP_INDIRECT          6
>> +
>> +/*
>>   * Maximum scatter/gather segments per request.
>>   * This is carefully chosen so that sizeof(blkif_ring_t) <=
PAGE_SIZE.
>>   * NB. This could be 12 if the ring indexes weren't stored in the
same page.
>> @@ -475,6 +499,11 @@
>>  #define BLKIF_MAX_SEGMENTS_PER_REQUEST 11
>>
>>  /*
>> + * Maximum number of indirect pages to use per request.
>> + */
>> +#define BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST 8
>> +
>> +/*
>>   * NB. first_sect and last_sect in blkif_request_segment, as well as
>>   * sector_number in blkif_request, are always expressed in 512-byte
units.
>>   * However they must be properly aligned to the real sector size of
the
>> @@ -517,6 +546,28 @@ struct blkif_request_discard {
>>  };
>>  typedef struct blkif_request_discard blkif_request_discard_t;
>>
>> +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.
The blkif request structure have always been a train wreck of ABI
incompatibilities.  The existing code in the backend for handling the
existing structures should handle this in a similar way.
>> +    uint64_t       id;           /* private guest value, echoed in
resp  */
>> +    blkif_sector_t sector_number;/* start sector idx on disk (r/w
only)  */
>> +    blkif_vdev_t   handle;       /* same as for read/write requests   
*/
>> +    grant_ref_t
>> indirect_grefs[BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST];
>> +#ifdef __i386__
>> +    uint64_t       pad;          /* Make it 64 byte aligned on i386   
*/
>> +#endif
This last field looks a bit odd though.  Why is it needed?
>> +};
>> +typedef struct blkif_request_indirect blkif_request_indirect_t;
>> +
>> +struct blkif_request_segment_aligned {
>> +    grant_ref_t gref;            /* reference to I/O buffer frame     
*/
>> +    /* @first_sect: first sector in frame to transfer (inclusive).  
*/
>> +    /* @last_sect: last sector in frame to transfer (inclusive).    
*/
>> +    uint8_t     first_sect, last_sect;
Missing uint8_t padding here?
>> +    uint16_t    _pad; /* padding to make it 8 bytes, so it's
cache-aligned */
>> +};
David

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

> -----Original Message-----
> From: David Vrabel
> Sent: 12 November 2013 14:12
> To: Paul Durrant
> Cc: Roger Pau Monne; xen-devel@lists.xenproject.org; Keir (Xen.org); Jan
> Beulich
> Subject: Re: [Xen-devel] [PATCH] blkif: add indirect descriptors interface
to
> public headers
> 
> On 12/11/13 13:46, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> >> bounces@lists.xen.org] On Behalf Of Roger Pau Monne
> >> Sent: 12 November 2013 10:37
> >> To: xen-devel@lists.xenproject.org
> >> Cc: Keir (Xen.org); Jan Beulich; Roger Pau Monne
> >> Subject: [Xen-devel] [PATCH] blkif: add indirect descriptors
interface to
> >> public headers
> >>
> >> Indirect descriptors introduce a new block operation
> >> (BLKIF_OP_INDIRECT) that passes grant references instead of
segments
> >> in the request. This grant references are filled with arrays of
> >> blkif_request_segment_aligned, this way we can send more segments
in
> a
> >> request.
> >>
> >> This interface is already implemented in Linux >= 3.11.
> >>
> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> Cc: Keir Fraser <keir@xen.org>
> >> Cc: Jan Beulich <jbeulich@suse.com>
> >> ---
> >>  xen/include/public/io/blkif.h |   51
> >> +++++++++++++++++++++++++++++++++++++++++
> >>  1 files changed, 51 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/xen/include/public/io/blkif.h
b/xen/include/public/io/blkif.h
> >> index b9b9d98..84eb7fd 100644
> >> --- a/xen/include/public/io/blkif.h
> >> +++ b/xen/include/public/io/blkif.h
> >> @@ -468,6 +468,30 @@
> >>  #define BLKIF_OP_DISCARD           5
> >>
> >>  /*
> >> + * Recognized if "feature-max-indirect-segments" in
present in the
> backend
> >> + * xenbus info. The "feature-max-indirect-segments"
node contains the
> >> maximum
> >> + * number of segments allowed by the backend per request. If the
node
> is
> >> + * present, the frontend might use blkif_request_indirect structs
in
> order to
> >> + * issue requests with more than
> BLKIF_MAX_SEGMENTS_PER_REQUEST
> >> (11). The
> >> + * maximum number of indirect segments is fixed by the backend,
but
> the
> >> + * frontend can issue requests with any number of indirect
segments as
> >> long as
> >> + * it's less than the number provided by the backend. The
indirect_grefs
> >> field
> >> + * in blkif_request_indirect should be filled by the frontend
with the
> >> + * grant references of the pages that are holding the indirect
segments.
> >> + * This pages are filled with an array of
blkif_request_segment_aligned
> >> + * that hold the information about the segments. The number of
indirect
> >> + * pages to use is determined by the maximum number of segments
> >> + * an indirect request contains. Every indirect page can contain
a
> maximum
> >> + * of 512 segments
> (PAGE_SIZE/sizeof(blkif_request_segment_aligned)),
> >> + * so to calculate the number of indirect pages to use we have to
do
> >> + * ceil(indirect_segments/512).
> >> + *
> >> + * If a backend does not recognize BLKIF_OP_INDIRECT, it should
*not*
> >> + * create the "feature-max-indirect-segments" node!
> >> + */
> >> +#define BLKIF_OP_INDIRECT          6
> >> +
> >> +/*
> >>   * Maximum scatter/gather segments per request.
> >>   * This is carefully chosen so that sizeof(blkif_ring_t) <=
PAGE_SIZE.
> >>   * NB. This could be 12 if the ring indexes weren't stored in
the same
> page.
> >> @@ -475,6 +499,11 @@
> >>  #define BLKIF_MAX_SEGMENTS_PER_REQUEST 11
> >>
> >>  /*
> >> + * Maximum number of indirect pages to use per request.
> >> + */
> >> +#define BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST 8
> >> +
> >> +/*
> >>   * NB. first_sect and last_sect in blkif_request_segment, as well
as
> >>   * sector_number in blkif_request, are always expressed in
512-byte
> units.
> >>   * However they must be properly aligned to the real sector size
of the
> >> @@ -517,6 +546,28 @@ struct blkif_request_discard {
> >>  };
> >>  typedef struct blkif_request_discard blkif_request_discard_t;
> >>
> >> +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.
> 
> The blkif request structure have always been a train wreck of ABI
> incompatibilities.  The existing code in the backend for handling the
> existing structures should handle this in a similar way.
> 
You mean the abi translation code? Why would we want to invoke that for a new
data structure, when sticking a 32-bit pad here would solve the problem?

  Paul
> >> +    uint64_t       id;           /* private guest value, echoed
in resp  */
> >> +    blkif_sector_t sector_number;/* start sector idx on disk (r/w
only)  */
> >> +    blkif_vdev_t   handle;       /* same as for read/write
requests      */
> >> +    grant_ref_t
> >> indirect_grefs[BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST];
> >> +#ifdef __i386__
> >> +    uint64_t       pad;          /* Make it 64 byte aligned on
i386      */
> >> +#endif
> 
> This last field looks a bit odd though.  Why is it needed?
> 
> >> +};
> >> +typedef struct blkif_request_indirect blkif_request_indirect_t;
> >> +
> >> +struct blkif_request_segment_aligned {
> >> +    grant_ref_t gref;            /* reference to I/O buffer frame
*/
> >> +    /* @first_sect: first sector in frame to transfer
(inclusive).   */
> >> +    /* @last_sect: last sector in frame to transfer (inclusive). 
*/
> >> +    uint8_t     first_sect, last_sect;
> 
> Missing uint8_t padding here?
> 
> >> +    uint16_t    _pad; /* padding to make it 8 bytes, so it's
cache-aligned */
> >> +};
> 
> David
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Tue, Nov 12, 2013 at 01:46:19PM +0000, Paul Durrant
wrote:
> > -----Original Message-----
> > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> > bounces@lists.xen.org] On Behalf Of Roger Pau Monne
> > Sent: 12 November 2013 10:37
> > To: xen-devel@lists.xenproject.org
> > Cc: Keir (Xen.org); Jan Beulich; Roger Pau Monne
> > Subject: [Xen-devel] [PATCH] blkif: add indirect descriptors interface
to
> > public headers
> > 
> > Indirect descriptors introduce a new block operation
> > (BLKIF_OP_INDIRECT) that passes grant references instead of segments
> > in the request. This grant references are filled with arrays of
> > blkif_request_segment_aligned, this way we can send more segments in a
> > request.
> > 
> > This interface is already implemented in Linux >= 3.11.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > Cc: Keir Fraser <keir@xen.org>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > ---
> >  xen/include/public/io/blkif.h |   51
> > +++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 51 insertions(+), 0 deletions(-)
> > 
> > diff --git a/xen/include/public/io/blkif.h
b/xen/include/public/io/blkif.h
> > index b9b9d98..84eb7fd 100644
> > --- a/xen/include/public/io/blkif.h
> > +++ b/xen/include/public/io/blkif.h
> > @@ -468,6 +468,30 @@
> >  #define BLKIF_OP_DISCARD           5
> > 
> >  /*
> > + * Recognized if "feature-max-indirect-segments" in present
in the backend
> > + * xenbus info. The "feature-max-indirect-segments" node
contains the
> > maximum
> > + * number of segments allowed by the backend per request. If the node
is
> > + * present, the frontend might use blkif_request_indirect structs in
order to
> > + * issue requests with more than BLKIF_MAX_SEGMENTS_PER_REQUEST
> > (11). The
> > + * maximum number of indirect segments is fixed by the backend, but
the
> > + * frontend can issue requests with any number of indirect segments
as
> > long as
> > + * it''s less than the number provided by the backend. The
indirect_grefs
> > field
> > + * in blkif_request_indirect should be filled by the frontend with
the
> > + * grant references of the pages that are holding the indirect
segments.
> > + * This pages are filled with an array of
blkif_request_segment_aligned
> > + * that hold the information about the segments. The number of
indirect
> > + * pages to use is determined by the maximum number of segments
> > + * an indirect request contains. Every indirect page can contain a
maximum
> > + * of 512 segments (PAGE_SIZE/sizeof(blkif_request_segment_aligned)),
> > + * so to calculate the number of indirect pages to use we have to do
> > + * ceil(indirect_segments/512).
> > + *
> > + * If a backend does not recognize BLKIF_OP_INDIRECT, it should *not*
> > + * create the "feature-max-indirect-segments" node!
> > + */
> > +#define BLKIF_OP_INDIRECT          6
> > +
> > +/*
> >   * Maximum scatter/gather segments per request.
> >   * This is carefully chosen so that sizeof(blkif_ring_t) <=
PAGE_SIZE.
> >   * NB. This could be 12 if the ring indexes weren''t stored
in the same page.
> > @@ -475,6 +499,11 @@
> >  #define BLKIF_MAX_SEGMENTS_PER_REQUEST 11
> > 
> >  /*
> > + * Maximum number of indirect pages to use per request.
> > + */
> > +#define BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST 8
> > +
> > +/*
> >   * NB. first_sect and last_sect in blkif_request_segment, as well as
> >   * sector_number in blkif_request, are always expressed in 512-byte
units.
> >   * However they must be properly aligned to the real sector size of
the
> > @@ -517,6 +546,28 @@ struct blkif_request_discard {
> >  };
> >  typedef struct blkif_request_discard blkif_request_discard_t;
> > 
> > +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 {
        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__));     

Wheew.
>   Paul
> 
> > +    uint64_t       id;           /* private guest value, echoed in
resp  */
> > +    blkif_sector_t sector_number;/* start sector idx on disk (r/w
only)  */
> > +    blkif_vdev_t   handle;       /* same as for read/write requests  
*/
> > +    grant_ref_t
> > indirect_grefs[BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST];
> > +#ifdef __i386__
> > +    uint64_t       pad;          /* Make it 64 byte aligned on i386  
*/
> > +#endif
> > +};
> > +typedef struct blkif_request_indirect blkif_request_indirect_t;
> > +
> > +struct blkif_request_segment_aligned {
> > +    grant_ref_t gref;            /* reference to I/O buffer frame    
*/
> > +    /* @first_sect: first sector in frame to transfer (inclusive).  
*/
> > +    /* @last_sect: last sector in frame to transfer (inclusive).    
*/
> > +    uint8_t     first_sect, last_sect;
> > +    uint16_t    _pad; /* padding to make it 8 bytes, so it''s
cache-aligned */
> > +};
> > +
> >  struct blkif_response {
> >      uint64_t        id;              /* copied from request */
> >      uint8_t         operation;       /* copied from request */
> > --
> > 1.7.7.5 (Apple Git-26)
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

For ARM we reference the specific standard[0]. It is up to the guest OS
to make sure that it''s version of the headers lay things out following
that standard (NB Linux blkif.h is currently buggy on ARM in this
regard, Julien has the details).

Ian.

[0]
http://xenbits.xen.org/docs/unstable/hypercall/arm/include,public,arch-arm.h.html#incontents_arm_abi

On Tue, Nov 12, 2013 at 02:29:01PM +0000, Ian Campbell
wrote:
> 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.
Right. I had hit that at some point with putting in a u64 and seeing
it mushroom up while if I did two u32 it padded it nicely.

Perhaps that is what we should do? The Linux struct has the same size
whether it be 64-bit or 32-bit.
> 
> 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?). 
Or perhaps just mention the size of the structure and offset?
> 
> For ARM we reference the specific standard[0]. It is up to the guest OS
> to make sure that it''s version of the headers lay things out
following
> that standard (NB Linux blkif.h is currently buggy on ARM in this
> regard, Julien has the details).
> 
> Ian.
> 
> [0]
>
http://xenbits.xen.org/docs/unstable/hypercall/arm/include,public,arch-arm.h.html#incontents_arm_abi
>

>>> On 12.11.13 at 15:12, David Vrabel <david.vrabel@citrix.com>
wrote:
> On 12/11/13 13:46, Paul Durrant wrote:
>>> +    uint64_t       id;           /* private guest value, echoed in
resp  */
>>> +    blkif_sector_t sector_number;/* start sector idx on disk (r/w
only)  */
>>> +    blkif_vdev_t   handle;       /* same as for read/write
requests      */
>>> +    grant_ref_t
>>> indirect_grefs[BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST];
>>> +#ifdef __i386__
>>> +    uint64_t       pad;          /* Make it 64 byte aligned on
i386      */
>>> +#endif
> 
> This last field looks a bit odd though.  Why is it needed?
On 64-bit there''s a 32 bit hole before "id" and another one
at the
end of the structure. In order for the structure size to be as
specified in the comment, both these holes need to be accounted
for.
>>> +};
>>> +typedef struct blkif_request_indirect blkif_request_indirect_t;
>>> +
>>> +struct blkif_request_segment_aligned {
>>> +    grant_ref_t gref;            /* reference to I/O buffer frame 
*/
>>> +    /* @first_sect: first sector in frame to transfer (inclusive).
*/
>>> +    /* @last_sect: last sector in frame to transfer (inclusive).  
*/
>>> +    uint8_t     first_sect, last_sect;
> 
> Missing uint8_t padding here?
Note that there are _two_ uint8_t-s.

Jan

On 12/11/13 14:43, Jan Beulich wrote:
>>>> On 12.11.13 at 15:12, David Vrabel
<david.vrabel@citrix.com> wrote:
>> On 12/11/13 13:46, Paul Durrant wrote:
>>>> +    uint64_t       id;           /* private guest value,
echoed in resp  */
>>>> +    blkif_sector_t sector_number;/* start sector idx on disk
(r/w only)  */
>>>> +    blkif_vdev_t   handle;       /* same as for read/write
requests      */
>>>> +    grant_ref_t
>>>> indirect_grefs[BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST];
>>>> +#ifdef __i386__
>>>> +    uint64_t       pad;          /* Make it 64 byte aligned on
i386      */
>>>> +#endif
>>
>> This last field looks a bit odd though.  Why is it needed?
> 
> On 64-bit there''s a 32 bit hole before "id" and another
one at the
> end of the structure. In order for the structure size to be as
> specified in the comment, both these holes need to be accounted
> for.
Ok.
>>>> +};
>>>> +typedef struct blkif_request_indirect
blkif_request_indirect_t;
>>>> +
>>>> +struct blkif_request_segment_aligned {
>>>> +    grant_ref_t gref;            /* reference to I/O buffer
frame        */
>>>> +    /* @first_sect: first sector in frame to transfer
(inclusive).   */
>>>> +    /* @last_sect: last sector in frame to transfer
(inclusive).     */
>>>> +    uint8_t     first_sect, last_sect;
>>
>> Missing uint8_t padding here?
> 
> Note that there are _two_ uint8_t-s.
Doh. That''s poor style though.   A line per field, please.

David

On Tue, 2013-11-12 at 09:42 -0500, Konrad Rzeszutek Wilk
wrote:
> On Tue, Nov 12, 2013 at 02:29:01PM +0000, Ian Campbell wrote:
> > 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.
> 
> Right. I had hit that at some point with putting in a u64 and seeing
> it mushroom up while if I did two u32 it padded it nicely.
> 
> Perhaps that is what we should do?
What you should do is make the struct declare something which matches
the required ABI. I don''t really care how it is achieved on the Linux
side.
>  The Linux struct has the same size
> whether it be 64-bit or 32-bit.
> 
> > 
> > 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?). 
> 
> Or perhaps just mention the size of the structure and offset?
You''d need to do that for every field of every struct. Which is really
why documentation of struct layout ABIs exist...

Ian.

On 12/11/13 15:49, David Vrabel wrote:
> On 12/11/13 14:43, Jan Beulich wrote:
>>>>> On 12.11.13 at 15:12, David Vrabel
<david.vrabel@citrix.com> wrote:
>>> On 12/11/13 13:46, Paul Durrant wrote:
>>>>> +};
>>>>> +typedef struct blkif_request_indirect
blkif_request_indirect_t;
>>>>> +
>>>>> +struct blkif_request_segment_aligned {
>>>>> +    grant_ref_t gref;            /* reference to I/O
buffer frame        */
>>>>> +    /* @first_sect: first sector in frame to transfer
(inclusive).   */
>>>>> +    /* @last_sect: last sector in frame to transfer
(inclusive).     */
>>>>> +    uint8_t     first_sect, last_sect;
>>>
>>> Missing uint8_t padding here?
>>
>> Note that there are _two_ uint8_t-s.
> 
> Doh. That''s poor style though.   A line per field, please.
I''ve mainly did it that way because that''s how
blkif_request_segment is
currently defined in blkif.h, but I can split it in two lines.

>>> On 12.11.13 at 15:54, Roger Pau Monné<roger.pau@citrix.com>
wrote:
> On 12/11/13 15:49, David Vrabel wrote:
>> On 12/11/13 14:43, Jan Beulich wrote:
>>>>>> On 12.11.13 at 15:12, David Vrabel
<david.vrabel@citrix.com> wrote:
>>>> On 12/11/13 13:46, Paul Durrant wrote:
>>>>>> +};
>>>>>> +typedef struct blkif_request_indirect
blkif_request_indirect_t;
>>>>>> +
>>>>>> +struct blkif_request_segment_aligned {
>>>>>> +    grant_ref_t gref;            /* reference to I/O
buffer frame        */
>>>>>> +    /* @first_sect: first sector in frame to transfer
(inclusive).   */
>>>>>> +    /* @last_sect: last sector in frame to transfer
(inclusive).     */
>>>>>> +    uint8_t     first_sect, last_sect;
>>>>
>>>> Missing uint8_t padding here?
>>>
>>> Note that there are _two_ uint8_t-s.
>> 
>> Doh. That's poor style though.   A line per field, please.
> 
> I've mainly did it that way because that's how
blkif_request_segment is
> currently defined in blkif.h, but I can split it in two lines.
In fact I'd prefer if it was kept in line with the other one. So _if_
you want to adjust the new one, please also adjust the old one.

Jan

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

> -----Original Message-----
> From: Ian Campbell
> Sent: 12 November 2013 14:29
> To: Konrad Rzeszutek Wilk
> Cc: Paul Durrant; xen-devel@lists.xenproject.org; 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.

  Paul
> For ARM we reference the specific standard[0]. It is up to the guest OS
> to make sure that it''s version of the headers lay things out
following
> that standard (NB Linux blkif.h is currently buggy on ARM in this
> regard, Julien has the details).
> 
> Ian.
> 
> [0]
> http://xenbits.xen.org/docs/unstable/hypercall/arm/include,public,arch-
> arm.h.html#incontents_arm_abi

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@lists.xenproject.org; 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.
> 
>   Paul
> 
> > For ARM we reference the specific standard[0]. It is up to the guest
OS
> > to make sure that it''s version of the headers lay things out
following
> > that standard (NB Linux blkif.h is currently buggy on ARM in this
> > regard, Julien has the details).
> > 
> > Ian.
> > 
> > [0]
> >
http://xenbits.xen.org/docs/unstable/hypercall/arm/include,public,arch-
> > arm.h.html#incontents_arm_abi
>

> -----Original Message-----
> From: Ian Campbell
> Sent: 13 November 2013 09:27
> To: Paul Durrant
> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org; 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@lists.xenproject.org; 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.

   Paul
> >
> >   Paul
> >
> > > For ARM we reference the specific standard[0]. It is up to the
guest OS
> > > to make sure that it''s version of the headers lay things
out following
> > > that standard (NB Linux blkif.h is currently buggy on ARM in this
> > > regard, Julien has the details).
> > >
> > > Ian.
> > >
> > > [0]
> > >
http://xenbits.xen.org/docs/unstable/hypercall/arm/include,public,arch-
> > > arm.h.html#incontents_arm_abi
> >
>

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@lists.xenproject.org; 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@lists.xenproject.org; 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.

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.

Ian.

> -----Original Message-----
> From: Ian Campbell
> Sent: 13 November 2013 11:11
> To: Paul Durrant
> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org; 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@lists.xenproject.org; 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@lists.xenproject.org; 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

Paul Durrant <Paul.Durrant@citrix.com> wrote:
>> -----Original Message-----
>> From: Ian Campbell
>> Sent: 13 November 2013 09:27
>> To: Paul Durrant
>> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org; 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@lists.xenproject.org; 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.
It was merged in 3.11 and now 3.12 has been released. Also it also in FreeBSD.
So there is backwards computability with that to think of.  Or did you mean 32
vs 64 size and offset? That should be the same.
>
>   Paul
>
>> >
>> >   Paul
>> >
>> > > For ARM we reference the specific standard[0]. It is up to
the
>guest OS
>> > > to make sure that it''s version of the headers lay
things out
>following
>> > > that standard (NB Linux blkif.h is currently buggy on ARM in
this
>> > > regard, Julien has the details).
>> > >
>> > > Ian.
>> > >
>> > > [0]
>> > >
>http://xenbits.xen.org/docs/unstable/hypercall/arm/include,public,arch-
>> > > arm.h.html#incontents_arm_abi
>> >
>>

On 12/11/13 15:58, Jan Beulich wrote:
>>>> On 12.11.13 at 15:54, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>> On 12/11/13 15:49, David Vrabel wrote:
>>> On 12/11/13 14:43, Jan Beulich wrote:
>>>>>>> On 12.11.13 at 15:12, David Vrabel
<david.vrabel@citrix.com> wrote:
>>>>> On 12/11/13 13:46, Paul Durrant wrote:
>>>>>>> +};
>>>>>>> +typedef struct blkif_request_indirect
blkif_request_indirect_t;
>>>>>>> +
>>>>>>> +struct blkif_request_segment_aligned {
>>>>>>> +    grant_ref_t gref;            /* reference to
I/O buffer frame        */
>>>>>>> +    /* @first_sect: first sector in frame to
transfer (inclusive).   */
>>>>>>> +    /* @last_sect: last sector in frame to
transfer (inclusive).     */
>>>>>>> +    uint8_t     first_sect, last_sect;
>>>>>
>>>>> Missing uint8_t padding here?
>>>>
>>>> Note that there are _two_ uint8_t-s.
>>>
>>> Doh. That's poor style though.   A line per field, please.
>>
>> I've mainly did it that way because that's how
blkif_request_segment is
>> currently defined in blkif.h, but I can split it in two lines.
> 
> In fact I'd prefer if it was kept in line with the other one. So _if_
> you want to adjust the new one, please also adjust the old one.
I'm fine with leaving them as they are, but I can also change both if
requested/needed for this to go in.


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

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@lists.xenproject.org; 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@lists.xenproject.org; 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@lists.xenproject.org; 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.

> -----Original Message-----
> From: Roger Pau Monné [mailto:roger.pau@citrix.com]
> Sent: 14 November 2013 10:06
> To: Paul Durrant; Ian Campbell
> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org; 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@lists.xenproject.org; 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@lists.xenproject.org;
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@lists.xenproject.org;
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@lists.xen.org
http://lists.xen.org/xen-devel

On 14/11/13 11:14, Paul Durrant wrote:
>> -----Original Message-----
>> From: Roger Pau Monné [mailto:roger.pau@citrix.com]
>> Sent: 14 November 2013 10:06
>> To: Paul Durrant; Ian Campbell
>> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org; 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@lists.xenproject.org; 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@lists.xenproject.org; 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@lists.xenproject.org; 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.


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

> -----Original Message-----
> From: Roger Pau Monné [mailto:roger.pau@citrix.com]
> Sent: 14 November 2013 10:27
> To: Paul Durrant; Ian Campbell
> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org; 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@citrix.com]
> >> Sent: 14 November 2013 10:06
> >> To: Paul Durrant; Ian Campbell
> >> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org; 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@lists.xenproject.org;
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@lists.xenproject.org; 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@lists.xenproject.org; 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?

  Paul

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

On 14/11/13 11:38, Paul Durrant wrote:
>> -----Original Message-----
>> From: Roger Pau Monné [mailto:roger.pau@citrix.com]
>> Sent: 14 November 2013 10:27
>> To: Paul Durrant; Ian Campbell
>> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org; 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@citrix.com]
>>>> Sent: 14 November 2013 10:06
>>>> To: Paul Durrant; Ian Campbell
>>>> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org; 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@lists.xenproject.org; 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@lists.xenproject.org; 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@lists.xenproject.org; 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?
There's already a comment describing the "protocol" xenstore node,
and
the possible values are in protocols.h. Adding a comment describing that
the layout of this structures should match the gcc ABI should probably
be set in a more generic header (like xen.h?), since I guess there might
be other cases for this.

From my POV, if we are going to add a comment regarding the ABI, it
should be done in a different patch, since it's not related to the
inclusion of indirect descriptors, it has always been there.


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

> -----Original Message-----
> From: Roger Pau Monné [mailto:roger.pau@citrix.com]
> Sent: 14 November 2013 10:52
> To: Paul Durrant; Ian Campbell
> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org; Keir (Xen.org);
> Jan Beulich
> Subject: Re: [Xen-devel] [PATCH] blkif: add indirect descriptors interface
to
> public headers
> 
> On 14/11/13 11:38, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Roger Pau Monné [mailto:roger.pau@citrix.com]
> >> Sent: 14 November 2013 10:27
> >> To: Paul Durrant; Ian Campbell
> >> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org; 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@citrix.com]
> >>>> Sent: 14 November 2013 10:06
> >>>> To: Paul Durrant; Ian Campbell
> >>>> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org;
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@lists.xenproject.org; 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@lists.xenproject.org; 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@lists.xenproject.org; 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?
> 
> There's already a comment describing the "protocol" xenstore
node, and
> the possible values are in protocols.h. Adding a comment describing that
> the layout of this structures should match the gcc ABI should probably
> be set in a more generic header (like xen.h?), since I guess there might
> be other cases for this.
> 
I agree that it's a broader issue, but I think the comments should go in the
individual protocol headers as that's where developers are going to look
when they are attempting to implement frontends or backends.
> From my POV, if we are going to add a comment regarding the ABI, it
> should be done in a different patch, since it's not related to the
> inclusion of indirect descriptors, it has always been there.
Ok, but it would be good if at least blkif were updated. Deferring it will
probably mean it's just forgotten (again).

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

Paul Durrant <Paul.Durrant@citrix.com> wrote:
>> -----Original Message-----
>> From: Roger Pau Monné [mailto:roger.pau@citrix.com]
>> Sent: 14 November 2013 10:27
>> To: Paul Durrant; Ian Campbell
>> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org; 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@citrix.com]
>> >> Sent: 14 November 2013 10:06
>> >> To: Paul Durrant; Ian Campbell
>> >> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org;
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@lists.xenproject.org; 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@lists.xenproject.org;
>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@lists.xenproject.org; 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@lists.xen.org
http://lists.xen.org/xen-devel

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: 14 November 2013 16:24
> To: Paul Durrant; Roger Pau Monne; Ian Campbell
> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Jan Beulich
> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect descriptors interface
to
> public headers
> 
> Paul Durrant <Paul.Durrant@citrix.com> wrote:
> >> -----Original Message-----
> >> From: Roger Pau Monné [mailto:roger.pau@citrix.com]
> >> Sent: 14 November 2013 10:27
> >> To: Paul Durrant; Ian Campbell
> >> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org; 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@citrix.com]
> >> >> Sent: 14 November 2013 10:06
> >> >> To: Paul Durrant; Ian Campbell
> >> >> Cc: Konrad Rzeszutek Wilk;
xen-devel@lists.xenproject.org; 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@lists.xenproject.org; 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@lists.xenproject.org;
> >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@lists.xenproject.org; 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.

  Paul
> I don't understand what the xenstore key has to do with this?
> >
> >  Paul
> 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Paul Durrant <Paul.Durrant@citrix.com> wrote:
>> -----Original Message-----
>> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
>> Sent: 14 November 2013 16:24
>> To: Paul Durrant; Roger Pau Monne; Ian Campbell
>> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Jan Beulich
>> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect descriptors
>interface to
>> public headers
>> 
>> Paul Durrant <Paul.Durrant@citrix.com> wrote:
>> >> -----Original Message-----
>> >> From: Roger Pau Monné [mailto:roger.pau@citrix.com]
>> >> Sent: 14 November 2013 10:27
>> >> To: Paul Durrant; Ian Campbell
>> >> Cc: Konrad Rzeszutek Wilk; xen-devel@lists.xenproject.org;
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@citrix.com]
>> >> >> Sent: 14 November 2013 10:06
>> >> >> To: Paul Durrant; Ian Campbell
>> >> >> Cc: Konrad Rzeszutek Wilk;
xen-devel@lists.xenproject.org; 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@lists.xenproject.org;
>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@lists.xenproject.org;
>> >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@lists.xenproject.org; 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@lists.xen.org
http://lists.xen.org/xen-devel

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: 14 November 2013 16:34
> To: Paul Durrant; Roger Pau Monne; Ian Campbell
> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Jan Beulich
> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect descriptors interface
to
> public headers
> 
> Paul Durrant <Paul.Durrant@citrix.com> wrote:
> >> -----Original Message-----
> >> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> >> Sent: 14 November 2013 16:24
> >> To: Paul Durrant; Roger Pau Monne; Ian Campbell
> >> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Jan Beulich
> >> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect descriptors
> >interface to
> >> public headers
> >>
> >> Paul Durrant <Paul.Durrant@citrix.com> wrote:
> >> >> -----Original Message-----
> >> >> From: Roger Pau Monné [mailto:roger.pau@citrix.com]
> >> >> Sent: 14 November 2013 10:27
> >> >> To: Paul Durrant; Ian Campbell
> >> >> Cc: Konrad Rzeszutek Wilk;
xen-devel@lists.xenproject.org; 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@citrix.com]
> >> >> >> Sent: 14 November 2013 10:06
> >> >> >> To: Paul Durrant; Ian Campbell
> >> >> >> Cc: Konrad Rzeszutek Wilk;
xen-devel@lists.xenproject.org; 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@lists.xenproject.org;
> >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@lists.xenproject.org;
> >> >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@lists.xenproject.org; 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
> >>
> 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Paul Durrant <Paul.Durrant@citrix.com> wrote:
>> -----Original Message-----
>> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
>> Sent: 14 November 2013 16:34
>> To: Paul Durrant; Roger Pau Monne; Ian Campbell
>> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Jan Beulich
>> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect descriptors
>interface to
>> public headers
>> 
>> Paul Durrant <Paul.Durrant@citrix.com> wrote:
>> >> -----Original Message-----
>> >> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
>> >> Sent: 14 November 2013 16:24
>> >> To: Paul Durrant; Roger Pau Monne; Ian Campbell
>> >> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Jan
Beulich
>> >> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect
descriptors
>> >interface to
>> >> public headers
>> >>
>> >> Paul Durrant <Paul.Durrant@citrix.com> wrote:
>> >> >> -----Original Message-----
>> >> >> From: Roger Pau Monné [mailto:roger.pau@citrix.com]
>> >> >> Sent: 14 November 2013 10:27
>> >> >> To: Paul Durrant; Ian Campbell
>> >> >> Cc: Konrad Rzeszutek Wilk;
xen-devel@lists.xenproject.org; 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@citrix.com]
>> >> >> >> Sent: 14 November 2013 10:06
>> >> >> >> To: Paul Durrant; Ian Campbell
>> >> >> >> Cc: Konrad Rzeszutek Wilk;
xen-devel@lists.xenproject.org;
>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@lists.xenproject.org;
>> >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@lists.xenproject.org;
>> >> >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@lists.xenproject.org; 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?

Please keep in mins that there is an extra uint8_t tacked on the start of this
(as this is part of another struct).

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

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: 14 November 2013 16:58
> To: Paul Durrant; Roger Pau Monne; Ian Campbell
> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Jan Beulich
> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect descriptors interface
to
> public headers
> 
> Paul Durrant <Paul.Durrant@citrix.com> wrote:
> >> -----Original Message-----
> >> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> >> Sent: 14 November 2013 16:34
> >> To: Paul Durrant; Roger Pau Monne; Ian Campbell
> >> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Jan Beulich
> >> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect descriptors
> >interface to
> >> public headers
> >>
> >> Paul Durrant <Paul.Durrant@citrix.com> wrote:
> >> >> -----Original Message-----
> >> >> From: Konrad Rzeszutek Wilk
[mailto:konrad.wilk@oracle.com]
> >> >> Sent: 14 November 2013 16:24
> >> >> To: Paul Durrant; Roger Pau Monne; Ian Campbell
> >> >> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Jan
Beulich
> >> >> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect
descriptors
> >> >interface to
> >> >> public headers
> >> >>
> >> >> Paul Durrant <Paul.Durrant@citrix.com> wrote:
> >> >> >> -----Original Message-----
> >> >> >> From: Roger Pau Monné
[mailto:roger.pau@citrix.com]
> >> >> >> Sent: 14 November 2013 10:27
> >> >> >> To: Paul Durrant; Ian Campbell
> >> >> >> Cc: Konrad Rzeszutek Wilk;
xen-devel@lists.xenproject.org; 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@citrix.com]
> >> >> >> >> Sent: 14 November 2013 10:06
> >> >> >> >> To: Paul Durrant; Ian Campbell
> >> >> >> >> Cc: Konrad Rzeszutek Wilk;
xen-devel@lists.xenproject.org;
> >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@lists.xenproject.org;
> >> >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@lists.xenproject.org;
> >> >> >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@lists.xenproject.org; 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;

would be 11 bytes long in a 32-bit build and 15 bytes long in a 64-bit build (as
it's part of a packed struct).
> Please keep in mins that there is an extra uint8_t tacked on the start of
this
> (as this is part of another struct).
Ok. That explains the alignment bit - not at all obvious though.

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

> > 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;
> 
> would be 11 bytes long in a 32-bit build and 15 bytes long in a 64-bit
build (as it''s part of a packed struct).
Damm! I somehow thought we had it done _Right_ so it would be the same
exact and offset on both 64-bit and 32-bit. ARGH!

konrad@phenom:/tmp$ ./blk-interface-64
RING TOTAL SIZE         2368
Each entry              64
Each request            60
64-bit
op:0, indirect_op=1, nr_segment=2,id=8 sector_number=16
gref[0] == (24)
gref[1] == (28)
gref[2] == (32)
gref[3] == (36)
gref[4] == (40)
gref[5] == (44)
gref[6] == (48)
gref[7] == (52)
konrad@phenom:/tmp$ ./blk-interface-32
konrad@phenom:/tmp$ ./blk-interface
RING TOTAL SIZE         1792
Each entry              48
Each request            48
32-bit
op:0, indirect_op=1, nr_segment=2,id=4 sector_number=8
gref[0] == (12)
gref[1] == (16)
gref[2] == (20)
gref[3] == (24)
gref[4] == (28)
gref[5] == (32)
gref[6] == (36)
gref[7] == (40)

Attached is the simple test program.

#include <stdio.h>
#include <sys/types.h>

#include <stddef.h>

/* These are only 64-bit defined */
typedef __signed__ char __s8;
typedef unsigned char __u8;

typedef __signed__ short __s16;
typedef unsigned short __u16;

typedef __signed__ int __s32;
typedef unsigned int __u32;

typedef __signed__ long __s64;
typedef unsigned long __u64;

typedef		__u8		uint8_t;
typedef		__u16		uint16_t;
typedef		__u32		uint32_t;

#if defined(__GNUC__)
typedef		__u64		uint64_t;
//typedef		__u64		u_int64_t;
//typedef		__s64		int64_t;
#endif

typedef uint16_t blkif_vdev_t;
typedef uint64_t blkif_sector_t;
typedef uint32_t grant_ref_t;

/*
 * Maximum scatter/gather segments per request.
 * This is carefully chosen so that sizeof(struct blkif_ring) <= PAGE_SIZE.
 * NB. This could be 12 if the ring indexes weren''t stored in the same
page.
 */
#define BLKIF_MAX_SEGMENTS_PER_REQUEST 11


//#define CONFIG_X86_64 1

struct blkif_request_rw {
	uint8_t        nr_segments;  /* number of segments                   */
	blkif_vdev_t   handle;       /* only for read/write requests         */
#ifdef CONFIG_X86_64
	uint32_t       _pad1;	     /* offsetof(blkif_request,u.rw.id) == 8 */
#endif
	uint64_t       id;           /* private guest value, echoed in resp  */
	blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
	struct blkif_request_segment {
		grant_ref_t gref;        /* reference to I/O buffer frame        */
		/* @first_sect: first sector in frame to transfer (inclusive).   */
		/* @last_sect: last sector in frame to transfer (inclusive).     */
		uint8_t     first_sect, last_sect;
	} seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
} __attribute__((__packed__));

#define BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST 8


/* so 64 bytes under 64-bit  */
struct blkif_request_indirect {
	uint8_t        indirect_op;  /* BLKIF_OP_* (usually READ or WRITE    */	// 1
	uint16_t       nr_segments;
#ifdef CONFIG_X86_64
	uint32_t       _pad1;	     /* offsetof(blkif_request,u.rw.id) == 8 */ // 2
#endif
	uint64_t       id;           /* private guest value, echoed in resp  */
	blkif_sector_t sector_number;
	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__));


struct blkif_request {
	uint8_t        operation;    /* BLKIF_OP_???                         */
	union {
		/* struct blkif_request_rw rw; */
		struct blkif_request_indirect indirect;
	} u;
} __attribute__((__packed__));

struct blkif_response {
	uint64_t        id;              /* copied from request */
	uint8_t         operation;       /* copied from request */
	int16_t         status;          /* BLKIF_RSP_???       */
};
typedef unsigned int RING_IDX; /* 4 bytes */

union blkif_sring_entry {
	struct blkif_request req;
	struct blkif_response rsp;
};

struct blkif_ring {
	/* 8 */
    RING_IDX req_prod, req_event; /* #1 front writest req_prod,
				 blkback updates req_event by interal reqs_cons consumed + 1 */
	/* 8 */
    RING_IDX rsp_prod, rsp_event; /* #2 blkback writes rsp_prod, and #4 blkfront
updates rsp_event
			by its internal rsp_cons + 1 */
    uint8_t  pad[48];
	/* 48 + 16 = 64 */
    union blkif_sring_entry ring[36]; /* variable-length */
};

void main(void)
{
	int i;
	printf("RING TOTAL SIZE\t\t%d\n", sizeof(struct blkif_ring));
	printf("Each entry\t\t%d\n", sizeof(union blkif_sring_entry));
	printf("Each request\t\t%d\n", sizeof(struct blkif_request));
	printf("%s\n", 
#ifdef CONFIG_X86_64
		"64-bit"
#else
		"32-bit"
#endif
	);

	printf("op:%d, indirect_op=%d, nr_segment=%d,id=%d
sector_number=%d\n",
		offsetof(struct blkif_request, operation),
		offsetof(struct blkif_request, u.indirect.indirect_op),
		offsetof(struct blkif_request, u.indirect.nr_segments),
		offsetof(struct blkif_request, u.indirect.id),
		offsetof(struct blkif_request, u.indirect.sector_number));

	for (i = 0; i < BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST; i++) {
		printf("gref[%d] == (%d)\n", i, offsetof(struct blkif_request,
u.indirect.indirect_grefs[i]));
	}
	return;
}

>> 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.
D''oh, I missed the pad at the end, so they are the same size but that
doesn''t stop the layout being different and hence needing to make sure
the ABI is set correctly in xenstore.

  Paul

On 14/11/13 18:13, Paul Durrant wrote:
>> -----Original Message-----
>> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
>> Sent: 14 November 2013 16:58
>> To: Paul Durrant; Roger Pau Monne; Ian Campbell
>> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Jan Beulich
>> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect descriptors
interface to
>> public headers
>>
>> Paul Durrant <Paul.Durrant@citrix.com> wrote:
>>>> -----Original Message-----
>>>> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
>>>> Sent: 14 November 2013 16:34
>>>> To: Paul Durrant; Roger Pau Monne; Ian Campbell
>>>> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Jan Beulich
>>>> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect
descriptors
>>> interface to
>>>> public headers
>>>>
>>>> Paul Durrant <Paul.Durrant@citrix.com> wrote:
>>>>>> -----Original Message-----
>>>>>> From: Konrad Rzeszutek Wilk
[mailto:konrad.wilk@oracle.com]
>>>>>> Sent: 14 November 2013 16:24
>>>>>> To: Paul Durrant; Roger Pau Monne; Ian Campbell
>>>>>> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Jan
Beulich
>>>>>> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect
descriptors
>>>>> interface to
>>>>>> public headers
>>>>>>
>>>>>> Paul Durrant <Paul.Durrant@citrix.com> wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: Roger Pau Monné
[mailto:roger.pau@citrix.com]
>>>>>>>> Sent: 14 November 2013 10:27
>>>>>>>> To: Paul Durrant; Ian Campbell
>>>>>>>> Cc: Konrad Rzeszutek Wilk;
xen-devel@lists.xenproject.org; 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@citrix.com]
>>>>>>>>>> Sent: 14 November 2013 10:06
>>>>>>>>>> To: Paul Durrant; Ian Campbell
>>>>>>>>>> Cc: Konrad Rzeszutek Wilk;
xen-devel@lists.xenproject.org;
>>> 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@lists.xenproject.org;
>>>>> 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@lists.xenproject.org;
>>>>>>> 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@lists.xenproject.org; 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.


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

On 14/11/13 20:16, Paul Durrant wrote:
>>> 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.
> 
> D''oh, I missed the pad at the end, so they are the same size but
that doesn''t stop the layout being different and hence needing to make
sure the ABI is set correctly in xenstore.
You already need to set the correct ABI in xenstore, or nothing is going
to work, normal non-indirect requests also have different sizes and offsets.

> -----Original Message-----
> From: Roger Pau Monné [mailto:roger.pau@citrix.com]
> Sent: 15 November 2013 08:02
> To: Paul Durrant; Konrad Rzeszutek Wilk; Ian Campbell
> Cc: xen-devel@lists.xenproject.org; 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@oracle.com]
> >> Sent: 14 November 2013 16:58
> >> To: Paul Durrant; Roger Pau Monne; Ian Campbell
> >> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Jan Beulich
> >> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect descriptors
interface
> to
> >> public headers
> >>
> >> Paul Durrant <Paul.Durrant@citrix.com> wrote:
> >>>> -----Original Message-----
> >>>> From: Konrad Rzeszutek Wilk
[mailto:konrad.wilk@oracle.com]
> >>>> Sent: 14 November 2013 16:34
> >>>> To: Paul Durrant; Roger Pau Monne; Ian Campbell
> >>>> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Jan
Beulich
> >>>> Subject: RE: [Xen-devel] [PATCH] blkif: add indirect
descriptors
> >>> interface to
> >>>> public headers
> >>>>
> >>>> Paul Durrant <Paul.Durrant@citrix.com> wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Konrad Rzeszutek Wilk
[mailto:konrad.wilk@oracle.com]
> >>>>>> Sent: 14 November 2013 16:24
> >>>>>> To: Paul Durrant; Roger Pau Monne; Ian Campbell
> >>>>>> Cc: xen-devel@lists.xenproject.org; Keir
(Xen.org); Jan Beulich
> >>>>>> Subject: RE: [Xen-devel] [PATCH] blkif: add
indirect descriptors
> >>>>> interface to
> >>>>>> public headers
> >>>>>>
> >>>>>> Paul Durrant <Paul.Durrant@citrix.com>
wrote:
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Roger Pau Monné
[mailto:roger.pau@citrix.com]
> >>>>>>>> Sent: 14 November 2013 10:27
> >>>>>>>> To: Paul Durrant; Ian Campbell
> >>>>>>>> Cc: Konrad Rzeszutek Wilk;
xen-devel@lists.xenproject.org; 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@citrix.com]
> >>>>>>>>>> Sent: 14 November 2013 10:06
> >>>>>>>>>> To: Paul Durrant; Ian Campbell
> >>>>>>>>>> Cc: Konrad Rzeszutek Wilk;
xen-devel@lists.xenproject.org;
> >>> 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@lists.xenproject.org;
> >>>>> 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@lists.xenproject.org;
> >>>>>>> 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@lists.xenproject.org; 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@lists.xen.org
http://lists.xen.org/xen-devel

> 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.
Apparently the gcc ABI on x86 is the System V ABI, hence I think these
are the references:
http://www.sco.com/developers/devspecs/abi386-4.pdf‎
http://www.sco.com/developers/gabi/

I'd suggest putting these into arch-x86.h or somewhere towards the top
level of the headers, since they should apply to all hypercall parameter
structs and not just the I/O interfaces.

Ian.


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

On 12/11/13 11:36, Roger Pau Monne wrote:
> Indirect descriptors introduce a new block operation
> (BLKIF_OP_INDIRECT) that passes grant references instead of segments
> in the request. This grant references are filled with arrays of
> blkif_request_segment_aligned, this way we can send more segments in a
> request.
> 
> This interface is already implemented in Linux >= 3.11.
Ping?

I think all comments regarding this patch have been resolved.

>>> On 28.11.13 at 18:02, Roger Pau Monné<roger.pau@citrix.com>
wrote:
> On 12/11/13 11:36, Roger Pau Monne wrote:
>> Indirect descriptors introduce a new block operation
>> (BLKIF_OP_INDIRECT) that passes grant references instead of segments
>> in the request. This grant references are filled with arrays of
>> blkif_request_segment_aligned, this way we can send more segments in a
>> request.
>> 
>> This interface is already implemented in Linux >= 3.11.
> 
> Ping?
> 
> I think all comments regarding this patch have been resolved.
So do I; I continue to have this in my to-be-applied list, pending
the necessary ack from Keir.

Jan

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

On Fri, 2013-11-29 at 10:14 +0000, Jan Beulich wrote:
> >>> On 28.11.13 at 18:02, Roger Pau
Monné<roger.pau@citrix.com> wrote:
> > On 12/11/13 11:36, Roger Pau Monne wrote:
> >> Indirect descriptors introduce a new block operation
> >> (BLKIF_OP_INDIRECT) that passes grant references instead of
segments
> >> in the request. This grant references are filled with arrays of
> >> blkif_request_segment_aligned, this way we can send more segments
in a
> >> request.
> >> 
> >> This interface is already implemented in Linux >= 3.11.
> > 
> > Ping?
> > 
> > I think all comments regarding this patch have been resolved.
> 
> So do I; I continue to have this in my to-be-applied list, pending
> the necessary ack from Keir.
I also had it in my queue and was thinking of applying it.


There are some implicit holes in the struct on 64-bit though (e.g.
between nr_segments and id). Perhaps you could add to your list making
these more explicit as a general clean up, at least with a doc comment,
if not an actual pad field? (Might be worth waiting for dissenting
opinions on that though)

Julien, IIRC you found an issue with the blk struct alignments on ARM
(not related to this patch -- it just reminded me). That was a Linux
side thing, right? Did you send a fix?
> 
> Jan
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


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

On 11/29/2013 10:28 AM, Ian Campbell wrote:
> On Fri, 2013-11-29 at 10:14 +0000, Jan Beulich wrote:
>>>>> On 28.11.13 at 18:02, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>>> On 12/11/13 11:36, Roger Pau Monne wrote:
>>>> Indirect descriptors introduce a new block operation
>>>> (BLKIF_OP_INDIRECT) that passes grant references instead of
segments
>>>> in the request. This grant references are filled with arrays of
>>>> blkif_request_segment_aligned, this way we can send more
segments in a
>>>> request.
>>>>
>>>> This interface is already implemented in Linux >= 3.11.
>>>
>>> Ping?
>>>
>>> I think all comments regarding this patch have been resolved.
>>
>> So do I; I continue to have this in my to-be-applied list, pending
>> the necessary ack from Keir.
>
> I also had it in my queue and was thinking of applying it.
>
>
> There are some implicit holes in the struct on 64-bit though (e.g.
> between nr_segments and id). Perhaps you could add to your list making
> these more explicit as a general clean up, at least with a doc comment,
> if not an actual pad field? (Might be worth waiting for dissenting
> opinions on that though)
>
> Julien, IIRC you found an issue with the blk struct alignments on ARM
> (not related to this patch -- it just reminded me). That was a Linux
> side thing, right? Did you send a fix?
Yes, on ARM (32 and 64 bits), the structure are aligned on 64 bits. 
Linux headers use packed attribute. So the header on the Xen and Linux 
doesn't provide the same structure.

I didn't yet send a fix yet, because we only support Linux for now and 
we will likely broke compatibility with older Linux. Do we want the fix now?

-- 
Julien Grall

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

On Fri, 2013-11-29 at 12:47 +0000, Julien Grall wrote:
> I didn''t yet send a fix yet, because we only support Linux for now
and
> we will likely broke compatibility with older Linux. Do we want the fix
now?
Yes, the sooner we fix Linux to use the defined ABI on ARM the better,
there is no benefit in waiting and letting it get more entrenched.

Ian.

On 29/11/2013 10:14, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> On 28.11.13 at 18:02, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>> On 12/11/13 11:36, Roger Pau Monne wrote:
>>> Indirect descriptors introduce a new block operation
>>> (BLKIF_OP_INDIRECT) that passes grant references instead of
segments
>>> in the request. This grant references are filled with arrays of
>>> blkif_request_segment_aligned, this way we can send more segments
in a
>>> request.
>>> 
>>> This interface is already implemented in Linux >= 3.11.
>> 
>> Ping?
>> 
>> I think all comments regarding this patch have been resolved.
> 
> So do I; I continue to have this in my to-be-applied list, pending
> the necessary ack from Keir.
Acked-by: Keir Fraser <keir@xen.org>
> Jan