Messages in this thread | | | Subject | Re: [PATCH v3 1/3] s390x: KVM: accept STSI for CPU topology information | From | Pierre Morel <> | Date | Tue, 7 Sep 2021 12:11:24 +0200 |
| |
On 9/6/21 8:14 PM, David Hildenbrand wrote: > On 01.09.21 11:43, Pierre Morel wrote: >> >> >> On 8/31/21 3:59 PM, David Hildenbrand wrote: >>> On 03.08.21 10:26, 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 when userland >>>> support the CPU Topology facility. >>>> >>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>> --- >>>> arch/s390/kvm/priv.c | 7 ++++++- >>>> 1 file changed, 6 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c >>>> index 9928f785c677..8581b6881212 100644 >>>> --- a/arch/s390/kvm/priv.c >>>> +++ b/arch/s390/kvm/priv.c >>>> @@ -856,7 +856,8 @@ 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) || >>>> + (fc == 15 && !test_kvm_facility(vcpu->kvm, 11))) { >>>> kvm_s390_set_psw_cc(vcpu, 3); >>>> return 0; >>>> } >>>> @@ -893,6 +894,10 @@ static int handle_stsi(struct kvm_vcpu *vcpu) >>>> goto out_no_data; >>>> handle_stsi_3_2_2(vcpu, (void *) mem); >>>> break; >>>> + case 15: >>>> + trace_kvm_s390_handle_stsi(vcpu, fc, sel1, sel2, operand2); >>>> + insert_stsi_usr_data(vcpu, operand2, ar, fc, sel1, sel2); >>>> + return -EREMOTE; >>>> } >>>> if (kvm_s390_pv_cpu_is_protected(vcpu)) { >>>> memcpy((void *)sida_origin(vcpu->arch.sie_block), (void >>>> *)mem, >>>> >>> >>> Sorry, I'm a bit rusty on s390x kvm facility handling. >>> >>> >>> For test_kvm_facility() to succeed, the facility has to be in both: >>> >>> a) fac_mask: actually available on the HW and supported by KVM >>> (kvm_s390_fac_base via FACILITIES_KVM, kvm_s390_fac_ext via >>> FACILITIES_KVM_CPUMODEL) >>> >>> b) fac_list: enabled for a VM >>> >>> AFAIU, facility 11 is neither in FACILITIES_KVM nor >>> FACILITIES_KVM_CPUMODEL, and I remember it's a hypervisor-managed bit. >>> >>> So unless we unlock facility 11 in FACILITIES_KVM_CPUMODEL, will >>> test_kvm_facility(vcpu->kvm, 11) ever successfully trigger here? >>> >>> >>> I'm pretty sure I am messing something up :) >>> >> >> I think it is the same remark that Christian did as wanted me to use the >> arch/s390/tools/gen_facilities.c to activate the facility. >> >> The point is that CONFIGURATION_TOPOLOGY, STFL, 11, is already defined >> inside QEMU since full_GEN10_GA1, so the test_kvm_facility() will >> succeed with the next patch setting the facility 11 in the mask when >> getting the KVM_CAP_S390_CPU_TOPOLOGY from userland. > > Ok, I see ... > > QEMU knows the facility and as soon as we present it to QEMU, QEMU will > want to automatically enable it in the "host" model. > > However, we'd like QEMU to join in and handle some part of it. > > So indeed, handling it like KVM_CAP_S390_VECTOR_REGISTERS or > KVM_CAP_S390_RI looks like a reasonable approach. > >> >> But if we activate it in KVM via any of the FACILITIES_KVM_xxx in the >> gen_facilities.c we will activate it for the guest what ever userland >> hypervizor we have, including old QEMU which will generate an exception. >> >> >> In this circumstances we have the choice between: >> >> - use FACILITY_KVM and handle everything in kernel >> - use FACILITY_KVM and use an extra CAPABILITY to handle part in kernel >> to avoid guest crash and part in userland > > This sounds quite nice to me. Implement minimal kernel support and > indicate the facility via stfl to user space. > > In addition, add a new capability that intercepts to user space instead. > > > ... but I can understand that it might not be worth it.
yes, since we need a CAPABILITY anyway I find it makes things more complicated. > > > This patch as it stands doesn't make any sense on its own. Either > document how it's supposed to work and why it is currently dead code, or > simply squash into the next patch (preferred IMHO). >
Yes, you are right, I will squash it with the next patch.
Thanks, Pierre
-- Pierre Morel IBM Lab Boeblingen
| |