suggesting wording fixes for virtio-spec 0.9.5

DISCUSSION Laszlo Ersek 2 participants 4 messages Apr 22, 2013 – Apr 29, 2013

Subsystems (2)

KVM virtio core

lore

Share & tools

lore

Laszlo Ersek

author

2013-04-22 17:55 UTC #0 lore raw

Hi,

(I'm not subscribed to either list,)

using the word "descriptor" is misleading in the following sections:

2.4.1.2 Updating The Available Ring

[...] However, in general we can add many descriptors before we update the idx fi
eld (at which point they become visible to the
device), so we keep a counter of how many we've added: [...]

and

2.4.1.3 Updating The Index Field

Once the idx fi
eld of the virtqueue is updated, the device will be
able to access the descriptor entries we've created and the memory they refer to. [...]

(The word "descriptor" in the above language is the reason I mis-implemented the virtio-blk guest driver in OVMF.)

In fact the available ring tracks *head* descriptors only. I suggest

s/many descriptors/many separate descriptor chains/ s/descriptor entries/separate descriptor chains/

for the above.

Similarly, 2.3.4 Available Ring should start with something like

The available ring describes what descriptor chains we are offering the device: each entry of the available ring refers to the head descriptor of a separate descriptor chain.

Thanks
Laszlo

Rusty Russell

2013-04-23 04:05 UTC #1 lore raw

Quoted 5 lines · Laszlo Ersek

Hi,
(I'm not subscribed to either list,)
using the word "descriptor" is misleading in the following sections:

Yes, I like the use of 'descriptor chains'. This is a definite improvement.

Here's the diff I ended up with (massaged to minimize it).

Thanks!
Rusty.

virtio-spec.txt renamed from virtio-spec.txt-old +7 -7

@@ -482,10 +482,10 @@
 
 2.3.4 Available Ring
 
-The available ring refers to what descriptors we are offering the 
-device: it refers to the head of a descriptor chain. The “flags” 
+The available ring refers to what descriptor chains we are offering the
+device: each entry refers to the head of a descriptor chain. The “flags”
 field is currently 0 or 1: 1 indicating that we do not need an 
-interrupt when the device consumes a descriptor from the 
+interrupt when the device consumes a descriptor chain from the
 available ring. Alternatively, the guest can ask the device to 
 delay interrupts until an entry with an index specified by the “
 used_event” field is written in the used ring (equivalently,

@@ -671,16 +671,16 @@
 
 avail->ring[avail->idx % qsz] = head;
 
-However, in general we can add many descriptors before we update 
-the “idx” field (at which point they become visible to the 
-device), so we keep a counter of how many we've added:
+However, in general we can add many separate descriptor chains before we update
+the “idx” field (at which point they become visible to the device),
+so we keep a counter of how many we've added:
 
 avail->ring[(avail->idx + added++) % qsz] = head;
 
 2.4.1.3 Updating The Index Field
 
 Once the idx field of the virtqueue is updated, the device will 
-be able to access the descriptor entries we've created and the 
+be able to access the descriptor chains we've created and the 
 memory they refer to. This is why a memory barrier is generally 
 used before the idx update, to ensure it sees the most up-to-date 
 copy.

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Laszlo Ersek

author Reviewed-by

2013-04-23 09:39 UTC #2 lore raw

Quoted 43 lines · Rusty Russell

Quoted 5 lines · Laszlo Ersek

Hi,
(I'm not subscribed to either list,)
using the word "descriptor" is misleading in the following sections:

Yes, I like the use of 'descriptor chains'. This is a definite improvement.

Here's the diff I ended up with (massaged to minimize it).

Thanks!
Rusty.

virtio-spec.txt renamed from virtio-spec.txt-old +6 -6

@@ -482,10 +482,10 @@
 
 2.3.4 Available Ring
 
-The available ring refers to what descriptors we are offering the 
-device: it refers to the head of a descriptor chain. The “flags” 
+The available ring refers to what descriptor chains we are offering the
+device: each entry refers to the head of a descriptor chain. The “flags”
 field is currently 0 or 1: 1 indicating that we do not need an 
-interrupt when the device consumes a descriptor from the 
+interrupt when the device consumes a descriptor chain from the
 available ring. Alternatively, the guest can ask the device to 
 delay interrupts until an entry with an index specified by the “
 used_event” field is written in the used ring (equivalently,

@@ -671,16 +671,16 @@
 
 avail->ring[avail->idx % qsz] = head;
 
-However, in general we can add many descriptors before we update 
-the “idx” field (at which point they become visible to the 
-device), so we keep a counter of how many we've added:
+However, in general we can add many separate descriptor chains before we update
+the “idx” field (at which point they become visible to the device),
+so we keep a counter of how many we've added:

virtio-spec.txt line 686

Quoted 10 lines

virtio-spec.txt renamed from virtio-spec.txt-old +1 -1

 avail->ring[(avail->idx + added++) % qsz] = head;
 
 2.4.1.3 Updating The Index Field
 
 Once the idx field of the virtqueue is updated, the device will 
-be able to access the descriptor entries we've created and the 
+be able to access the descriptor chains we've created and the 
 memory they refer to. This is why a memory barrier is generally 
 used before the idx update, to ensure it sees the most up-to-date 
 copy.

Not sure if it's customary here or if you need it / want it, but anyway

Reviewed-by Laszlo Ersek <[email protected]>

(Also I've fixed the OVMF driver; just reposting the patch today with a better commit message.)

Thanks much!
Laszlo
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Rusty Russell

2013-04-29 00:45 UTC #3 lore raw

Quoted 10 lines · Laszlo Ersek

Quoted 9 lines · Rusty Russell
Quoted 5 lines · Laszlo Ersek
Hi,
(I'm not subscribed to either list,)
using the word "descriptor" is misleading in the following sections:
Yes, I like the use of 'descriptor chains'. This is a definite improvement.

...

Quoted 3 lines

Not sure if it's customary here or if you need it / want it, but anyway
Reviewed-by Laszlo Ersek <[email protected]>

Not needed, but always useful to ahve confirmation that I didn't introduce another mistake while fixing my old ones!

Quoted 5 lines

(Also I've fixed the OVMF driver; just reposting the patch today with a better commit message.)
Thanks much!
Laszlo

Cheers,
Rusty.