2021-07-22 17:05:33

by Pierre Morel

[permalink] [raw]
Subject: [PATCH v2 0/2] s390x: KVM: CPU Topology

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
s390:kvm: Topology expose TOPOLOGY facility

arch/s390/kvm/kvm-s390.c | 1 +
arch/s390/kvm/priv.c | 7 ++++++-
include/uapi/linux/kvm.h | 1 +
3 files changed, 8 insertions(+), 1 deletion(-)

--
2.25.1

Changelog:

- Add a KVM capability to let QEMU know we support PTF and STSI 15
(David)

- check KVM facility 11 before accepting STSI fc 15
(David)

- handle all we can in userland
(David)

- add tracing to STSI fc 15
(Connie)


2021-07-22 17:05:49

by Pierre Morel

[permalink] [raw]
Subject: [PATCH v2 2/2] s390:kvm: Topology expose TOPOLOGY facility

We add a KVM extension KVM_CAP_S390_CPU_TOPOLOGY to tell the
userland hypervisor it is safe to activate the CPU Topology facility.

Signed-off-by: Pierre Morel <[email protected]>
---
arch/s390/kvm/kvm-s390.c | 1 +
include/uapi/linux/kvm.h | 1 +
2 files changed, 2 insertions(+)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index b655a7d82bf0..8c695ee79612 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -568,6 +568,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
case KVM_CAP_S390_VCPU_RESETS:
case KVM_CAP_SET_GUEST_DEBUG:
case KVM_CAP_S390_DIAG318:
+ case KVM_CAP_S390_CPU_TOPOLOGY:
r = 1;
break;
case KVM_CAP_SET_GUEST_DEBUG2:
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index d9e4aabcb31a..081ce0cd44b9 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1112,6 +1112,7 @@ struct kvm_ppc_resize_hpt {
#define KVM_CAP_BINARY_STATS_FD 203
#define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
#define KVM_CAP_ARM_MTE 205
+#define KVM_CAP_S390_CPU_TOPOLOGY 206

#ifdef KVM_CAP_IRQ_ROUTING

--
2.25.1

2021-07-23 08:17:26

by Christian Borntraeger

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] s390:kvm: Topology expose TOPOLOGY facility



On 22.07.21 19:02, Pierre Morel wrote:
> We add a KVM extension KVM_CAP_S390_CPU_TOPOLOGY to tell the
> userland hypervisor it is safe to activate the CPU Topology facility.

I think the old variant of using the CPU model was actually better.
It was just the patch description that was wrong.

> Signed-off-by: Pierre Morel <[email protected]>
> ---
> arch/s390/kvm/kvm-s390.c | 1 +
> include/uapi/linux/kvm.h | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index b655a7d82bf0..8c695ee79612 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -568,6 +568,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> case KVM_CAP_S390_VCPU_RESETS:
> case KVM_CAP_SET_GUEST_DEBUG:
> case KVM_CAP_S390_DIAG318:
> + case KVM_CAP_S390_CPU_TOPOLOGY:
> r = 1;
> break;
> case KVM_CAP_SET_GUEST_DEBUG2:
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index d9e4aabcb31a..081ce0cd44b9 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1112,6 +1112,7 @@ struct kvm_ppc_resize_hpt {
> #define KVM_CAP_BINARY_STATS_FD 203
> #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
> #define KVM_CAP_ARM_MTE 205
> +#define KVM_CAP_S390_CPU_TOPOLOGY 206
>
> #ifdef KVM_CAP_IRQ_ROUTING
>
>

2021-07-23 09:00:36

by Cornelia Huck

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] s390:kvm: Topology expose TOPOLOGY facility

On Fri, Jul 23 2021, Christian Borntraeger <[email protected]> wrote:

> On 22.07.21 19:02, Pierre Morel wrote:
>> We add a KVM extension KVM_CAP_S390_CPU_TOPOLOGY to tell the
>> userland hypervisor it is safe to activate the CPU Topology facility.
>
> I think the old variant of using the CPU model was actually better.
> It was just the patch description that was wrong.

I thought we wanted a cap that userspace can enable to get ptf
intercepts? I'm confused.

>
>> Signed-off-by: Pierre Morel <[email protected]>
>> ---
>> arch/s390/kvm/kvm-s390.c | 1 +
>> include/uapi/linux/kvm.h | 1 +
>> 2 files changed, 2 insertions(+)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index b655a7d82bf0..8c695ee79612 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -568,6 +568,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>> case KVM_CAP_S390_VCPU_RESETS:
>> case KVM_CAP_SET_GUEST_DEBUG:
>> case KVM_CAP_S390_DIAG318:
>> + case KVM_CAP_S390_CPU_TOPOLOGY:
>> r = 1;
>> break;
>> case KVM_CAP_SET_GUEST_DEBUG2:
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index d9e4aabcb31a..081ce0cd44b9 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1112,6 +1112,7 @@ struct kvm_ppc_resize_hpt {
>> #define KVM_CAP_BINARY_STATS_FD 203
>> #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
>> #define KVM_CAP_ARM_MTE 205
>> +#define KVM_CAP_S390_CPU_TOPOLOGY 206
>>
>> #ifdef KVM_CAP_IRQ_ROUTING
>>
>>

Regardless of what we end up with: we need documentation for any new cap
:)

