On 12/01/2011 04:14 AM, Rusty Russell wrote:

I'd prefer to see the spec only cover things
which are implemented and tested, otherwise the risk of a flaw in the
spec is really high in my experience.

In general I agree, and I did that for virtio-specific things such as 
the eventq and the configuration space.  This is also why I don't want 
to add untested controlq requests that people suggested.

However, there's tension between this and providing a complete SCSI 
transport.  SCSI is roughly defined as a set of RPC interfaces ("Send 
command", "Abort task", etc.) and transports provide the RPC protocol. 
The SCSI command set changes relatively often, but the RPC interfaces 
are pretty stable.  This stability limits the risk, and having a mapping 
for all interfaces also makes future changes less likely.

Comments below:

quoted

   num_queues is the total number of virtqueues exposed by the
     device. The driver is free to use only one request queue, or
     it can use more to achieve better performance.

s/total number of virtqueues/total number of request virtqueues/ ?

Ok.

quoted

   max_channel, max_target and max_lun can be used by the driver
     as hints for scanning the logical units on the host. In the
     current version of the spec, they will always be respectively
     0, 255 and 16383.

s/hints for scanning/hints to constrain scanning/ ? (I assume).

Yes.

quoted

The driver queues requests to an arbitrary request queue, and they are
used by the device on that same queue. In this version of the spec,
if a driver uses more than one queue it is the responsibility of the
driver to ensure strict request ordering; commands placed on different
queue will be consumed with no order constraints.

Suggest simplification of second sentence:

It is the responsibility of the driver to ensure strict request
ordering; commands placed on different queues will be consumed with no
order constraints.

Agreed.

quoted

Task_attr, prio and crn should be left to zero: command priority
is explicitly not supported by this version of the device;
task_attr defines the task attribute as in the table above, but
all task attributes may be mapped to SIMPLE by the device; crn
may also be provided by clients, but is generally expected to be
0. The maximum CRN value defined by the protocol is 255, since
CRN is stored in an 8-bit integer.

Be braver in your language please.  It helps poor implementers who are
already confused by learning SCSI and virtio:

  Task_attr, and prio must be zero.[1] task_attr defines the task
  attribute as in the table above, but all task attributes may be mapped
  to SIMPLE by the device; crn may also be provided by clients, but is
  generally expected to be 0.

  [1] Future extensions may use these fields.

Is it useful for a driver to specify ordered (or other) modes, knowing
it could be reduced to SIMPLE without it being aware?  Or should we use
feature bits to indicate what the device supports?

This is actually mandated by SCSI. (!)  It defines all the modes, but 
explicitly says that they can be reduced to SIMPLE.

quoted

   Note that since ACA is not supported by this version of the
   spec, VIRTIO_SCSI_T_TMF_CLEAR_ACA is always a no-operation.

I think if you don't support ACA in the spec, don't define this.  How
will a driver author use this information?

I will remove the text, no one will notice. :)

However, leaving the #define is preferrable because it keeps the SCSI 
transport complete.  SCSI unfortunately is full of obsolete concepts 
that no one implements but are still in the standard (and have funny 
names: ACA stands for Auto Contingent Allegiance).  Fallbacks are 
allowed and indeed defined by the standard, but an implementation is 
still supposed to provide the "concepts".  You can see this everywhere 
in drivers/target.

quoted

     struct virtio_scsi_ctrl_an {
	 u32 type;
	 u8  lun[8];
	 u32 event_requested;
	 u32 event_actual;
	 u8  response;
     }

With all these structures, you might want a comment indicating the
read-only and write-only (from the device POV) parts of the struct, eg:

      struct virtio_scsi_ctrl_an {
          // Read-only part
	 u32 type;
	 u8  lun[8];
	 u32 event_requested;
          // Write-only part
	 u32 event_actual;
	 u8  response;
      }

(Very) good idea.

Paolo