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

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 9 Feb 2021 14:58:53 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

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

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