Re: [PATCH 26/28] KVM: arm64: Add FEAT_NV3 detection

[Marc Zyngier <maz@kernel.org>]

On Thu, 02 Jul 2026 17:39:50 +0100,
sashiko-bot@kernel.org wrote:
> 
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Writes to the SYS_NVHCR_EL2 control system register are not followed by an immediate isb().
> - [High] Writes to CNTKCTL_EL1 and CNTHCTL_EL2 control registers using sysreg_clear_set() lack an immediate isb().
> --
> 
> commit b72de4ae5666d0aaa35b94915af5eab1152df83b
> Author: Marc Zyngier <maz@kernel.org>
> KVM: arm64: Add FEAT_NV3 detection
> 
> This commit adds the detection code for FEAT_NV3 to the cpufeature
> capabilities. This engages the FEAT_NV3 fast-path when the hardware
> supports it.
> 
> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > index c9c124b0ccc8e..6ae1c816e2010 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -2627,6 +2627,13 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
> >  		.matches = has_cpuid_feature,
> >  		ARM64_CPUID_FIELDS(ID_AA64MMFR4_EL1, NV_frac, NV2P1)
> >  	},
> 
> [Severity: High]
> Could the system operate under a stale configuration when updating timer
> control registers based on the ARM64_HAS_NV2P1 capability?
> 
> In arch/arm64/kvm/arch_timer.c:timer_set_traps(), the control registers
> are modified without an immediate isb() following the update:
> 
> arch/arm64/kvm/arch_timer.c:timer_set_traps()
> 	if (!cpus_have_final_cap(ARM64_HAS_NV2P1))
> 		sysreg_clear_set(cnthctl_el2, clr, set);
> 	else
> 		sysreg_clear_set(cntkctl_el1, clr, set);
> 
> The sysreg_clear_set() macro expands to a read-modify-write sequence that
> does not include an isb() as the very next instruction. Is an explicit
> barrier required here to prevent undefined pipeline state?

No. And "undefined pipeline state" means fsck all.

> 
> > +	{
> > +		.desc = "FEAT_NV3",
> > +		.capability = ARM64_HAS_NV3,
> > +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
> > +		.matches = has_cpuid_feature,
> > +		ARM64_CPUID_FIELDS(ID_AA64MMFR4_EL1, NV_frac, NV3)
> > +	},
> 
> [Severity: High]
> Does enabling the ARM64_HAS_NV3 capability expose missing synchronization
> when writing the SYS_NVHCR_EL2 control register?
> 
> Looking at __compute_hcr() in arch/arm64/kvm/hyp/vhe/switch.c, the guest
> view is published to SYS_NVHCR_EL2 without an immediate isb():
> 
> arch/arm64/kvm/hyp/vhe/switch.c:__compute_hcr()
> 		if (cpus_have_final_cap(ARM64_HAS_NV3) && vcpu_el2_e2h_is_set(vcpu))
> 			write_sysreg_s(__vcpu_sys_reg(vcpu, HCR_EL2), SYS_NVHCR_EL2);
> 		else
> 			__vcpu_assign_sys_reg(vcpu, NVHCR_EL2, __vcpu_sys_reg(vcpu, HCR_EL2));
> 
> Similarly, the L2 view of HCR_EL2 is restored without an immediate isb()
> before returning:

ERET in the guest is a CSE. The end.

	M.

-- 
Jazz isn't dead. It just smells funny.