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

[Alex Williamson]

On Mon, 3 Aug 2026 10:18:56 +0800
Longfang Liu <liulongfang@xxxxxxxxxx> wrote:

>     In the previous implementation of live migration support for
> Hisilicon accelerator devices, there was insufficient consideration
> for the fact that PFs cannot support virtualization live migration.
> If a user unbinds the PF device driver from the host and directly
> passes it through to a VM, then attempts a live migration operation,
> it will trigger a calltrace exception.
> 
>     To address this, we conducted a detailed analysis of potential
> failure points. We added checks for all operations that depend on
> PF driver commands and incorporated relevant conditional judgments
> to prevent system calltrace exceptions when users attempt live
> migration after passing PFs through to VMs.

I don't understand your core premise here.  hisi_acc_vfio_pci_probe()
sets the default ops to hisi_acc_vfio_pci_ops.  This ops structure uses
vfio-pci-core callbacks for everything except .open_device, for which it
uses hisi_acc_vfio_pci_open_device().  This function has exactly one
migration related branch, which is entered only when core_vdev->mig_ops
is set, but mig_ops is only set in the .init callback of the ops
structure supporting migration.

In order to get the migration ops structure, hisi_acc_get_pf_qm() must
return a pf_qm, the version of that object must be at least QM_HW_V3,
and the vf_id must be valid.  hisi_acc_get_pf_qm()'s very first action
is:

        if (!pdev->is_virtfn)
                return NULL;

Therefore, how is a PF ever getting associated to the migration ops
structure?
 
