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

[liulongfang]

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:


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.

Thanks.
Longfang.


> Alex
> .
>