Hi Alex,
On 04/08/2026 21:10, Alex Williamson wrote:
On Thu, 30 Jul 2026 15:47:13 +0100 Matt Evans matt@ozlabs.org wrote:
Hi Alex,
On 29/07/2026 18:52, Alex Williamson wrote:
On Wed, 15 Jul 2026 18:47:30 +0100 Matt Evans matt@ozlabs.org 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