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

[Cornelia Huck]

On Tue, 16 Feb 2021 15:21:45 +0100
Thomas Huth <thuth@redhat.com> wrote:

> 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;
> >>       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".  
> 
> Which paragraph in the virtio spec are you refering to here? I can't find 
> that part right now...

Step 4 in "2.7.13 Supplying Buffers to The Device":

"The driver performs a suitable memory barrier to ensure the device
sees the updated descriptor table and available ring before the next
step."

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

OK, if they are both implemented with the same code, it should not
really matter. We don't run on any hardware that doesn't support bcr14
anyway.