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

On 2026/8/5 11:15, Alex Williamson wrote:
> 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.
>

You are right. With if (!pdev->is_virtfn), the PF is not passed through,
and those migration-internal functions are indeed unreachable.
We will remove these redundant internal guard checks in the next revision,
and keep checks only at the three real entry points:
probe, the AER/reset callbacks (hisi_acc_vf_pci_reset_prepare / hisi_acc_vf_pci_aer_reset_done),
and the migration .init (hisi_acc_vfio_pci_migrn_init_dev).

Thanks.
Longfang!

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