Skip to contentSkip to conversation

KVM: s390: Add new reset vcpu API

PATCH Superseded Janosch Frank 4 participants 12 messages Dec 5, 2019 – Jan 8, 2020

Subsystems (4)
Documentation KVM s390 KVM s390

v3 of 10 Superseded Latest is v10 →

lore
Share & tools
lore

Janosch Frank

author maintainer
#0 lore raw

PATCH v3 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.

Also we add a ioctl for the clear reset to have all resets exposed to userspace. Currently the clear reset falls back to the initial reset, but we plan to have clear reset specific code in the future.

Signed-off-by Janosch Frank <[email protected]>
Reviewed-by David Hildenbrand <[email protected]>
---
 Documentation/virt/kvm/api.txt | 45 ++++++++++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.c       | 14 +++++++++++
 include/uapi/linux/kvm.h       |  5 ++++
 3 files changed, 64 insertions(+)
Documentation/virt/kvm/api.txt +45 -0
index 4833904d32a5..3b62489308e0 100644
@@ -4126,6 +4126,44 @@ Valid values for 'action':
 #define KVM_PMU_EVENT_ALLOW 0
 #define KVM_PMU_EVENT_DENY 1
 
+4.121 KVM_S390_NORMAL_RESET
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that QEMU
+can't access via the kvm_run structure. The normal reset clears local
+interrupts, the riccb and PSW bit 24.
+
+4.122 KVM_S390_INITIAL_RESET
+
+Capability: none
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that QEMU
+can't access via the kvm_run structure. The initial reset is a
+superset of the normal reset and additionally clears the psw, prefix,
+timing related registers, as well as setting the control registers to
+their initial value.
+
+4.123 KVM_S390_CLEAR_RESET
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that QEMU
+can't access via the kvm_run structure. The clear reset is a superset
+of the initial reset and additionally clears general, access, floating
+and vector registers.
 
 5. The kvm_run structure
 ------------------------

@@ -5322,3 +5360,10 @@ handling by KVM (as some KVM hypercall may be mistakenly treated as TLB
 flush hypercalls by Hyper-V) so userspace should disable KVM identification
 in CPUID and only exposes Hyper-V identification. In this case, guest
 thinks it's running on Hyper-V and only use Hyper-V hypercalls.
+
+8.22 KVM_CAP_S390_VCPU_RESETS
+
+Architectures: s390
+
+This capability indicates that the KVM_S390_NORMAL_RESET and
+KVM_S390_CLEAR_RESET ioctls are available.
arch/s390/kvm/kvm-s390.c +14 -0
index d9e6bf3d54f0..7f3ede0b2715 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:

@@ -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,9 +4371,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
 		break;
 	}
+
+	case KVM_S390_CLEAR_RESET:
+		/* fallthrough */
 	case KVM_S390_INITIAL_RESET:
 		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
 		break;
