Re: [Xen-devel] [PATCH V4] Update pvSCSI protocol description

[Jürgen Groß <jgross@xxxxxxxx>]

On 08/26/2014 12:06 PM, David Vrabel wrote:
> On 26/08/14 05:15, Juergen Gross wrote:
> > Update the protocol description of the pvSCSI framework used to pass through
> > SCSI devices to a guest (pv or hvm).
> 4 versions in 24 hours?!  Please allow a few days for people to review
> before posting updated versions.
Jan requested some substantial changes, so I replied quickly to
make further reviews easier.

> > --- a/xen/include/public/io/vscsiif.h
> > +++ b/xen/include/public/io/vscsiif.h
> > @@ -1,8 +1,11 @@
> >   
> > /******************************************************************************
> >    * 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.
> There shouldn't be anything Linux-specific about this ABI.  I would drop
> this paragraph.
Yeah, you are right.

> > +/*
> > + * Request a SCSI operation specified via a CDB in vscsiif_request.cmnd.
> > + * The target is specified via channel, id and lun.
> > + * The operation to be performed is specified via a CDB in cmnd[], the length
> > + * of the CDB is in cmd_len. sc_data_direction specifies the direction of data
> > + * (to the device, from the device, or none at all).
> > + * If data is to be transferred to or from the device the buffer(s) in the
> Blank lines between paragraphs. please.
Okay.

> > + * If "feature-sg-grant" in the Xenstore is set it is possible to specify more
> > + * than VSCSIIF_SG_TABLESIZE scsiif_request_segment elements via indirection.
> > + * The maximum number of allowed scsiif_request_segment elements is the value
> > + * of the "feature-sg-grant" entry from Xenstore. When using indirection the
> > + * seg[] array doesn't contain specifications of the data buffers, but
> > + * references to scsiif_request_segment arrays, which in turn reference the
> > + * data buffers. While nr_segments holds the number of populated seg[] entries
> > + * (plus the set VSCSIIF_SG_GRANT bit), the number of scsiif_request_segment
> > + * elements referencing the target data buffers is calculated from the lengths
> > + * of the seg[] elements (the sum of all valid seg[].length divided by the
> > + * size of one scsiif_request_segment structure).
> Add a sentence such as "The frontend may use a mix of direct and
> indirect requests."
>
> A #define for the number of scsiif_request_segments per page might be
> useful.
I don't mind adding it.

> >   /*
> > - * based on Linux kernel 2.6.18
> > + * based on Linux kernel 2.6.18, still valid
> > + * Changing these values requires support of multiple protocols via the rings
> > + * as "old clients" will blindly use these values and the resulting structure
> > + * sizes.
> What does this comment about being "based on Linux kernel" mean?  Is it
> useful?
It describes the origin of the "magic" numbers. I'd like to keep it.

> With the minor typographical corrections made:
>
> Reviewed-by: David Vrabel <david.vrabel@xxxxxxxxxx>
Thanks,

Juergen


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