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

[Richard Henderson]

On 2/16/21 8:15 AM, Thomas Huth wrote:
> 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)

This is patching the wrong thing, IMO.  You're adding barriers to the guest
that I think ought not be architecturally required -- memory accesses on s390x
are strongly ordered, with or without diag/svc/whatever.

With

diff --git a/tcg/aarch64/tcg-target.c.inc b/tcg/aarch64/tcg-target.c.inc
index 1376cdc404..3c5f38be62 100644
--- a/tcg/aarch64/tcg-target.c.inc
+++ b/tcg/aarch64/tcg-target.c.inc
@@ -1622,6 +1622,8 @@ static void tcg_out_tlb_read
     TCGType mask_type;
     uint64_t compare_mask;

+    tcg_out_mb(s, TCG_MO_ALL);
+
     mask_type = (TARGET_PAGE_BITS + CPU_TLB_DYN_MAX_BITS > 32
                  ? TCG_TYPE_I64 : TCG_TYPE_I32);

which is a gigantic hammer, adding a host barrier before every qemu guest
access, I can no longer provoke a failure (previously visible 1 in 4, now no
failures in 100).

With that as a data point for success, I'm going to try to use host
load-acquire / store-release instructions, and then apply TCG_GUEST_DEFAULT_MO
and see if I can find something that works reasonably.


r~