| Message ID | 20241014144622.876731-3-david@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-mem: s390 support | expand |
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!
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
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.
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.
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
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(-)