On Tue, Apr 12, 2022 at 02:17:16PM +0200, Jan Beulich wrote: > 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.
Right, but at the point where fill_qpt() gets called hd->arch.amd.root_table == NULL, and hence it seems completely pointless to wrap this in a mapping_lock locked region. > 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. The lock would need to be held if pages tables are modified while being set in hd->arch.amd.root_table, or at least that's my understanding. This is a special case anyway, as the page tables are not per-domain but per-device, but it seems clear to me that if the page tables are not set in hd->arch.amd.root_table the lock in hd->arch.mapping_lock is not supposed to be protecting them. > > 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. I think it's likely we will need such lock for other purposes if we ever manage to convert the pcidevs lock into a rwlock, so my comment was not so much as it's required for the use case here, but a side effect if we ever manage to change pcidevs lock. Thanks, Roger.
