[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-dev] [PATCH] message framing: make ANY_LAYOUT implicit
"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!
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]