Re: [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.