KVM: arm64: Reorganise vcpu first run

PATCH Superseded Marc Zyngier 2 participants 9 messages Oct 15, 2021 – Oct 16, 2021

Subsystems (3)

KVM arm64 KVM arm64

lore

Share & tools

lore

Marc Zyngier

author maintainer

2021-10-15 09:10 UTC #0 lore raw

cover KVM: arm64: Reorganise vcpu first run

KVM/arm64 relies heavily on a bunch of things to be done on the first run of the vcpu. We also do a bunch of things on PID change. It turns out that these two things are pretty similar (the first PID change is also the first run).

This small series aims at simplifying all that, and to get rid of the vcpu->arch.has_run_once state.

Marc Zyngier (5):
KVM: arm64: Move SVE state mapping at HYP to finalize-time
KVM: arm64: Move kvm_arch_vcpu_run_pid_change() out of line
KVM: arm64: Merge kvm_arch_vcpu_run_pid_change() and
kvm_vcpu_first_run_init()
KVM: arm64: Restructure the point where has_run_once is advertised
KVM: arm64: Drop vcpu->arch.has_run_once for vcpu->pid

 arch/arm64/include/asm/kvm_host.h | 12 +++------
 arch/arm64/kvm/arm.c              | 43 ++++++++++++++++++-------------
 arch/arm64/kvm/fpsimd.c           | 11 --------
 arch/arm64/kvm/reset.c            | 11 +++++++-
 arch/arm64/kvm/vgic/vgic-init.c   |  2 +-
 5 files changed, 39 insertions(+), 40 deletions(-)

Signature · 1 line

2.30.2

Marc Zyngier

author maintainer

2021-10-15 09:10 UTC #1 lore raw

PATCH 1/5 KVM: arm64: Move SVE state mapping at HYP to finalize-time

We currently map the SVE state to HYP on detection of a PID change. Although this matches what we do for FPSIMD, this is pretty pointless for SVE, as the buffer is per-vcpu and has nothing to do with the thread that is being run.

Move the mapping of the SVE state to finalize-time, which is where we allocate the state memory, and thus the most logical place to do this.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/fpsimd.c | 11 -----------
 arch/arm64/kvm/reset.c  | 11 ++++++++++-
 2 files changed, 10 insertions(+), 12 deletions(-)

arch/arm64/kvm/fpsimd.c +0 ~~-11~~

index 5621020b28de..62c0d78da7be 100644
@@ -43,17 +43,6 @@ int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu)
 	if (ret)
 		goto error;
 
-	if (vcpu->arch.sve_state) {
-		void *sve_end;
-
-		sve_end = vcpu->arch.sve_state + vcpu_sve_state_size(vcpu);
-
-		ret = create_hyp_mappings(vcpu->arch.sve_state, sve_end,
-					  PAGE_HYP);
-		if (ret)
-			goto error;
-	}
-
 	vcpu->arch.host_thread_info = kern_hyp_va(ti);
 	vcpu->arch.host_fpsimd_state = kern_hyp_va(fpsimd);
 error:

arch/arm64/kvm/reset.c +10 -1

index 5ce36b0a3343..9e904a9244c1 100644
@@ -94,6 +94,8 @@ static int kvm_vcpu_finalize_sve(struct kvm_vcpu *vcpu)
 {
 	void *buf;
 	unsigned int vl;
+	size_t reg_sz;
+	int ret;
 
 	vl = vcpu->arch.sve_max_vl;

@@ -106,10 +108,17 @@ static int kvm_vcpu_finalize_sve(struct kvm_vcpu *vcpu)
 		    vl > SVE_VL_ARCH_MAX))
 		return -EIO;
 
-	buf = kzalloc(SVE_SIG_REGS_SIZE(sve_vq_from_vl(vl)), GFP_KERNEL);
+	reg_sz = vcpu_sve_state_size(vcpu);
+	buf = kzalloc(reg_sz, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
+	ret = create_hyp_mappings(buf, buf + reg_sz, PAGE_HYP);
+	if (ret) {
+		kfree(buf);
+		return ret;
+	}
+	
 	vcpu->arch.sve_state = buf;
 	vcpu->arch.flags |= KVM_ARM64_VCPU_SVE_FINALIZED;
 	return 0;

Signature · 1 line

2.30.2

Marc Zyngier

author maintainer

2021-10-15 09:10 UTC #2 lore raw

PATCH 2/5 KVM: arm64: Move kvm_arch_vcpu_run_pid_change() out of line

In reply to Marc Zyngier

Having kvm_arch_vcpu_run_pid_change() inline doesn't bring anything to the table. Move it next to kvm_vcpu_first_run_init(), which will be convenient for what is next to come.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/include/asm/kvm_host.h | 7 +------
 arch/arm64/kvm/arm.c              | 5 +++++
 2 files changed, 6 insertions(+), 6 deletions(-)

arch/arm64/include/asm/kvm_host.h +1 -6

index f8be56d5342b..d7107d627c54 100644
@@ -748,12 +748,7 @@ static inline bool kvm_pmu_counter_deferred(struct perf_event_attr *attr)
 void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu);
 
-#ifdef CONFIG_KVM /* Avoid conflicts with core headers if CONFIG_KVM=n */
-static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
-{
-	return kvm_arch_vcpu_run_map_fp(vcpu);
-}
-
+#ifdef CONFIG_KVM
 void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr);
 void kvm_clr_pmu_events(u32 clr);

arch/arm64/kvm/arm.c +5 -0

index fe102cd2e518..ccb59ac54976 100644
@@ -583,6 +583,11 @@ static void update_vmid(struct kvm_vmid *vmid)
 	spin_unlock(&kvm_vmid_lock);
 }
 
+int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
+{
+	return kvm_arch_vcpu_run_map_fp(vcpu);
+}
+
 static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
 {
 	struct kvm *kvm = vcpu->kvm;

Signature · 1 line

2.30.2

Marc Zyngier

author maintainer

2021-10-15 09:10 UTC #3 lore raw

PATCH 3/5 KVM: arm64: Merge kvm_arch_vcpu_run_pid_change() and kvm_vcpu_first_run_init()

In reply to Marc Zyngier

The kvm_arch_vcpu_run_pid_change() helper gets called on each PID change. The kvm_vcpu_first_run_init() helper gets run on the... first run(!) of a vcpu.

As it turns out, the first run of a vcpu also triggers a PID change event (vcpu->pid is initially NULL).

Use this property to merge these two helpers and get rid of another arm64-specific oddity.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/arm.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

arch/arm64/kvm/arm.c +13 ~~-13~~

index ccb59ac54976..30692497c4ea 100644
@@ -583,22 +583,26 @@ static void update_vmid(struct kvm_vmid *vmid)
 	spin_unlock(&kvm_vmid_lock);
 }
 
+/*
+ * Handle both the initialisation that is being done when the vcpu is
+ * run for the first time, as well as the updates that must be
+ * performed each time we get a new thread dealing with this vcpu.
+ */
 int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
