On 11.04.2022 12:01, Andrew Cooper wrote: > On 11/04/2022 10:35, Jan Beulich wrote: >> Prior extension of these functions to enable per-device quarantine page >> tables already didn't add more locking there, but merely left in place >> what had been there before. But really locking is unnecessary here: >> We're running with pcidevs_lock held (i.e. multiple invocations of the >> same function [or their teardown equivalents] are impossible, and hence >> there are no "local" races), while all consuming of the data being >> populated here can't race anyway due to happening sequentially >> afterwards. See also the comment in struct arch_pci_dev. >> >> Signed-off-by: Jan Beulich <jbeul...@suse.com> > > It is only legitimate to drop these calls if you delete the mapping_lock > entirely. Otherwise you're breaking the semantics of mapping_lock. > > Your argument of "well this is already guarded by the pci lock" means > that these are uncontended lock/unlock operations, and therefore not > interesting to drop in the first place. > > This patch is specifically setting us up for an XSA in the future when > the behaviour of the the PCI lock changes, the fix for which will be > revert this patch.
Further to my earlier reply, may I remind you that changes to the PCI lock won't go unnoticed here, as there are respective ASSERT()s in place. Jan
