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