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

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 at 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(-)

Signed-off-by: Laszlo Ersek <lersek at 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

Signed-off-by: Laszlo Ersek <lersek at 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

Signed-off-by: Laszlo Ersek <lersek at 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

Signed-off-by: Laszlo Ersek <lersek at 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

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 at 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

On Sat, Jun 8, 2013 at 7:39 PM, Laszlo Ersek <lersek at 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 at 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 at redhat.com>

Stefan Hajnoczi <stefanha at gmail.com> writes:
> On Sat, Jun 8, 2013 at 7:39 PM, Laszlo Ersek <lersek at 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.