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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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,


1. Setting GPRS to 0

I was wondering why we have the "vcpu->run->s.regs.gprs[0] = 0;"
for existing fc 1,2,3 in case we set cc=0.

Looking at the doc, all I find is:

"CC 0: Requested configuration-level number placed in
general register 0 or requested SYSIB informa-
tion stored"

But I don't find where it states that we are supposed to set
general register 0 to 0. Wouldn't we also have to do it for
fc=15 or for none?

If fc 1,2,3 and 15 are to be handled equally, I suggest the following:

diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index 9928f785c677..6eb86fa58b0b 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -893,17 +893,23 @@ 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;
+               break;
          }
-       if (kvm_s390_pv_cpu_is_protected(vcpu)) {
-               memcpy((void *)sida_origin(vcpu->arch.sie_block), (void *)mem,
-                      PAGE_SIZE);
-               rc = 0;
-       } else {
-               rc = write_guest(vcpu, operand2, ar, (void *)mem, PAGE_SIZE);
-       }
-       if (rc) {
-               rc = kvm_s390_inject_prog_cond(vcpu, rc);
-               goto out;
+       if (mem) {
+               if (kvm_s390_pv_cpu_is_protected(vcpu)) {
+                       memcpy((void *)sida_origin(vcpu->arch.sie_block),
+                              (void *)mem, PAGE_SIZE);
+               } else {
+                       rc = write_guest(vcpu, operand2, ar, (void *)mem,
+                                        PAGE_SIZE);
+                       if (rc) {
+                               rc = kvm_s390_inject_prog_cond(vcpu, rc);
+                               goto out;
+                       }
+               }
          }
          if (vcpu->kvm->arch.user_stsi) {
                  insert_stsi_usr_data(vcpu, operand2, ar, fc, sel1, sel2);

Something like that sounds good, the code is getting a bit convoluted.



2. maximum-MNest facility

"
1. If the maximum-MNest facility is installed and
selector 2 exceeds the nonzero model-depen-
dent maximum-selector-2 value."

2. If the maximum-MNest facility is not installed and
selector 2 is not specified as two.
"

We will we be handling the presence/absence of the maximum-MNest facility
(for our guest?) in QEMU, corect?

I do wonder if we should just let any fc=15 go to user space let the whole
sel1 / sel2 checking be handled there. I don't think it's a fast path after all.
But no strong opinion.

If that makes handling easier, I think it would be a good idea.


How do we identify availability of maximum-MNest facility?


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?


--
Thanks,

David / dhildenb




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux