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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Aug 26, 2014 at 12:14:25PM +0200, Jürgen Groß wrote:
> 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,

Please tack on:

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

Thank you.
> 
> Juergen
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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