Re: [PATCH 08/17] arm64/hyp-stub: Enable access to ALLINT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing `isb()` instruction after writing to the control-plane system register `SYS_HCRX_EL2`.
- [High] Uninitialized register `x1` used for NMI feature check, leading to dead code / skipped initialization.
--

commit e25e512407767afb5fe8ee7e72e600b4c00a8c3a
Author: Mark Brown <broonie@kernel.org>
Subject: arm64/hyp-stub: Enable access to ALLINT

This commit updates HCRX_EL2 to ensure TALLINT is not set when support
for NMIs is detected. This allows the system to use NMIs without them
being trapped.

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: High]
Is it possible that x1 is uninitialized here?

When compiled for the host kernel context, the __check_override macro
expects its first temporary register (x1) to be preloaded with the target
system register value. 

Because this calls __check_override directly instead of using the
check_override wrapper, and lacks a preceding mrs x1, id_aa64pfr1_el1
instruction, it looks like it might evaluate leftover garbage in x1 from
the preceding SME block. 

Could this cause the NMI setup to be silently skipped?

> +
> +.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]
Does this control-plane register write require synchronization?

Writes to SYS_HCRX_EL2 typically require an immediate isb() to ensure the
context synchronization event completes before subsequent instructions
execute. 

Since this macro is expanded in callers like __finalise_el2, the
instruction immediately following this msr_s might observe undefined
pipeline state. Should an isb() be added as the very next instruction?

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

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