2021-07-23 09:31:25

by Christian Borntraeger

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] s390:kvm: Topology expose TOPOLOGY facility



On 23.07.21 10:55, Cornelia Huck wrote:
> On Fri, Jul 23 2021, Christian Borntraeger <[email protected]> wrote:
>
>> On 22.07.21 19:02, Pierre Morel wrote:
>>> We add a KVM extension KVM_CAP_S390_CPU_TOPOLOGY to tell the
>>> userland hypervisor it is safe to activate the CPU Topology facility.
>>
>> I think the old variant of using the CPU model was actually better.
>> It was just the patch description that was wrong.
>
> I thought we wanted a cap that userspace can enable to get ptf
> intercepts? I'm confused.
>

PTF goes to userspace in any case as every instruction that is
not handled by kvm and where interpretion is not enabled.
Now, having said that, we actually want PTF interpretion to be enabled
for "Check topology-change status" as this is supposed to be a fast
operation. Some OSes do query that in their interrupt handlers.


>>
>>> Signed-off-by: Pierre Morel <[email protected]>
>>> ---
>>> arch/s390/kvm/kvm-s390.c | 1 +
>>> include/uapi/linux/kvm.h | 1 +
>>> 2 files changed, 2 insertions(+)
>>>
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index b655a7d82bf0..8c695ee79612 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -568,6 +568,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>> case KVM_CAP_S390_VCPU_RESETS:
>>> case KVM_CAP_SET_GUEST_DEBUG:
>>> case KVM_CAP_S390_DIAG318:
>>> + case KVM_CAP_S390_CPU_TOPOLOGY:
>>> r = 1;
>>> break;
>>> case KVM_CAP_SET_GUEST_DEBUG2:
>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>>> index d9e4aabcb31a..081ce0cd44b9 100644
>>> --- a/include/uapi/linux/kvm.h
>>> +++ b/include/uapi/linux/kvm.h
>>> @@ -1112,6 +1112,7 @@ struct kvm_ppc_resize_hpt {
>>> #define KVM_CAP_BINARY_STATS_FD 203
>>> #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
>>> #define KVM_CAP_ARM_MTE 205
>>> +#define KVM_CAP_S390_CPU_TOPOLOGY 206
>>>
>>> #ifdef KVM_CAP_IRQ_ROUTING
>>>
>>>
>
> Regardless of what we end up with: we need documentation for any new cap
> :)
>

2021-07-23 10:26:26

by Christian Borntraeger

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] s390:kvm: Topology expose TOPOLOGY facility



On 23.07.21 12:10, Pierre Morel wrote:
>
>
> On 7/23/21 11:28 AM, Christian Borntraeger wrote:
>>
>>
>> On 23.07.21 10:55, Cornelia Huck wrote:
>>> On Fri, Jul 23 2021, Christian Borntraeger <[email protected]> wrote:
>>>
>>>> On 22.07.21 19:02, Pierre Morel wrote:
>>>>> We add a KVM extension KVM_CAP_S390_CPU_TOPOLOGY to tell the
>>>>> userland hypervisor it is safe to activate the CPU Topology facility.
>>>>
>>>> I think the old variant of using the CPU model was actually better.
>>>> It was just the patch description that was wrong.
>>>
>>> I thought we wanted a cap that userspace can enable to get ptf
>>> intercepts? I'm confused.
>>>
>>
>> PTF goes to userspace in any case as every instruction that is
>> not handled by kvm and where interpretion is not enabled.
>> Now, having said that, we actually want PTF interpretion to be enabled
>> for "Check topology-change status" as this is supposed to be a fast
>> operation. Some OSes do query that in their interrupt handlers.
>>
>
> An old QEMU getting the PTF instruction will send a OPERATION exception to the guest if the facility 11 is actzivated.
> Facility 11 is in QEMU since GAEN10_GA1, if I enable the facility in the CPU model all cpu model starting with GEN10_GA1 will panic on PTF.

Hmm, if qemu enables facility 11 this would be a bug in QEMU. And indeed that seems to be the case here. So yes, the proper solution (cpu model) does not work unfortunately.

As I said, we want PTF interpretion enabled in KVM anyway as PTF FC==2 Check topology-change status is performance critical.
So this needs some bigger rework still. For example QEMU must then be able to tell KVM to tell SIE that the interpreted PTF will indicate a topology change.





