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

[Thomas Huth <thuth@redhat.com>]

On 16/02/2021 15.40, Halil Pasic 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)
> 
> I suggest to the barrier after incrementing the idx field for two
> reasons. First: If the device were to see the notification, but
> not see the incremented idx field, it would effectively loose
> initiative. That is pretty straight forward, because the
> notification just says 'check out that queue', and if we don't
> see the incremented index, miss the buffer that was made available
> by incrementing idx.

I was just about to reply that this is certainly not necessary, since
the DIAGNOSE instruction that we use for the notification hypercall
should be serializing anyway ... but after looking at the PoP, it
actually is not marked as a serializing instruction! (while e.g.
SVC - supervisor call - is explicitly marked as serializing)

So maybe that's worth a try: Peter, could you please apply this patch
on top an see whether it makes a difference?

diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -54,6 +54,7 @@ static long kvm_hypercall(unsigned long nr, unsigned long param1,
      register ulong r_param3 asm("4") = param3;
      register long retval asm("2");
  
+    virtio_mb();
      asm volatile ("diag 2,4,0x500"
                    : "=d" (retval)
                    : "d" (r_nr), "0" (r_param1), "r"(r_param2), "d"(r_param3)

  Thanks,
   Thomas