+	case KVM_S390_NORMAL_RESET:
+		r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
+		break;
 	case KVM_SET_ONE_REG:
 	case KVM_GET_ONE_REG: {
 		struct kvm_one_reg reg;
include/uapi/linux/kvm.h +5 -0
index 52641d8ca9e8..edbb2da43f02 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 176
 
 #ifdef KVM_CAP_IRQ_ROUTING
 

@@ -1461,6 +1462,10 @@ struct kvm_enc_region {
 /* Available with KVM_CAP_ARM_SVE */
 #define KVM_ARM_VCPU_FINALIZE	  _IOW(KVMIO,  0xc2, int)
 
+/* 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 */
Signature · 1 line
2.20.1
#1 lore raw
Quoted 4 lines · On Thu, 5 Dec 2019 07:09:56 -0500 Janosch Frank <[email protected]> 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.

I'm still confused what this commit is supposed to be a reminder of :)

Quoted 20 lines

Also we add a ioctl for the clear reset to have all resets exposed to userspace. Currently the clear reset falls back to the initial reset, but we plan to have clear reset specific code in the future.

Signed-off-by Janosch Frank <[email protected]>
Reviewed-by David Hildenbrand <[email protected]>
---
 Documentation/virt/kvm/api.txt | 45 ++++++++++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.c       | 14 +++++++++++
 include/uapi/linux/kvm.h       |  5 ++++
 3 files changed, 64 insertions(+)
Documentation/virt/kvm/api.txt +0 -0
index 4833904d32a5..3b62489308e0 100644
@@ -4126,6 +4126,44 @@ Valid values for 'action':
 #define KVM_PMU_EVENT_ALLOW 0
 #define KVM_PMU_EVENT_DENY 1

Documentation/virt/kvm/api.txt Valid values for 'action': · line 4137

Quoted 10 lines
Documentation/virt/kvm/api.txt +9 -0
 
+4.121 KVM_S390_NORMAL_RESET
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that QEMU

s/QEMU/userspace/ (also below)

Documentation/virt/kvm/api.txt Valid values for 'action': · line 4139

Quoted 2 lines
Documentation/virt/kvm/api.txt +2 -0
+can't access via the kvm_run structure. The normal reset clears local
+interrupts, the riccb and PSW bit 24.

What about:

"It is intended to be called when a normal reset is performed on the vcpu and clears..."

Documentation/virt/kvm/api.txt Valid values for 'action': · line 4141

Quoted 2 lines
Documentation/virt/kvm/api.txt +2 -0
+
+4.122 KVM_S390_INITIAL_RESET

Oh, we had never documented it before?

Documentation/virt/kvm/api.txt Valid values for 'action': · line 4153

Quoted 12 lines
Documentation/virt/kvm/api.txt +12 -0
+
+Capability: none
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that QEMU
+can't access via the kvm_run structure. The initial reset is a
+superset of the normal reset and additionally clears the psw, prefix,
+timing related registers, as well as setting the control registers to
+their initial value.

"It is intended to be called when an initial reset (which is a superset of the normal reset) is performed on the vcpu and additionally clears..."

?

Documentation/virt/kvm/api.txt Valid values for 'action': · line 4166

Quoted 13 lines
Documentation/virt/kvm/api.txt +13 -0
+
+4.123 KVM_S390_CLEAR_RESET
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that QEMU
+can't access via the kvm_run structure. The clear reset is a superset
+of the initial reset and additionally clears general, access, floating
+and vector registers.

You can probably guess what I was going to write :)

Quoted 4 lines
Documentation/virt/kvm/api.txt +0 -0
 5. The kvm_run structure
 ------------------------

Documentation/virt/kvm/api.txt handling by KVM (as some KVM hypercall may be mistakenly treated as TLB · line 5369

Quoted 10 lines
Documentation/virt/kvm/api.txt +7 -0
@@ -5322,3 +5360,10 @@ handling by KVM (as some KVM hypercall may be mistakenly treated as TLB
 flush hypercalls by Hyper-V) so userspace should disable KVM identification
 in CPUID and only exposes Hyper-V identification. In this case, guest
 thinks it's running on Hyper-V and only use Hyper-V hypercalls.
+
+8.22 KVM_CAP_S390_VCPU_RESETS
+
+Architectures: s390
+
+This capability indicates that the KVM_S390_NORMAL_RESET and
+KVM_S390_CLEAR_RESET ioctls are available.

(...)

The code looks good.

author maintainer
#2 lore raw

PATCH v4 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.

Also we add a ioctl for the clear reset to have all resets exposed to userspace. Currently the clear reset falls back to the initial reset, but we plan to have clear reset specific code in the future.

Signed-off-by Janosch Frank <[email protected]>
Reviewed-by David Hildenbrand <[email protected]>
---
 Documentation/virt/kvm/api.txt | 48 ++++++++++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.c       | 14 ++++++++++
 include/uapi/linux/kvm.h       |  5 ++++
 3 files changed, 67 insertions(+)
Documentation/virt/kvm/api.txt +48 -0
index 4833904d32a5..296e51f9df70 100644
@@ -4126,6 +4126,47 @@ Valid values for 'action':
 #define KVM_PMU_EVENT_ALLOW 0
 #define KVM_PMU_EVENT_DENY 1
 
+4.121 KVM_S390_NORMAL_RESET
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that userspace
+can't access via the kvm_run structure. It is intended to be called
+when a normal reset is performed on the vcpu and clears local
+interrupts, the riccb and PSW bit 24.
+
+4.122 KVM_S390_INITIAL_RESET
+
+Capability: none
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that userspace
+can't access via the kvm_run structure. It is intended to be called
+when an initial reset (which is a superset of the normal reset) is
+performed on the vcpu and additionally clears the psw, prefix, timing
+related registers, as well as setting the control registers to their
+initial value.
+
+4.123 KVM_S390_CLEAR_RESET
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that userspace
+can't access via the kvm_run structure. It is intended to be called
+when an initial reset (which is a superset of the normal reset) is
+performed on the vcpu and additionally clears general, access,
+floating and vector registers.
 
 5. The kvm_run structure
 ------------------------

@@ -5322,3 +5363,10 @@ handling by KVM (as some KVM hypercall may be mistakenly treated as TLB
 flush hypercalls by Hyper-V) so userspace should disable KVM identification
 in CPUID and only exposes Hyper-V identification. In this case, guest
 thinks it's running on Hyper-V and only use Hyper-V hypercalls.
+
+8.22 KVM_CAP_S390_VCPU_RESETS
+
+Architectures: s390
+
+This capability indicates that the KVM_S390_NORMAL_RESET and
+KVM_S390_CLEAR_RESET ioctls are available.
arch/s390/kvm/kvm-s390.c +14 -0
index d9e6bf3d54f0..7f3ede0b2715 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:

@@ -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,9 +4371,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
 		break;
 	}
+
+	case KVM_S390_CLEAR_RESET:
+		/* fallthrough */
 	case KVM_S390_INITIAL_RESET:
 		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
 		break;
+	case KVM_S390_NORMAL_RESET:
+		r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
+		break;
 	case KVM_SET_ONE_REG:
 	case KVM_GET_ONE_REG: {
 		struct kvm_one_reg reg;
include/uapi/linux/kvm.h +5 -0
index 52641d8ca9e8..edbb2da43f02 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 176
 
 #ifdef KVM_CAP_IRQ_ROUTING
 

@@ -1461,6 +1462,10 @@ struct kvm_enc_region {
 /* Available with KVM_CAP_ARM_SVE */
 #define KVM_ARM_VCPU_FINALIZE	  _IOW(KVMIO,  0xc2, int)
 
+/* 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 */
Signature · 1 line
2.20.1
Reviewed-by on v4 1/1
#3 lore raw
Quoted 54 lines · On Thu, 5 Dec 2019 07:28:10 -0500 Janosch Frank <[email protected]> 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.

Also we add a ioctl for the clear reset to have all resets exposed to userspace. Currently the clear reset falls back to the initial reset, but we plan to have clear reset specific code in the future.

Signed-off-by Janosch Frank <[email protected]>
Reviewed-by David Hildenbrand <[email protected]>
---
 Documentation/virt/kvm/api.txt | 48 ++++++++++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.c       | 14 ++++++++++
 include/uapi/linux/kvm.h       |  5 ++++
 3 files changed, 67 insertions(+)
Documentation/virt/kvm/api.txt +29 -0
index 4833904d32a5..296e51f9df70 100644
@@ -4126,6 +4126,47 @@ Valid values for 'action':
 #define KVM_PMU_EVENT_ALLOW 0
 #define KVM_PMU_EVENT_DENY 1
 
+4.121 KVM_S390_NORMAL_RESET
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that userspace
+can't access via the kvm_run structure. It is intended to be called
+when a normal reset is performed on the vcpu and clears local
+interrupts, the riccb and PSW bit 24.
+
+4.122 KVM_S390_INITIAL_RESET
+
+Capability: none
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that userspace
+can't access via the kvm_run structure. It is intended to be called
+when an initial reset (which is a superset of the normal reset) is
+performed on the vcpu and additionally clears the psw, prefix, timing
+related registers, as well as setting the control registers to their
+initial value.
+
+4.123 KVM_S390_CLEAR_RESET

Documentation/virt/kvm/api.txt Valid values for 'action': · line 4167

Quoted 10 lines
Documentation/virt/kvm/api.txt +10 -0
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that userspace
+can't access via the kvm_run structure. It is intended to be called
+when an initial reset (which is a superset of the normal reset) is

s/initial/clear/
s/normal/initial/

(no need to respin, just fix up while applying :)

Quoted 6 lines
Documentation/virt/kvm/api.txt +2 -0
+performed on the vcpu and additionally clears general, access,
+floating and vector registers.
 
 5. The kvm_run structure
 ------------------------

Documentation/virt/kvm/api.txt handling by KVM (as some KVM hypercall may be mistakenly treated as TLB · line 5372

Quoted 10 lines
Documentation/virt/kvm/api.txt +7 -0
@@ -5322,3 +5363,10 @@ handling by KVM (as some KVM hypercall may be mistakenly treated as TLB
 flush hypercalls by Hyper-V) so userspace should disable KVM identification
 in CPUID and only exposes Hyper-V identification. In this case, guest
 thinks it's running on Hyper-V and only use Hyper-V hypercalls.
+
+8.22 KVM_CAP_S390_VCPU_RESETS
+
+Architectures: s390
+
+This capability indicates that the KVM_S390_NORMAL_RESET and
+KVM_S390_CLEAR_RESET ioctls are available.
Reviewed-by Cornelia Huck <[email protected]>
#4 lore raw
In reply to Janosch Frank
Quoted 16 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.

Also we add a ioctl for the clear reset to have all resets exposed to userspace. Currently the clear reset falls back to the initial reset, but we plan to have clear reset specific code in the future.

Signed-off-by Janosch Frank <[email protected]>
Reviewed-by David Hildenbrand <[email protected]>
---
 Documentation/virt/kvm/api.txt | 48 ++++++++++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.c       | 14 ++++++++++
 include/uapi/linux/kvm.h       |  5 ++++
 3 files changed, 67 insertions(+)

[...]

Documentation/virt/kvm/api.txt Valid values for 'action': · line 4169

Quoted 13 lines

+4.123 KVM_S390_CLEAR_RESET

Documentation/virt/kvm/api.txt +12 -0
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that userspace
+can't access via the kvm_run structure. It is intended to be called
+when an initial reset (which is a superset of the normal reset) is
+performed on the vcpu and additionally clears general, access,
+floating and vector registers.

So now you've documented that this ioctl clears the GPRs, ARs and FRs/VRs ... but the implementation does not! That's quite ugly. Can you please state clearly that it is the job of userspace to clear these registers (in the default, non-protected case) and that this ioctl should be called on top?

Same problem with the PSW bit 24 and riccb during normal reset.

Or should the kernel code maybe also clear these in addition to userspace, just to be in line with the initial reset?

Thomas

#5 lore raw
In reply to Janosch Frank

On 05.12.19 13:09, Janosch Frank wrote:
[...]

Review

Quoted 12 lines

+4.123 KVM_S390_CLEAR_RESET

(quoted diff) +11 -0
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that QEMU
+can't access via the kvm_run structure. The clear reset is a superset
+of the initial reset and additionally clears general, access, floating
+and vector registers.

As Thomas outlined, make it more obvious that userspace does the remaining parts. I do not think that we want the kernel to do the things (unless it helps you in some way for the ultravisor guests)

[...]

Review

Quoted 6 lines
(quoted diff) +5 -0
+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;

Shouldnt we also do

        if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
                kvm_s390_vcpu_stop(vcpu);

here?

Quoted 6 lines
(quoted diff) +2 -0
+}
+
 static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
 {
 	kvm_s390_vcpu_initial_reset(vcpu);

Review long kvm_arch_vcpu_ioctl(struct file *filp, · line 4379

Quoted 9 lines
(quoted diff) +3 -0
@@ -4363,9 +4371,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
 		break;
 	}
+
+	case KVM_S390_CLEAR_RESET:
+		/* fallthrough */
 	case KVM_S390_INITIAL_RESET:
 		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
 		break;

Then we could also do a fallthrough here and do:

arch/s390/kvm/kvm-s390.c +0 -4
index d9e6bf3..c715ae3 100644
@@ -2867,10 +2867,6 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu)
        vcpu->arch.sie_block->pp = 0;
        vcpu->arch.sie_block->fpf &= ~FPF_BPBC;
        vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
-       kvm_clear_async_pf_completion_queue(vcpu);
-       if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
-               kvm_s390_vcpu_stop(vcpu);
-       kvm_s390_clear_local_irqs(vcpu);
 }
 
 void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
Quoted 5 lines
(quoted diff) +3 -0
+	case KVM_S390_NORMAL_RESET:
+		r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
+		break;
 	case KVM_SET_ONE_REG:
 	case KVM_GET_ONE_REG: {
author maintainer
#6 lore raw
Quoted 4 lines · Christian Borntraeger

On 05.12.19 13:09, Janosch Frank wrote:
[...]

Quoted 16 lines

Review

Quoted 12 lines

+4.123 KVM_S390_CLEAR_RESET

(quoted diff) +11 -0
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that QEMU
+can't access via the kvm_run structure. The clear reset is a superset
+of the initial reset and additionally clears general, access, floating
+and vector registers.

As Thomas outlined, make it more obvious that userspace does the remaining parts. I do not think that we want the kernel to do the things (unless it helps you in some way for the ultravisor guests)

Ok, will do

Quoted 10 lines

[...]

Review

Quoted 6 lines
(quoted diff) +5 -0
+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;

Shouldnt we also do

Quoted 4 lines
        if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
                kvm_s390_vcpu_stop(vcpu);

here?

Isn't userspace cpu state ctrl basically a prereq anyway for getting those new ioctls?

Quoted 44 lines
Quoted 8 lines
(quoted diff) +2 -0
+}
+
 static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
 {
 	kvm_s390_vcpu_initial_reset(vcpu);

Review long kvm_arch_vcpu_ioctl(struct file *filp, · line 4379

Quoted 9 lines
(quoted diff) +3 -0
@@ -4363,9 +4371,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
 		break;
 	}
+
+	case KVM_S390_CLEAR_RESET:
+		/* fallthrough */
 	case KVM_S390_INITIAL_RESET:
 		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
 		break;

Then we could also do a fallthrough here and do:

arch/s390/kvm/kvm-s390.c +0 -4
index d9e6bf3..c715ae3 100644
@@ -2867,10 +2867,6 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu)
        vcpu->arch.sie_block->pp = 0;
        vcpu->arch.sie_block->fpf &= ~FPF_BPBC;
        vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
-       kvm_clear_async_pf_completion_queue(vcpu);
-       if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
-               kvm_s390_vcpu_stop(vcpu);
-       kvm_s390_clear_local_irqs(vcpu);
 }
 
 void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
Quoted 9 lines
(quoted diff) +3 -0
+	case KVM_S390_NORMAL_RESET:
+		r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
+		break;
 	case KVM_SET_ONE_REG:
 	case KVM_GET_ONE_REG: {
#7 lore raw
Quoted 22 lines · Janosch Frank
Quoted 4 lines · Christian Borntraeger

On 05.12.19 13:09, Janosch Frank wrote:
[...]

Quoted 16 lines

Review

Quoted 12 lines

+4.123 KVM_S390_CLEAR_RESET

(quoted diff) +11 -0
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that QEMU
+can't access via the kvm_run structure. The clear reset is a superset
+of the initial reset and additionally clears general, access, floating
+and vector registers.

As Thomas outlined, make it more obvious that userspace does the remaining parts. I do not think that we want the kernel to do the things (unless it helps you in some way for the ultravisor guests)

Quoted 1 line

Ok, will do

I changed my mind (see my other mail) but I would like Thomas, Conny or David to ack/nack.

Quoted 14 lines
Quoted 14 lines

[...]

Review

Quoted 6 lines
(quoted diff) +5 -0
+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;

Shouldnt we also do

        if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
                kvm_s390_vcpu_stop(vcpu);
Quoted 4 lines
Quoted 1 line

here?

Isn't userspace cpu state ctrl basically a prereq anyway for getting those new ioctls?

I do not see a reason why we should mandate userspace cpu state for the normal reset. Using this if also enables the fallthrough below - which I like.

Quoted 47 lines
Quoted 45 lines
Quoted 9 lines
(quoted diff) +2 -0
+}
+
 static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
 {
 	kvm_s390_vcpu_initial_reset(vcpu);

Review long kvm_arch_vcpu_ioctl(struct file *filp, · line 4379

Quoted 9 lines
(quoted diff) +3 -0
@@ -4363,9 +4371,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
 		break;
 	}
+
+	case KVM_S390_CLEAR_RESET:
+		/* fallthrough */
 	case KVM_S390_INITIAL_RESET:
 		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
 		break;

Then we could also do a fallthrough here and do:

arch/s390/kvm/kvm-s390.c +0 -4
index d9e6bf3..c715ae3 100644
@@ -2867,10 +2867,6 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu)
        vcpu->arch.sie_block->pp = 0;
        vcpu->arch.sie_block->fpf &= ~FPF_BPBC;
        vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
-       kvm_clear_async_pf_completion_queue(vcpu);
-       if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
-               kvm_s390_vcpu_stop(vcpu);
-       kvm_s390_clear_local_irqs(vcpu);
 }
 
 void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
