Let's make it a generic KVM hypercall, allowing other subfunctions to
be more independent of virtio.

This is a preparation for documenting a new hypercall.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 Documentation/virt/kvm/s390/s390-diag.rst | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/virt/kvm/s390/s390-diag.rst b/Documentation/virt/kvm/s390/s390-diag.rst
index ca85f030eb0b..d9b7c6cbc99e 100644
--- a/Documentation/virt/kvm/s390/s390-diag.rst
+++ b/Documentation/virt/kvm/s390/s390-diag.rst
@@ -35,15 +35,16 @@ DIAGNOSE function codes not specific to KVM, please refer to the
 documentation for the s390 hypervisors defining them.
 
 
-DIAGNOSE function code 'X'500' - KVM virtio functions
------------------------------------------------------
+DIAGNOSE function code 'X'500' - KVM functions
+----------------------------------------------
 
-If the function code specifies 0x500, various virtio-related functions
-are performed.
+If the function code specifies 0x500, various KVM-specific functions
+are performed, including virtio functions.
 
-General register 1 contains the virtio subfunction code. Supported
-virtio subfunctions depend on KVM's userspace. Generally, userspace
-provides either s390-virtio (subcodes 0-2) or virtio-ccw (subcode 3).
+General register 1 contains the subfunction code. Supported subfunctions
+depend on KVM's userspace. Regarding virtio subfunctions, generally
+userspace provides either s390-virtio (subcodes 0-2) or virtio-ccw
+(subcode 3).
 
 Upon completion of the DIAGNOSE instruction, general register 2 contains
 the function's return code, which is either a return code or a subcode
-- 
2.46.1

On Mon, Oct 14, 2024 at 04:46:14PM +0200, David Hildenbrand wrote:
> Let's make it a generic KVM hypercall, allowing other subfunctions to
> be more independent of virtio.
> 
> This is a preparation for documenting a new hypercall.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  Documentation/virt/kvm/s390/s390-diag.rst | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)

...

> -DIAGNOSE function code 'X'500' - KVM virtio functions
> ------------------------------------------------------
> +DIAGNOSE function code 'X'500' - KVM functions
> +----------------------------------------------
>  
> -If the function code specifies 0x500, various virtio-related functions
> -are performed.
> +If the function code specifies 0x500, various KVM-specific functions
> +are performed, including virtio functions.
>  
> -General register 1 contains the virtio subfunction code. Supported
> -virtio subfunctions depend on KVM's userspace. Generally, userspace
> -provides either s390-virtio (subcodes 0-2) or virtio-ccw (subcode 3).
> +General register 1 contains the subfunction code. Supported subfunctions
> +depend on KVM's userspace. Regarding virtio subfunctions, generally
> +userspace provides either s390-virtio (subcodes 0-2) or virtio-ccw
> +(subcode 3).

Reading this file leaves a number of questions open: how does one know
which subcodes are supported, and what happens if an unsupported
subcode is used?

I'm afraid there is no indication available and the only way to figure
out is to try and if it is unsupported the result is a specification
exception. Is that correct?

If so, it would be nice to document that too; but that is not
necessarily your problem.

I guess we won't see too many new diag 500 subcodes, or would it make
sense to implement some query subcode?

On 14.10.24 20:04, Heiko Carstens wrote:
> On Mon, Oct 14, 2024 at 04:46:14PM +0200, David Hildenbrand wrote:
>> Let's make it a generic KVM hypercall, allowing other subfunctions to
>> be more independent of virtio.
>>
>> This is a preparation for documenting a new hypercall.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   Documentation/virt/kvm/s390/s390-diag.rst | 15 ++++++++-------
>>   1 file changed, 8 insertions(+), 7 deletions(-)
> 
> ...
> 
>> -DIAGNOSE function code 'X'500' - KVM virtio functions
>> ------------------------------------------------------
>> +DIAGNOSE function code 'X'500' - KVM functions
>> +----------------------------------------------
>>   
>> -If the function code specifies 0x500, various virtio-related functions
>> -are performed.
>> +If the function code specifies 0x500, various KVM-specific functions
>> +are performed, including virtio functions.
>>   
>> -General register 1 contains the virtio subfunction code. Supported
>> -virtio subfunctions depend on KVM's userspace. Generally, userspace
>> -provides either s390-virtio (subcodes 0-2) or virtio-ccw (subcode 3).
>> +General register 1 contains the subfunction code. Supported subfunctions
>> +depend on KVM's userspace. Regarding virtio subfunctions, generally
>> +userspace provides either s390-virtio (subcodes 0-2) or virtio-ccw
>> +(subcode 3).
> 
> Reading this file leaves a number of questions open: how does one know
> which subcodes are supported, and what happens if an unsupported
> subcode is used?

Currently they have to be sensed
> 
> I'm afraid there is no indication available and the only way to figure
> out is to try and if it is unsupported the result is a specification
> exception. Is that correct?

Yes exactly.

> 
> If so, it would be nice to document that too; but that is not
> necessarily your problem.

I can squash:

diff --git a/Documentation/virt/kvm/s390/s390-diag.rst b/Documentation/virt/kvm/s390/s390-diag.rst
index d9b7c6cbc99e..48a326d41cc0 100644
--- a/Documentation/virt/kvm/s390/s390-diag.rst
+++ b/Documentation/virt/kvm/s390/s390-diag.rst
@@ -50,6 +50,9 @@ Upon completion of the DIAGNOSE instruction, general register 2 contains
  the function's return code, which is either a return code or a subcode
  specific value.
  
+If the specified subfunction is not supported, a SPECIFICATION exception
+will be triggered.
+
  Subcode 0 - s390-virtio notification and early console printk
      Handled by userspace.
  


> 
> I guess we won't see too many new diag 500 subcodes, or would it make
> sense to implement some query subcode?

In the context of STORAGE LIMIT, a "query" subfunction is not really beneficial:

it's either one invocation of "query", conditionally followed by one invocation of "STORAGE LIMIT"
vs. one invocation of "STORAGE LIMIT".

Once there might be a bunch of other subfunctions, a "query" might make more sense.

Thanks!

-- 
Cheers,

David / dhildenb

On Mon, Oct 14, 2024 at 09:35:27PM +0200, David Hildenbrand wrote:
> On 14.10.24 20:04, Heiko Carstens wrote:
> > On Mon, Oct 14, 2024 at 04:46:14PM +0200, David Hildenbrand wrote:
> > If so, it would be nice to document that too; but that is not
> > necessarily your problem.
> 
> I can squash:
> 
> diff --git a/Documentation/virt/kvm/s390/s390-diag.rst b/Documentation/virt/kvm/s390/s390-diag.rst
> index d9b7c6cbc99e..48a326d41cc0 100644
> --- a/Documentation/virt/kvm/s390/s390-diag.rst
> +++ b/Documentation/virt/kvm/s390/s390-diag.rst
> @@ -50,6 +50,9 @@ Upon completion of the DIAGNOSE instruction, general register 2 contains
>  the function's return code, which is either a return code or a subcode
>  specific value.
> +If the specified subfunction is not supported, a SPECIFICATION exception
> +will be triggered.
> +

Looks good. Thanks!

> > I guess we won't see too many new diag 500 subcodes, or would it make
> > sense to implement some query subcode?
> 
> In the context of STORAGE LIMIT, a "query" subfunction is not really beneficial:
> 
> it's either one invocation of "query", conditionally followed by one invocation of "STORAGE LIMIT"
> vs. one invocation of "STORAGE LIMIT".
> 
> Once there might be a bunch of other subfunctions, a "query" might make more sense.

"If only there would be a query subcode available, so that the program
check handling would not be necessary; but in particular my new subcode
is not worth adding it" :)

Anyway, I do not care too much.

On 15.10.24 10:12, Heiko Carstens wrote:
> On Mon, Oct 14, 2024 at 09:35:27PM +0200, David Hildenbrand wrote:
>> On 14.10.24 20:04, Heiko Carstens wrote:
>>> On Mon, Oct 14, 2024 at 04:46:14PM +0200, David Hildenbrand wrote:
>>> If so, it would be nice to document that too; but that is not
>>> necessarily your problem.
>>
>> I can squash:
>>
>> diff --git a/Documentation/virt/kvm/s390/s390-diag.rst b/Documentation/virt/kvm/s390/s390-diag.rst
>> index d9b7c6cbc99e..48a326d41cc0 100644
>> --- a/Documentation/virt/kvm/s390/s390-diag.rst
>> +++ b/Documentation/virt/kvm/s390/s390-diag.rst
>> @@ -50,6 +50,9 @@ Upon completion of the DIAGNOSE instruction, general register 2 contains
>>   the function's return code, which is either a return code or a subcode
>>   specific value.
>> +If the specified subfunction is not supported, a SPECIFICATION exception
>> +will be triggered.
>> +
> 
> Looks good. Thanks!
> 
>>> I guess we won't see too many new diag 500 subcodes, or would it make
>>> sense to implement some query subcode?
>>
>> In the context of STORAGE LIMIT, a "query" subfunction is not really beneficial:
>>
>> it's either one invocation of "query", conditionally followed by one invocation of "STORAGE LIMIT"
>> vs. one invocation of "STORAGE LIMIT".
>>
>> Once there might be a bunch of other subfunctions, a "query" might make more sense.
> 
> "If only there would be a query subcode available, so that the program
> check handling would not be necessary; but in particular my new subcode
> is not worth adding it" :)
> 
> Anyway, I do not care too much.
> 

Okay, I see your point: it would allow for removing the program check 
handling from the STORAGE LIMIT invocation.

... if only we wouldn't need the exact same program check handling for 
the new query subfunction :P

-- 
Cheers,

David / dhildenb

