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

On Wed, 5 Aug 2026 10:19:35 +0800
liulongfang <[email protected]> wrote:

> On 2026/8/5 3:28, Alex Williamson wrote:
> > On Mon, 3 Aug 2026 10:18:56 +0800
> > Longfang Liu <[email protected]> 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?
> >  
> 
> This Hisilicon live migration driver actually utilizes two hardware-related
> configuration functions. The first is the PF control function, obtained directly
> through hisi_acc_get_pf_qm, which handles mailbox command operations, device health
> status checks, device reset verification, device stop commands, and other device
> control processes. The second is the VF configuration function, which is passed
> through via VFIO direct assignment and serves as the main entity for device live
> migration, responsible for current service device data migration and recovery operations.
> 
> The aforementioned issue occurs when users incorrectly pass the PF directly to this
> driver through driver_override. In this scenario, the vf_dev in the driver structure
> erroneously points to this PF. When the PF attempts to migrate itself, it will directly
> cause exceptions. Therefore, it's necessary to add pdev checks here.

This doesn't answer the question.  How does binding the PF to the
driver pass the existing is_virtfn test in hisi_acc_get_pf_qm() in
order to map the migration ops structure to the device rather than the
default vfio-pci-core wrapper ops structure?

vf_dev is ONLY set in hisi_acc_vfio_pci_migrn_init_dev(), which is
called through the migration ops structure.  If the PF is not mapped to
the migration ops structure it CANNOT set vf_dev.

Your own patch below gates the mapping of the migration ops on
is_virtfn, so we know this field is correct for the PF.  Therefore
hisi_acc_get_pf_qm() already returns NULL for the PF.  Therefore the
vfio-pci-core wrapper ops are used for the PF.  Therefore vf_dev is
never set and most of the functions being modified in the name of
correcting falsely advertised migration support on the PF (which seems
untrue) are not reachable.

Alex

> >> Fixes: b0eed085903e ("hisi_acc_vfio_pci: Add support for VFIO live migration")
> >> Signed-off-by: Longfang Liu <[email protected]>
> >> ---
> >>  .../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
> > .
> >