-{
-	return kvm_arch_vcpu_run_map_fp(vcpu);
-}
-
-static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
 {
 	struct kvm *kvm = vcpu->kvm;
-	int ret = 0;
-
-	if (likely(vcpu->arch.has_run_once))
-		return 0;
+	int ret;
 
 	if (!kvm_arm_vcpu_is_finalized(vcpu))
 		return -EPERM;
 
+	ret = kvm_arch_vcpu_run_map_fp(vcpu);
+	if (ret)
+		return ret;
+
+	if (likely(vcpu->arch.has_run_once))
+		return 0;
+
 	vcpu->arch.has_run_once = true;
 
 	kvm_arm_vcpu_init_debug(vcpu);

@@ -778,10 +782,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 	if (unlikely(!kvm_vcpu_initialized(vcpu)))
 		return -ENOEXEC;
 
-	ret = kvm_vcpu_first_run_init(vcpu);
-	if (ret)
-		return ret;
-
 	if (run->exit_reason == KVM_EXIT_MMIO) {
 		ret = kvm_handle_mmio_return(vcpu);
 		if (ret)

Signature · 1 line

2.30.2

Marc Zyngier

author maintainer

2021-10-15 09:10 UTC #4 lore raw

PATCH 4/5 KVM: arm64: Restructure the point where has_run_once is advertised

In reply to Marc Zyngier

Restructure kvm_arch_vcpu_run_pid_change() to set the has_run_once flag after having completed all the "run once" activities.

This includes moving the flip of the userspace irqchip static key to a point where nothing can fail (the current code could end-up in a bizarre state in a few error cases).

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/arm.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

arch/arm64/kvm/arm.c +12 -8

index 30692497c4ea..5bcdf8073854 100644
@@ -603,8 +603,6 @@ int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
 	if (likely(vcpu->arch.has_run_once))
 		return 0;
 
-	vcpu->arch.has_run_once = true;
-
 	kvm_arm_vcpu_init_debug(vcpu);
 
 	if (likely(irqchip_in_kernel(kvm))) {

@@ -615,12 +613,6 @@ int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
 		ret = kvm_vgic_map_resources(kvm);
 		if (ret)
 			return ret;
-	} else {
-		/*
-		 * Tell the rest of the code that there are userspace irqchip
-		 * VMs in the wild.
-		 */
-		static_branch_inc(&userspace_irqchip_in_use);
 	}
 
 	ret = kvm_timer_enable(vcpu);

@@ -628,6 +620,18 @@ int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
 		return ret;
 
 	ret = kvm_arm_pmu_v3_enable(vcpu);
+	if (ret)
+		return ret;
+
+	if (!irqchip_in_kernel(kvm)) {
+		/*
+		 * Tell the rest of the code that there are userspace irqchip
+		 * VMs in the wild.
+		 */
+		static_branch_inc(&userspace_irqchip_in_use);
+	}
+
+	vcpu->arch.has_run_once = true;
 
 	return ret;
 }

Signature · 1 line

2.30.2

Marc Zyngier

author maintainer

2021-10-15 09:10 UTC #5 lore raw

PATCH 5/5 KVM: arm64: Drop vcpu->arch.has_run_once for vcpu->pid

In reply to Marc Zyngier

With the transition to kvm_arch_vcpu_run_pid_change() to handle the "run once" activities, it becomes obvious that has_run_once is now an exact shadow of vcpu->pid.

Replace vcpu->arch.has_run_once with a new vcpu_has_run_once() helper that directly checks for vcpu->pid, and get rid of the now unused field.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/include/asm/kvm_host.h | 5 ++---
 arch/arm64/kvm/arm.c              | 8 +++-----
 arch/arm64/kvm/vgic/vgic-init.c   | 2 +-
 3 files changed, 6 insertions(+), 9 deletions(-)

arch/arm64/include/asm/kvm_host.h +2 -3

index d7107d627c54..e0de761ecbf2 100644
@@ -366,9 +366,6 @@ struct kvm_vcpu_arch {
 	int target;
 	DECLARE_BITMAP(features, KVM_VCPU_MAX_FEATURES);
 
-	/* Detect first run of a vcpu */
-	bool has_run_once;
-
 	/* Virtual SError ESR to restore when HCR_EL2.VSE is set */
 	u64 vsesr_el2;

@@ -605,6 +602,8 @@ int __kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
 void kvm_arm_halt_guest(struct kvm *kvm);
 void kvm_arm_resume_guest(struct kvm *kvm);
 
+#define vcpu_has_run_once(vcpu)	!!rcu_access_pointer((vcpu)->pid)
+
 #ifndef __KVM_NVHE_HYPERVISOR__
 #define kvm_call_hyp_nvhe(f, ...)						\
 	({								\

arch/arm64/kvm/arm.c +3 -5

index 5bcdf8073854..ebcb1adf80fb 100644
@@ -350,7 +350,7 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
 
 void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
 {
-	if (vcpu->arch.has_run_once && unlikely(!irqchip_in_kernel(vcpu->kvm)))
+	if (vcpu_has_run_once(vcpu) && unlikely(!irqchip_in_kernel(vcpu->kvm)))
 		static_branch_dec(&userspace_irqchip_in_use);
 
 	kvm_mmu_free_memory_cache(&vcpu->arch.mmu_page_cache);

@@ -600,7 +600,7 @@ int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
 	if (ret)
 		return ret;
 
-	if (likely(vcpu->arch.has_run_once))
+	if (likely(vcpu_has_run_once(vcpu)))
 		return 0;
 
 	kvm_arm_vcpu_init_debug(vcpu);

@@ -631,8 +631,6 @@ int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
 		static_branch_inc(&userspace_irqchip_in_use);
 	}
 
-	vcpu->arch.has_run_once = true;
-
 	return ret;
 }

@@ -1123,7 +1121,7 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
 	 * need to invalidate the I-cache though, as FWB does *not*
 	 * imply CTR_EL0.DIC.
 	 */
-	if (vcpu->arch.has_run_once) {
+	if (vcpu_has_run_once(vcpu)) {
 		if (!cpus_have_final_cap(ARM64_HAS_STAGE2_FWB))
 			stage2_unmap_vm(vcpu->kvm);
 		else

arch/arm64/kvm/vgic/vgic-init.c +1 -1

index 340c51d87677..8d89ca15f613 100644
@@ -91,7 +91,7 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
 		return ret;
 
 	kvm_for_each_vcpu(i, vcpu, kvm) {
-		if (vcpu->arch.has_run_once)
+		if (vcpu_has_run_once(vcpu))
 			goto out_unlock;
 	}
 	ret = 0;

Signature · 1 line

2.30.2

Andrew Jones

Reviewed-by on the v1 series

2021-10-15 09:49 UTC #6 lore raw

In reply to Marc Zyngier

Quoted 8 lines · Marc Zyngier

KVM/arm64 relies heavily on a bunch of things to be done on the first run of the vcpu. We also do a bunch of things on PID change. It turns out that these two things are pretty similar (the first PID change is also the first run).
This small series aims at simplifying all that, and to get rid of the vcpu->arch.has_run_once state.

Quoted 6 lines

Marc Zyngier (5):
KVM: arm64: Move SVE state mapping at HYP to finalize-time
KVM: arm64: Move kvm_arch_vcpu_run_pid_change() out of line
KVM: arm64: Merge kvm_arch_vcpu_run_pid_change() and
kvm_vcpu_first_run_init()
KVM: arm64: Restructure the point where has_run_once is advertised

Maybe do the restructuring before the merging in order to avoid the potential for bizarre states?

Quoted 17 lines

KVM: arm64: Drop vcpu->arch.has_run_once for vcpu->pid

 arch/arm64/include/asm/kvm_host.h | 12 +++------
 arch/arm64/kvm/arm.c              | 43 ++++++++++++++++++-------------
 arch/arm64/kvm/fpsimd.c           | 11 --------
 arch/arm64/kvm/reset.c            | 11 +++++++-
 arch/arm64/kvm/vgic/vgic-init.c   |  2 +-
 5 files changed, 39 insertions(+), 40 deletions(-)

Signature · 6 lines

2.30.2

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

For the series

Reviewed-by Andrew Jones <[email protected]>

Thanks,
drew

Andrew Jones

2021-10-15 10:05 UTC #7 lore raw

Quoted 16 lines · Andrew Jones

Quoted 8 lines · Marc Zyngier
KVM/arm64 relies heavily on a bunch of things to be done on the first run of the vcpu. We also do a bunch of things on PID change. It turns out that these two things are pretty similar (the first PID change is also the first run).
This small series aims at simplifying all that, and to get rid of the vcpu->arch.has_run_once state.
Quoted 6 lines
Marc Zyngier (5):
KVM: arm64: Move SVE state mapping at HYP to finalize-time
KVM: arm64: Move kvm_arch_vcpu_run_pid_change() out of line
KVM: arm64: Merge kvm_arch_vcpu_run_pid_change() and
kvm_vcpu_first_run_init()
KVM: arm64: Restructure the point where has_run_once is advertised

Quoted 2 lines

Maybe do the restructuring before the merging in order to avoid the potential for bizarre states?

Also, before we do the merge I think we need to duplicate the

        if (unlikely(!kvm_vcpu_initialized(vcpu)))
                return -ENOEXEC;

that we currently have above the call of kvm_vcpu_first_run_init() into kvm_arch_vcpu_run_pid_change() because kvm_arch_vcpu_run_pid_change() is called before kvm_arch_vcpu_ioctl_run() in KVM_RUN.

Thanks,
drew

Quoted 19 lines

Quoted 18 lines

KVM: arm64: Drop vcpu->arch.has_run_once for vcpu->pid

 arch/arm64/include/asm/kvm_host.h | 12 +++------
 arch/arm64/kvm/arm.c              | 43 ++++++++++++++++++-------------
 arch/arm64/kvm/fpsimd.c           | 11 --------
 arch/arm64/kvm/reset.c            | 11 +++++++-
 arch/arm64/kvm/vgic/vgic-init.c   |  2 +-
 5 files changed, 39 insertions(+), 40 deletions(-)

Signature · 6 lines

2.30.2

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Quoted 6 lines

For the series
Reviewed-by Andrew Jones <[email protected]>
Thanks,
drew

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

author maintainer

2021-10-16 10:50 UTC #8 lore raw

Quoted 18 lines · Andrew Jones

Quoted 16 lines · Andrew Jones
Quoted 14 lines · Marc Zyngier
KVM/arm64 relies heavily on a bunch of things to be done on the first run of the vcpu. We also do a bunch of things on PID change. It turns out that these two things are pretty similar (the first PID change is also the first run).
This small series aims at simplifying all that, and to get rid of the vcpu->arch.has_run_once state.
Marc Zyngier (5):
KVM: arm64: Move SVE state mapping at HYP to finalize-time
KVM: arm64: Move kvm_arch_vcpu_run_pid_change() out of line
KVM: arm64: Merge kvm_arch_vcpu_run_pid_change() and
kvm_vcpu_first_run_init()
KVM: arm64: Restructure the point where has_run_once is advertised

Quoted 2 lines

Quoted 2 lines
Maybe do the restructuring before the merging in order to avoid the potential for bizarre states?

Yup, can do.

Quoted 10 lines

Also, before we do the merge I think we need to duplicate the
        if (unlikely(!kvm_vcpu_initialized(vcpu)))
                return -ENOEXEC;
that we currently have above the call of kvm_vcpu_first_run_init() into kvm_arch_vcpu_run_pid_change() because kvm_arch_vcpu_run_pid_change() is called before kvm_arch_vcpu_ioctl_run() in KVM_RUN.

Well spotted.

I think this check should be moved into kvm_arch_vcpu_run_pid_change() instead of duplicated though, just like we have the check for 'finalized' there. After all, they are the two sides of the same coin.

This nicely moves all checks on the slow path.

Thanks,

M.

Signature · 1 line

Without deviation from the norm, progress is not possible.