suggesting wording fixes for virtio-spec 0.9.5

suggesting wording fixes for virtio-spec 0.9.5

[Laszlo Ersek]

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 \x10idx\x11 fi\x1celd (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\x1celd 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 off^[ering
  the device: each entry of the available ring refers to the head
  descriptor of a separate descriptor chain.

Thanks
Laszlo

Re: suggesting wording fixes for virtio-spec 0.9.5

[Rusty Russell]

Laszlo Ersek <lersek@redhat.com> writes:
> 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-old	2013-04-23 13:22:21.339158214 +0930
+++ virtio-spec.txt	2013-04-23 13:34:14.055176464 +0930
@@ -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
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: suggesting wording fixes for virtio-spec 0.9.5

[Laszlo Ersek]

On 04/23/13 06:05, Rusty Russell wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
>> 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-old	2013-04-23 13:22:21.339158214 +0930
> +++ virtio-spec.txt	2013-04-23 13:34:14.055176464 +0930
> @@ -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.
> 

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

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

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

Thanks much!
Laszlo
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: suggesting wording fixes for virtio-spec 0.9.5

[Rusty Russell]

Laszlo Ersek <lersek@redhat.com> writes:
> On 04/23/13 06:05, Rusty Russell wrote:
>> Laszlo Ersek <lersek@redhat.com> writes:
>>> 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.
...
> Not sure if it's customary here or if you need it / want it, but anyway
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

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

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

Cheers,
Rusty.