Re: [PATCH v5 7/9] vfio/pci: Clean up BAR zap and revocation

[Matt Evans <matt@ozlabs.org>]

Hola Alex,

On 12/08/2026 21:06, Alex Williamson wrote:
> On Tue, 11 Aug 2026 16:58:43 +0100
> Matt Evans <matt@ozlabs.org> wrote:
> 
>> Hi Alex,
>>
>> [snip]
>> Isn't the init-time 'event horizon' for writing the bitfield the
>> vfio_register_group_dev() in vfio_pci_core_register_device(), after
>> which synchronisation is needed?
>>
>> The hisi_acc_vfio_pci driver's .probe calls
>> vfio_pci_core_register_device() and _after that_ sets
>> zap_bars_on_revoke, and that's now in the "needs synchronisation to
>> write the bitfield" phase.
>>
>> (Re-reading my comment in vfio_pci_core_register_device() I'd noted
>> this, "Drivers can opt out after registration".  Has to be done after by
>> definition as the default's set in vfio_pci_core_register_device().)
>>
>> The concern is just blatting neighbours in the bitfield, not the window
>> of time before the flag's set.  The flag's an opt-out of a safe but (for
>> this driver) unnecessary zap, so having it unset for a short time is OK.
>>
>> I still think this really should be a standalone bool, not a bit in the
>> bitfield.  It has to be set after registration and having to take a lock
>> to do that has downsides.
> 
> You're right on the ordering, the device is live after
> vfio_pci_core_register_device().  However, I think that's evidence that
> vfio-pci-core is setting the default polarity, inferred from the mmap
> op, in the wrong place.  It should happen in init, not register_device.
> 
> The same mmap op pointer is available in vfio_pci_core_init_dev(), which

Ahaa, they're passed into vfio_alloc_device()!

> is used by all vfio-pci variant drivers in their init callback.  The
> proposed vfio_pci_core_register_device() change just needs to be lifted
> into vfio_pci_core_init_dev().  hisi_acc is then modified to fix the
> flag after vfio_pci_core_init_dev(), something like below.
> 
> I think that's better than anticipating it being dynamic when we don't
> have a use case that requires it.  Thanks,

Yes, that works nicely.  Done.  As ever, thanks for the suggestion.


Matt


PS: Series v6 has several improvements/review fixes ready, but I'm 
holding off posting it.  It'd depend on resolving the awful deadlock I 
posted about on patch [4/9].




> 
> Alex
> 
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> @@ -1564,6 +1564,7 @@ static int hisi_acc_vfio_pci_migrn_init_dev(struct vfio_device *core_vdev)
>          struct hisi_acc_vf_core_device *hisi_acc_vdev = hisi_acc_get_vf_dev(core_vdev);
>          struct pci_dev *pdev = to_pci_dev(core_vdev->dev);
>          struct hisi_qm *pf_qm = hisi_acc_get_pf_qm(pdev);
> +       int ret;
>   
>          hisi_acc_vdev->vf_id = pci_iov_vf_id(pdev) + 1;
>          hisi_acc_vdev->pf_qm = pf_qm;
> @@ -1575,7 +1576,9 @@ static int hisi_acc_vfio_pci_migrn_init_dev(struct vfio_device *core_vdev)
>          core_vdev->migration_flags = VFIO_MIGRATION_STOP_COPY | VFIO_MIGRATION_PRE_COPY;
>          core_vdev->mig_ops = &hisi_acc_vfio_pci_migrn_state_ops;
>   
> -       return vfio_pci_core_init_dev(core_vdev);
> +       ret = vfio_pci_core_init_dev(core_vdev);
> +       hisi_acc_vdev->core_device.zap_bars_on_revoke = false;
> +       return ret;
>   }
>   
>   static const struct vfio_device_ops hisi_acc_vfio_pci_migrn_ops = {