Re: [PATCH v2 06/17] KVM: arm64: Add system register reset framework for protected VMs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing EL1 system registers from the pKVM reset array `pvm_sys_reg_reset_vals`.
- [Medium] Invalid numeric comparison (>=) over sparse `enum vcpu_sysreg` values to enforce fragile array sorting.
--

commit 7a5dc29475a7c4550659db2df410ae56a785635e
Author: Fuad Tabba <fuad.tabba@linux.dev>
Subject: KVM: arm64: Add system register reset framework for protected VMs

This patch introduces the system register reset framework for protected VMs,
adding kvm_reset_pvm_sys_regs() and its driving array pvm_sys_reg_reset_vals.
It populates reset values for sysregs that need to be reset to UNKNOWN or
explicit values for pKVM guests.

> diff --git a/arch/arm64/kvm/hyp/nvhe/sys_regs.c b/arch/arm64/kvm/hyp/nvhe/sys_regs.c
> index 8758c68017765..03d2c2447e0fd 100644
> --- a/arch/arm64/kvm/hyp/nvhe/sys_regs.c
> +++ b/arch/arm64/kvm/hyp/nvhe/sys_regs.c

[ ... ]

> +/* Sorted ascending by reg; kvm_check_pvm_sysreg_table() enforces it. */
> +static const struct sys_reg_desc_reset pvm_sys_reg_reset_vals[] = {
> +	RESET_FUNC(MPIDR_EL1, reset_mpidr),
> +	RESET_UNKNOWN(TPIDR_EL0),
> +	RESET_UNKNOWN(TPIDRRO_EL0),
> +	RESET_UNKNOWN(TPIDR_EL1),

[Severity: Medium]
Are we missing several EL1 system registers from this array? The commit
message mentions this table is supposed to hold the registers that
sys_regs.c resets to UNKNOWN. However, it seems to omit several newer
architectural registers (like PIR_EL1, PIRE0_EL1, POR_EL1, POR_EL0, 
TFSR_EL1, TFSRE0_EL1) which __sysreg_restore_state_nvhe() explicitly restores
for a protected vCPU.

Because the hyp vCPU page is zeroed, these omitted registers will be
zero-initialized instead of receiving the intended poison value
(0x1de7ec7edbadc0deULL) when kvm_reset_pvm_sys_regs() is called for a
protected VM.

[ ... ]

> @@ -550,16 +610,21 @@ void kvm_init_pvm_id_regs(struct kvm_vcpu *vcpu)

[ ... ]

>  int kvm_check_pvm_sysreg_table(void)
>  {
>  	unsigned int i;
>  
>  	for (i = 1; i < ARRAY_SIZE(pvm_sys_reg_descs); i++) {
> -		if (cmp_sys_reg(&pvm_sys_reg_descs[i-1], &pvm_sys_reg_descs[i]) >= 0)
> +		if (cmp_sys_reg(&pvm_sys_reg_descs[i - 1], &pvm_sys_reg_descs[i]) >= 0)
> +			return 1;
> +	}
> +
> +	for (i = 1; i < ARRAY_SIZE(pvm_sys_reg_reset_vals); i++) {
> +		if (pvm_sys_reg_reset_vals[i - 1].reg >= pvm_sys_reg_reset_vals[i].reg)
>  			return 1;
>  	}

[Severity: Medium]
Is it safe to use a numeric comparison (>=) over the .reg field here? 
Since pvm_sys_reg_reset_vals[].reg contains enum vcpu_sysreg values,
and these are indexed by their VNCR-page byte offset rather than in
declaration order, a layout change or new enum value could reorder their
integer values.

If that happens, the static array's manually sorted order would break, 
triggering a fatal BUG_ON() at boot during hypervisor initialization. 
Should this enforce an explicit allowlist or a different sorting mechanism
instead of a numeric range check?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907070002.3333525-1-fuad.tabba@linux.dev?part=6