Re: [PATCH v1 1/2] s390x: KVM: accept STSI for CPU topology information

From: Pierre Morel
Date: Thu Jul 15 2021 - 07:37:14 EST




On 7/15/21 12:19 PM, David Hildenbrand wrote:
On 15.07.21 12:16, Cornelia Huck wrote:
On Thu, Jul 15 2021, David Hildenbrand <david@xxxxxxxxxx> wrote:

On 15.07.21 11:30, Cornelia Huck wrote:
On Thu, Jul 15 2021, David Hildenbrand <david@xxxxxxxxxx> wrote:

On 14.07.21 17:25, Pierre Morel wrote:
STSI(15.1.x) gives information on the CPU configuration topology.
Let's accept the interception of STSI with the function code 15 and
let the userland part of the hypervisor handle it.

Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
---
    arch/s390/kvm/priv.c | 11 ++++++++++-
    1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index 9928f785c677..4ab5f8b7780e 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -856,7 +856,7 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
        if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
            return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
-    if (fc > 3) {
+    if (fc > 3 && fc != 15) {
            kvm_s390_set_psw_cc(vcpu, 3);
            return 0;
        }
@@ -893,6 +893,15 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
                goto out_no_data;
            handle_stsi_3_2_2(vcpu, (void *) mem);
            break;
+    case 15:
+        if (sel1 != 1 || sel2 < 2 || sel2 > 6)
+            goto out_no_data;
+        if (vcpu->kvm->arch.user_stsi) {
+            insert_stsi_usr_data(vcpu, operand2, ar, fc, sel1, sel2);
+            return -EREMOTE;

This bypasses the trace event further down.

+        }
+        kvm_s390_set_psw_cc(vcpu, 3);
+        return 0;
        }
        if (kvm_s390_pv_cpu_is_protected(vcpu)) {
            memcpy((void *)sida_origin(vcpu->arch.sie_block), (void *)mem,
3. User space awareness

How can user space identify that we actually forward these intercepts?
How can it enable them? The old KVM_CAP_S390_USER_STSI capability
is not sufficient.

Why do you think that it is not sufficient? USER_STSI basically says
"you may get an exit that tells you about a buffer to fill in some more
data for a stsi command, and we also tell you which call". If userspace
does not know what to add for a certain call, it is free to just do
nothing, and if it does not get some calls it would support, that should
not be a problem, either?

If you migrate your VM from machine a to machine b, from kernel a to
kernel b, and kernel b does not trigger exits to user space for fc=15,
how could QEMU spot and catch the different capabilities to make sure
the guest can continue using the feature?

Wouldn't that imply that the USER_STSI feature, in the function-agnostic
way it is documented, was broken from the start?

Likely. We should have forwarded everything to user space most probably and not try being smart in the kernel.


Hm. Maybe we need some kind of facility where userspace can query the
kernel and gets a list of the stsi subcodes it may get exits for, and
possibly fail to start the migration. Having a new capability to be
enabled for every new subcode feels like overkill. I don't think we can
pass a payload ("enable these subfunctions") to a cap.

Maybe a new capability that forwards everything to user space when enabled, and lets user space handle errors.

Or a specialized one only to unlock fc=15.


I think the lack of a good comment in patch 2/2 is the problem.
Facility 11 belong to CPU model and enables both the STSI 15 and the PFT instructions

Sorry about that.

--
Pierre Morel
IBM Lab Boeblingen