[PATCH] blkif: add indirect descriptors interface to public headers

[PATCH] blkif: add indirect descriptors interface to public headers

[Roger Pau Monne]

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

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

[Paul Durrant]

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

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

[David Vrabel]

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

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

[Paul Durrant]

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

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

[Jan Beulich]

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

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

[David Vrabel]

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

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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

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

[Roger Pau Monné]

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

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

[Konrad Rzeszutek Wilk]

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

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

[Ian Campbell]

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

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

[Konrad Rzeszutek Wilk]

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
> 

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

[Ian Campbell]

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.

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

[Paul Durrant]

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

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

[Ian Campbell]

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
> 

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

[Paul Durrant]

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

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

[Ian Campbell]

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.

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

[Paul Durrant]

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

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

[Roger Pau Monné]

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.

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

[Paul Durrant]

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

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

[Roger Pau Monné]

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

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

[Paul Durrant]

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

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

[Roger Pau Monné]

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

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

[Paul Durrant]

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

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

[Konrad Rzeszutek Wilk]

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

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

[Paul Durrant]

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

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

[Konrad Rzeszutek Wilk]

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

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

[Paul Durrant]

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

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

[Konrad Rzeszutek Wilk]

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

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

[Paul Durrant]

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

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

[Konrad Rzeszutek Wilk]

> > 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;
}

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

[Roger Pau Monné]

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

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

[Paul Durrant]

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

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

[Ian Campbell]

> 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

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

[Paul Durrant]

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

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

[Roger Pau Monné]

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.

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

[Konrad Rzeszutek Wilk]

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

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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

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

[Ian Campbell]

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

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

[Julien Grall]

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

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

[Ian Campbell]

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.

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

[Keir Fraser]

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