Re: [PATCH v2] RISC-V: KVM: Serialize IMSIC attributes with vCPU migration

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Silent loss of attribute writes due to un-serialized CPU hot-unplug. The patch relies on `vcpu->mutex` to protect the VS-file, but `kvm_riscv_aia_disable()` bypasses this mutex during CPU offline.
--

--- Patch [1]: [PATCH v2] RISC-V: KVM: Serialize IMSIC attributes with vCPU migration ---
commit 27600b866db246ae439be14c51b6bb1e480a817e
Author: Xie Bo <[email protected]>

RISC-V: KVM: Serialize IMSIC attributes with vCPU migration

This commit serializes KVM device ioctls for IMSIC attributes against
KVM_RUN by acquiring the target vCPU mutex in kvm_riscv_aia_imsic_rw_attr().
This aims to prevent the VS-file from being migrated and recycled while
the attribute is being accessed.

Link: https://lore.kernel.org/r/[email protected]

> diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
> --- a/arch/riscv/kvm/aia_imsic.c
> +++ b/arch/riscv/kvm/aia_imsic.c

[ ... ]

> @@ -995,6 +1000,8 @@ int kvm_riscv_aia_imsic_rw_attr(struct kvm *kvm, unsigned long type,
>  		rc = imsic_vsfile_rw(vsfile_hgei, vsfile_cpu, imsic->nr_eix,
>  				     isel, write, val);

[Severity: High]
This is a pre-existing issue, but does relying on vcpu->mutex here leave a
race window with CPU hot-unplug?

If a concurrent KVM_SET_DEVICE_ATTR resolves the hardware HGEI and drops
imsic->vsfile_lock, couldn't a simultaneous CPU hot-unplug event cause
kvm_riscv_aia_disable() to run on the offlining CPU?

Looking at the CPU offline path in arch/riscv/kvm/aia.c:

kvm_riscv_aia_disable() {
    ...
    /* Notify IMSIC */
    kvm_riscv_vcpu_aia_imsic_release(vcpu);
    ...
}

This bypasses vcpu->mutex, locklessly flushes the vCPU's HGEI to the SW
file, and frees the HGEI.

If that happens before the IPI is sent in imsic_vsfile_rw(), would we end up
modifying the freed hardware HGEI instead of the SW file, or failing to send
the IPI entirely if the CPU goes fully offline?

Could this result in silently losing attribute updates during live migration?

> +out_unlock:
> +	mutex_unlock(&vcpu->mutex);
>  	return rc;
>  }

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