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. Thanks, Matt
