From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:48066) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QQygN-0006HR-To for qemu-devel@nongnu.org; Mon, 30 May 2011 05:22:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QQygM-0003f4-P9 for qemu-devel@nongnu.org; Mon, 30 May 2011 05:22:43 -0400 Received: from mail-pz0-f45.google.com ([209.85.210.45]:43687) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QQygM-0003ez-Ge for qemu-devel@nongnu.org; Mon, 30 May 2011 05:22:42 -0400 Received: by pzk30 with SMTP id 30so1665673pzk.4 for ; Mon, 30 May 2011 02:22:41 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <4DE361DB.7090801@redhat.com> Date: Mon, 30 May 2011 11:22:35 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <4DD6246F.4080802@gnu.org> <20110528193321.GA32647@stefanha-thinkpad.localdomain> In-Reply-To: <20110528193321.GA32647@stefanha-thinkpad.localdomain> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] virtio scsi host draft specification, v2 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu-devel On 05/28/2011 09:33 PM, Stefan Hajnoczi wrote: >> Virtqueues >> 0:control transmitq >> 1:control receiveq > > I find these names weird because control commands are actually processed > and completed on the transmitq. The receiveq is only for receiving > asynchronous notifications. > > 0:control commandq > 1:control eventq > > This seems clearer to me although it's not as generic as > transmit/receive if we want to extend its semantics in the future. I took the names from virtio-serial, IIRC. But now I simplified your proposal further to controlq/eventq. >> The driver queues requests to the virtqueue, and they are used by the device >> (not necessarily in order). > > What do you mean by "no necessarily in order"? Doesn't the SAM already > define the available ordering semantics and how the target processes > requests - I think there is no need to mention anything here. Right. >> >> Requests have the following format: >> >> struct virtio_scsi_req_cmd { >> u8 lun[8]; >> u64 id; >> u8 task_attr; >> u8 prio; >> u8 crn; >> u32 num_dataout, num_datain; > > These fields can be discovered from the in and out counts that virtio > provides. They seem redundant to me. I'm not sure, perhaps in the future more variable-sized fields may be added. I added a note that requests will be failed if the driver detects inconsistencies between the actual number of buffers, and the count specified in num_dataout/num_datain. > SAMr4 5.1 The Execute Command procedure call: > "The CRN value zero shall be reserved for use as defined by the SCSI > transport protocol." > > FWIW the SRP spec simply doesn't include CRN and I think we could do the > same. I don't know what it is actually used for in other transports... I wasn't sure of what would happen in the case of SCSI passthrough to protocols that do use CRN. It seems "free" to leave it in. >> The request shall have num_dataout read-only data buffers and >> num_datain write-only data buffers. One of these two values must be >> zero if the VIRTIO_SCSI_F_INOUT has not been negotiated. > > What happens if this VIRTIO_SCSI_F_INOUT has not been negotiated and > both values are non-zero? Perhaps the request should be immediately > returned with response = VIRTIO_SCSI_S_FAILURE. > > I think we should define behavior for all inputs - otherwise we end up > with QEMU-side code that calls abort() which is bad ;). I agree, and I made the change. >> The outcome of the task management function is written by the device >> in the response field. Return values map 1-to-1 with those defined >> in SAM. > > We have no transport-specific response field here like we do with CDBs. > I guess this is okay because the SAM defines SERVICE DELIVERY OR TARGET > FAILURE, which we could use if there is a problem. Yes, that is VIRTIO_SCSI_S_FAILURE. >> The control receiveq is used by the device to report information on >> logical units that are attached to it. The driver should always >> leave a few (?) buffers ready in the control receiveq. The device may > > "The driver should always leave buffers ready in the control receiveq" > > Also, I think it should say "the device must drop events if it finds no > buffer ready". The spec goes into detail on how to notify about dropped > events, using "must" instead of "may" seems right. "Must" seems too strong. Dropped events are a racy event, so it is not really possible to guve any guarantee. I changed it to "will" though. Paolo