Re: [Xen-devel] [PATCH 1/4] Add XEN pvSCSI protocol description

[Juergen Gross <jgross@xxxxxxxx>]

On 06/27/2014 07:11 PM, Konrad Rzeszutek Wilk wrote:
> On Fri, Jun 27, 2014 at 04:34:33PM +0200, jgross@xxxxxxxx wrote:
> > From: Juergen Gross <jgross@xxxxxxxx>
> >
> > Add the definition of pvSCSI protocol used between the pvSCSI frontend in a
> > XEN domU and the pvSCSI backend in a XEN driver domain (usually Dom0).
> >
> > This header was originally provided by Fujitsu for XEN based on Linux 2.6.18.
> > Changes are:
> > - added comment
> > - adapt to Linux style guide
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> >   include/xen/interface/io/vscsiif.h | 110 +++++++++++++++++++++++++++++++++++++
> >   1 file changed, 110 insertions(+)
> >   create mode 100644 include/xen/interface/io/vscsiif.h
> >
> > diff --git a/include/xen/interface/io/vscsiif.h 
> > b/include/xen/interface/io/vscsiif.h
> > new file mode 100644
> > index 0000000..f8420f3
> > --- /dev/null
> > +++ b/include/xen/interface/io/vscsiif.h
> > @@ -0,0 +1,110 @@
> > +/******************************************************************************
> > + * vscsiif.h
> > + *
> > + * Based on the blkif.h code.
> > + *
> > + * This interface is to be regarded as a stable API between XEN domains
> > + * running potentially different Linux kernel versions.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a copy
> > + * of this software and associated documentation files (the "Software"), to
> > + * deal in the Software without restriction, including without limitation the
> > + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> > + * sell copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> > + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > + * DEALINGS IN THE SOFTWARE.
> > + *
> > + * Copyright(c) FUJITSU Limited 2008.
> > + */
> > +
> > +#ifndef __XEN__PUBLIC_IO_SCSI_H__
> > +#define __XEN__PUBLIC_IO_SCSI_H__
> > +
> > +#include "ring.h"
> > +#include "../grant_table.h"
> > +
> > +/* commands between backend and frontend */
> > +#define VSCSIIF_ACT_SCSI_CDB           1       /* SCSI CDB command */
> > +#define VSCSIIF_ACT_SCSI_ABORT         2       /* SCSI Device(Lun) Abort*/
> > +#define VSCSIIF_ACT_SCSI_RESET         3       /* SCSI Device(Lun) Reset*/
> > +#define VSCSIIF_ACT_SCSI_SG_PRESET     4       /* Preset SG elements */
> > +
> > +/*
> > + * Maximum scatter/gather segments per request.
> > + *
> > + * Considering balance between allocating at least 16 "vscsiif_request"
> > + * structures on one page (4096 bytes) and the number of scatter/gather
> > + * elements needed, we decided to use 26 as a magic number.
> > + */
> > +#define VSCSIIF_SG_TABLESIZE           26
> > +
> > +/*
> > + * based on Linux kernel 2.6.18
> This being a bit more .. new, - do these sizes make sense anymore?
> Should they be extended a bit? Or have support for using the
> old ones (as default) and then negotiate new sizes with the kernel?
> (If of course there is a difference?)
The sizes are still the same. Additionally the sizes can't be changed
without breaking the interface (the request structure must not change
size). Added a comment.

> > + */
> > +#define VSCSIIF_MAX_COMMAND_SIZE       16
> > +#define VSCSIIF_SENSE_BUFFERSIZE       96
> > +
> > +struct scsiif_request_segment {
> > +       grant_ref_t gref;
> > +       uint16_t offset;
> > +       uint16_t length;
> > +};
> > +typedef struct scsiif_request_segment vscsiif_segment_t;
> No typedefs please.
Okay.

> > +
> > +struct vscsiif_request {
> > +       uint16_t rqid;          /* private guest value, echoed in resp  */
> > +       uint8_t act;            /* command between backend and frontend */
> > +       uint8_t cmd_len;
> > +
> > +       uint8_t cmnd[VSCSIIF_MAX_COMMAND_SIZE];
> > +       uint16_t timeout_per_command;   /* The command is issued by twice
> > +                                          the value in Backend. */
> > +       uint16_t channel, id, lun;
> > +       uint16_t padding;
> > +       uint8_t sc_data_direction;      /* for DMA_TO_DEVICE(1)
> > +                                          DMA_FROM_DEVICE(2)
> > +                                          DMA_NONE(3) requests */
> > +       uint8_t nr_segments;            /* Number of pieces of scatter-gather */
> > +
> > +       vscsiif_segment_t seg[VSCSIIF_SG_TABLESIZE];
> > +       uint32_t reserved[3];
> > +};
> Could you add a comment saying what the size should be. Just in case
> somebody extends this in the future - they will know the limits.
Okay.

> > +typedef struct vscsiif_request vscsiif_request_t;
> Ditto.
> > +
> > +#define VSCSIIF_SG_LIST_SIZE ((sizeof(vscsiif_request_t) - 4) /                
> > \
> > +                               sizeof(vscsiif_segment_t))
> That -4 ought to have a comment. In case the scsiif_request_segment
> gets a new member or such.
I just removed VSCSIIF_SG_LIST_SIZE and struct vscsiif_sg_list as they
are never used.

> > +
> > +struct vscsiif_sg_list {
> > +       /* First two fields must match struct vscsiif_request! */
> > +       uint16_t rqid;          /* private guest value, must match main req */
> > +       uint8_t act;            /* VSCSIIF_ACT_SCSI_SG_PRESET */
> > +       uint8_t nr_segments;    /* Number of pieces of scatter-gather */
> > +       vscsiif_segment_t seg[VSCSIIF_SG_LIST_SIZE];
> > +};
> > +typedef struct vscsiif_sg_list vscsiif_sg_list_t;
> Ditto.
> > +
> > +struct vscsiif_response {
> > +       uint16_t rqid;
> > +       uint8_t act;            /* valid only when backend supports SG_PRESET */
> Same comment? First two fields must match 'struct vscsiif_request' ?
No. act has no meaning (SG_PRESET was never supported), rqid is used to
identify the request to which the response belongs. Added a comment.

> > +       uint8_t sense_len;
> > +       uint8_t sense_buffer[VSCSIIF_SENSE_BUFFERSIZE];
> > +       int32_t rslt;
> > +       uint32_t residual_len;  /* request bufflen -
> > +                                  return the value from physical device */
> > +       uint32_t reserved[36];
> > +};
> > +typedef struct vscsiif_response vscsiif_response_t;
> I think you know what I am going to say :-)
No idea. ;-)


Juergen

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