Re: [PATCH] KVM: arm64: Decouple computation of the GICv5 trap registers from FEAT_FGT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: arm64: populate_nv_trap_config() skips GICv5 FGTs when FEAT_FGT is absent
--

commit 78d2daa299df3d77e7dfa794f19cd0a1eee7919c
Author: Gustavo Romero <gromero@redhat.com>

KVM: arm64: Decouple computation of the GICv5 trap registers from FEAT_FGT

This commit moves the computation of ICH_HFGRTR_EL2, ICH_HFGWTR_EL2, and
ICH_HFGITR_EL2 in kvm_vcpu_load_fgt() ahead of the ARM64_HAS_FGT check so
that GICv5 trap register values are computed whenever ARM64_HAS_GICV5_CPUIF
is present, even if the CPU lacks FEAT_FGT.

> diff --git a/arch/arm64/kvm/config.c b/arch/arm64/kvm/config.c
> index e93919cadf860..0833453efe4bb 100644
> --- a/arch/arm64/kvm/config.c
> +++ b/arch/arm64/kvm/config.c
> @@ -1744,6 +1744,12 @@ static void __compute_ich_hfgwtr(struct kvm_vcpu *vcpu)
>  
>  void kvm_vcpu_load_fgt(struct kvm_vcpu *vcpu)
>  {
> +	if (cpus_have_final_cap(ARM64_HAS_GICV5_CPUIF)) {
> +		__compute_ich_hfgrtr(vcpu);
> +		__compute_ich_hfgwtr(vcpu);
> +		__compute_fgt(vcpu, ICH_HFGITR_EL2);
> +	}
> +
>  	if (!cpus_have_final_cap(ARM64_HAS_FGT))
>  		return;

[Severity: High]
Does populate_nv_trap_config() in arch/arm64/kvm/emulate-nested.c also need
to be updated to store the ICH_HFGRTR_GROUP and ICH_HFGITR_GROUP entries into
sr_forward_xa when ARM64_HAS_GICV5_CPUIF is present but ARM64_HAS_FGT is
absent?

Currently, populate_nv_trap_config() skips storing any entry into
sr_forward_xa when ARM64_HAS_FGT is not supported:

arch/arm64/kvm/emulate-nested.c:populate_nv_trap_config() {
	...
		for_each_encoding_in(enc, fgt->encoding, fgt->end) {
			...
			if (!cpus_have_final_cap(ARM64_HAS_FGT))
				continue;

			prev = xa_store(&sr_forward_xa, enc,
					xa_mk_value(tc.val), GFP_KERNEL);
			...
		}
}

Because of that, on a system with ARM64_HAS_GICV5_CPUIF and !ARM64_HAS_FGT,
the GICv5 entries in sr_forward_xa are left with tc.fgt == __NO_FGT_GROUP__
(populated only with tc.sri by populate_sysreg_config() for registers in
sys_reg_descs[], or left at 0 for all other GICv5 registers and instructions).

When a guest running without FEAT_GCIE (for example, a GICv3 guest in
compatibility mode, or a VM where ID_AA64PFR2_EL1.GCIE was cleared) executes
a GICv5 system register or instruction access, kvm_vcpu_load_fgt() has
cleared the trap bits in ICH_HFGRTR_EL2, ICH_HFGWTR_EL2, and ICH_HFGITR_EL2
from kvm->arch.fgu[], so the access traps to kvm_handle_sys_reg() ->
triage_sysreg_trap():

arch/arm64/kvm/emulate-nested.c:triage_sysreg_trap() {
	...
	if (tc.fgt != __NO_FGT_GROUP__ &&
	    (vcpu->kvm->arch.fgu[tc.fgt] & BIT(tc.bit))) {
		kvm_inject_undefined(vcpu);
		return true;
	}
	...
}

With tc.fgt == __NO_FGT_GROUP__, this FGU check is bypassed instead of
injecting an UNDEF:

- A guest read of SYS_ICC_PPI_ENABLER0_EL1 or SYS_ICC_PPI_ENABLER1_EL1
  dispatches to access_gicv5_ppi_enabler() in arch/arm64/kvm/sys_regs.c with
  !p->is_write, triggering WARN_ON_ONCE(!p->is_write).

- A guest write to SYS_ICC_PPI_ENABLER0_EL1 dispatches to
  access_gicv5_ppi_enabler() and writes to
  vcpu->arch.vgic_cpu.vgic_v5.vgic_ppi_enabler, which shares a union with
  vgic_v3 in struct vgic_cpu.

- Guest reads of SYS_ICC_IDR0_EL1 or SYS_ICC_IAFFIDR_EL1 execute
  access_gicv5_idr0() or access_gicv5_iaffid() instead of injecting UNDEF.

- Guest accesses to other GICv5 registers or instructions fall through to the
  local label with !tc.sri and log "Unsupported guest access at:" on the host
  before injecting UNDEF.

>  
> @@ -1761,10 +1767,4 @@ void kvm_vcpu_load_fgt(struct kvm_vcpu *vcpu)
>  		__compute_fgt(vcpu, HDFGRTR2_EL2);
>  		__compute_fgt(vcpu, HDFGWTR2_EL2);
>  	}
> -
> -	if (cpus_have_final_cap(ARM64_HAS_GICV5_CPUIF)) {
> -		__compute_ich_hfgrtr(vcpu);
> -		__compute_ich_hfgwtr(vcpu);
> -		__compute_fgt(vcpu, ICH_HFGITR_EL2);
> -	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020104138.2641586-1-gromero@redhat.com?part=1