[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] pc-bios/s390-ccw: Use memory barriers in virtio code

From: Thomas Huth
Subject: Re: [PATCH] pc-bios/s390-ccw: Use memory barriers in virtio code
Date: Tue, 16 Feb 2021 15:21:45 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0

On 16/02/2021 12.47, Cornelia Huck wrote:
On Tue, 16 Feb 2021 12:00:56 +0100
Thomas Huth <thuth@redhat.com> wrote:

According to the virtio specification, a memory barrier should be
used before incrementing the idx field in the "available" ring.
So far, we did not do this in the s390-ccw bios yet, but recently
Peter Maydell saw problems with the s390-ccw bios when running
the qtests on an aarch64 host (the bios panic'ed with the message:
"SCSI cannot report LUNs: response VS RESP=09"), which could
maybe be related to the missing memory barriers. Thus let's add
those barriers now. Since we've only seen the problem on TCG so far,
a "bcr 14,0" should be sufficient here to trigger the tcg_gen_mb()
in the TCG translate code.

(Note: The virtio spec also talks about using a memory barrier
*after* incrementing the idx field, but if I understood correctly
this is only required when using notification suppression - which
we don't use in the s390-ccw bios here)

Signed-off-by: Thomas Huth <thuth@redhat.com>
  pc-bios/s390-ccw/virtio-net.c | 1 +
  pc-bios/s390-ccw/virtio.c     | 1 +
  pc-bios/s390-ccw/virtio.h     | 2 ++
  3 files changed, 4 insertions(+)

diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
index 2fcb0a58c5..25598a7a97 100644
--- a/pc-bios/s390-ccw/virtio-net.c
+++ b/pc-bios/s390-ccw/virtio-net.c
@@ -127,6 +127,7 @@ int recv(int fd, void *buf, int maxlen, int flags)
/* Mark buffer as available to the host again */
      rxvq->avail->ring[rxvq->avail->idx % rxvq->num] = id;
+    virtio_mb();
      rxvq->avail->idx = rxvq->avail->idx + 1;
diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index ab49840db8..fb9687f9b3 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -154,6 +154,7 @@ void vring_send_buf(VRing *vr, void *p, int len, int flags)
/* Chains only have a single ID */
      if (!(flags & VRING_DESC_F_NEXT)) {
+        virtio_mb();

I think you need to also need barriers for changes to the buffers, as
the spec talks about "manipulating the descriptor table".

Which paragraph in the virtio spec are you refering to here? I can't find that part right now...

diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
index 19fceb6495..6ac65482a9 100644
--- a/pc-bios/s390-ccw/virtio.h
+++ b/pc-bios/s390-ccw/virtio.h
@@ -271,6 +271,8 @@ struct VirtioCmd {
  typedef struct VirtioCmd VirtioCmd;
+#define virtio_mb() asm volatile("bcr 14,0" : : : "memory")

The bios is built for z900, so you probably need a bcr15 here?

I thought about that, too, but for TCG, it currently should not matter since both, 14 and 15, end up with the same code in op_bc() in target/s390x/translate.c. And on a real host, we've never seen this problem to occur, so it should not matter there, too. But if you prefer (e.g. in case somebody tweaks the TCG implementation one day), I can also switch to bcr15 instead.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]