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

[Alex Williamson]

On Thu, 17 Sep 2026 14:13:31 +0800
liulongfang <liulongfang@xxxxxxxxxx> wrote:

> On 2026/9/12 1:37, Alex Williamson wrote:
> > On Mon, 31 Aug 2026 17:09:49 +0800
> > Longfang Liu <liulongfang@xxxxxxxxxx> wrote:
> >   
> >> When a PF device is bound to the live migration driver in passthrough mode,
> >> it cannot support live migration functionality, and key pointers will
> >> remain uninitialized. Although most migration functions within the driver
> >> are unreachable, low-level error handling callbacks may be triggered
> >> directly, causing a crash due to null pointer dereference.
> >> The fix involves adding validity checks at three entry points: device
> >> probe, error handling, and migration initialization. If the pointer is
> >> invalid, the operation is exited or rejected directly to avoid crashes,
> >> while redundant internal checks are removed.
> >>
> >> 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    | 24 ++++++++++++++-----
> >>  1 file changed, 18 insertions(+), 6 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..e95d0ab0f11a 100644
> >> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> >> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> >> @@ -1154,9 +1154,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);
> >> @@ -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);
> >> +		else
> >> +			dev_err(&pdev->dev, "PF QM not available for reset done\n");
> >> +	}  
> > 
> > set_reset_flag is only set by reset_prepare, which per the previous
> > chunk can only occur if qm && qm->io_base, so this chunk is redundant.
> > Why not just promote the mig_ops tests in both?
> >   
> >>  
> >>  	if (!hisi_acc_vdev->core_device.vdev.mig_ops)
> >>  		return;
> >> @@ -1565,6 +1574,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;
> >> +	}  
> > 
> > This function is only reached via hisi_acc_vfio_pci_migrn_ops, which is
> > already validated in probe to have a pf_qm with version >= QM_HW_V3.
> >   
> >> +
> >>  	hisi_acc_vdev->vf_id = pci_iov_vf_id(pdev) + 1;
> >>  	hisi_acc_vdev->pf_qm = pf_qm;
> >>  	hisi_acc_vdev->vf_dev = pdev;
> >> @@ -1670,13 +1684,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");  
> > 
> > This is not reachable as a VF:
> > 
> > static struct hisi_qm *hisi_acc_get_pf_qm(struct pci_dev *pdev)
> > {
> >         struct hisi_qm  *pf_qm;
> >         struct pci_driver *pf_driver;
> > 
> >         if (!pdev->is_virtfn)
> >                 return NULL;
> > 
> > 
> > pf_qm is NULL, the branch is never taken for a PF.  Also:
> > 
> > int pci_iov_vf_id(struct pci_dev *dev)
> > {
> >         struct pci_dev *pf;
> > 
> >         if (!dev->is_virtfn)
> >                 return -EINVAL;
> > 
> > So even the redundant test is already here.  What are you trying to
> > accomplish in this chunk?  Thanks,
> >  
> 
> Thank you for the review. Regarding the first two checks in `aer_reset_done` and `migrn_init_dev`,
> you are correct that they are redundant; we will remove them in the next revision.
> 
> However, the PF passthrough scenario still crashes. The calltrace we actually tested occurs on
> the following path:

I didn't argue this fact, I only noted the .reset_done fix is redundant
and suggested a better ordering.

> 
> hisi_acc_vfio_pci_open_device
>   → vfio_pci_core_enable
>     → pci_try_reset_function
>       → hisi_acc_vf_pci_reset_prepare   ← NULL pointer dereference
> 
> 
> `reset_prepare` is a PCI error handler callback invoked by `pci_try_reset_function` inside
> `vfio_pci_core_enable`, which happens before the `mig_ops` check in `open_device`.
> At this point, the PF's `pf_qm` is NULL, and `reset_prepare` dereferences `&qm->pdev->dev`,
> causing an oops:
> 
> 
> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
> pc : hisi_acc_vf_pci_reset_prepare+0x34/0xc0
> Call trace:
>  hisi_acc_vf_pci_reset_prepare
>  pci_dev_save_and_disable
>  pci_try_reset_function
>  vfio_pci_core_enable
>  hisi_acc_vfio_pci_open_device
>  vfio_df_open
> 
> 
> This shows that the `vf_id`-based check in `probe` does not actually prevent the PF
> from reaching `reset_prepare`;
> the PF still triggers the calltrace through this path. The fix here will follow your
> suggestion: elevate the `mig_ops` check at the entry of `reset_prepare`, and also
> move the existing `mig_ops` check in `aer_reset_done` ahead of the `set_reset_flag`
> handling.
> The `is_virtfn` check in `probe` will be retained to maintain consistent semantics with
> other vendor drivers.

I don't see this provides any consistency with other drivers, only the
virtio driver has a is_virtfn test and it is the fundamental test that
driver uses to select the migration ops.  It's not comparable to this
use case.  Thanks,

Alex