KVM: s390: Add new reset vcpu API

PATCH Superseded Janosch Frank 4 participants 9 messages Nov 29, 2019 – Dec 2, 2019

Subsystems (3)

KVM s390 KVM s390

lore

Share & tools

lore

Janosch Frank

author maintainer

2019-11-29 14:21 UTC #0 lore raw

PATCH 1/1 KVM: s390: Add new reset vcpu API

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 <[email protected]>

---
 arch/s390/kvm/kvm-s390.c | 25 ++++++++++++++++++++++++-
 include/uapi/linux/kvm.h |  7 +++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

arch/s390/kvm/kvm-s390.c +24 -1

index d9e6bf3d54f0..2f74ff46b176 100644
@@ -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:

@@ -3293,6 +3294,25 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
+static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu, unsigned long type)
+{
+	int rc = -EINVAL;
+
+	switch (type) {
+	case KVM_S390_VCPU_RESET_NORMAL:
+		rc = 0;
+		kvm_clear_async_pf_completion_queue(vcpu);
+		kvm_s390_clear_local_irqs(vcpu);
+		break;
+	case KVM_S390_VCPU_RESET_INITIAL:
+		/* fallthrough */
+	case KVM_S390_VCPU_RESET_CLEAR:
+		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
+		break;
+	}
+	return rc;
+}
+
 int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
 	vcpu_load(vcpu);

@@ -4364,7 +4384,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		break;
 	}
 	case KVM_S390_INITIAL_RESET:
-		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
+		arg = KVM_S390_VCPU_RESET_INITIAL;
+		/* fallthrough */
+	case KVM_S390_VCPU_RESET:
+		r = kvm_arch_vcpu_ioctl_reset(vcpu, arg);
 		break;
 	case KVM_SET_ONE_REG:
 	case KVM_GET_ONE_REG: {

include/uapi/linux/kvm.h +7 -0

index 52641d8ca9e8..6da16b1f2c86 100644
@@ -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,12 @@ struct kvm_enc_region {
 /* Available with KVM_CAP_ARM_SVE */
 #define KVM_ARM_VCPU_FINALIZE	  _IOW(KVMIO,  0xc2, int)
 
+#define KVM_S390_VCPU_RESET_NORMAL	0
+#define KVM_S390_VCPU_RESET_INITIAL	1
+#define KVM_S390_VCPU_RESET_CLEAR	2
+
+#define KVM_S390_VCPU_RESET    _IO(KVMIO,   0xc3)
+
 /* Secure Encrypted Virtualization command */
 enum sev_cmd_id {
 	/* Guest initialization commands */

Signature · 1 line

2.20.1

David Hildenbrand

2019-11-29 14:31 UTC #1 lore raw

Quoted 33 lines · Janosch Frank

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 <[email protected]>
---
 arch/s390/kvm/kvm-s390.c | 25 ++++++++++++++++++++++++-
 include/uapi/linux/kvm.h |  7 +++++++
 2 files changed, 31 insertions(+), 1 deletion(-)
arch/s390/kvm/kvm-s390.c +5 -0
index d9e6bf3d54f0..2f74ff46b176 100644
@@ -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:
@@ -3293,6 +3294,25 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
+static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu, unsigned long type)
+{
+	int rc = -EINVAL;
+

arch/s390/kvm/kvm-s390.c static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu) · line 3310

Quoted 10 lines

arch/s390/kvm/kvm-s390.c +10 -0

+	switch (type) {
+	case KVM_S390_VCPU_RESET_NORMAL:
+		rc = 0;
+		kvm_clear_async_pf_completion_queue(vcpu);
+		kvm_s390_clear_local_irqs(vcpu);
+		break;
+	case KVM_S390_VCPU_RESET_INITIAL:
+		/* fallthrough */
+	case KVM_S390_VCPU_RESET_CLEAR:
+		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);

As we now have two interfaces to achieve the same thing (initial reset), I do wonder if we should simply introduce

KVM_S390_NORMAL_RESET
KVM_S390_CLEAR_RESET

instead ...

Then you can do KVM_S390_NORMAL_RESET for the bugfix and KVM_S390_CLEAR_RESET later for PV.

Does anything speak against that?

Signature · 3 lines

Thanks,

David / dhildenb

Janosch Frank

author maintainer

2019-11-29 14:33 UTC #2 lore raw

Quoted 51 lines · David Hildenbrand

Quoted 33 lines · Janosch Frank
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 <[email protected]>
---
 arch/s390/kvm/kvm-s390.c | 25 ++++++++++++++++++++++++-
 include/uapi/linux/kvm.h |  7 +++++++
 2 files changed, 31 insertions(+), 1 deletion(-)
arch/s390/kvm/kvm-s390.c +5 -0
index d9e6bf3d54f0..2f74ff46b176 100644
@@ -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:
@@ -3293,6 +3294,25 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
+static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu, unsigned long type)
+{
+	int rc = -EINVAL;
+
arch/s390/kvm/kvm-s390.c static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu) · line 3310
Quoted 10 lines
arch/s390/kvm/kvm-s390.c +10 -0
+	switch (type) {
+	case KVM_S390_VCPU_RESET_NORMAL:
+		rc = 0;
+		kvm_clear_async_pf_completion_queue(vcpu);
+		kvm_s390_clear_local_irqs(vcpu);
+		break;
+	case KVM_S390_VCPU_RESET_INITIAL:
+		/* fallthrough */
+	case KVM_S390_VCPU_RESET_CLEAR:
+		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
As we now have two interfaces to achieve the same thing (initial reset), I do wonder if we should simply introduce
KVM_S390_NORMAL_RESET
KVM_S390_CLEAR_RESET

Quoted 6 lines

instead ...
Then you can do KVM_S390_NORMAL_RESET for the bugfix and KVM_S390_CLEAR_RESET later for PV.
Does anything speak against that?

Apart from loosing one more ioctl number probably not

David Hildenbrand

2019-11-29 14:34 UTC #3 lore raw

Quoted 52 lines · Janosch Frank

Quoted 51 lines · David Hildenbrand
Quoted 33 lines · Janosch Frank
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 <[email protected]>
---
 arch/s390/kvm/kvm-s390.c | 25 ++++++++++++++++++++++++-
 include/uapi/linux/kvm.h |  7 +++++++
 2 files changed, 31 insertions(+), 1 deletion(-)
arch/s390/kvm/kvm-s390.c +5 -0
index d9e6bf3d54f0..2f74ff46b176 100644
@@ -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:
@@ -3293,6 +3294,25 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
+static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu, unsigned long type)
+{
+	int rc = -EINVAL;
+
arch/s390/kvm/kvm-s390.c static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu) · line 3310
Quoted 10 lines
arch/s390/kvm/kvm-s390.c +10 -0
+	switch (type) {
+	case KVM_S390_VCPU_RESET_NORMAL:
+		rc = 0;
+		kvm_clear_async_pf_completion_queue(vcpu);
+		kvm_s390_clear_local_irqs(vcpu);
+		break;
+	case KVM_S390_VCPU_RESET_INITIAL:
+		/* fallthrough */
+	case KVM_S390_VCPU_RESET_CLEAR:
+		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
As we now have two interfaces to achieve the same thing (initial reset), I do wonder if we should simply introduce
KVM_S390_NORMAL_RESET
KVM_S390_CLEAR_RESET

Quoted 8 lines

Quoted 6 lines
instead ...
Then you can do KVM_S390_NORMAL_RESET for the bugfix and KVM_S390_CLEAR_RESET later for PV.
Does anything speak against that?
Apart from loosing one more ioctl number probably not

Do we care? (I think not, but maybe I am missing something :) )

Signature · 3 lines

Thanks,

David / dhildenb

Janosch Frank

author maintainer

2019-11-29 14:38 UTC #4 lore raw

Quoted 55 lines · David Hildenbrand

Quoted 54 lines · Janosch Frank
Quoted 53 lines · David Hildenbrand
Quoted 33 lines · Janosch Frank
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 <[email protected]>
---
 arch/s390/kvm/kvm-s390.c | 25 ++++++++++++++++++++++++-
 include/uapi/linux/kvm.h |  7 +++++++
 2 files changed, 31 insertions(+), 1 deletion(-)
arch/s390/kvm/kvm-s390.c +5 -0
index d9e6bf3d54f0..2f74ff46b176 100644
@@ -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:
@@ -3293,6 +3294,25 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
+static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu, unsigned long type)
+{
+	int rc = -EINVAL;
+
arch/s390/kvm/kvm-s390.c static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu) · line 3310
Quoted 10 lines
arch/s390/kvm/kvm-s390.c +10 -0
+	switch (type) {
+	case KVM_S390_VCPU_RESET_NORMAL:
+		rc = 0;
+		kvm_clear_async_pf_completion_queue(vcpu);
+		kvm_s390_clear_local_irqs(vcpu);
+		break;
+	case KVM_S390_VCPU_RESET_INITIAL:
+		/* fallthrough */
+	case KVM_S390_VCPU_RESET_CLEAR:
+		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
As we now have two interfaces to achieve the same thing (initial reset), I do wonder if we should simply introduce
KVM_S390_NORMAL_RESET
KVM_S390_CLEAR_RESET
instead ...

Quoted 9 lines

Quoted 6 lines
Quoted 4 lines
Then you can do KVM_S390_NORMAL_RESET for the bugfix and KVM_S390_CLEAR_RESET later for PV.
Does anything speak against that?
Apart from loosing one more ioctl number probably not
Do we care? (I think not, but maybe I am missing something :) )

