[PATCH] KVM: arm64: Check cpu_has_spe() before initializing PMSCR_EL1 in VHE

[PATCH] KVM: arm64: Check cpu_has_spe() before initializing PMSCR_EL1 in VHE

[Mukesh Ojha]

commit efad60e46057 ("KVM: arm64: Initialize PMSCR_EL1 when in VHE")
initializes PMSCR_EL1 to 0 which is making the boot up stuck when KVM
runs in VHE mode and reverting the change is fixing the issue.

[    2.967447] RPC: Registered tcp NFSv4.1 backchannel transport module.
[    2.974061] PCI: CLS 0 bytes, default 64
[    2.978171] Unpacking initramfs...
[    2.982889] kvm [1]: nv: 568 coarse grained trap handlers
[    2.988573] kvm [1]: IPA Size Limit: 40 bits

Lets guard the change with cpu_has_spe() check so that it only affects
the cpu which has SPE feature supported.

Fixes: efad60e46057 ("KVM: arm64: Initialize PMSCR_EL1 when in VHE")
Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
 arch/arm64/kvm/debug.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 3515a273eaa2..d9fd45f0db9a 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -15,6 +15,14 @@
 #include <asm/kvm_arm.h>
 #include <asm/kvm_emulate.h>
 
+static int cpu_has_spe(void)
+{
+	u64 dfr0 = read_sysreg(id_aa64dfr0_el1);
+
+	return cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_PMSVer_SHIFT) &&
+	       !(read_sysreg_s(SYS_PMBIDR_EL1) & PMBIDR_EL1_P);
+}
+
 /**
  * kvm_arm_setup_mdcr_el2 - configure vcpu mdcr_el2 value
  *
@@ -80,8 +88,7 @@ void kvm_init_host_debug_data(void)
 	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))
+	if (cpu_has_spe())
 		host_data_set_flag(HAS_SPE);
 
 	/* Check if we have BRBE implemented and available at the host */
@@ -101,8 +108,8 @@ void kvm_init_host_debug_data(void)
 
 void kvm_debug_init_vhe(void)
 {
-	/* Clear PMSCR_EL1.E{0,1}SPE which reset to UNKNOWN values. */
-	if (SYS_FIELD_GET(ID_AA64DFR0_EL1, PMSVer, read_sysreg(id_aa64dfr0_el1)))
+	if (cpu_has_spe())
+		/* Clear PMSCR_EL1.E{0,1}SPE which reset to UNKNOWN values. */
 		write_sysreg_el1(0, SYS_PMSCR);
 }
 
-- 
2.50.1

Re: [PATCH] KVM: arm64: Check cpu_has_spe() before initializing PMSCR_EL1 in VHE

[Oliver Upton]

Hi Mukesh,

I find it a bit odd to refer to cpu_has_spe() in the shortlog, which
doesn't exist prior to this patch.

On Tue, Oct 07, 2025 at 11:53:56PM +0530, Mukesh Ojha wrote:
> commit efad60e46057 ("KVM: arm64: Initialize PMSCR_EL1 when in VHE")
> initializes PMSCR_EL1 to 0 which is making the boot up stuck when KVM
> runs in VHE mode and reverting the change is fixing the issue.
> 
> [    2.967447] RPC: Registered tcp NFSv4.1 backchannel transport module.
> [    2.974061] PCI: CLS 0 bytes, default 64
> [    2.978171] Unpacking initramfs...
> [    2.982889] kvm [1]: nv: 568 coarse grained trap handlers
> [    2.988573] kvm [1]: IPA Size Limit: 40 bits
> 
> Lets guard the change with cpu_has_spe() check so that it only affects
> the cpu which has SPE feature supported.

This could benefit from being spelled out a bit more. In both cases we
check for the presence of FEAT_SPE, however I believe the issue you
observe is EL3 hasn't delegated ownership of the Profiling Buffer to
Non-secure nor does it reinject an UNDEF in response to the sysreg trap.

I agree that the change is correct but the rationale needs to be clear.

Thanks,
Oliver

