On 12.04.2022 13:05, Roger Pau Monné wrote: > On Mon, Apr 11, 2022 at 11:35:54AM +0200, 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. > > I would explicitly say that none of the code in the locked region > touches any data in the domain_iommu struct, so taking the > mapping_lock is unneeded.
But that would limit what the mapping_lock protects more than it actually does: The entire page tables hanging off of the root table are also protected by that lock. It's just that for a pdev, after having installed identity mappings, the root doesn't hang off of hd. But in principle - i.e. if the per-device mappings weren't static once created - the lock would be the one to hold whenever any of these page tables was modified. > Long term we might wish to implemented a per-device lock that could be > used here, instead of relying on the pcidevs lock. Well, I would want to avoid this unless a need arises to not hold the pcidevs lock here. Or, of course, if a need arose to dynamically alter these page tables. >> Signed-off-by: Jan Beulich <jbeul...@suse.com> > > Reviewed-by: Roger Pau Monné <roger....@citrix.com> Thanks. Jan
