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.
Not for all domains other than DomIO. And DomIO is, well, special. As is at what times quarantine_init() is actually being invoked. > 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. That wouldn't suffice then. As said in the description, and as discussed during the development of the XSA-400 series, further locking would need adding then. Jan
