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

[liulongfang]

On 2026/9/18 11:10, Alex Williamson wrote:
> 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.
>

Yes, in the reset_done function, the existing mig_ops check can intercept abnormal PF devices.

>>
>> 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,
> 

After intercepting live migration support for 64K page systems in probe, using the
`is_virtfn` check allows us to eliminate some redundant code and simplify the handling.
I've posted the new revision; please take a look.

Thanks.
Longfang.

> Alex
> .
>