Re: [PATCH 11/17] KVM: arm64: Hide FEAT_NMI from guests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The HCRX_EL2.TALLINT trap is inadvertently bypassed for the guest and erroneously enabled for the host due to a register overwrite and a polluted host context save.
- [High] Missing Context Synchronization Event (ISB) after writing to the control-plane system register SYS_HCRX_EL2.
--

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

KVM: arm64: Hide FEAT_NMI from guests

This commit hides FEAT_NMI from guests pending vGIC support. It masks out
the feature from the ID register and sets HCRX_EL2.TALLINT to trap guest
writes to ALLINT.ALLINT.

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

> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index 18131e395e24c..21b19f331a12f 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -346,6 +346,9 @@ static inline void __activate_traps_common(struct kvm_vcpu *vcpu)
>  		vcpu_set_flag(vcpu, PMUSERENR_ON_CPU);
>  	}
>  
> +	if (cpus_have_final_cap(ARM64_HAS_NMI))
> +		sysreg_clear_set_s(SYS_HCRX_EL2, 0, HCRX_EL2_TALLINT);
> +
>  	if (cpus_have_final_cap(ARM64_HAS_HCX)) {
>  		u64 hcrx = vcpu->arch.hcrx_el2;

[Severity: High]
Does this sequence inadvertently drop the trap for the guest and pollute the
host context?

Because FEAT_HCX is mandatory with FEAT_NMI, the code will execute the HCX
block immediately after setting TALLINT:

arch/arm64/kvm/hyp/include/hyp/switch.h:__activate_traps_common() {
    ...
    if (cpus_have_final_cap(ARM64_HAS_HCX)) {
        u64 hcrx = vcpu->arch.hcrx_el2;
        ...
        ctxt_sys_reg(hctxt, HCRX_EL2) = read_sysreg_s(SYS_HCRX_EL2);
        write_sysreg_s(hcrx, SYS_HCRX_EL2);
    }
}

Since SYS_HCRX_EL2 is immediately overwritten with the guest's hcrx_el2
(which lacks the TALLINT bit), it appears the trap is bypassed for the guest.

Additionally, this reads the newly modified SYS_HCRX_EL2 (with TALLINT set)
into the host's context. When the host context is later restored, will it
leave the host running with the TALLINT trap incorrectly active?


[Severity: High]
Can this read-back observe architecturally undefined pipeline state?

Immediately after sysreg_clear_set_s() writes to the control-plane system
register SYS_HCRX_EL2, the ARM64_HAS_HCX block executes a read-back via
read_sysreg_s(SYS_HCRX_EL2) without an intervening context synchronization
event (isb()).

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