> So I think we need the capability so that new QEMU enable the facility once it has the right handling for PTF.
>
>
>>
>>>>> Signed-off-by: Pierre Morel <[email protected]>
>>>>> ---
>>>>>    arch/s390/kvm/kvm-s390.c | 1 +
>>>>>    include/uapi/linux/kvm.h | 1 +
>>>>>    2 files changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>>> index b655a7d82bf0..8c695ee79612 100644
>>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>>> @@ -568,6 +568,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>>>>        case KVM_CAP_S390_VCPU_RESETS:
>>>>>        case KVM_CAP_SET_GUEST_DEBUG:
>>>>>        case KVM_CAP_S390_DIAG318:
>>>>> +    case KVM_CAP_S390_CPU_TOPOLOGY:
>>>>>            r = 1;
>>>>>            break;
>>>>>        case KVM_CAP_SET_GUEST_DEBUG2:
>>>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>>>>> index d9e4aabcb31a..081ce0cd44b9 100644
>>>>> --- a/include/uapi/linux/kvm.h
>>>>> +++ b/include/uapi/linux/kvm.h
>>>>> @@ -1112,6 +1112,7 @@ struct kvm_ppc_resize_hpt {
>>>>>    #define KVM_CAP_BINARY_STATS_FD 203
>>>>>    #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
>>>>>    #define KVM_CAP_ARM_MTE 205
>>>>> +#define KVM_CAP_S390_CPU_TOPOLOGY 206
>>>>>    #ifdef KVM_CAP_IRQ_ROUTING
>>>>>
>>>
>>> Regardless of what we end up with: we need documentation for any new cap
>>> :)
>>>
>

2021-07-23 10:27:37

by Pierre Morel

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] s390:kvm: Topology expose TOPOLOGY facility



On 7/23/21 11:28 AM, Christian Borntraeger wrote:
>
>
> On 23.07.21 10:55, Cornelia Huck wrote:
>> On Fri, Jul 23 2021, Christian Borntraeger <[email protected]>
>> wrote:
>>
>>> On 22.07.21 19:02, Pierre Morel wrote:
>>>> We add a KVM extension KVM_CAP_S390_CPU_TOPOLOGY to tell the
>>>> userland hypervisor it is safe to activate the CPU Topology facility.
>>>
>>> I think the old variant of using the CPU model was actually better.
>>> It was just the patch description that was wrong.
>>
>> I thought we wanted a cap that userspace can enable to get ptf
>> intercepts? I'm confused.
>>
>
> PTF goes to userspace in any case as every instruction that is
> not handled by kvm and where interpretion is not enabled.
> Now, having said that, we actually want PTF interpretion to be enabled
> for "Check topology-change status" as this is supposed to be a fast
> operation. Some OSes do query that in their interrupt handlers.
>

An old QEMU getting the PTF instruction will send a OPERATION exception
to the guest if the facility 11 is actzivated.
Facility 11 is in QEMU since GAEN10_GA1, if I enable the facility in the
CPU model all cpu model starting with GEN10_GA1 will panic on PTF.

So I think we need the capability so that new QEMU enable the facility
once it has the right handling for PTF.


>
>>>> Signed-off-by: Pierre Morel <[email protected]>
>>>> ---
>>>>    arch/s390/kvm/kvm-s390.c | 1 +
>>>>    include/uapi/linux/kvm.h | 1 +
>>>>    2 files changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>> index b655a7d82bf0..8c695ee79612 100644
>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>> @@ -568,6 +568,7 @@ int kvm_vm_ioctl_check_extension(struct kvm
>>>> *kvm, long ext)
>>>>        case KVM_CAP_S390_VCPU_RESETS:
>>>>        case KVM_CAP_SET_GUEST_DEBUG:
>>>>        case KVM_CAP_S390_DIAG318:
>>>> +    case KVM_CAP_S390_CPU_TOPOLOGY:
>>>>            r = 1;
>>>>            break;
>>>>        case KVM_CAP_SET_GUEST_DEBUG2:
>>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>>>> index d9e4aabcb31a..081ce0cd44b9 100644
>>>> --- a/include/uapi/linux/kvm.h
>>>> +++ b/include/uapi/linux/kvm.h
>>>> @@ -1112,6 +1112,7 @@ struct kvm_ppc_resize_hpt {
>>>>    #define KVM_CAP_BINARY_STATS_FD 203
>>>>    #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
>>>>    #define KVM_CAP_ARM_MTE 205
>>>> +#define KVM_CAP_S390_CPU_TOPOLOGY 206
>>>>    #ifdef KVM_CAP_IRQ_ROUTING
>>>>
>>
>> Regardless of what we end up with: we need documentation for any new cap
>> :)
>>

--
Pierre Morel
IBM Lab Boeblingen