[virtio-spec PATCH 0/5] Receiving Used Buffers example code: cleanups and an extra mb()

[virtio-spec PATCH 0/5] Receiving Used Buffers example code: cleanups and an extra mb()

[Laszlo Ersek]

Patches before the last are small cleanups.

In the last patch I'm trying to extract / generalize an idea from Stefan
Hajnoczi's review of my virtio-net driver for OVMF.

Unfortunately I can't find Stefan's email on any mailing list archive
(sourceforge, gmane, mail-archive etc. all have only my response), so
I'll quote it here.

The patch Stefan was reviewing is
<http://thread.gmane.org/gmane.comp.bios.tianocore.devel/2804/focus=2819>:

  [PATCH v3 10/15]
  OvmfPkg: VirtioNetDxe: implement Tx: SNP.Transmit and SNP.GetStatus

On 06/07/13 16:17, Stefan Hajnoczi wrote:

> There is no read memory barrier between fetching TxCurUsed and
> fetching UsedElem[].Id.  In theory I think there is no guarantee that
> Dev->TxRing.Used.UsedElem[UsedElemIdx].Id is fetched *after*
> Dev->TxRing.Used.Idx.  On x86 it shouldn't be a problem but I expected
> a read memory barrier after comparing fetching Dev->TxRing.Used.Idx.

(Solely for the record, my response is at
<http://thread.gmane.org/gmane.comp.bios.tianocore.devel/3052>.)

In the last patch I'm trying to apply this remark to the virtio spec.
Hopefully I'm not misrepresenting the idea, nor glossing over any
important differences between the VirtioNetDxe code Stefan was actually
reviewing and the example code in the virtio spec.

I didn't add

  Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>

to the last patch, because my translation may easily have corrupted his
idea, but if the patch is deemed worthwhile, please do add his
Suggested-by.


I'm not subscribed to the virtualization list, please keep me CC'd.

Thanks!

Laszlo Ersek (5):
  Receiving Used Buffers: fix typo in "ring empty" condition in example
    code
  Receiving Used Buffers: re-disable interrupts when staying in the
    loop
  Receiving Used Buffers: variable for Queue Size is called "qsz"
    elsewhere
  Receiving Used Buffers: switch . and -> operators, add missing &
  Receiving Used Buffers: prevent speculative load when not
    sequentially consistent

 virtio-spec.lyx |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

[virtio-spec PATCH 1/5] Receiving Used Buffers: fix typo in "ring empty" condition in example code

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 virtio-spec.lyx |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index 6e188d0..41a763c 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -2942,7 +2942,7 @@ for (;;) {
 
 \begin_layout Plain Layout
 
-    if (vq->last_seen_used != vring->used.idx) {
+    if (vq->last_seen_used == vring->used.idx) {
 \end_layout
 
 \begin_layout Plain Layout
@@ -2957,7 +2957,7 @@ for (;;) {
 
 \begin_layout Plain Layout
 
-		if (vq->last_seen_used != vring->used.idx)
+		if (vq->last_seen_used == vring->used.idx)
 \end_layout
 
 \begin_layout Plain Layout
-- 
1.7.1

[virtio-spec PATCH 2/5] Receiving Used Buffers: re-disable interrupts when staying in the loop

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 virtio-spec.lyx |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index 41a763c..851a0cf 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -2967,6 +2967,11 @@ for (;;) {
 
 \begin_layout Plain Layout
 
+		vring_disable_interrupts(vq);
+\end_layout
+
+\begin_layout Plain Layout
+
     }
 \end_layout
 
-- 
1.7.1

[virtio-spec PATCH 3/5] Receiving Used Buffers: variable for Queue Size is called "qsz" elsewhere

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 virtio-spec.lyx |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index 851a0cf..1a0923b 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -2977,7 +2977,7 @@ for (;;) {
 
 \begin_layout Plain Layout
 
-    struct vring_used_elem *e = vring.used->ring[vq->last_seen_used%vsz];
+    struct vring_used_elem *e = vring.used->ring[vq->last_seen_used % qsz];
 \end_layout
 
 \begin_layout Plain Layout
-- 
1.7.1

[virtio-spec PATCH 4/5] Receiving Used Buffers: switch . and -> operators, add missing &

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 virtio-spec.lyx |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index 1a0923b..51f41dc 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -2977,7 +2977,7 @@ for (;;) {
 
 \begin_layout Plain Layout
 
-    struct vring_used_elem *e = vring.used->ring[vq->last_seen_used % qsz];
+    struct vring_used_elem *e = &vring->used.ring[vq->last_seen_used % qsz];
 \end_layout
 
 \begin_layout Plain Layout
-- 
1.7.1

[virtio-spec PATCH 5/5] Receiving Used Buffers: prevent speculative load when not sequentially consistent

[Laszlo Ersek]

The example code in "Receiving Used Buffers From The Device" goes as
follows:

    vring_disable_interrupts(vq);
    for (;;) {
        if (vq->last_seen_used == vring->used.idx) {
            vring_enable_interrupts(vq);
            mb();
            if (vq->last_seen_used == vring->used.idx)
                break;
            vring_disable_interrupts(vq);
        }
        struct vring_used_elem *e =
            &vring->used.ring[vq->last_seen_used % qsz];
        process_buffer(e);
        vq->last_seen_used++;
    }

Suppose that process_buffer() does something like this:

    process_buffer(struct vring_used_elem *e)
    {
        u32 id;
        u32 len;

        id = e->id;
        len = e->len;

        /* go on to process the descriptor chain starting at "id" */
        process_desc_chain(id, len);
    }

On an architecture that is not sequentially consistent, the "e->id"
load could be executed out of order, speculatively; for example:

    vring_disable_interrupts(vq);
    for (;;) {
        struct vring_used_elem *e =
            &vring->used.ring[vq->last_seen_used % qsz];
        u32 id = e->id;
        u32 len = e->len;

        if (vq->last_seen_used == vring->used.idx) {
            vring_enable_interrupts(vq);
            mb();
            if (vq->last_seen_used == vring->used.idx)
                break;
            vring_disable_interrupts(vq);
        }

        process_desc_chain(id, len);
        vq->last_seen_used++;
    }

The (*e) pointer dereferences are OK, but the pointer targets could be
stale at the time of the speculative load (just adding comments):

    vring_disable_interrupts(vq);
    for (;;) {

        /* ring is empty, load stale data into "id" and "len" */

        struct vring_used_elem *e =
            &vring->used.ring[vq->last_seen_used % qsz];
        u32 id = e->id;
        u32 len = e->len;

        /* at this point the host goes through all of the following:
         * - fill in a descriptor chain
         * - place the index of its head descriptor on the used ring
         * - store the number of processed bytes too
         * - mb()
         * - bump used index
         * - mb()
         *
         * and the used index happens to become visible to the guest,
         * enabling it to move past the next check:
         */

        if (vq->last_seen_used == vring->used.idx) {
            vring_enable_interrupts(vq);
            mb();
            if (vq->last_seen_used == vring->used.idx)
                break;
            vring_disable_interrupts(vq);
        }

        /* use stale "id" and "len" */

        process_desc_chain(id, len);
        vq->last_seen_used++;
    }

Prevent this by adding a barrier between the ring emptiness check and the
access to the used element.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 virtio-spec.lyx |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index 51f41dc..2186d35 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -2982,6 +2982,11 @@ for (;;) {
 
 \begin_layout Plain Layout
 
+    mb();
+\end_layout
+
+\begin_layout Plain Layout
+
     process_buffer(e);
 \end_layout
 
-- 
1.7.1

Re: [virtio-spec PATCH 0/5] Receiving Used Buffers example code: cleanups and an extra mb()

[Stefan Hajnoczi]

On Sat, Jun 8, 2013 at 7:39 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> Patches before the last are small cleanups.
>
> In the last patch I'm trying to extract / generalize an idea from Stefan
> Hajnoczi's review of my virtio-net driver for OVMF.
>
> Unfortunately I can't find Stefan's email on any mailing list archive
> (sourceforge, gmane, mail-archive etc. all have only my response), so
> I'll quote it here.
>
> The patch Stefan was reviewing is
> <http://thread.gmane.org/gmane.comp.bios.tianocore.devel/2804/focus=2819>:
>
>   [PATCH v3 10/15]
>   OvmfPkg: VirtioNetDxe: implement Tx: SNP.Transmit and SNP.GetStatus
>
> On 06/07/13 16:17, Stefan Hajnoczi wrote:
>
>> There is no read memory barrier between fetching TxCurUsed and
>> fetching UsedElem[].Id.  In theory I think there is no guarantee that
>> Dev->TxRing.Used.UsedElem[UsedElemIdx].Id is fetched *after*
>> Dev->TxRing.Used.Idx.  On x86 it shouldn't be a problem but I expected
>> a read memory barrier after comparing fetching Dev->TxRing.Used.Idx.
>
> (Solely for the record, my response is at
> <http://thread.gmane.org/gmane.comp.bios.tianocore.devel/3052>.)
>
> In the last patch I'm trying to apply this remark to the virtio spec.
> Hopefully I'm not misrepresenting the idea, nor glossing over any
> important differences between the VirtioNetDxe code Stefan was actually
> reviewing and the example code in the virtio spec.
>
> I didn't add
>
>   Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
>
> to the last patch, because my translation may easily have corrupted his
> idea, but if the patch is deemed worthwhile, please do add his
> Suggested-by.
>
>
> I'm not subscribed to the virtualization list, please keep me CC'd.
>
> Thanks!
>
> Laszlo Ersek (5):
>   Receiving Used Buffers: fix typo in "ring empty" condition in example
>     code
>   Receiving Used Buffers: re-disable interrupts when staying in the
>     loop
>   Receiving Used Buffers: variable for Queue Size is called "qsz"
>     elsewhere
>   Receiving Used Buffers: switch . and -> operators, add missing &
>   Receiving Used Buffers: prevent speculative load when not
>     sequentially consistent
>
>  virtio-spec.lyx |   16 +++++++++++++---
>  1 files changed, 13 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [virtio-spec PATCH 0/5] Receiving Used Buffers example code: cleanups and an extra mb()

[Rusty Russell]

Stefan Hajnoczi <stefanha@gmail.com> writes:
> On Sat, Jun 8, 2013 at 7:39 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>> Patches before the last are small cleanups.
>>
>> In the last patch I'm trying to extract / generalize an idea from Stefan
>> Hajnoczi's review of my virtio-net driver for OVMF.

How about a single patch which just replaces the completely broken
example? :)

>>   Receiving Used Buffers: prevent speculative load when not
>>     sequentially consistent

Yes, though this only needs to be a rmb().

In the OASIS 1.0 spec, I'd like an appendix with tested code for doing
these operations (probably based on a simplifeid version of vringh.c).

Thanks,
Rusty.

Re: [virtio-spec PATCH 0/5] Receiving Used Buffers example code: cleanups and an extra mb()

[Laszlo Ersek]

On 06/17/13 04:17, Rusty Russell wrote:
> Stefan Hajnoczi <stefanha@gmail.com> writes:
>> On Sat, Jun 8, 2013 at 7:39 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>>> Patches before the last are small cleanups.
>>>
>>> In the last patch I'm trying to extract / generalize an idea from Stefan
>>> Hajnoczi's review of my virtio-net driver for OVMF.
> 
> How about a single patch which just replaces the completely broken
> example? :)

Do you want me to squash the series into one patch and repost?

>>>   Receiving Used Buffers: prevent speculative load when not
>>>     sequentially consistent
> 
> Yes, though this only needs to be a rmb().

Probably, but the spec never qualifies the memory barriers it
recommends. (Maybe their read/write types should be obvious to the
reader. I didn't give their types much thought because in edk2/OVMF
there's only MemoryFence().)

In any case I'll take this as your approval of the 5/5 patch. Thanks!

> In the OASIS 1.0 spec, I'd like an appendix with tested code for doing
> these operations (probably based on a simplifeid version of vringh.c).

(
I assume the tested code should come from one of the more sophisticated
drivers (that also have a unix-y coding style). Also, I've been actively
avoiding reading other virtio code (qemu, kernel, SeaBIOS, iPXE etc)
except when I was stuck with an OVMF driver and (a) it looked like there
was some silent requirement/assumption in qemu that had not been spelled
out in the spec, (b) I was being dyslexic.

Thus I can't really suggest code for the appendix (although the OVMF
drivers do work).
)

Thanks,
Laszlo