Re: [PATCH v1 07/14] s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT

[David Hildenbrand <david@redhat.com>]

On 30.09.24 13:11, Christian Borntraeger wrote:
> Am 27.09.24 um 20:05 schrieb Halil Pasic:
>> On Thu, 12 Sep 2024 10:19:00 +0200
>> Thomas Huth <thuth@redhat.com> wrote:
>>
>>>> diff --git a/hw/s390x/s390-hypercall.h b/hw/s390x/s390-hypercall.h
>>>> index b7ac29f444..f0ca62bcbb 100644
>>>> --- a/hw/s390x/s390-hypercall.h
>>>> +++ b/hw/s390x/s390-hypercall.h
>>>> @@ -18,6 +18,7 @@
>>>>     #define DIAG500_VIRTIO_RESET            1 /* legacy */
>>>>     #define DIAG500_VIRTIO_SET_STATUS       2 /* legacy */
>>>>     #define DIAG500_VIRTIO_CCW_NOTIFY       3 /* KVM_S390_VIRTIO_CCW_NOTIFY */
>>>> +#define DIAG500_STORAGE_LIMIT           4
>>>>     
>>>>     int handle_diag_500(CPUS390XState *env);
>>>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>
>>> Sounds very reasonable to me - but it would be good to get an
>>> Ack/Reviewed-by from IBM folks here (in case they prefer a different
>>> interface)... hope they'll join the discussion!
>>>
>>>     Thomas
>>
>> According to Documentation/virt/kvm/s390/s390-diag.rst in the
>> linux source tree DIAG 500 is for kvm virtio funcions.
>>
>> Based on the commit message this storagelimit DIAG is rather loosely
>> tied to virtio if at all, so from that perspective DIAG may not be a
>> perfect fit. OTOH I don't see a better fit either. I would prefer to
>> have Christian's opinion on this. I have no strong opinion myself.
> 
> Some remarks:
> 500 with a new subcode would work, it is marked as KVM hypervisor call
> in our docs. 501 was used in the past for software breakpoints.

Right, we use it in the absence of KVM_CAP_S390_USER_INSTR0.

> So we could use 502 as the next free one (This is reserved for KVM).
> We do have kvm_stat counters for 500, not sure if people debugging virtio
> will care.

It would be one additional trigger during system boot, so likely not 
really an issue. We could always add new stats for selected subcodes 
(i.e, KVM_S390_VIRTIO_CCW_NOTIFY).

> The only important question for me is, what code is generated by gcc for
> the switch statement and will any variant slow down the virtio doorbell.
> diag.c has
>           if (!vcpu->kvm->arch.css_support ||
>               (vcpu->run->s.regs.gprs[1] != KVM_S390_VIRTIO_CCW_NOTIFY))
>                   return -EOPNOTSUPP;
> 
> So 500+4 should probably not cause any harm apart from branch prediction
> going wrong the first 2 or 3 notifies.

Right, it's very unlikely to be noticeable at all.

Thanks for the feedback!

-- 
Cheers,

David / dhildenb