On Tue, 9 Feb 2021 14:58:53 +0000
Peter Maydell <peter.mayd...@linaro.org> wrote:
> On Mon, 8 Feb 2021 at 12:08, Peter Maydell <peter.mayd...@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.
The Linux virtio-ccw code uses 'weak barriers', i.e. the heavy bcr15
should not be needed. We might well miss other (lightweight) barriers
in other parts of the code part, though.
> 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...
Yeah, the bios virtio code could probably use some love.
I'm wondering how much memory ordering on the host platform influences
things. I doubt many people try to run an s390x guest on an aarch64
host...
