Re: [PATCH V4] Update pvSCSI protocol description

[David Vrabel <david.vrabel@citrix.com>]

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.

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

> +/*
> + * 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.

> + * 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.

>  /*
> - * 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?

With the minor typographical corrections made:

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David