Re: [PATCH] KVM: arm64: Check cpu_has_spe() before initializing PMSCR_EL1 in VHE

[Marc Zyngier]

On Tue, 07 Oct 2025 19:31:45 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Hi Mukesh,
> 
> I find it a bit odd to refer to cpu_has_spe() in the shortlog, which
> doesn't exist prior to this patch.
> 
> On Tue, Oct 07, 2025 at 11:53:56PM +0530, Mukesh Ojha wrote:
> > commit efad60e46057 ("KVM: arm64: Initialize PMSCR_EL1 when in VHE")
> > initializes PMSCR_EL1 to 0 which is making the boot up stuck when KVM
> > runs in VHE mode and reverting the change is fixing the issue.
> > 
> > [    2.967447] RPC: Registered tcp NFSv4.1 backchannel transport module.
> > [    2.974061] PCI: CLS 0 bytes, default 64
> > [    2.978171] Unpacking initramfs...
> > [    2.982889] kvm [1]: nv: 568 coarse grained trap handlers
> > [    2.988573] kvm [1]: IPA Size Limit: 40 bits
> > 
> > Lets guard the change with cpu_has_spe() check so that it only affects
> > the cpu which has SPE feature supported.
> 
> This could benefit from being spelled out a bit more. In both cases we
> check for the presence of FEAT_SPE, however I believe the issue you
> observe is EL3 hasn't delegated ownership of the Profiling Buffer to
> Non-secure nor does it reinject an UNDEF in response to the sysreg trap.
> 
> I agree that the change is correct but the rationale needs to be clear.

To me, this smells a lot more like some sort of papering over a
firmware bug. Why isn't SPE available the first place?

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] KVM: arm64: Check cpu_has_spe() before initializing PMSCR_EL1 in VHE

[Leo Yan]

On Wed, Oct 08, 2025 at 11:46:55AM +0100, Marc Zyngier wrote:

[...]

> > > Lets guard the change with cpu_has_spe() check so that it only affects
> > > the cpu which has SPE feature supported.
> > 
> > This could benefit from being spelled out a bit more. In both cases we
> > check for the presence of FEAT_SPE, however I believe the issue you
> > observe is EL3 hasn't delegated ownership of the Profiling Buffer to
> > Non-secure nor does it reinject an UNDEF in response to the sysreg trap.
> > 
> > I agree that the change is correct but the rationale needs to be clear.
> 
> To me, this smells a lot more like some sort of papering over a
> firmware bug. Why isn't SPE available the first place?

TF-a grants permission to non-secure world [1], only access from secure
world or realm will trap to EL3.

So yes, it would be good to check if any issue in firmware.

Thanks,
Leo

[1] https://git.trustedfirmware.org/plugins/gitiles/TF-A/trusted-firmware-a.git/+/refs/heads/master/lib/extensions/spe/spe.c#52

Re: [PATCH] KVM: arm64: Check cpu_has_spe() before initializing PMSCR_EL1 in VHE

[Mukesh Ojha]

On Wed, Oct 08, 2025 at 01:40:40PM +0100, Leo Yan wrote:
> On Wed, Oct 08, 2025 at 11:46:55AM +0100, Marc Zyngier wrote:
> 
> [...]
> 
> > > > Lets guard the change with cpu_has_spe() check so that it only affects
> > > > the cpu which has SPE feature supported.
> > > 
> > > This could benefit from being spelled out a bit more. In both cases we
> > > check for the presence of FEAT_SPE, however I believe the issue you
> > > observe is EL3 hasn't delegated ownership of the Profiling Buffer to
> > > Non-secure nor does it reinject an UNDEF in response to the sysreg trap.
> > > 
> > > I agree that the change is correct but the rationale needs to be clear.
> > 
> > To me, this smells a lot more like some sort of papering over a
> > firmware bug. Why isn't SPE available the first place?
> 
> TF-a grants permission to non-secure world [1], only access from secure
> world or realm will trap to EL3.
> 
> So yes, it would be good to check if any issue in firmware.

We have our own implementation of EL3 and not using TF-A.

