* Re: [PATCH v2] KVM: s390: Add new reset vcpu API
2019-12-03 16:20 [PATCH v2] KVM: s390: Add new reset vcpu API Janosch Frank
@ 2019-12-03 16:24 ` David Hildenbrand
2019-12-03 16:28 ` Cornelia Huck
2019-12-04 9:32 ` Thomas Huth
2 siblings, 0 replies; 6+ messages in thread
From: David Hildenbrand @ 2019-12-03 16:24 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: thuth, borntraeger, mihajlov, cohuck, linux-s390
On 03.12.19 17:20, Janosch Frank wrote:
> The architecture states that we need to reset local IRQs for all CPU
> resets. Because the old reset interface did not support the normal CPU
> reset we never did that.
>
> Now that we have a new interface, let's properly clear out local IRQs
> and let this commit be a reminder.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> arch/s390/kvm/kvm-s390.c | 13 +++++++++++++
> include/uapi/linux/kvm.h | 4 ++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index d9e6bf3d54f0..602214c5616c 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -529,6 +529,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> case KVM_CAP_S390_CMMA_MIGRATION:
> case KVM_CAP_S390_AIS:
> case KVM_CAP_S390_AIS_MIGRATION:
> + case KVM_CAP_S390_VCPU_RESETS:
> r = 1;
> break;
> case KVM_CAP_S390_HPAGE_1M:
> @@ -3287,6 +3288,13 @@ static int kvm_arch_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu,
> return r;
> }
>
> +static int kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
> +{
> + kvm_clear_async_pf_completion_queue(vcpu);
> + kvm_s390_clear_local_irqs(vcpu);
> + return 0;
> +}
> +
> static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
> {
> kvm_s390_vcpu_initial_reset(vcpu);
> @@ -4363,7 +4371,12 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
> break;
> }
> + case KVM_S390_NORMAL_RESET:
> + r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
> + break;
> case KVM_S390_INITIAL_RESET:
> + /* fallthrough */
> + case KVM_S390_CLEAR_RESET:
> r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
> break;
> case KVM_SET_ONE_REG:
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 52641d8ca9e8..f4fc865775a5 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1000,6 +1000,7 @@ struct kvm_ppc_resize_hpt {
> #define KVM_CAP_PMU_EVENT_FILTER 173
> #define KVM_CAP_ARM_IRQ_LINE_LAYOUT_2 174
> #define KVM_CAP_HYPERV_DIRECT_TLBFLUSH 175
> +#define KVM_CAP_S390_VCPU_RESETS 180
>
> #ifdef KVM_CAP_IRQ_ROUTING
>
> @@ -1461,6 +1462,9 @@ struct kvm_enc_region {
> /* Available with KVM_CAP_ARM_SVE */
> #define KVM_ARM_VCPU_FINALIZE _IOW(KVMIO, 0xc2, int)
>
Maybe also a comment like
"Available with KVM_CAP_S390_VCPU_RESETS" ?
> +#define KVM_S390_NORMAL_RESET _IO(KVMIO, 0xc3)
> +#define KVM_S390_CLEAR_RESET _IO(KVMIO, 0xc4)
> +
> /* Secure Encrypted Virtualization command */
> enum sev_cmd_id {
> /* Guest initialization commands */
>
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: s390: Add new reset vcpu API
2019-12-03 16:20 [PATCH v2] KVM: s390: Add new reset vcpu API Janosch Frank
2019-12-03 16:24 ` David Hildenbrand
@ 2019-12-03 16:28 ` Cornelia Huck
2019-12-04 9:32 ` Thomas Huth
2 siblings, 0 replies; 6+ messages in thread
From: Cornelia Huck @ 2019-12-03 16:28 UTC (permalink / raw)
To: Janosch Frank; +Cc: kvm, thuth, david, borntraeger, mihajlov, linux-s390
On Tue, 3 Dec 2019 11:20:55 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:
> The architecture states that we need to reset local IRQs for all CPU
> resets. Because the old reset interface did not support the normal CPU
> reset we never did that.
>
> Now that we have a new interface, let's properly clear out local IRQs
> and let this commit be a reminder.
Well, the new interface did not appear magically, this patch introduces
it :) (The whole sentence is a bit confusing; what is this commit
supposed to remind us of?)
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> arch/s390/kvm/kvm-s390.c | 13 +++++++++++++
> include/uapi/linux/kvm.h | 4 ++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index d9e6bf3d54f0..602214c5616c 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -529,6 +529,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> case KVM_CAP_S390_CMMA_MIGRATION:
> case KVM_CAP_S390_AIS:
> case KVM_CAP_S390_AIS_MIGRATION:
> + case KVM_CAP_S390_VCPU_RESETS:
> r = 1;
> break;
> case KVM_CAP_S390_HPAGE_1M:
> @@ -3287,6 +3288,13 @@ static int kvm_arch_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu,
> return r;
> }
>
> +static int kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
> +{
> + kvm_clear_async_pf_completion_queue(vcpu);
> + kvm_s390_clear_local_irqs(vcpu);
> + return 0;
> +}
> +
> static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
> {
> kvm_s390_vcpu_initial_reset(vcpu);
> @@ -4363,7 +4371,12 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
> break;
> }
> + case KVM_S390_NORMAL_RESET:
> + r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
> + break;
> case KVM_S390_INITIAL_RESET:
> + /* fallthrough */
> + case KVM_S390_CLEAR_RESET:
> r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
The cover letter probably should also mention that clear reset falls
back to initial reset for now?
> break;
> case KVM_SET_ONE_REG:
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 52641d8ca9e8..f4fc865775a5 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1000,6 +1000,7 @@ struct kvm_ppc_resize_hpt {
> #define KVM_CAP_PMU_EVENT_FILTER 173
> #define KVM_CAP_ARM_IRQ_LINE_LAYOUT_2 174
> #define KVM_CAP_HYPERV_DIRECT_TLBFLUSH 175
> +#define KVM_CAP_S390_VCPU_RESETS 180
>
> #ifdef KVM_CAP_IRQ_ROUTING
>
> @@ -1461,6 +1462,9 @@ struct kvm_enc_region {
> /* Available with KVM_CAP_ARM_SVE */
> #define KVM_ARM_VCPU_FINALIZE _IOW(KVMIO, 0xc2, int)
>
> +#define KVM_S390_NORMAL_RESET _IO(KVMIO, 0xc3)
> +#define KVM_S390_CLEAR_RESET _IO(KVMIO, 0xc4)
> +
> /* Secure Encrypted Virtualization command */
> enum sev_cmd_id {
> /* Guest initialization commands */
Looks sane, but please also document the new ioctls and the new cap.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: s390: Add new reset vcpu API
2019-12-03 16:20 [PATCH v2] KVM: s390: Add new reset vcpu API Janosch Frank
2019-12-03 16:24 ` David Hildenbrand
2019-12-03 16:28 ` Cornelia Huck
@ 2019-12-04 9:32 ` Thomas Huth
2019-12-04 10:06 ` Janosch Frank
2 siblings, 1 reply; 6+ messages in thread
From: Thomas Huth @ 2019-12-04 9:32 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: david, borntraeger, mihajlov, cohuck, linux-s390
On 03/12/2019 17.20, Janosch Frank wrote:
> The architecture states that we need to reset local IRQs for all CPU
> resets. Because the old reset interface did not support the normal CPU
> reset we never did that.
>
> Now that we have a new interface, let's properly clear out local IRQs
> and let this commit be a reminder.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> arch/s390/kvm/kvm-s390.c | 13 +++++++++++++
> include/uapi/linux/kvm.h | 4 ++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index d9e6bf3d54f0..602214c5616c 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -529,6 +529,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> case KVM_CAP_S390_CMMA_MIGRATION:
> case KVM_CAP_S390_AIS:
> case KVM_CAP_S390_AIS_MIGRATION:
> + case KVM_CAP_S390_VCPU_RESETS:
> r = 1;
> break;
> case KVM_CAP_S390_HPAGE_1M:
> @@ -3287,6 +3288,13 @@ static int kvm_arch_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu,
> return r;
> }
>
> +static int kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
> +{
> + kvm_clear_async_pf_completion_queue(vcpu);
> + kvm_s390_clear_local_irqs(vcpu);
> + return 0;
> +}
> +
> static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
> {
> kvm_s390_vcpu_initial_reset(vcpu);
> @@ -4363,7 +4371,12 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
> break;
> }
> + case KVM_S390_NORMAL_RESET:
> + r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
> + break;
> case KVM_S390_INITIAL_RESET:
> + /* fallthrough */
> + case KVM_S390_CLEAR_RESET:
> r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
> break;
Isn't clear reset supposed to do more than the initial reset? If so, I'd
rather expect it the other way round:
case KVM_S390_CLEAR_RESET:
/* fallthrough */
case KVM_S390_INITIAL_RESET:
r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
break;
... so you can later add the additional stuff before the "/* fallthrough
*/"?
Thomas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: s390: Add new reset vcpu API
2019-12-04 9:32 ` Thomas Huth
@ 2019-12-04 10:06 ` Janosch Frank
2019-12-04 10:37 ` Thomas Huth
0 siblings, 1 reply; 6+ messages in thread
From: Janosch Frank @ 2019-12-04 10:06 UTC (permalink / raw)
To: Thomas Huth, kvm; +Cc: david, borntraeger, mihajlov, cohuck, linux-s390
[-- Attachment #1.1: Type: text/plain, Size: 2605 bytes --]
On 12/4/19 10:32 AM, Thomas Huth wrote:
> On 03/12/2019 17.20, Janosch Frank wrote:
>> The architecture states that we need to reset local IRQs for all CPU
>> resets. Because the old reset interface did not support the normal CPU
>> reset we never did that.
>>
>> Now that we have a new interface, let's properly clear out local IRQs
>> and let this commit be a reminder.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> arch/s390/kvm/kvm-s390.c | 13 +++++++++++++
>> include/uapi/linux/kvm.h | 4 ++++
>> 2 files changed, 17 insertions(+)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index d9e6bf3d54f0..602214c5616c 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -529,6 +529,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>> case KVM_CAP_S390_CMMA_MIGRATION:
>> case KVM_CAP_S390_AIS:
>> case KVM_CAP_S390_AIS_MIGRATION:
>> + case KVM_CAP_S390_VCPU_RESETS:
>> r = 1;
>> break;
>> case KVM_CAP_S390_HPAGE_1M:
>> @@ -3287,6 +3288,13 @@ static int kvm_arch_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu,
>> return r;
>> }
>>
>> +static int kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
>> +{
>> + kvm_clear_async_pf_completion_queue(vcpu);
>> + kvm_s390_clear_local_irqs(vcpu);
>> + return 0;
>> +}
>> +
>> static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
>> {
>> kvm_s390_vcpu_initial_reset(vcpu);
>> @@ -4363,7 +4371,12 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>> r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
>> break;
>> }
>> + case KVM_S390_NORMAL_RESET:
>> + r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
>> + break;
>> case KVM_S390_INITIAL_RESET:
>> + /* fallthrough */
>> + case KVM_S390_CLEAR_RESET:
>> r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>> break;
>
> Isn't clear reset supposed to do more than the initial reset? If so, I'd
> rather expect it the other way round:
In the PV patch I remove the fallthrough and add a function for the
clear reset. I add the UV reset calls into the
kvm_arch_vcpu_ioctl_*_reset() functions, therefore I can't fallthrough
because the Ultravisor does currently not allow staged resets (i.e.
first initial then clear if a clear reset is needed)
>
> case KVM_S390_CLEAR_RESET:
> /* fallthrough */
> case KVM_S390_INITIAL_RESET:
> r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
> break;
>
> ... so you can later add the additional stuff before the "/* fallthrough
> */"?
>
> Thomas
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: s390: Add new reset vcpu API
2019-12-04 10:06 ` Janosch Frank
@ 2019-12-04 10:37 ` Thomas Huth
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2019-12-04 10:37 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: david, borntraeger, mihajlov, cohuck, linux-s390
[-- Attachment #1.1: Type: text/plain, Size: 2564 bytes --]
On 04/12/2019 11.06, Janosch Frank wrote:
> On 12/4/19 10:32 AM, Thomas Huth wrote:
>> On 03/12/2019 17.20, Janosch Frank wrote:
>>> The architecture states that we need to reset local IRQs for all CPU
>>> resets. Because the old reset interface did not support the normal CPU
>>> reset we never did that.
>>>
>>> Now that we have a new interface, let's properly clear out local IRQs
>>> and let this commit be a reminder.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>> arch/s390/kvm/kvm-s390.c | 13 +++++++++++++
>>> include/uapi/linux/kvm.h | 4 ++++
>>> 2 files changed, 17 insertions(+)
>>>
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index d9e6bf3d54f0..602214c5616c 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -529,6 +529,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>> case KVM_CAP_S390_CMMA_MIGRATION:
>>> case KVM_CAP_S390_AIS:
>>> case KVM_CAP_S390_AIS_MIGRATION:
>>> + case KVM_CAP_S390_VCPU_RESETS:
>>> r = 1;
>>> break;
>>> case KVM_CAP_S390_HPAGE_1M:
>>> @@ -3287,6 +3288,13 @@ static int kvm_arch_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu,
>>> return r;
>>> }
>>>
>>> +static int kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
>>> +{
>>> + kvm_clear_async_pf_completion_queue(vcpu);
>>> + kvm_s390_clear_local_irqs(vcpu);
>>> + return 0;
>>> +}
>>> +
>>> static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
>>> {
>>> kvm_s390_vcpu_initial_reset(vcpu);
>>> @@ -4363,7 +4371,12 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>>> r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
>>> break;
>>> }
>>> + case KVM_S390_NORMAL_RESET:
>>> + r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
>>> + break;
>>> case KVM_S390_INITIAL_RESET:
>>> + /* fallthrough */
>>> + case KVM_S390_CLEAR_RESET:
>>> r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>>> break;
>>
>> Isn't clear reset supposed to do more than the initial reset? If so, I'd
>> rather expect it the other way round:
>
> In the PV patch I remove the fallthrough and add a function for the
> clear reset. I add the UV reset calls into the
> kvm_arch_vcpu_ioctl_*_reset() functions, therefore I can't fallthrough
> because the Ultravisor does currently not allow staged resets (i.e.
> first initial then clear if a clear reset is needed)
Ok, then it's fine. But in case you respin your patch, it's maybe less
confusing if you swap it.
Thomas
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread