Re: [PATCH v5 35/49] KVM: arm64: gic-v5: Implement save/restore mechanisms for ISTs

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Restoration of GICv5 IRS registers and IST state unconditionally fails before the VM runs.
- [High] Missing memory barrier between DMA memory reads and MMIO writes in IST save paths allows hardware to corrupt IST state.
- [Medium] Early returns in `vgic_v5_vmte_release` leak VM memory structures if IST freeing fails.
--

--- Patch [35]: [PATCH v5 35/49] KVM: arm64: gic-v5: Implement save/restore mechanisms for ISTs ---
commit 32376ab3f3a6714615e18eef70c631afa3b5cd05
Author: Sascha Bischoff <[email protected]>

KVM: arm64: gic-v5: Implement save/restore mechanisms for ISTs

This patch adds support for saving and restoring GICv5 Interrupt State Tables
(ISTs) during virtual machine migration. It introduces interfaces to export
the internal SPI and LPI IST states to userspace and inject pending
interrupts upon restoration. The state is checked for hardware quiescence
during the save operation.

> diff --git a/arch/arm64/kvm/vgic/vgic-irs-v5.c b/arch/arm64/kvm/vgic/vgic-irs-v5.c
> --- a/arch/arm64/kvm/vgic/vgic-irs-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-irs-v5.c
[ ... ]
> @@ -646,13 +661,23 @@ static int vgic_v5_mmio_uaccess_write_irs(struct kvm_vcpu *vcpu, gpa_t addr,
>  			return -EINVAL;
>  		break;
>  	case GICV5_IRS_IST_BASER:
> -		if (irs_data->ist_baser.valid &&
> -		    !vgic_v5_ist_baser_matches(irs_data, val))
> +		ret = vgic_v5_lpi_ist_exists(vcpu->kvm);
> +		if (ret < 0)
> +			return ret;

[Severity: High]
Is this check going to unconditionally fail during live migration?

Userspace typically restores registers before the first KVM_RUN. However,
vgic_v5_lpi_ist_exists() requires the vmi structure to be present in the
vm_info XArray, which is only allocated in vgic_v5_vmte_init() (triggered by
the first KVM_RUN).

Will this cause -ENXIO to be returned when restoring GICV5_IRS_IST_BASER?

[ ... ]
> diff --git a/arch/arm64/kvm/vgic/vgic-v5-tables.c b/arch/arm64/kvm/vgic/vgic-v5-tables.c
> --- a/arch/arm64/kvm/vgic/vgic-v5-tables.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5-tables.c
[ ... ]
> @@ -604,6 +662,9 @@ int vgic_v5_vmte_release(struct kvm *kvm)
>  	if (!vmi)
>  		goto no_vmi;
>  
> +	/* Unlikely, but possible. Avoid leaking the memory. */
> +	vgic_v5_drain_pending_irqs(kvm, vmi, false);
> +
>  	/* If we have an LPI IST, free it */
>  	if (vmi->h_lpi_ist) {
>  		ret = vgic_v5_lpi_ist_free(kvm);

[Severity: Medium]
Does returning early from vgic_v5_vmte_release() cause a memory leak?

If vgic_v5_lpi_ist_free() or vgic_v5_spi_ist_free() fails, the function
returns immediately without freeing vmi->vped_base, vmi->vpet_base,
vmi->vmd_base, and the vmi structure itself.

Should these teardown paths make a best-effort attempt to free the remaining
resources rather than aborting?

[ ... ]
> diff --git a/arch/arm64/kvm/vgic/vgic-v5.c b/arch/arm64/kvm/vgic/vgic-v5.c
> --- a/arch/arm64/kvm/vgic/vgic-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5.c
[ ... ]
> @@ -1267,3 +1300,292 @@ void vgic_v5_save_state(struct kvm_vcpu *vcpu)
[ ... ]
> +	/* Save the SPI IST to the userspace buffer. */
> +	ret = vgic_v5_save_spi_ist(kvm, &ist_attr);
> +	if (ret)
> +		goto out_unlock;
> +
> +	ret = vgic_v5_irs_is_quiesced(vm_id);

[Severity: High]
Can the hardware corrupt the IST state during the read loop?

The vgic_v5_save_spi_ist() function reads from DMA memory. It is immediately
followed by a call to vgic_v5_irs_is_quiesced(), which uses writeq_relaxed()
to issue an MMIO write.

Since raw_spin_lock_irqsave() only provides an ACQUIRE barrier, is there
anything preventing the CPU from reordering the relaxed MMIO write before
the DMA reads complete? If reordered, the hardware could break quiescence and
modify the ISTs concurrently, while the software mistakenly concludes the VM
remained quiesced.

Would adding a dma_rmb() or rmb() ensure the DMA reads complete before
checking the hardware status?

[ ... ]
> +int vgic_v5_irs_restore_ists(struct kvm *kvm, struct kvm_device_attr *attr)
> +{
[ ... ]
> +	ret = vgic_v5_lpi_ist_exists(kvm);
> +	if (ret) {
> +		if (ret > 0)
> +			ret = -EBUSY;
> +		goto out_unlock;
> +	}

[Severity: High]
Does this have the same migration failure as vgic_v5_mmio_uaccess_write_irs()?

Since vmi hasn't been allocated by the first KVM_RUN yet,
vgic_v5_lpi_ist_exists() will return -ENXIO, causing the restoration to
fail unconditionally.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=35