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

[Stefan Hajnoczi]

On Fri, May 20, 2011 at 10:21:03AM +0200, Paolo Bonzini wrote:
> Configuration
> -------------
> 
> Subsystem Device ID
>     TBD
> 
> 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.

>     2:requestq
> 
> Feature bits
>     VIRTIO_SCSI_F_INOUT - Whether a single request can include both
>         read-only and write-only data buffers.
> 
> Device configuration layout
>     struct virtio_scsi_config {
>     }
> 
>     (Still empty)
> 
> Device initialization
> ---------------------
> 
> The initialization routine should first of all discover the device's
> control virtqueues.
> 
> The driver should then place at least a buffer in the control receiveq.
> Buffers returned by the device on the control receiveq may be referred
> to as "events" in the rest of the document.
> 
> The driver can immediately issue requests (for example, INQUIRY or
> REPORT LUNS) or task management functions (for example, I_T RESET).
> 
> Device operation: request queue
> -------------------------------
> 
> 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.

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

>         char cdb[];
>         char data[][num_dataout+num_datain];
>         u8 sense[];
>         u32 sense_len;
>         u32 residual;
>         u16 status_qualifier;
>         u8 status;
>         u8 response;
>     };
> 
>     /* command-specific response values */
>     #define VIRTIO_SCSI_S_OK              0
>     #define VIRTIO_SCSI_S_UNDERRUN        1
>     #define VIRTIO_SCSI_S_ABORTED         2
>     #define VIRTIO_SCSI_S_FAILURE         3
> 
>     The lun field addresses a bus, target and logical unit in the SCSI
>     host.  The id field is the command identifier as defined in SAM.
> 
>     The task_attr, prio field should always be zero, as task
>     attributes other than SIMPLE, as well as command priority, are
>     explicitly not supported by this version of the device.
>     CRN is also as defined in SAM; while it is generally expected to
>     be 0, clients can provide it.  The maximum CRN value defined by
>     the protocol is 255, since CRN is stored in an 8-bit integer.

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

> 
>     All of these fields are always read-only.
> 
>     The cdb, data and sense fields must reside in separate buffers.
>     The cdb field is always read-only.  The data buffers may be either
>     read-only or write-only, depending on the request, with the read-only
>     buffers coming first.  The sense buffer is always write-only.
> 
>     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 ;).

> 
>     Remaining fields are filled in by the device.  The sense_len field
>     indicates the number of bytes actually written to the sense buffer,
>     while the residual field indicates the residual size, calculated as
>     data_length - number_of_transferred_bytes.
> 
>     The status byte is written by the device to be the SCSI status code.
> 
>     The response byte is written by the device to be one of the following:
> 
>     - VIRTIO_SCSI_S_OK when the request was completed and the status byte
>       is filled with a SCSI status code (not necessarily "GOOD").
> 
>     - VIRTIO_SCSI_S_UNDERRUN if the content of the CDB requires transferring
>       more data than is available in the data buffers.
> 
>     - VIRTIO_SCSI_S_ABORTED if the request was cancelled due to a reset
>       or another task management function.
> 
>     - VIRTIO_SCSI_S_FAILURE for other host or guest error.
> 
> Device operation: control transmitq
> -----------------------------------
> 
> The control transmitq is used for other SCSI transport operations.
> These are not placed on the requestq so that they can be sent out-of-band,
> even when the requestq is full.  This is particularly important for task
> management functions.
> 
> Requests have the following format:
> 
>     struct virtio_scsi_ctrl
>     {
>         u32 type;
>         ...
>         u8 response;
>     }
> 
>     The type identifies the remaining fields.
> 
> The following commands are defined:
> 
> - Task management function
> 
>     #define VIRTIO_SCSI_T_TMF                      0
> 
>     #define VIRTIO_SCSI_T_TMF_ABORT_TASK           0
>     #define VIRTIO_SCSI_T_TMF_ABORT_TASK_SET       1
>     #define VIRTIO_SCSI_T_TMF_CLEAR_ACA            2
>     #define VIRTIO_SCSI_T_TMF_CLEAR_TASK_SET       3
>     #define VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET      4
>     #define VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET   5
>     #define VIRTIO_SCSI_T_TMF_QUERY_TASK           6
>     #define VIRTIO_SCSI_T_TMF_QUERY_TASK_SET       7
> 
>     struct virtio_scsi_ctrl_tmf
>     {
>         u32 type;
>         u32 subtype;
>         u8 lun[8];
>         u64 id;
>         u8 additional[];
>         u8 response;
>     }
> 
>     /* command-specific response values */
>     #define VIRTIO_SCSI_S_FUNCTION_COMPLETE        0
>     #define VIRTIO_SCSI_S_FAILURE                  3
>     #define VIRTIO_SCSI_S_FUNCTION_SUCCEEDED       4
>     #define VIRTIO_SCSI_S_FUNCTION_REJECTED        5
>     #define VIRTIO_SCSI_S_INCORRECT_LUN            6
> 
>     The type is VIRTIO_SCSI_T_TMF.  All fields but the last one are
>     filled by the driver, the response field is filled in by the device.
>     The id command must match the id in a SCSI command.  Irrelevant fields
>     for the requested TMF are ignored.
> 
>     Note that since ACA is not supported by this version of the spec,
>     VIRTIO_SCSI_T_TMF_CLEAR_ACA is always a no-operation.
> 
>     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.

> - Asynchronous notification query
> 
>     #define VIRTIO_SCSI_T_AN_QUERY                    1
> 
>     struct virtio_scsi_ctrl_an {
>         u32 type;
>         u8  lun[8];
>         u32 event_requested;
>         u32 event_actual;
>         u8  response;
>     }
> 
>     #define VIRTIO_SCSI_EVT_ASYNC_MEDIA_CHANGE        16
> 
>     By sending this command, the driver asks the device which events
>     the given LUN can report, as described in Annex A of the SCSI MMC
>     specification.  The driver writes the events it is interested in
>     into the event_requested; the device responds by writing the events
>     that it supports into event_actual.
> 
>     The type is VIRTIO_SCSI_T_AN_QUERY.  The lun and event_requested
>     fields are written by the driver.  The event_actual and response
>     fields are written by the device.
> 
>     Valid values of the response byte are VIRTIO_SCSI_S_OK or
>     VIRTIO_SCSI_S_FAILURE (with the same meaning as above).
> 
> - Asynchronous notification subscription
> 
>     #define VIRTIO_SCSI_T_AN_SUBSCRIBE                2
> 
>     struct virtio_scsi_ctrl_an {
>         u32 type;
>         u8  lun[8];
>         u32 event_requested;
>         u32 event_actual;
>         u8  response;
>     }
> 
>     #define VIRTIO_SCSI_EVT_ASYNC_MEDIA_CHANGE        16
> 
>     By sending this command, the driver asks the specified LUN to report
>     events for its physical interface, as described in Annex A of the SCSI
>     MMC specification.  The driver writes the events it is interested in
>     into the event_requested; the device responds by writing the events
>     that it supports into event_actual.
> 
>     The type is VIRTIO_SCSI_T_AN_SUBSCRIBE.  The lun and event_requested
>     fields are written by the driver.  The event_actual and response
>     fields are written by the device.
> 
>     Valid values of the response byte are VIRTIO_SCSI_S_OK,
>     VIRTIO_SCSI_S_FAILURE (with the same meaning as above).
> 
> Device operation: control receiveq
> ----------------------------------
> 
> 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.

Stefan