[PATCH v2 0/2] KVM: arm64: VHE: Debug fixes

[PATCH v2 0/2] KVM: arm64: VHE: Debug fixes

[Alexandru Elisei]

This is v2 of the patch that zeroes PMSCR_EL1 [1] at boot, hopefully now it's
not as bad.

Picked up another fix - patch #2 ("KVM: arm64: VHE: Save and restore host
MDCR_EL2 value correctly") is new.

[1] https://lore.kernel.org/kvmarm/20241106122654.38234-1-alexandru.elisei@arm.com/

Alexandru Elisei (2):
  KVM: arm64: VHE: Initialize PMSCR_EL1
  KVM: arm64: VHE: Save and restore host MDCR_EL2 value correctly

 arch/arm64/kvm/debug.c                  | 20 ++++++++++++++++----
 arch/arm64/kvm/hyp/include/hyp/switch.h |  5 -----
 arch/arm64/kvm/hyp/nvhe/switch.c        |  6 ++++++
 3 files changed, 22 insertions(+), 9 deletions(-)


base-commit: b320789d6883cc00ac78ce83bccbfe7ed58afcf0
-- 
2.51.0

[PATCH v2 1/2] KVM: arm64: VHE: Initialize PMSCR_EL1

[Alexandru Elisei]

According to the pseudocode for StatisticalProfilingEnabled() from Arm
DDI0487L.b, PMSCR_EL1 controls profiling at EL1 and EL0:

- PMSCR_EL1.E1SPE controls profiling at EL1.
- PMSCR_EL1.E0SPE controls profiling at EL0 if HCR_EL2.TGE=0.

These two fields reset to UNKNOWN values.

When KVM runs in VHE mode and profiling is enabled in the host, before
entering a guest, KVM does not touch any of the SPE registers, leaving the
buffer enabled, and it clears HCR_EL2.TGE. As a result, depending on the
reset value for the E1SPE and E0SPE fields, KVM might unintentionally
profile a guest. Make the behaviour consistent and predictable by clearing
PMSCR_EL1 when KVM initialises the host debug configuration.

Note that this is not a problem for nVHE, because KVM clears
PMSCR_EL1.{E1SPE,E0SPE} before entering the guest.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---

Tested by setting PMSCR_EL1.{E1SPE,E0SPE} in __finalise_el2 (to simulate a
system where the bits reset to 1) and clearing MDCR_EL2.TPMS (so guest
accesses to PMSCR_EL1 are not trapped). I booted the kernel in the model
with FEAT_SPE, but without FEAT_FGT (so I wouldn't have to fiddle with
those traps too).  Then ran a hacked kvm-unit-tests test that reads
PMSCR_EL1.

Without the patch: PMSCR_EL1 = 0x3 - E1SPE and E0SPE are set.
With the patch: PMSCR_EL1 = 0x0- E1SPE and E0SPE are not set.

Also tested by running the series on a cubie a5e (doesn't have FEAT_SPE),
in vhe, nvhe and protected modes.

 arch/arm64/kvm/debug.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 381382c19fe4..e7ce0d5a622d 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -74,13 +74,19 @@ void kvm_init_host_debug_data(void)
 	*host_data_ptr(debug_brps) = SYS_FIELD_GET(ID_AA64DFR0_EL1, BRPs, dfr0);
 	*host_data_ptr(debug_wrps) = SYS_FIELD_GET(ID_AA64DFR0_EL1, WRPs, dfr0);
 
+	if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_PMSVer_SHIFT) &&
+	    !(read_sysreg_s(SYS_PMBIDR_EL1) & PMBIDR_EL1_P)) {
+		if (has_vhe()) {
+			/* Clear E{0,1}SPE, which reset to UNKNOWN values. */
+			write_sysreg_el1(0, SYS_PMSCR);
+		} else {
+			host_data_set_flag(HAS_SPE);
+		}
+	}
+
 	if (has_vhe())
 		return;
 
-	if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_PMSVer_SHIFT) &&
-	    !(read_sysreg_s(SYS_PMBIDR_EL1) & PMBIDR_EL1_P))
-		host_data_set_flag(HAS_SPE);
-
 	/* Check if we have BRBE implemented and available at the host */
 	if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT))
 		host_data_set_flag(HAS_BRBE);
-- 
2.51.0

[PATCH v2 2/2] KVM: arm64: VHE: Save and restore host MDCR_EL2 value correctly

[Alexandru Elisei]

Prior to commit 75a5fbaf6623 ("KVM: arm64: Compute MDCR_EL2 at
vcpu_load()"), host MDCR_EL2 was saved correctly:

kvm_arch_vcpu_load()
  kvm_vcpu_load_debug() /* Doesn't touch hardware MDCR_EL2. */
  kvm_vcpu_load_vhe()
    __activate_traps_common()
       /* Saves host MDCR_EL2. */
       *host_data_ptr(host_debug_state.mdcr_el2) = read_sysreg(mdcr_el2)
       /* Writes VCPU MDCR_EL2. */
       write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2)

The MDCR_EL2 value saved previously was restored in
kvm_arch_vcpu_put() -> kvm_vcpu_put_vhe().

After the aforementioned commit, host MDCR_EL2 is never saved:

kvm_arch_vcpu_load()
  kvm_vcpu_load_debug() /* Writes VCPU MDCR_EL2 */
  kvm_vcpu_load_vhe()
    __activate_traps_common()
       /* Saves **VCPU** MDCR_EL2. */
       *host_data_ptr(host_debug_state.mdcr_el2) = read_sysreg(mdcr_el2)
       /* Writes VCPU MDCR_EL2 a second time. */
       write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2)

kvm_arch_vcpu_put() -> kvm_vcpu_put_vhe() then restores the VCPU MDCR_EL2
value. Also VCPU's MDCR_EL2 value gets written to hardware twice now.

Fix this by saving the host MDCR_EL2 in kvm_arch_vcpu_load() before it gets
overwritten by the VCPU's MDCR_EL2 value, and restore it on VCPU put.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---

Tested on a cubie a5e by adding a few print statements. Without the patch:

[ 1009.416572] kvm_arch_init_vm: MDCR_EL2=0x6
[ 1028.397204] kvm_arch_destroy_vm: MDCR_EL2=0xe66
[ 1049.271878] kvm_arch_init_vm: MDCR_EL2=0xe66
[ 1066.030000] kvm_arch_destroy_vm: MDCR_EL2=0xe66

With the patch:

[  977.814892] kvm_arch_init_vm: MDCR_EL2=0x6
[ 1040.084919] kvm_arch_destroy_vm: MDCR_EL2=0x6
[ 1045.987826] kvm_arch_init_vm: MDCR_EL2=0x6
[ 1128.720366] kvm_arch_destroy_vm: MDCR_EL2=0x6

 arch/arm64/kvm/debug.c                  | 6 ++++++
 arch/arm64/kvm/hyp/include/hyp/switch.h | 5 -----
 arch/arm64/kvm/hyp/nvhe/switch.c        | 6 ++++++
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index e7ce0d5a622d..c81b11c42e4d 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -144,6 +144,9 @@ void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu)
 	/* Must be called before kvm_vcpu_load_vhe() */
 	KVM_BUG_ON(vcpu_get_flag(vcpu, SYSREGS_ON_CPU), vcpu->kvm);
 
+	if (has_vhe())
+		*host_data_ptr(host_debug_state.mdcr_el2) = read_sysreg(mdcr_el2);
+
 	/*
 	 * Determine which of the possible debug states we're in:
 	 *
@@ -190,6 +193,9 @@ void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu)
 
 void kvm_vcpu_put_debug(struct kvm_vcpu *vcpu)
 {
+	if (has_vhe())
+		write_sysreg(*host_data_ptr(host_debug_state.mdcr_el2), mdcr_el2);
+
 	if (likely(!(vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)))
 		return;
 
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
index 84ec4e100fbb..b6682202edf3 100644
--- a/arch/arm64/kvm/hyp/include/hyp/switch.h
+++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
@@ -431,9 +431,6 @@ static inline void __activate_traps_common(struct kvm_vcpu *vcpu)
 		vcpu_set_flag(vcpu, PMUSERENR_ON_CPU);
 	}
 
-	*host_data_ptr(host_debug_state.mdcr_el2) = read_sysreg(mdcr_el2);
-	write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
-
 	if (cpus_have_final_cap(ARM64_HAS_HCX)) {
 		u64 hcrx = vcpu->arch.hcrx_el2;
 		if (is_nested_ctxt(vcpu)) {
@@ -454,8 +451,6 @@ static inline void __deactivate_traps_common(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpu_context *hctxt = host_data_ptr(host_ctxt);
 
-	write_sysreg(*host_data_ptr(host_debug_state.mdcr_el2), mdcr_el2);
-
 	write_sysreg(0, hstr_el2);
 	if (system_supports_pmuv3()) {
 		write_sysreg(ctxt_sys_reg(hctxt, PMUSERENR_EL0), pmuserenr_el0);
diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
index ccd575d5f6de..d3b9ec8a7c28 100644
--- a/arch/arm64/kvm/hyp/nvhe/switch.c
+++ b/arch/arm64/kvm/hyp/nvhe/switch.c
@@ -50,6 +50,10 @@ extern void kvm_nvhe_prepare_backtrace(unsigned long fp, unsigned long pc);
 static void __activate_traps(struct kvm_vcpu *vcpu)
 {
 	___activate_traps(vcpu, vcpu->arch.hcr_el2);
+
+	*host_data_ptr(host_debug_state.mdcr_el2) = read_sysreg(mdcr_el2);
+	write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
+
 	__activate_traps_common(vcpu);
 	__activate_cptr_traps(vcpu);
 
@@ -93,6 +97,8 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu)
 		isb();
 	}
 
+	write_sysreg(*host_data_ptr(host_debug_state.mdcr_el2), mdcr_el2);
+
 	__deactivate_traps_common(vcpu);
 
 	write_sysreg_hcr(this_cpu_ptr(&kvm_init_params)->hcr_el2);
-- 
2.51.0

Re: [PATCH v2 1/2] KVM: arm64: VHE: Initialize PMSCR_EL1

[Oliver Upton]

On Tue, Sep 02, 2025 at 02:08:32PM +0100, Alexandru Elisei wrote:
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 381382c19fe4..e7ce0d5a622d 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -74,13 +74,19 @@ void kvm_init_host_debug_data(void)
>  	*host_data_ptr(debug_brps) = SYS_FIELD_GET(ID_AA64DFR0_EL1, BRPs, dfr0);
>  	*host_data_ptr(debug_wrps) = SYS_FIELD_GET(ID_AA64DFR0_EL1, WRPs, dfr0);
>  
> +	if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_PMSVer_SHIFT) &&
> +	    !(read_sysreg_s(SYS_PMBIDR_EL1) & PMBIDR_EL1_P)) {
> +		if (has_vhe()) {
> +			/* Clear E{0,1}SPE, which reset to UNKNOWN values. */
> +			write_sysreg_el1(0, SYS_PMSCR);
> +		} else {
> +			host_data_set_flag(HAS_SPE);
> +		}
> +	}
> +

nit: While this is correct, from a code organization perspective it
doesn't belong here. The rest of this function is concerned with probing
hardware state and initializing the corresponding host data.

I prefer Marc's suggestion which could be wrapped up in a function called
'kvm_debug_init_vhe()' or similar.

Thanks,
Oliver

Re: [PATCH v2 2/2] KVM: arm64: VHE: Save and restore host MDCR_EL2 value correctly

[Oliver Upton]