On Tue, Oct 15, 2024 at 10:16:20AM +0200, David Hildenbrand wrote:
> On 15.10.24 10:12, Heiko Carstens wrote:
> > On Mon, Oct 14, 2024 at 09:35:27PM +0200, David Hildenbrand wrote:
> > > On 14.10.24 20:04, Heiko Carstens wrote:
> > "If only there would be a query subcode available, so that the program
> > check handling would not be necessary; but in particular my new subcode
> > is not worth adding it" :)
> > 
> > Anyway, I do not care too much.
> > 
> 
> Okay, I see your point: it would allow for removing the program check
> handling from the STORAGE LIMIT invocation.
> 
> ... if only we wouldn't need the exact same program check handling for the
> new query subfunction :P

Yeah yeah, but I think you got that this might help in the future.

On 15.10.24 10:21, Heiko Carstens wrote:
> On Tue, Oct 15, 2024 at 10:16:20AM +0200, David Hildenbrand wrote:
>> On 15.10.24 10:12, Heiko Carstens wrote:
>>> On Mon, Oct 14, 2024 at 09:35:27PM +0200, David Hildenbrand wrote:
>>>> On 14.10.24 20:04, Heiko Carstens wrote:
>>> "If only there would be a query subcode available, so that the program
>>> check handling would not be necessary; but in particular my new subcode
>>> is not worth adding it" :)
>>>
>>> Anyway, I do not care too much.
>>>
>>
>> Okay, I see your point: it would allow for removing the program check
>> handling from the STORAGE LIMIT invocation.
>>
>> ... if only we wouldn't need the exact same program check handling for the
>> new query subfunction :P
> 
> Yeah yeah, but I think you got that this might help in the future.

Right. Adding it later also doesn't quite help to get rid of the checks 
here, because some user space might implement STORAGE LIMIT without QUERY.

So strategically, the right approach would indeed be to add QUERY now.

Thoughts from the KVM folks? Unfortunately subfunction 0 is taken, which 
is usually QUERY IIRC.

-- 
Cheers,

David / dhildenb

On Tue, Oct 15, 2024 at 10:32:43AM +0200, David Hildenbrand wrote:
> On 15.10.24 10:21, Heiko Carstens wrote:
> > On Tue, Oct 15, 2024 at 10:16:20AM +0200, David Hildenbrand wrote:
> > > On 15.10.24 10:12, Heiko Carstens wrote:
> > > > On Mon, Oct 14, 2024 at 09:35:27PM +0200, David Hildenbrand wrote:
> > > > > On 14.10.24 20:04, Heiko Carstens wrote:
> > > > "If only there would be a query subcode available, so that the program
> > > > check handling would not be necessary; but in particular my new subcode
> > > > is not worth adding it" :)
> > > > 
> > > > Anyway, I do not care too much.
> > > > 
> > > 
> > > Okay, I see your point: it would allow for removing the program check
> > > handling from the STORAGE LIMIT invocation.
> > > 
> > > ... if only we wouldn't need the exact same program check handling for the
> > > new query subfunction :P
> > 
> > Yeah yeah, but I think you got that this might help in the future.
> 
> Right. Adding it later also doesn't quite help to get rid of the checks
> here, because some user space might implement STORAGE LIMIT without QUERY.

This would only help if the diag500 documentation would state that
implementation of the QUERY subcode is mandatory. That is: for every
new subcode larger than the QUERY subcode QUERY must also exist.

That way we only would have to implement program check handling once,
if a program check happens on QUERY none of the newer subcodes is
available, otherwise the return value would indicate that.

Otherwise this whole excercise would be pointless.

On 15.10.24 10:46, Heiko Carstens wrote:
> On Tue, Oct 15, 2024 at 10:32:43AM +0200, David Hildenbrand wrote:
>> On 15.10.24 10:21, Heiko Carstens wrote:
>>> On Tue, Oct 15, 2024 at 10:16:20AM +0200, David Hildenbrand wrote:
>>>> On 15.10.24 10:12, Heiko Carstens wrote:
>>>>> On Mon, Oct 14, 2024 at 09:35:27PM +0200, David Hildenbrand wrote:
>>>>>> On 14.10.24 20:04, Heiko Carstens wrote:
>>>>> "If only there would be a query subcode available, so that the program
>>>>> check handling would not be necessary; but in particular my new subcode
>>>>> is not worth adding it" :)
>>>>>
>>>>> Anyway, I do not care too much.
>>>>>
>>>>
>>>> Okay, I see your point: it would allow for removing the program check
>>>> handling from the STORAGE LIMIT invocation.
>>>>
>>>> ... if only we wouldn't need the exact same program check handling for the
>>>> new query subfunction :P
>>>
>>> Yeah yeah, but I think you got that this might help in the future.
>>
>> Right. Adding it later also doesn't quite help to get rid of the checks
>> here, because some user space might implement STORAGE LIMIT without QUERY.
> 
> This would only help if the diag500 documentation would state that
> implementation of the QUERY subcode is mandatory. That is: for every
> new subcode larger than the QUERY subcode QUERY must also exist.
> 
> That way we only would have to implement program check handling once,
> if a program check happens on QUERY none of the newer subcodes is
> available, otherwise the return value would indicate that.
> 
> Otherwise this whole excercise would be pointless.

Yes, that would be the idea.

-- 
Cheers,

David / dhildenb