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

[David Hildenbrand <david@redhat.com>]

On 27.09.24 20:05, Halil Pasic wrote:
> 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.

I assume you skimmed the QEMU patches, including the one that suggest 
changing (or rather extending) that.

> 
> 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.
> 
 > If we decide to go with a DIAG, I would like to see> 
Documentation/virt/kvm/s390/s390-diag.rst
> updated accordingly.

I'll document wherever people fancy. I'll even document in multiple 
locations :)

> 
> Also if decide to go with DIAG 500, maybe leaving some space
> between the codes more closely tied to virtio and between
> the ones less closely tied to virito (for the unlikely case
> that we end up wanting another DIAG 500 subcode for virtio)
> might make sense. I.e e could make DIAG500_STORAGE_LIMIT
> 8 or 16 instead of 4. Again nothing important, just an idea.

I don't see a reason to do that, but in the end I don't care as long as 
people let me know what to do instead.

Thanks for taking a look!

-- 
Cheers,

David / dhildenb