Re: [PATCH 09/17] arm64/idreg: Add an override for FEAT_NMI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing `isb()` after writing to the `SYS_HCRX_EL2` control system register in the `finalise_el2_state` macro.
- [Medium] The EL2 setup logic for FEAT_NMI unconditionally fails because `__check_override` operates on a garbage value instead of the actual `id_aa64pfr1_el1` register.
--

commit e25e512407767afb5fe8ee7e72e600b4c00a8c3a
Author: Mark Brown <broonie@kernel.org>

arm64/hyp-stub: Enable access to ALLINT

This commit updates HCRX_EL2 to ensure that TALLINT is not set when support
for NMIs is detected. This prevents traps from being enabled for NMIs,
allowing them to be used properly.

Link: https://lore.kernel.org/r/20221112151708.175147-7-broonie@kernel.org

> diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
> index aa8ec9df80243..73b8997f8d432 100644
> --- a/arch/arm64/include/asm/el2_setup.h
> +++ b/arch/arm64/include/asm/el2_setup.h
> @@ -600,6 +600,19 @@
>  
>  	msr_s	SYS_SMPRIMAP_EL2, xzr		// Make all priorities equal
>  .Lskip_sme_\@:
> +
> +	// NMIs
> +	__check_override id_aa64pfr1, ID_AA64PFR1_EL1_NMI_SHIFT, 4, .Linit_nmi_\@, .Lskip_nmi_\@, x1, x2

[Severity: Medium]
Could this inadvertently bypass the NMI setup by using __check_override
instead of check_override?

Because __check_override expects the hardware register value to already be
loaded into x1, and it isn't loaded here, x1 contains leftover data from the
preceding SME checks.

As a result, the bitfield extraction will always evaluate to 0,
unconditionally skipping the .Linit_nmi_\@ block and nullifying this
initialization.

> +
> +.Linit_nmi_\@:
> +	mrs	x1, id_aa64mmfr1_el1		// HCRX_EL2 present?
> +	ubfx	x1, x1, #ID_AA64MMFR1_EL1_HCX_SHIFT, #4
> +	cbz	x1, .Lskip_nmi_\@
> +
> +	mrs_s	x1, SYS_HCRX_EL2
> +	and	x1, x1, # ~HCRX_EL2_TALLINT_MASK	// Don't trap ALLINT
> +	msr_s	SYS_HCRX_EL2, x1

[Severity: High]
Is an isb instruction required immediately after writing to the SYS_HCRX_EL2
control register here?

Because the finalise_el2_state macro immediately terminates, the CPU will
execute subsequent instructions without a context synchronization event.
For instance, in arch/arm64/kvm/hyp/nvhe/hyp-init.S:

__kvm_init_el2_state:
    init_el2_state
    finalise_el2_state
    ret

The ret instruction will execute against undefined pipeline state before the
new configuration is synchronized.

> +.Lskip_nmi_\@:
>  .endm
>  
>  #endif /* __ARM_KVM_INIT_H__ */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703100135.2512312-1-ruanjinjie@huawei.com?part=9