On Tue, Sep 02, 2025 at 02:08:33PM +0100, Alexandru Elisei wrote:
> Prior to commit 75a5fbaf6623 ("KVM: arm64: Compute MDCR_EL2 at
> vcpu_load()"), host MDCR_EL2 was saved correctly:
> 
> kvm_arch_vcpu_load()
>   kvm_vcpu_load_debug() /* Doesn't touch hardware MDCR_EL2. */
>   kvm_vcpu_load_vhe()
>     __activate_traps_common()
>        /* Saves host MDCR_EL2. */
>        *host_data_ptr(host_debug_state.mdcr_el2) = read_sysreg(mdcr_el2)
>        /* Writes VCPU MDCR_EL2. */
>        write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2)
> 
> The MDCR_EL2 value saved previously was restored in
> kvm_arch_vcpu_put() -> kvm_vcpu_put_vhe().
> 
> After the aforementioned commit, host MDCR_EL2 is never saved:
> 
> kvm_arch_vcpu_load()
>   kvm_vcpu_load_debug() /* Writes VCPU MDCR_EL2 */
>   kvm_vcpu_load_vhe()
>     __activate_traps_common()
>        /* Saves **VCPU** MDCR_EL2. */
>        *host_data_ptr(host_debug_state.mdcr_el2) = read_sysreg(mdcr_el2)
>        /* Writes VCPU MDCR_EL2 a second time. */
>        write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2)
> 
> kvm_arch_vcpu_put() -> kvm_vcpu_put_vhe() then restores the VCPU MDCR_EL2
> value. Also VCPU's MDCR_EL2 value gets written to hardware twice now.
> 
> Fix this by saving the host MDCR_EL2 in kvm_arch_vcpu_load() before it gets
> overwritten by the VCPU's MDCR_EL2 value, and restore it on VCPU put.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

Thanks,
Oliver

Re: [PATCH v2 1/2] KVM: arm64: VHE: Initialize PMSCR_EL1

[Alexandru Elisei]

Hi Oliver,

On Wed, Sep 03, 2025 at 11:55:00PM -0700, Oliver Upton wrote:
> On Tue, Sep 02, 2025 at 02:08:32PM +0100, Alexandru Elisei wrote:
> > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> > index 381382c19fe4..e7ce0d5a622d 100644
> > --- a/arch/arm64/kvm/debug.c
> > +++ b/arch/arm64/kvm/debug.c
> > @@ -74,13 +74,19 @@ void kvm_init_host_debug_data(void)
> >  	*host_data_ptr(debug_brps) = SYS_FIELD_GET(ID_AA64DFR0_EL1, BRPs, dfr0);
> >  	*host_data_ptr(debug_wrps) = SYS_FIELD_GET(ID_AA64DFR0_EL1, WRPs, dfr0);
> >  
> > +	if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_PMSVer_SHIFT) &&
> > +	    !(read_sysreg_s(SYS_PMBIDR_EL1) & PMBIDR_EL1_P)) {
> > +		if (has_vhe()) {
> > +			/* Clear E{0,1}SPE, which reset to UNKNOWN values. */
> > +			write_sysreg_el1(0, SYS_PMSCR);
> > +		} else {
> > +			host_data_set_flag(HAS_SPE);
> > +		}
> > +	}
> > +
> 
> nit: While this is correct, from a code organization perspective it
> doesn't belong here. The rest of this function is concerned with probing
> hardware state and initializing the corresponding host data.

Yes, you're totally right.

> 
> I prefer Marc's suggestion which could be wrapped up in a function called
> 'kvm_debug_init_vhe()' or similar.

I forgot about Marc's suggestion, my bad. Will do that for the next iteration.

Thanks,
Alex

> 
> Thanks,
> Oliver

Re: [PATCH v2 0/2] KVM: arm64: VHE: Debug fixes

[Oliver Upton]

On Tue, 02 Sep 2025 14:08:31 +0100, Alexandru Elisei wrote:
> This is v2 of the patch that zeroes PMSCR_EL1 [1] at boot, hopefully now it's
> not as bad.
> 
> Picked up another fix - patch #2 ("KVM: arm64: VHE: Save and restore host
> MDCR_EL2 value correctly") is new.
> 
> [1] https://lore.kernel.org/kvmarm/20241106122654.38234-1-alexandru.elisei@arm.com/
> 
> [...]

I squashed the suggestion into the second patch.

Applied to fixes, thanks!

[1/2] KVM: arm64: VHE: Initialize PMSCR_EL1
      https://git.kernel.org/kvmarm/kvmarm/c/e643542c7160
[2/2] KVM: arm64: VHE: Save and restore host MDCR_EL2 value correctly
      https://git.kernel.org/kvmarm/kvmarm/c/2f67b9a64842

--
Best,
Oliver