On Tue, 16 Feb 2021 12:00:56 +0100
Thomas Huth <th...@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 <th...@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;
> vring_notify(rxvq);
>
> 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".
> vr->avail->idx++;
> }
> }
> 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?
> +
> bool vring_notify(VRing *vr);
> int drain_irqs(SubChannelId schid);
> void vring_send_buf(VRing *vr, void *p, int len, int flags);
