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

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