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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: David Vrabel
> Sent: 12 November 2013 14:12
> To: Paul Durrant
> Cc: Roger Pau Monne; xen-devel@xxxxxxxxxxxxxxxxxxxx; 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@xxxxxxxxxxxxx [mailto:xen-devel-
> >> bounces@xxxxxxxxxxxxx] On Behalf Of Roger Pau Monne
> >> Sent: 12 November 2013 10:37
> >> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> >> 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@xxxxxxxxxx>
> >> Cc: Keir Fraser <keir@xxxxxxx>
> >> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> >> ---
> >>  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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel