On 12.04.2022 14:54, Roger Pau Monné wrote: > 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.
There are multiple models possible, one being that for per-device page tables DomIO's lock protects all of them. Hence my hesitance to say something along these lines in the description. >>> 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. Such a need would further depend on whether the code paths leading here would hold the lock in read or write mode. But yes, it is reasonable to expect that it would only be read mode. Jan
