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

On Tue, 11 Aug 2026 16:58:43 +0100
Matt Evans <[email protected]> wrote:

> Hi Alex,
> 
> On 04/08/2026 21:10, Alex Williamson wrote:
> > On Thu, 30 Jul 2026 15:47:13 +0100
> > Matt Evans <[email protected]> wrote:
> >   
> >> Hi Alex,
> >>
> >> On 29/07/2026 18:52, Alex Williamson wrote:  
> >>> On Wed, 15 Jul 2026 18:47:30 +0100
> >>> Matt Evans <[email protected]> wrote:    
> >>>> diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
> >>>> index 9a1674c152aa..e2b4252e7c3f 100644
> >>>> --- a/include/linux/vfio_pci_core.h
> >>>> +++ b/include/linux/vfio_pci_core.h
> >>>> @@ -134,6 +134,7 @@ struct vfio_pci_core_device {
> >>>>  	bool			pm_intx_masked;
> >>>>  	bool			pm_runtime_engaged;
> >>>>  	bool			sriov_active;
> >>>> +	bool			zap_bars_on_revoke;
> >>>>  	struct pci_saved_state	*pci_saved_state;
> >>>>  	struct pci_saved_state	*pm_save;
> >>>>  	int			ioeventfds_nr;    
> >>>
> >>> This should be in the bitfield usage group since it's only modified at
> >>> init time.    
> >>
> >> This was intentional, but happy to change it if you're certain ofc.  Is
> >> it inconceivable that a sub-driver could set it after init?  I'd say
> >> they _shouldn't_, but only review will stop them and this placement
> >> intended to be cautious.  It seemed a low cost way to avoid issues
> >> around synchronisation on the bitfield.  
> > 
> > I'd agree with the statement that they shouldn't, it would be difficult
> > to synchronize setting the flag once there are any active mappings of
> > the BARs.  Also, if we put it in the bitfield category under the
> > comment that the value is only modified at setup/release, it documents
> > the intentions, hopefully to the extent the author or reviewers notice.
> > An argument can always be made to change it if there's a worthwhile use
> > case.  Thanks,  
> 
> (Having noticed in moving this to the bitfield; my previous reply didn't
> supply the real reason it wasn't in the bitfield.)
> 
> 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
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,

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 = {