Re: [Qemu-devel] virtio scsi host draft specification, v2

[Paolo Bonzini <pbonzini@redhat.com>]

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