Re: getting the console output for s390 cdrom-test?

[Peter Maydell]

On Mon, 8 Feb 2021 at 12:08, Peter Maydell <peter.maydell@linaro.org> wrote:
> Yes, virtio_scsi_parse_req() returns ENOTSUP because it
> fails the "if (out_size && in_size)" test.
>
> I am becoming somewhat suspicious that the s390-ccw BIOS's
> implementation of virtio is not putting in sufficient barriers,
> and so if you get unlucky then the QEMU thread sees an inconsistent
> view of the in-memory virtio data structures.

As a test of this theory, I tried adding some barrier insns
(with the definition of the barrier insn borrowed from s390x Linux):

diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index ab49840db85..0af901264b6 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -17,6 +17,12 @@
 #include "helper.h"
 #include "s390-time.h"

+#define membarrier() do { asm volatile("bcr 15,0\n" :: : "memory"); } while (0)
+
+#define __ASM_BARRIER "bcr 15,0\n"
+
+
+
 #define VRING_WAIT_REPLY_TIMEOUT 30

 static VRing block[VIRTIO_MAX_VQS];
@@ -154,12 +160,15 @@ void vring_send_buf(VRing *vr, void *p, int len,
int flags)

     /* Chains only have a single ID */
     if (!(flags & VRING_DESC_F_NEXT)) {
+        membarrier();
         vr->avail->idx++;
+        membarrier();
     }
 }

 int vr_poll(VRing *vr)
 {
+    membarrier();
     if (vr->used->idx == vr->used_idx) {
         vring_notify(vr);
         yield();
@@ -169,7 +178,9 @@ int vr_poll(VRing *vr)
     vr->used_idx = vr->used->idx;
     vr->next_idx = 0;
     vr->desc[0].len = 0;
+    membarrier();
     vr->desc[0].flags = 0;
+    membarrier();
     return 1; /* vr has been updated */
 }

This change significantly reduces the frequency with which I see
the hang; but it doesn't get rid of it altogether. Also I couldn't
really figure out from the virtio spec exactly where barriers
were required: I think I would need to read the whole thing in
more detail rather than trying to fish out the information by
just reading small pieces of it. But some of the ordering of
operations the spec describes doesn't match how the s390-ccw
BIOS code is doing it at all (eg the spec says that when feeding
a batch of descriptors to the device the driver isn't supposed to
update the flags on the first descriptor until it's finished
writing all of the descriptors, but the code doesn't seem to
try to do that). So I think the code could use an overhaul from
somebody with a better understanding of virtio than me...

thanks
-- PMM