Re: [virtio-dev] [PATCH] message framing: make ANY_LAYOUT implicit

[Rusty Russell <rusty@au1.ibm.com>]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Mon, Sep 16, 2013 at 05:12:34PM +0930, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> > This resolves issue VIRTIO-10
>> >
>> > This also creates a new section for legacy feature
>> > bits which will be handy for VIRTIO-13 if that is
>> > accepted.
>> >
>> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> 
>> This is very similar to the patch I came up with: I'm too slow!
>> 
>> Pasted below, in particular the 2.4.2.5.1 header needs to move up a few
>> lines, too.
>> 
>> Cheers,
>> Rusty.
>
> Cool. A couple of comments below.

>> +2.1.4.2.1. Legacy Interface: Message Framing
>> +-----------------------
>>  
>>  Regrettably, initial driver implementations used simple layouts, and
>> -devices came to rely on it, despite this specification wording[10]. It
>> -is thus recommended that drivers be conservative in their assumptions,
>> -unless the VIRTIO_F_ANY_LAYOUT feature is accepted. In addition, some
>> -implementations may have large-but-reasonable restrictions on total
>> -descriptor size (such as based on IOV_MAX in the host OS). This has
>> -not been a problem in practice: little sympathy will be given to
>> -drivers which create unreasonably-sized descriptors such as by
>> -dividing a network packet into 1500 single-byte descriptors!
>> +devices came to rely on it, despite this specification wording.  In
>> +addition, the specification for virtio_blk SCSI commands required
>> +intuiting field lengths from frame boundaries (see "2.4.2.5.1 Legacy
>> +Interface: Device Operation")
>> +
>> +It is thus recommended that
>
> Maybe add:
> " when using legacy interfaces, transitional "
>
> to stress that it does not need to be checked in non legacy
> setting.

Yep, good idea.

>
>> drivers be conservative in their
>> +assumptions, unless the VIRTIO_F_ANY_LAYOUT feature is accepted.
>
>>  
>>  2.1.4.3. The Virtqueue Descriptor Table
>>  --------------------------------------
>> @@ -1193,7 +1201,7 @@ features.
>>    VIRTIO_NET_F_MAC (5) Device has given MAC address.
>>  
>>    VIRTIO_NET_F_GSO (6) (Deprecated) device handles packets with
>> -    any GSO type.[12]
>> +    any GSO type.[11]
>>  
>>    VIRTIO_NET_F_GUEST_TSO4 (7) Guest can receive TSOv4.
>>  
>
> This generated a huge diff because of renumbering.
> What I generally do is take out a chunk and commit,
> then make renumbering a separate
> commit, it does not need review :)
>
> If I add a chunk, and numbering does not fit naturally,
> I add it with a random huge number like 123 - this makes
> it possible to add cross-references directly.
> Again commit then re-number separately.

Right, I'll start doing the same.

>>  2.4.5.6.1. Memory Statistics
>>  ---------------------------
>> @@ -2577,10 +2585,7 @@ contents of the event field. The following events are defined:
>>  2.5. Reserved Feature Bits
>>  =========================
>>  
>> -Currently there are four device-independent feature bits defined:
>> -
>> -  VIRTIO_F_ANY_LAYOUT (27) This feature indicates that the device accepts arbitrary
>> -  descriptor layouts, as described in Section "2.1.4.2. Message Framing".
>> +Currently there are three device-independent feature bits defined:
>>  
>>    VIRTIO_F_RING_INDIRECT_DESC (28) Negotiating this feature indicates
>>    that the driver can use descriptors with the VRING_DESC_F_INDIRECT
>> @@ -2621,6 +2626,10 @@ VIRTIO_F_NOTIFY_ON_EMPTY (24) Negotiating this feature
>>    using a timer if the device interrupts it when all the packets
>>    are transmitted.
>>  
>> +VIRTIO_F_ANY_LAYOUT (27) This feature indicates that the device
>> +  accepts arbitrary descriptor layouts, as described in Section
>> +  "2.1.4.2.1. Legacy Interface: Message Framing".
>> +
>
> Is this on top of a patch making NOTIFY_ON_EMPTY legacy?
>
> We should also move BAD_FEATURE here.

Yes, I have a working tree.  I'm switching to per-topic branches, since
merge order isn't determinate.

Here is the result:

commit b245c20be903005dd79dc5a23abb54d56ead73ca
Author: Rusty Russell <rusty@au1.ibm.com>
Date:   Mon Sep 9 17:40:32 2013 +0930

    Any layout is allowed (deprecate VIRTIO_F_ANY_LAYOUT)
    
    As per issue virtio-10.
    
    Signed-off-by: Rusty Russell <rusty@au1.ibm.com>

