diff mbox series

[v2,2/6] KVM: arm64: unify trap setup code

Message ID 20240426104950.7382-3-sebott@redhat.com (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: emulation for CTR_EL0 | expand

Commit Message

Sebastian Ott April 26, 2024, 10:49 a.m. UTC 
There are 2 functions to set up traps via HCR_EL2:
* kvm_init_sysreg() called via KVM_RUN (before the 1st run or when
  the pid changes)
* vcpu_reset_hcr() called via KVM_ARM_VCPU_INIT

To unify these 2 and to support traps that are dependent on the
ID register configuration, move vcpu_reset_hcr() to sys_regs.c
and call it via kvm_init_sysreg().

While at it rename kvm_init_sysreg() to kvm_setup_traps() to
better reflect what it's doing.

Signed-off-by: Sebastian Ott <sebott@redhat.com>
---
 arch/arm64/include/asm/kvm_emulate.h | 37 -----------------------
 arch/arm64/include/asm/kvm_host.h    |  2 +-
 arch/arm64/kvm/arm.c                 |  3 +-
 arch/arm64/kvm/sys_regs.c            | 44 ++++++++++++++++++++++++++--
 4 files changed, 44 insertions(+), 42 deletions(-)

Comments

Oliver Upton May 1, 2024, 6:51 a.m. UTC | #1 
On Fri, Apr 26, 2024 at 12:49:46PM +0200, Sebastian Ott wrote:
> There are 2 functions to set up traps via HCR_EL2:

nitpick: these functions *calculate* the trap values, but do not
actually set them up. HCR_EL2 doesn't get written to until further down
the line on KVM_RUN.

> +	if (cpus_have_final_cap(ARM64_HAS_STAGE2_FWB)) {
> +		vcpu->arch.hcr_el2 |= HCR_FWB;
> +	} else {
> +		/*
> +		 * For non-FWB CPUs, we trap VM ops (HCR_EL2.TVM) until M+C
> +		 * get set in SCTLR_EL1 such that we can detect when the guest
> +		 * MMU gets turned on and do the necessary cache maintenance
> +		 * then.
> +		 */
> +		vcpu->arch.hcr_el2 |= HCR_TVM;
> +	}

It seems to me like calling this once for the lifetime of a vCPU will
break non-FWB behavior.

Like the comment suggests, these traps are needed to catch the moment
the S1 MMU is turned on and do cache maintenance to make sure D$ agrees
with what the guest was doing before enabling the MMU.

KVM_ARM_VCPU_INIT resets SCTLR_EL1, but it seems we'd miss setting
HCR_TVM in that case.
Sebastian Ott May 3, 2024, 3:06 p.m. UTC | #2 
On Wed, 1 May 2024, Oliver Upton wrote:
> On Fri, Apr 26, 2024 at 12:49:46PM +0200, Sebastian Ott wrote:
>> There are 2 functions to set up traps via HCR_EL2:
>
> nitpick: these functions *calculate* the trap values, but do not
> actually set them up. HCR_EL2 doesn't get written to until further down
> the line on KVM_RUN.
>
>> +	if (cpus_have_final_cap(ARM64_HAS_STAGE2_FWB)) {
>> +		vcpu->arch.hcr_el2 |= HCR_FWB;
>> +	} else {
>> +		/*
>> +		 * For non-FWB CPUs, we trap VM ops (HCR_EL2.TVM) until M+C
>> +		 * get set in SCTLR_EL1 such that we can detect when the guest
>> +		 * MMU gets turned on and do the necessary cache maintenance
>> +		 * then.
>> +		 */
>> +		vcpu->arch.hcr_el2 |= HCR_TVM;
>> +	}
>
> It seems to me like calling this once for the lifetime of a vCPU will
> break non-FWB behavior.
>
> Like the comment suggests, these traps are needed to catch the moment
> the S1 MMU is turned on and do cache maintenance to make sure D$ agrees
> with what the guest was doing before enabling the MMU.
>
> KVM_ARM_VCPU_INIT resets SCTLR_EL1, but it seems we'd miss setting
> HCR_TVM in that case.

Ugh, I didn't think about KVM_ARM_VCPU_INIT being called more than once.

But in that case don't we loose the changes done to hcr_el2 in the current
code? E.g.:

void kvm_init_sysreg(struct kvm_vcpu *vcpu)
{
...
 	if (!kvm_has_feat(kvm, ID_AA64ISAR0_EL1, TLB, OS))
 		vcpu->arch.hcr_el2 |= HCR_TTLBOS;
...
}

static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
{
 	vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS;
...
}

Thanks,
Sebastian
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 975af30af31f..9e71fcbb033d 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -67,43 +67,6 @@  static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu)
 }
 #endif
 
-static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
-{
-	vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS;
-	if (has_vhe() || has_hvhe())
-		vcpu->arch.hcr_el2 |= HCR_E2H;
-	if (cpus_have_final_cap(ARM64_HAS_RAS_EXTN)) {
-		/* route synchronous external abort exceptions to EL2 */
-		vcpu->arch.hcr_el2 |= HCR_TEA;
-		/* trap error record accesses */
-		vcpu->arch.hcr_el2 |= HCR_TERR;
-	}
-
-	if (cpus_have_final_cap(ARM64_HAS_STAGE2_FWB)) {
-		vcpu->arch.hcr_el2 |= HCR_FWB;
-	} else {
-		/*
-		 * For non-FWB CPUs, we trap VM ops (HCR_EL2.TVM) until M+C
-		 * get set in SCTLR_EL1 such that we can detect when the guest
-		 * MMU gets turned on and do the necessary cache maintenance
-		 * then.
-		 */
-		vcpu->arch.hcr_el2 |= HCR_TVM;
-	}
-
-	if (cpus_have_final_cap(ARM64_HAS_EVT) &&
-	    !cpus_have_final_cap(ARM64_MISMATCHED_CACHE_TYPE))
-		vcpu->arch.hcr_el2 |= HCR_TID4;
-	else
-		vcpu->arch.hcr_el2 |= HCR_TID2;
-
-	if (vcpu_el1_is_32bit(vcpu))
-		vcpu->arch.hcr_el2 &= ~HCR_RW;
-
-	if (kvm_has_mte(vcpu->kvm))
-		vcpu->arch.hcr_el2 |= HCR_ATA;
-}
-
 static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu)
 {
 	return (unsigned long *)&vcpu->arch.hcr_el2;
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 9e8a496fb284..696acba883c1 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1115,7 +1115,7 @@  int __init populate_nv_trap_config(void);
 bool lock_all_vcpus(struct kvm *kvm);
 void unlock_all_vcpus(struct kvm *kvm);
 
-void kvm_init_sysreg(struct kvm_vcpu *);
+void kvm_setup_traps(struct kvm_vcpu *);
 
 /* MMIO helpers */
 void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long data);
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index c4a0a35e02c7..d6c27d8a8f2f 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -683,7 +683,7 @@  int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
 	 * This needs to happen after NV has imposed its own restrictions on
 	 * the feature set
 	 */
-	kvm_init_sysreg(vcpu);
+	kvm_setup_traps(vcpu);
 
 	ret = kvm_timer_enable(vcpu);
 	if (ret)
@@ -1438,7 +1438,6 @@  static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
 			icache_inval_all_pou();
 	}
 
-	vcpu_reset_hcr(vcpu);
 	vcpu->arch.cptr_el2 = kvm_get_reset_cptr_el2(vcpu);
 
 	/*
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 131f5b0ca2b9..ac366d0b614a 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -4020,11 +4020,43 @@  int kvm_vm_ioctl_get_reg_writable_masks(struct kvm *kvm, struct reg_mask_range *
 	return 0;
 }
 
-void kvm_init_sysreg(struct kvm_vcpu *vcpu)
+static void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
 {
 	struct kvm *kvm = vcpu->kvm;
 
-	mutex_lock(&kvm->arch.config_lock);
+	vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS;
+	if (has_vhe() || has_hvhe())
+		vcpu->arch.hcr_el2 |= HCR_E2H;
+	if (cpus_have_final_cap(ARM64_HAS_RAS_EXTN)) {
+		/* route synchronous external abort exceptions to EL2 */
+		vcpu->arch.hcr_el2 |= HCR_TEA;
+		/* trap error record accesses */
+		vcpu->arch.hcr_el2 |= HCR_TERR;
+	}
+
+	if (cpus_have_final_cap(ARM64_HAS_STAGE2_FWB)) {
+		vcpu->arch.hcr_el2 |= HCR_FWB;
+	} else {
+		/*
+		 * For non-FWB CPUs, we trap VM ops (HCR_EL2.TVM) until M+C
+		 * get set in SCTLR_EL1 such that we can detect when the guest
+		 * MMU gets turned on and do the necessary cache maintenance
+		 * then.
+		 */
+		vcpu->arch.hcr_el2 |= HCR_TVM;
+	}
+
+	if (cpus_have_final_cap(ARM64_HAS_EVT) &&
+	    !cpus_have_final_cap(ARM64_MISMATCHED_CACHE_TYPE))
+		vcpu->arch.hcr_el2 |= HCR_TID4;
+	else
+		vcpu->arch.hcr_el2 |= HCR_TID2;
+
+	if (vcpu_el1_is_32bit(vcpu))
+		vcpu->arch.hcr_el2 &= ~HCR_RW;
+
+	if (kvm_has_mte(vcpu->kvm))
+		vcpu->arch.hcr_el2 |= HCR_ATA;
 
 	/*
 	 * In the absence of FGT, we cannot independently trap TLBI
@@ -4033,6 +4065,14 @@  void kvm_init_sysreg(struct kvm_vcpu *vcpu)
 	 */
 	if (!kvm_has_feat(kvm, ID_AA64ISAR0_EL1, TLB, OS))
 		vcpu->arch.hcr_el2 |= HCR_TTLBOS;
+}
+
+void kvm_setup_traps(struct kvm_vcpu *vcpu)
+{
+	struct kvm *kvm = vcpu->kvm;
+
+	mutex_lock(&kvm->arch.config_lock);
+	vcpu_reset_hcr(vcpu);
 
 	if (cpus_have_final_cap(ARM64_HAS_HCX)) {
 		vcpu->arch.hcrx_el2 = HCRX_GUEST_FLAGS;