Re: [PATCH v2 1/2] hisi_acc_vfio_pci: fix live migration enable conditions for PF passthrough

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

Pre-existing issues:
- [High] The `set_reset_flag` lock tracker is never reset to false after a successful lock acquisition, allowing subsequent failed resets to erroneously clear the global `QM_RESETTING` lock.
--

--- Patch [1]: [PATCH v2 1/2] hisi_acc_vfio_pci: fix live migration enable conditions for PF passthrough ---
commit e8e63b8a17edd44cc9b69e8ca3472ee6c656873c
Author: Longfang Liu <[email protected]>

hisi_acc_vfio_pci: fix live migration enable conditions for PF passthrough

This commit fixes a crash in PF passthrough mode by adding validity
checks for uninitialized PF pointers during device probe, error
handling, and migration initialization.

> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> index 86362ec424a50..e95d0ab0f11ab 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
[ ... ]
> @@ -1174,8 +1179,12 @@ static void hisi_acc_vf_pci_aer_reset_done(struct pci_dev *pdev)
>  	struct hisi_acc_vf_core_device *hisi_acc_vdev = hisi_acc_drvdata(pdev);
>  	struct hisi_qm *qm = hisi_acc_vdev->pf_qm;
>  
> -	if (hisi_acc_vdev->set_reset_flag)
> -		clear_bit(QM_RESETTING, &qm->misc_ctl);
> +	if (hisi_acc_vdev->set_reset_flag) {
> +		if (qm && qm->io_base)
> +			clear_bit(QM_RESETTING, &qm->misc_ctl);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible that
hisi_acc_vdev->set_reset_flag is never reset to false after a successful
lock acquisition?

If a prior reset succeeds, hisi_acc_vdev->set_reset_flag is left as true.

If a subsequent reset attempt times out in hisi_acc_vf_pci_reset_prepare()
because another agent already holds the QM_RESETTING lock:

hisi_acc_vf_pci_reset_prepare() {
    ...
    while (test_and_set_bit(QM_RESETTING, &qm->misc_ctl)) {
        msleep(1);
        if (++delay > QM_RESET_WAIT_TIMEOUT) {
            dev_err(dev, "reset prepare failed\n");
            return;
        }
    }
    ...
}

The code returns early and leaves set_reset_flag as true.

When hisi_acc_vf_pci_aer_reset_done() is subsequently invoked by the PCI
error recovery flow, it observes the stale true flag and unconditionally
clears QM_RESETTING.

Does this prematurely clear the lock currently owned by the other agent?

> +		else
> +			dev_err(&pdev->dev, "PF QM not available for reset done\n");
> +	}
>  
>  	if (!hisi_acc_vdev->core_device.vdev.mig_ops)
>  		return;

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