To provide Topology information to the guest through the STSI
instruction, we need to forward STSI with Function Code 15 to
QEMU which will take care to provide the right information to
the guest.
To let the guest use the PTF instruction and ask if a topology
change occured we add a new KVM capability to enable the topology
facility.
Pierre Morel (2):
s390x: KVM: accept STSI for CPU topology information
KVM: s390: Topology expose TOPOLOGY facility
arch/s390/kvm/priv.c | 11 ++++++++++-
arch/s390/tools/gen_facilities.c | 1 +
2 files changed, 11 insertions(+), 1 deletion(-)
--
2.25.1
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 <[email protected]>
---
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;
+ }
+ 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,
--
2.25.1
We add the KVM extension KVM_CAP_S390_CPU_TOPOLOGY, this will
allow the userland hypervisor to handle the interception of the
PTF (Perform topology Function) instruction.
Signed-off-by: Pierre Morel <[email protected]>
---
arch/s390/tools/gen_facilities.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c
index 606324e56e4e..2c260eb22bae 100644
--- a/arch/s390/tools/gen_facilities.c
+++ b/arch/s390/tools/gen_facilities.c
@@ -112,6 +112,7 @@ static struct facility_def facility_defs[] = {
.name = "FACILITIES_KVM_CPUMODEL",
.bits = (int[]){
+ 11, /* configuration topology facility */
12, /* AP Query Configuration Information */
15, /* AP Facilities Test */
156, /* etoken facility */
--
2.25.1
On 14.07.21 17:25, Pierre Morel wrote:
> We add the KVM extension KVM_CAP_S390_CPU_TOPOLOGY, this will
> allow the userland hypervisor to handle the interception of the
> PTF (Perform topology Function) instruction.
Ehm, no you don't add any new capability. Or my eyes are too tired to
spot it :)
>
> Signed-off-by: Pierre Morel <[email protected]>
> ---
> arch/s390/tools/gen_facilities.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c
> index 606324e56e4e..2c260eb22bae 100644
> --- a/arch/s390/tools/gen_facilities.c
> +++ b/arch/s390/tools/gen_facilities.c
> @@ -112,6 +112,7 @@ static struct facility_def facility_defs[] = {
>
> .name = "FACILITIES_KVM_CPUMODEL",
> .bits = (int[]){
> + 11, /* configuration topology facility */
> 12, /* AP Query Configuration Information */
> 15, /* AP Facilities Test */
> 156, /* etoken facility */
>
--
Thanks,
David / dhildenb
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 <[email protected]>
> ---
> 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;
> + }
> + 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);
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.
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.
I do wonder if we want KVM_CAP_S390_USER_STSI_15 or sth like that to change
the behavior once enabled by user space.
4. Without vcpu->kvm->arch.user_stsi, we indicate cc=0 to our guest,
also for fc 1,2,3. Is that actually what we want? (or do we simply not care
because the guest is not supposed to use stsi?)
--
Thanks,
David / dhildenb
On Thu, Jul 15 2021, David Hildenbrand <[email protected]> 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 <[email protected]>
>> ---
>> 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?
>
> I do wonder if we want KVM_CAP_S390_USER_STSI_15 or sth like that to change
> the behavior once enabled by user space.
>
>
> 4. Without vcpu->kvm->arch.user_stsi, we indicate cc=0 to our guest,
> also for fc 1,2,3. Is that actually what we want? (or do we simply not care
> because the guest is not supposed to use stsi?)
If returning an empty buffer is ok, it should not be a problem, I
guess. (I have not looked yet at the actual definitions.)
On 15.07.21 12:16, Cornelia Huck wrote:
> On Thu, Jul 15 2021, David Hildenbrand <[email protected]> wrote:
>
>> On 15.07.21 11:30, Cornelia Huck wrote:
>>> On Thu, Jul 15 2021, David Hildenbrand <[email protected]> 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 <[email protected]>
>>>>> ---
>>>>> 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.
--
Thanks,
David / dhildenb
On 7/15/21 11:30 AM, Cornelia Huck wrote:
> On Thu, Jul 15 2021, David Hildenbrand <[email protected]> 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 <[email protected]>
>>> ---
>>> 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.
>
Right, I can add a trace.
Note that we had no trace in the past.
>>> + }
>>> + 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.
>
OK for me, in that case we can also suppress the check for FC=15 and let
that to user space as it was suggested in a previous comment
>>
>>
>> 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.
OK too
>
>>
>> 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?
>
>>
>> I do wonder if we want KVM_CAP_S390_USER_STSI_15 or sth like that to change
>> the behavior once enabled by user space.
>>
>>
>> 4. Without vcpu->kvm->arch.user_stsi, we indicate cc=0 to our guest,
>> also for fc 1,2,3. Is that actually what we want? (or do we simply not care
>> because the guest is not supposed to use stsi?)
>
> If returning an empty buffer is ok, it should not be a problem, I
> guess. (I have not looked yet at the actual definitions.)
>
When user_stsi is 0 for fc 1,2,3 the buffer is filled in the kernel, for
15 the kernel can not do this.
--
Pierre Morel
IBM Lab Boeblingen
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 <[email protected]> wrote:
>>
>>> On 15.07.21 11:30, Cornelia Huck wrote:
>>>> On Thu, Jul 15 2021, David Hildenbrand <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>> 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
On 15.07.21 11:30, Cornelia Huck wrote:
> On Thu, Jul 15 2021, David Hildenbrand <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
On 7/15/21 10:52 AM, David Hildenbrand wrote:
> On 14.07.21 17:25, Pierre Morel wrote:
>> We add the KVM extension KVM_CAP_S390_CPU_TOPOLOGY, this will
>> allow the userland hypervisor to handle the interception of the
>> PTF (Perform topology Function) instruction.
>
> Ehm, no you don't add any new capability. Or my eyes are too tired to
> spot it :)
hum, yes, sorry, seems I kept my old commit message as I let fall the
capability after internal reviews.
>
>>
>> Signed-off-by: Pierre Morel <[email protected]>
>> ---
>> arch/s390/tools/gen_facilities.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/s390/tools/gen_facilities.c
>> b/arch/s390/tools/gen_facilities.c
>> index 606324e56e4e..2c260eb22bae 100644
>> --- a/arch/s390/tools/gen_facilities.c
>> +++ b/arch/s390/tools/gen_facilities.c
>> @@ -112,6 +112,7 @@ static struct facility_def facility_defs[] = {
>> .name = "FACILITIES_KVM_CPUMODEL",
>> .bits = (int[]){
>> + 11, /* configuration topology facility */
>> 12, /* AP Query Configuration Information */
>> 15, /* AP Facilities Test */
>> 156, /* etoken facility */
>>
>
>
--
Pierre Morel
IBM Lab Boeblingen
On Thu, Jul 15 2021, David Hildenbrand <[email protected]> wrote:
> On 15.07.21 11:30, Cornelia Huck wrote:
>> On Thu, Jul 15 2021, David Hildenbrand <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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?
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.
Or can we tie a subcode to another feature, like the mystery cap that
was mentioned in the description, but does not seem to appear in the
code?
On 7/15/21 10:51 AM, David Hildenbrand 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 <[email protected]>
>> ---
>> 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;
>> + }
>> + 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?
It is only needed for fc 0, I do not see any reference to register 0
being set on output for other function code.
>
> 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);
>
>
> 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?
Yes
>
> 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.
We can, here it is only tested about values that are always false and
not depending on MNEST.
sel 1 is always 1
sel 2 can only be valid between 2 and 6
>
> How do we identify availability of maximum-MNest facility?
>
With SCLP.
Here is no test on MNEST
>
> 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.
>
> I do wonder if we want KVM_CAP_S390_USER_STSI_15 or sth like that to change
> the behavior once enabled by user space.
>
Userland can check the configuration toplogy facility to see if the
STSI-15 is available.
Sorry about the old commit message in the next patch it does not make
things clear.
Configuration topology facility, 11, enables function code 15 in the
STSI instruction.
>
> 4. Without vcpu->kvm->arch.user_stsi, we indicate cc=0 to our guest,
> also for fc 1,2,3. Is that actually what we want? (or do we simply not care
> because the guest is not supposed to use stsi?)
>
Without the user_stsi, the information is gathered in the kernel.
For what can be gathered.
Patch e44fc8c9dab215ac0e from Ekaterina Tumanova introduced post
handlers for STSI in user space.
Only what need to be done in userspace is checking the user_stsi.
--
Pierre Morel
IBM Lab Boeblingen