I believe, we should check in a similar way as we are doing for nVHE
case.

if (host_data_test_flag(HAS_SPE))
	write_sysreg_el1(0, SYS_PMSCR);

> 
> Thanks,
> Leo
> 
> [1] https://git.trustedfirmware.org/plugins/gitiles/TF-A/trusted-firmware-a.git/+/refs/heads/master/lib/extensions/spe/spe.c#52

-- 
-Mukesh Ojha

Re: [PATCH] KVM: arm64: Check cpu_has_spe() before initializing PMSCR_EL1 in VHE

[Leo Yan]

On Wed, Oct 08, 2025 at 10:20:58PM +0530, Mukesh Ojha wrote:

[...]

> > > To me, this smells a lot more like some sort of papering over a
> > > firmware bug. Why isn't SPE available the first place?
> > 
> > TF-a grants permission to non-secure world [1], only access from secure
> > world or realm will trap to EL3.
> > 
> > So yes, it would be good to check if any issue in firmware.
> 
> We have our own implementation of EL3 and not using TF-A.

If you don't fix your firmware, you won't be able to use Arm SPE with
the Linux kernel.

> I believe, we should check in a similar way as we are doing for nVHE
> case.
> 
> if (host_data_test_flag(HAS_SPE))
> 	write_sysreg_el1(0, SYS_PMSCR);

The document Documentation/arch/arm64/booting.rst does not state that
permission for Arm SPE is mandatory, and the SPE driver [2] can
tolerate the lack of permission. So I'm fine with applying your patch
to fix the boot issue.

Thanks,
Leo