Quoted 9 lines
(quoted diff) +3 -0
+	case KVM_S390_NORMAL_RESET:
+		r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
+		break;
 	case KVM_SET_ONE_REG:
 	case KVM_GET_ONE_REG: {
#8 lore raw
Quoted 25 lines · Christian Borntraeger
Quoted 22 lines · Janosch Frank
Quoted 20 lines · Christian Borntraeger

On 05.12.19 13:09, Janosch Frank wrote:
[...]

Review

Quoted 12 lines

+4.123 KVM_S390_CLEAR_RESET

(quoted diff) +11 -0
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that QEMU
+can't access via the kvm_run structure. The clear reset is a superset
+of the initial reset and additionally clears general, access, floating
+and vector registers.

As Thomas outlined, make it more obvious that userspace does the remaining parts. I do not think that we want the kernel to do the things (unless it helps you in some way for the ultravisor guests)

Quoted 4 lines
Quoted 1 line

Ok, will do

I changed my mind (see my other mail) but I would like Thomas, Conny or David to ack/nack.

I don't mind too much as long as it is properly documented, but I also slightly prefer to be consistent here, i.e. let the kernel clear the rest here, too, just like we do it already with the initial reset.

Thomas

#9 lore raw
Quoted 26 lines · On Wed, 8 Jan 2020 15:44:57 +0100 Thomas Huth <[email protected]> wrote:
Quoted 25 lines · Christian Borntraeger
Quoted 22 lines · Janosch Frank
Quoted 20 lines · Christian Borntraeger

On 05.12.19 13:09, Janosch Frank wrote:
[...]

Review

Quoted 12 lines

+4.123 KVM_S390_CLEAR_RESET

(quoted diff) +11 -0
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that QEMU
+can't access via the kvm_run structure. The clear reset is a superset
+of the initial reset and additionally clears general, access, floating
+and vector registers.

As Thomas outlined, make it more obvious that userspace does the remaining parts. I do not think that we want the kernel to do the things (unless it helps you in some way for the ultravisor guests)

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

Ok, will do

I changed my mind (see my other mail) but I would like Thomas, Conny or David to ack/nack.

I don't mind too much as long as it is properly documented, but I also slightly prefer to be consistent here, i.e. let the kernel clear the rest here, too, just like we do it already with the initial reset.

I definitely agree with the 'properly documented' part :) It's probably enough to just state that the kernel resets the stuff, no need to go into details (and we also don't need to update this for later versions.)

