Re: [PATCH v3 11/40] KVM: arm64: gic-v5: Implement VMT/vIST IRS MMIO Ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The error path unconditionally frees VMT memory if VMT assignment times out, risking hardware DMA into freed memory.
- [High] The error path for KVM device registration unconditionally returns `-ENODEV`, completely bypassing the intended fallback to GICv3-legacy mode.
- [Medium] `vgic_v5_irs_assign_vmt()` writes to IRS control registers without first waiting for the hardware interface to be idle.
--

commit 7badcef9a2165d8ec4899755d30ae11f3202995d
Author: Sascha Bischoff <Sascha.Bischoff@arm.com>

KVM: arm64: gic-v5: Implement VMT/vIST IRS MMIO Ops

This commit adds support for making VM and IST tables valid via the host
IRS MMIO interface. It introduces functions to allocate, assign, and clear
the VMT, and wires up vcpu_affinity commands to manipulate VMTE and VIST
validity.

> diff --git a/arch/arm64/kvm/vgic/vgic-v5.c b/arch/arm64/kvm/vgic/vgic-v5.c
> index 02d5e383a87ed..14adc82b65828 100644
> --- a/arch/arm64/kvm/vgic/vgic-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5.c
[ ... ]
> @@ -105,13 +131,54 @@ int vgic_v5_probe(const struct gic_kvm_info *info)
[ ... ]
> +	/*
> +	 * We've now allocated the VM table, but the host's IRS doesn't know
> +	 * about it yet. Provide the base address of the VMT to the IRS, as well
> +	 * as the number of ID bits that it covers and the structure used
> +	 * (linear/two-level).
> +	 */
> +	ret = vgic_v5_irs_assign_vmt(irs_caps.two_level_vmt_support,
> +				     ilog2(irs_caps.max_vms),
> +				     vgic_v5_get_vmt_base());
> +	if (ret) {
> +		kvm_err("Failed to assign the GICv5 VM tables to the IRS; no GICv5 support\n");
> +		vgic_v5_vmt_free();

[Severity: Critical]
If vgic_v5_irs_assign_vmt() fails, such as due to a hardware timeout after
writing the physical address to GICV5_IRS_VMT_BASER, doesn't the hardware
still hold the active pointer to this memory?

Calling vgic_v5_vmt_free() here unconditionally frees the VMT memory. If
the hardware recovers or continues processing, could this lead to the
hardware performing DMA into freed memory? 

Should this path attempt to clear the VMT pointer in the hardware and only
free the memory if the clear operation succeeds?

> +		return -ENODEV;
> +	}
> +
>  	kvm_vgic_global_state.max_gic_vcpus = min(irs_caps.max_vpes,
>  						  VGIC_V5_MAX_CPUS);
>  
>  	ret = kvm_register_vgic_device(KVM_DEV_TYPE_ARM_VGIC_V5);
>  	if (ret) {
>  		kvm_err("Cannot register GICv5 KVM device.\n");
> -		goto skip_v5;
> +		/*
> +		 * Don't free the VMT itself if the hardware still has a valid
> +		 * pointer to it.
> +		 */
> +		if (!vgic_v5_irs_clear_vmt())
> +			vgic_v5_vmt_free();
> +		return -ENODEV;

[Severity: High]
Does returning unconditionally here bypass the intended fallback to
GICv3-legacy mode?

The commit message mentions falling back to the legacy path once the IRS VMT
state is cleared, and the original code used a goto to jump to skip_v5. By
returning -ENODEV directly here, it seems KVM will fail to initialize any GIC
support when vGICv5 registration fails.

>  	}
>  
>  	v5_registered = true;
[ ... ]
> @@ -153,18 +221,136 @@ int vgic_v5_probe(const struct gic_kvm_info *info)
[ ... ]
> +static int vgic_v5_irs_assign_vmt(bool two_level, u8 vm_id_bits,
> +				  phys_addr_t vmt_base)
> +{
> +	u64 vmt_baser;
> +	u32 vmt_cfgr;
> +
> +	guard(raw_spinlock_irqsave)(&global_irs_lock);
> +
> +	vmt_baser = irs_readq_relaxed(GICV5_IRS_VMT_BASER);
> +	if (!!FIELD_GET(GICV5_IRS_VMT_BASER_VALID, vmt_baser))
> +		return -EBUSY;
> +
> +	vmt_cfgr = FIELD_PREP(GICV5_IRS_VMT_CFGR_VM_ID_BITS, vm_id_bits);
> +	if (two_level)
> +		vmt_cfgr |= FIELD_PREP(GICV5_IRS_VMT_CFGR_STRUCTURE,
> +				       GICV5_IRS_VMT_CFGR_STRUCTURE_TWO_LEVEL);
> +
> +	irs_writel_relaxed(vmt_cfgr, GICV5_IRS_VMT_CFGR);
> +
> +	/* The base address is intentionally only masked and not shifted */
> +	vmt_baser = FIELD_PREP(GICV5_IRS_VMT_BASER_VALID, true) |
> +		    (vmt_base & GICV5_IRS_VMT_BASER_ADDR);
> +	irs_writeq_relaxed(vmt_baser, GICV5_IRS_VMT_BASER);

[Severity: Medium]
Should there be a check to ensure the hardware interface is idle before
writing to these IRS control registers?

Unlike vgic_v5_irs_write_vm_mmio_reg(), which explicitly calls
vgic_v5_irs_wait_for_vm_op() before writing, this function directly writes
to GICV5_IRS_VMT_CFGR and GICV5_IRS_VMT_BASER immediately after acquiring
the lock. Could this corrupt the hardware configuration if the IRS is busy
processing a previous operation (e.g., during kexec)?

> +
> +	return vgic_v5_irs_wait_for_vm_op();
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703154811.3355680-1-sascha.bischoff@arm.com?part=11