[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/arm_spe_pmu.c#n1084

Re: [PATCH] KVM: arm64: Check cpu_has_spe() before initializing PMSCR_EL1 in VHE

[Oliver Upton]

On Wed, Oct 08, 2025 at 11:46:55AM +0100, Marc Zyngier wrote:
> On Tue, 07 Oct 2025 19:31:45 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > Hi Mukesh,
> > 
> > I find it a bit odd to refer to cpu_has_spe() in the shortlog, which
> > doesn't exist prior to this patch.
> > 
> > On Tue, Oct 07, 2025 at 11:53:56PM +0530, Mukesh Ojha wrote:
> > > commit efad60e46057 ("KVM: arm64: Initialize PMSCR_EL1 when in VHE")
> > > initializes PMSCR_EL1 to 0 which is making the boot up stuck when KVM
> > > runs in VHE mode and reverting the change is fixing the issue.
> > > 
> > > [    2.967447] RPC: Registered tcp NFSv4.1 backchannel transport module.
> > > [    2.974061] PCI: CLS 0 bytes, default 64
> > > [    2.978171] Unpacking initramfs...
> > > [    2.982889] kvm [1]: nv: 568 coarse grained trap handlers
> > > [    2.988573] kvm [1]: IPA Size Limit: 40 bits
> > > 
> > > Lets guard the change with cpu_has_spe() check so that it only affects
> > > the cpu which has SPE feature supported.
> > 
> > This could benefit from being spelled out a bit more. In both cases we
> > check for the presence of FEAT_SPE, however I believe the issue you
> > observe is EL3 hasn't delegated ownership of the Profiling Buffer to
> > Non-secure nor does it reinject an UNDEF in response to the sysreg trap.
> > 
> > I agree that the change is correct but the rationale needs to be clear.
> 
> To me, this smells a lot more like some sort of papering over a
> firmware bug. Why isn't SPE available the first place?

While I agree this points the finger at a half-assed EL3, the
architecture explicitly allows this sort of crap and we cope with the
accessibility of SPE in almost every other case.

We should at least be consistent in how we handle an inaccessible SPE.

Thanks,
Oliver

Re: [PATCH] KVM: arm64: Check cpu_has_spe() before initializing PMSCR_EL1 in VHE

[Suzuki K Poulose]

On 08/10/2025 19:26, Oliver Upton wrote:
> On Wed, Oct 08, 2025 at 11:46:55AM +0100, Marc Zyngier wrote:
>> On Tue, 07 Oct 2025 19:31:45 +0100,
>> Oliver Upton <oliver.upton@linux.dev> wrote:
>>>
>>> Hi Mukesh,
>>>
>>> I find it a bit odd to refer to cpu_has_spe() in the shortlog, which
>>> doesn't exist prior to this patch.
>>>
>>> On Tue, Oct 07, 2025 at 11:53:56PM +0530, Mukesh Ojha wrote:
>>>> commit efad60e46057 ("KVM: arm64: Initialize PMSCR_EL1 when in VHE")
>>>> initializes PMSCR_EL1 to 0 which is making the boot up stuck when KVM
>>>> runs in VHE mode and reverting the change is fixing the issue.
>>>>
>>>> [    2.967447] RPC: Registered tcp NFSv4.1 backchannel transport module.
>>>> [    2.974061] PCI: CLS 0 bytes, default 64
>>>> [    2.978171] Unpacking initramfs...
>>>> [    2.982889] kvm [1]: nv: 568 coarse grained trap handlers
>>>> [    2.988573] kvm [1]: IPA Size Limit: 40 bits
>>>>
>>>> Lets guard the change with cpu_has_spe() check so that it only affects
>>>> the cpu which has SPE feature supported.
>>>
>>> This could benefit from being spelled out a bit more. In both cases we
>>> check for the presence of FEAT_SPE, however I believe the issue you
>>> observe is EL3 hasn't delegated ownership of the Profiling Buffer to
>>> Non-secure nor does it reinject an UNDEF in response to the sysreg trap.
>>>
>>> I agree that the change is correct but the rationale needs to be clear.
>>
>> To me, this smells a lot more like some sort of papering over a
>> firmware bug. Why isn't SPE available the first place?
> 
> While I agree this points the finger at a half-assed EL3, the
> architecture explicitly allows this sort of crap and we cope with the
> accessibility of SPE in almost every other case.

One of the reasons behind this control is to work around errata, where
the higher EL could prevent the OS from using a broken implementation.
Also, for KVM could use the EL2 controls to disable SPE for Guest.

Suzuki

> 
> We should at least be consistent in how we handle an inaccessible SPE.
> 
> Thanks,
> Oliver

Re: [PATCH] KVM: arm64: Check cpu_has_spe() before initializing PMSCR_EL1 in VHE

[Mukesh Ojha]

On Tue, Oct 07, 2025 at 11:31:45AM -0700, Oliver Upton wrote:
> Hi Mukesh,
> 
> I find it a bit odd to refer to cpu_has_spe() in the shortlog, which
> doesn't exist prior to this patch.

My bad !!
will fix this

> 
> On Tue, Oct 07, 2025 at 11:53:56PM +0530, Mukesh Ojha wrote:
> > commit efad60e46057 ("KVM: arm64: Initialize PMSCR_EL1 when in VHE")
> > initializes PMSCR_EL1 to 0 which is making the boot up stuck when KVM
> > runs in VHE mode and reverting the change is fixing the issue.
> > 
> > [    2.967447] RPC: Registered tcp NFSv4.1 backchannel transport module.
> > [    2.974061] PCI: CLS 0 bytes, default 64
> > [    2.978171] Unpacking initramfs...
> > [    2.982889] kvm [1]: nv: 568 coarse grained trap handlers
> > [    2.988573] kvm [1]: IPA Size Limit: 40 bits
> > 
> > Lets guard the change with cpu_has_spe() check so that it only affects
> > the cpu which has SPE feature supported.
> 
> This could benefit from being spelled out a bit more. In both cases we
> check for the presence of FEAT_SPE, however I believe the issue you
> observe is EL3 hasn't delegated ownership of the Profiling Buffer to
> Non-secure nor does it reinject an UNDEF in response to the sysreg trap.

You are right, writing to SYS_PMSCR is causing an issue.

is it not the right to check if the profiling buffer is implemented
rather than just checking the version ?

> 
> I agree that the change is correct but the rationale needs to be clear.

Will correct it.

> 
> Thanks,
> Oliver

-- 
-Mukesh Ojha