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

[Jan Beulich <jbeulich@suse.com>]

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 <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

Jan