Q: Are we sure that we will always be able to reset everything from the kernel?

#10 lore raw
In reply to Christian Borntraeger
Quoted 4 lines · Christian Borntraeger

On 05.12.19 13:09, Janosch Frank wrote:
[...]

Quoted 16 lines

Review

Quoted 12 lines

+4.123 KVM_S390_CLEAR_RESET

(quoted diff) +11 -0
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that QEMU
+can't access via the kvm_run structure. The clear reset is a superset
+of the initial reset and additionally clears general, access, floating
+and vector registers.

As Thomas outlined, make it more obvious that userspace does the remaining parts. I do not think that we want the kernel to do the things (unless it helps you in some way for the ultravisor guests)

On the other hand. todays initial cpu reset DOES everything. So I guess the other ones should do the same. That actually makes the semantics clearer - when you call it it will have done whatever reset you have asked for.

Quoted 58 lines

[...]

Review

Quoted 6 lines
(quoted diff) +5 -0
+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;

Shouldnt we also do

        if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
                kvm_s390_vcpu_stop(vcpu);

here?

Quoted 6 lines
(quoted diff) +2 -0
+}
+
 static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
 {
 	kvm_s390_vcpu_initial_reset(vcpu);

Review long kvm_arch_vcpu_ioctl(struct file *filp, · line 4379

Quoted 9 lines
(quoted diff) +3 -0
@@ -4363,9 +4371,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
 		break;
 	}
+
+	case KVM_S390_CLEAR_RESET:
+		/* fallthrough */
 	case KVM_S390_INITIAL_RESET:
 		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
 		break;

Then we could also do a fallthrough here and do:

arch/s390/kvm/kvm-s390.c +0 -4
index d9e6bf3..c715ae3 100644
@@ -2867,10 +2867,6 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu)
        vcpu->arch.sie_block->pp = 0;
        vcpu->arch.sie_block->fpf &= ~FPF_BPBC;
        vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
