Re: [PATCH V4] Update pvSCSI protocol description

["Jürgen Groß" <jgross@suse.com>]

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@citrix.com>

Thanks,

Juergen