diff --git a/virtio-v1.0-wd01-part1-specification.txt b/virtio-v1.0-wd01-part1-specification.txt
index 743467a..02bb789 100644
--- a/virtio-v1.0-wd01-part1-specification.txt
+++ b/virtio-v1.0-wd01-part1-specification.txt
@@ -268,25 +268,34 @@ is not a significant issue.
 
 2.1.4.2. Message Framing
 -----------------------
-The original intent of the specification was that message framing (the
-particular layout of descriptors) be independent of the contents of
-the buffers. For example, a network transmit buffer consists of a 12
-byte header followed by the network packet. This could be most simply
-placed in the descriptor table as a 12 byte output descriptor followed
-by a 1514 byte output descriptor, but it could also consist of a
-single 1526 byte output descriptor in the case where the header and
-packet are adjacent, or even three or more descriptors (possibly with
-loss of efficiency in that case).
+The message framing (the particular layout of descriptors) is
+independent of the contents of the buffers. For example, a network
+transmit buffer consists of a 12 byte header followed by the network
+packet. This could be most simply placed in the descriptor table as a
+12 byte output descriptor followed by a 1514 byte output descriptor,
+but it could also consist of a single 1526 byte output descriptor in
+the case where the header and packet are adjacent, or even three or
+more descriptors (possibly with loss of efficiency in that case).
+
+Note that, some implementations may have large-but-reasonable
+restrictions on total descriptor size (such as based on IOV_MAX in the
+host OS). This has not been a problem in practice: little sympathy
+will be given to drivers which create unreasonably-sized descriptors
+such as by dividing a network packet into 1500 single-byte
+descriptors!
+
+2.1.4.2.1. Legacy Interface: Message Framing
+-----------------------
 
 Regrettably, initial driver implementations used simple layouts, and
-devices came to rely on it, despite this specification wording[10]. It
-is thus recommended that drivers be conservative in their assumptions,
-unless the VIRTIO_F_ANY_LAYOUT feature is accepted. In addition, some
-implementations may have large-but-reasonable restrictions on total
-descriptor size (such as based on IOV_MAX in the host OS). This has
-not been a problem in practice: little sympathy will be given to
-drivers which create unreasonably-sized descriptors such as by
-dividing a network packet into 1500 single-byte descriptors!
+devices came to rely on it, despite this specification wording.  In
+addition, the specification for virtio_blk SCSI commands required
+intuiting field lengths from frame boundaries (see "2.4.2.5.1 Legacy
+Interface: Device Operation")
+
+It is thus recommended that when using legacy interfaces, transitional
+drivers be conservative in their assumptions, unless the
+VIRTIO_F_ANY_LAYOUT feature is accepted.
 
 2.1.4.3. The Virtqueue Descriptor Table
 --------------------------------------
@@ -1729,6 +1738,9 @@ error or VIRTIO_BLK_S_UNSUPP for a request unsupported by host:
 	#define VIRTIO_BLK_S_IOERR     1
 	#define VIRTIO_BLK_S_UNSUPP    2
 
+2.4.2.5.1 Legacy Interface: Device Operation
+------------------------
+
 Historically, devices assumed that the fields type, ioprio and
 sector reside in a single, separate read-only buffer; the fields
 errors, data_len, sense_len and residual reside in a single,
@@ -2583,9 +2595,6 @@ Currently there are five device-independent feature bits defined:
   using a timer if the device interrupts it when all the packets
   are transmitted.
 
-  VIRTIO_F_ANY_LAYOUT (27) This feature indicates that the device accepts arbitrary
-  descriptor layouts, as described in Section "2.1.4.2. Message Framing".
-
   VIRTIO_F_RING_INDIRECT_DESC (28) Negotiating this feature indicates
   that the driver can use descriptors with the VRING_DESC_F_INDIRECT
   flag set, as described in "2.1.4.3.1. Indirect Descriptors".
@@ -2609,6 +2618,16 @@ In addition, bit 30 is used by qemu's implementation to check for experimental
 early versions of virtio which did not perform correct feature negotiation,
 and should not be used.
 
+2.5.1 Legacy Interface: Reserved Feature Bits
+--------------------------------------------
+
+Legacy or transitional devices may offer the following:
+
+VIRTIO_F_ANY_LAYOUT (27) This feature indicates that the device
+  accepts arbitrary descriptor layouts, as described in Section
+  "2.1.4.2.1. Legacy Interface: Message Framing".
+
+
 2.6. virtio_ring.h
 =================
 
@@ -2883,12 +2902,6 @@ of this expected condition is necessary.
 
 [9] https://lists.linux-foundation.org/mailman/listinfo/virtualization
 
-[10] It was previously asserted that framing should be independent of message
-contents, yet invariably drivers layed out messages in reliable ways and
-devices assumed it.
-In addition, the specifications for virtio_blk and virtio_scsi require
-intuiting field lengths from frame boundaries.
-
 [11] Even if it does mean documenting design or implementation
 mistakes!