-       kvm_clear_async_pf_completion_queue(vcpu);
-       if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
-               kvm_s390_vcpu_stop(vcpu);
-       kvm_s390_clear_local_irqs(vcpu);
 }
 
 void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
Quoted 9 lines
(quoted diff) +3 -0
+	case KVM_S390_NORMAL_RESET:
+		r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
+		break;
 	case KVM_SET_ONE_REG:
 	case KVM_GET_ONE_REG: {
author maintainer
#11 lore raw
Quoted 24 lines · Christian Borntraeger
Quoted 4 lines · Christian Borntraeger

On 05.12.19 13:09, Janosch Frank wrote:
[...]

Quoted 16 lines

Review

Quoted 12 lines

+4.123 KVM_S390_CLEAR_RESET

(quoted diff) +11 -0
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that QEMU
+can't access via the kvm_run structure. The clear reset is a superset
+of the initial reset and additionally clears general, access, floating
+and vector registers.

As Thomas outlined, make it more obvious that userspace does the remaining parts. I do not think that we want the kernel to do the things (unless it helps you in some way for the ultravisor guests)

Quoted 3 lines

On the other hand. todays initial cpu reset DOES everything. So I guess the other ones should do the same. That actually makes the semantics clearer - when you call it it will have done whatever reset you have asked for.

Ok, will do, thanks for the clearup

Quoted 61 lines
Quoted 60 lines

[...]

Review

Quoted 6 lines
(quoted diff) +5 -0
+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;

Shouldnt we also do

        if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
                kvm_s390_vcpu_stop(vcpu);

here?

Quoted 6 lines
(quoted diff) +2 -0
+}
+
 static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
 {
 	kvm_s390_vcpu_initial_reset(vcpu);

Review long kvm_arch_vcpu_ioctl(struct file *filp, · line 4379

Quoted 9 lines
(quoted diff) +3 -0
@@ -4363,9 +4371,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
 		break;
 	}
+
+	case KVM_S390_CLEAR_RESET:
+		/* fallthrough */
 	case KVM_S390_INITIAL_RESET:
 		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
 		break;

Then we could also do a fallthrough here and do:

arch/s390/kvm/kvm-s390.c +0 -4
index d9e6bf3..c715ae3 100644
@@ -2867,10 +2867,6 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu)
        vcpu->arch.sie_block->pp = 0;
        vcpu->arch.sie_block->fpf &= ~FPF_BPBC;
        vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
-       kvm_clear_async_pf_completion_queue(vcpu);
-       if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
-               kvm_s390_vcpu_stop(vcpu);
-       kvm_s390_clear_local_irqs(vcpu);
 }
 
 void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
Quoted 9 lines
(quoted diff) +3 -0
+	case KVM_S390_NORMAL_RESET:
+		r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
+		break;
 	case KVM_SET_ONE_REG:
 	case KVM_GET_ONE_REG: {