I don't, maybe somebody else does
Btw. I'm struggling to find a good name for the capability:
KVM_CAP_S390_VCPU_ADDITIONAL_RESETS ?

Christian Borntraeger

2019-11-29 14:39 UTC #5 lore raw

On 29.11.19 15:38, Janosch Frank wrote:
[...]

Quoted 13 lines

Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
As we now have two interfaces to achieve the same thing (initial reset), I do wonder if we should simply introduce
KVM_S390_NORMAL_RESET
KVM_S390_CLEAR_RESET
instead ...
Then you can do KVM_S390_NORMAL_RESET for the bugfix and KVM_S390_CLEAR_RESET later for PV.
Does anything speak against that?

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
Apart from loosing one more ioctl number probably not
Do we care? (I think not, but maybe I am missing something :) )
I don't, maybe somebody else does
Btw. I'm struggling to find a good name for the capability:
KVM_CAP_S390_VCPU_ADDITIONAL_RESETS ?

KVM_CAP_S390_VCPU_RESETS ?

David Hildenbrand

2019-11-29 14:48 UTC #6 lore raw

Quoted 19 lines · Christian Borntraeger

On 29.11.19 15:38, Janosch Frank wrote:
[...]
Quoted 15 lines
Quoted 15 lines
Quoted 14 lines
Quoted 6 lines
As we now have two interfaces to achieve the same thing (initial reset), I do wonder if we should simply introduce
KVM_S390_NORMAL_RESET
KVM_S390_CLEAR_RESET
Quoted 6 lines
instead ...
Then you can do KVM_S390_NORMAL_RESET for the bugfix and KVM_S390_CLEAR_RESET later for PV.
Does anything speak against that?
Apart from loosing one more ioctl number probably not

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Do we care? (I think not, but maybe I am missing something :) )
I don't, maybe somebody else does
Btw. I'm struggling to find a good name for the capability:
KVM_CAP_S390_VCPU_ADDITIONAL_RESETS ?
KVM_CAP_S390_VCPU_RESETS ?

Either that or two separate ones if you're not going to introduce them at the same time ...

Signature · 3 lines

Thanks,

David / dhildenb

Janosch Frank

author maintainer

2019-11-29 14:57 UTC #7 lore raw

Quoted 23 lines · David Hildenbrand

Quoted 22 lines · Christian Borntraeger
On 29.11.19 15:38, Janosch Frank wrote:
[...]
Quoted 18 lines
Quoted 17 lines
Quoted 6 lines
Quoted 6 lines
As we now have two interfaces to achieve the same thing (initial reset), I do wonder if we should simply introduce
KVM_S390_NORMAL_RESET
KVM_S390_CLEAR_RESET
Quoted 8 lines
Quoted 6 lines
instead ...
Then you can do KVM_S390_NORMAL_RESET for the bugfix and KVM_S390_CLEAR_RESET later for PV.
Does anything speak against that?
Apart from loosing one more ioctl number probably not
Do we care? (I think not, but maybe I am missing something :) )

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
I don't, maybe somebody else does
Btw. I'm struggling to find a good name for the capability:
KVM_CAP_S390_VCPU_ADDITIONAL_RESETS ?
KVM_CAP_S390_VCPU_RESETS ?
Either that or two separate ones if you're not going to introduce them at the same time ...

This is starting to get messy...

Cornelia Huck

2019-12-02 08:54 UTC #8 lore raw

Quoted 28 lines · On Fri, 29 Nov 2019 15:57:25 +0100 Janosch Frank <[email protected]> wrote:

Quoted 27 lines · David Hildenbrand
Quoted 26 lines · Christian Borntraeger
On 29.11.19 15:38, Janosch Frank wrote:
[...]
Quoted 21 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
As we now have two interfaces to achieve the same thing (initial reset), I do wonder if we should simply introduce
KVM_S390_NORMAL_RESET
KVM_S390_CLEAR_RESET
instead ...
Quoted 9 lines
Quoted 6 lines
Quoted 4 lines
Then you can do KVM_S390_NORMAL_RESET for the bugfix and KVM_S390_CLEAR_RESET later for PV.
Does anything speak against that?
Apart from loosing one more ioctl number probably not
Do we care? (I think not, but maybe I am missing something :) )
I don't, maybe somebody else does
Btw. I'm struggling to find a good name for the capability:
KVM_CAP_S390_VCPU_ADDITIONAL_RESETS ?

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
KVM_CAP_S390_VCPU_RESETS ?
Either that or two separate ones if you're not going to introduce them at the same time ...
This is starting to get messy...

In order to reduce the mess, simply introduce them at the same time? I might be missing something, but is there anything speaking against it, as you can simply invoke the initial reset handler for clear reset for now?

Also:
KVM_CAP_S390_ENHANCED_VCPU_RESETS, maybe?