From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?J=FCrgen_Gro=DF?= Subject: Re: [PATCH V4] Update pvSCSI protocol description Date: Tue, 26 Aug 2014 12:14:25 +0200 Message-ID: <53FC5E01.6080908@suse.com> References: <1409026507-4909-1-git-send-email-jgross@suse.com> <53FC5C21.6090301@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53FC5C21.6090301@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: David Vrabel , xen-devel@lists.xen.org, ian.campbell@citrix.com, ian.jackson@eu.citrix.com, jbeulich@suse.com, keir@xen.org, tim@xen.org List-Id: xen-devel@lists.xenproject.org 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 Thanks, Juergen