> Fixes: b0eed085903e ("hisi_acc_vfio_pci: Add support for VFIO live migration")
> Signed-off-by: Longfang Liu <liulongfang@xxxxxxxxxx>
> ---
>  .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c    | 48 +++++++++++++++----
>  1 file changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> index 86362ec424a5..36490be7a61a 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> @@ -378,6 +378,11 @@ static int vf_qm_check_match(struct hisi_acc_vf_core_device *hisi_acc_vdev,
>  	if (migf->total_length < QM_MATCH_SIZE || hisi_acc_vdev->match_done)
>  		return 0;
>  
> +	if (!pf_qm || !pf_qm->io_base) {
> +		dev_err(dev, "failed to match check for PF QM migration\n");
> +		return -ENODEV;
> +	}
> +

This function is called by hisi_acc_vf_resume_write(), which is part of
hisi_acc_vf_resume_fops, which is set as the file ops for the migration file created in hisi_acc_vf_pci_resume().  The call path is:

hisi_acc_vfio_pci_migrn_state_ops.migration_set_state (hisi_acc_vfio_pci_set_device_state())
  hisi_acc_vf_set_device_state()
    hisi_acc_vf_pci_resume()

hisi_acc_vfio_pci_migrn_state_ops is mig_ops.  It's never set for a PF!

>  	ret = vf_qm_version_check(vf_data, dev);
>  	if (ret) {
>  		dev_err(dev, "failed to match ACC_DEV_MAGIC\n");
> @@ -423,10 +428,15 @@ static int vf_qm_get_match_data(struct hisi_acc_vf_core_device *hisi_acc_vdev,
>  				struct acc_vf_data *vf_data)
>  {
>  	struct hisi_qm *pf_qm = hisi_acc_vdev->pf_qm;
> -	struct device *dev = &pf_qm->pdev->dev;
> +	struct device *dev = &hisi_acc_vdev->vf_dev->dev;
>  	int vf_id = hisi_acc_vdev->vf_id;
>  	int ret;
>  
> +	if (!pf_qm || !pf_qm->io_base) {
> +		dev_err(dev, "failed to check PF QM available!\n");
> +		return -ENODEV;
> +	}
> +
>  	vf_data->acc_magic = ACC_DEV_MAGIC_V2;
>  	vf_data->major_ver = ACC_DRV_MAJOR_VER;
>  	vf_data->minor_ver = ACC_DRV_MINOR_VER;

This is called from opening the migration save file.  Prove how a PF
can get here.


> @@ -601,9 +611,14 @@ hisi_acc_check_int_state(struct hisi_acc_vf_core_device *hisi_acc_vdev)
>  	struct hisi_qm *vfqm = &hisi_acc_vdev->vf_qm;
>  	struct hisi_qm *qm = hisi_acc_vdev->pf_qm;
>  	struct pci_dev *vf_pdev = hisi_acc_vdev->vf_dev;
> -	struct device *dev = &qm->pdev->dev;
> +	struct device *dev = &vf_pdev->dev;
>  	u32 state;
>  
> +	if (!qm || !qm->io_base) {
> +		dev_err(dev, "failed to interrupt state check for PF QM!\n");
> +		return -ENODEV;
> +	}
> +
>  	/* Check RAS state */
>  	state = qm_check_reg_state(qm, QM_ABNORMAL_INT_STATUS);
>  	if (state) {

This is called from hisi_acc_vf_stop_device(), the same set_device path
as above.  A PF cannot get here!

> @@ -1154,9 +1169,14 @@ static void hisi_acc_vf_pci_reset_prepare(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;
> -	struct device *dev = &qm->pdev->dev;
> +	struct device *dev = &pdev->dev;
>  	u32 delay = 0;
>  
> +	if (!qm || !qm->io_base) {
> +		dev_err(dev, "PF QM not available for reset\n");
> +		return;
> +	}
> +
>  	/* All reset requests need to be queued for processing */
>  	while (test_and_set_bit(QM_RESETTING, &qm->misc_ctl)) {
>  		msleep(1);

Finally, something that matches the fix this patch claims, a function
reachable by the PF!

Is this actually the issue you're trying to fix, not a migration
induced fault, migration isn't reachable by a PF, but an error handling
bug?

> @@ -1174,8 +1194,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);
> +		else
> +			dev_err(&pdev->dev, "PF QM not available for reset done\n");
> +	}
>  
>  	if (!hisi_acc_vdev->core_device.vdev.mig_ops)
>  		return;

Another, but notice you've already accounted for the non-migration case
here.  Can they be combined?

> @@ -1193,6 +1217,11 @@ static int hisi_acc_vf_qm_init(struct hisi_acc_vf_core_device *hisi_acc_vdev)
>  	struct pci_dev *vf_dev = vdev->pdev;
>  	u32 val;
>  
> +	if (!pf_qm || !pf_qm->io_base) {
> +		dev_err(&vf_dev->dev, "PF QM not available for init\n");
> +		return -ENODEV;
> +	}
> +
>  	val = readl(pf_qm->io_base + QM_MIG_REGION_SEL);
>  	if (pf_qm->ver > QM_HW_V3 && (val & QM_MIG_REGION_EN))
>  		hisi_acc_vdev->drv_mode = HW_ACC_MIG_PF_CTRL;

Unreachable by PF.

> @@ -1565,6 +1594,11 @@ static int hisi_acc_vfio_pci_migrn_init_dev(struct vfio_device *core_vdev)
>  	struct pci_dev *pdev = to_pci_dev(core_vdev->dev);
>  	struct hisi_qm *pf_qm = hisi_acc_get_pf_qm(pdev);
>  
> +	if (!pf_qm) {
> +		dev_err(&pdev->dev, "PF driver not loaded, cannot enable migration\n");
> +		return -ENODEV;
> +	}
> +
>  	hisi_acc_vdev->vf_id = pci_iov_vf_id(pdev) + 1;
>  	hisi_acc_vdev->pf_qm = pf_qm;
>  	hisi_acc_vdev->vf_dev = pdev;

Unreachable by PF.

> @@ -1670,13 +1704,11 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device
>  	struct hisi_acc_vf_core_device *hisi_acc_vdev;
>  	const struct vfio_device_ops *ops = &hisi_acc_vfio_pci_ops;
>  	struct hisi_qm *pf_qm;
> -	int vf_id;
>  	int ret;
>  
>  	pf_qm = hisi_acc_get_pf_qm(pdev);
>  	if (pf_qm && pf_qm->ver >= QM_HW_V3) {
> -		vf_id = pci_iov_vf_id(pdev);
> -		if (vf_id >= 0)
> +		if (pdev->is_virtfn)
>  			ops = &hisi_acc_vfio_pci_migrn_ops;
>  		else
>  			pci_warn(pdev, "migration support failed, continue with generic interface\n");

Redundant to the test in hisi_acc_get_pf_pm() which would have returned
NULL, so we can't even get here.

Please do better.  Thanks,

Alex