Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 16 Mar 2022 14:45:11 +0200
From:   Mika Westerberg <mika.westerberg@linux.intel.com>
To:     Robin Murphy <robin.murphy@arm.com>
Cc:     andreas.noever@gmail.com, michael.jamet@intel.com,
        YehezkelShB@gmail.com, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org,
        mario.limonciello@amd.com, hch@lst.de
Subject: Re: [PATCH] thunderbolt: Stop using iommu_present()
Message-ID: <YjHb1xCx4UAmUjrR@lahna>
References: <b4356b228db9cb88d12db6559e28714ce26e022e.1647429348.git.robin.murphy@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b4356b228db9cb88d12db6559e28714ce26e022e.1647429348.git.robin.murphy@arm.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

Hi Robin,

On Wed, Mar 16, 2022 at 11:25:51AM +0000, Robin Murphy wrote:
> Even if an IOMMU might be present for some PCI segment in the system,
> that doesn't necessarily mean it provides translation for the device
> we care about. Furthermore, the presence or not of one firmware flag
> doesn't imply anything about the IOMMU driver's behaviour, which may
> still depend on other firmware properties and kernel options too. What
> actually matters is whether an IOMMU is enforcing protection for our
> device - regardless of whether that stemmed from firmware policy, kernel
> config, or user control - at the point we need to decide whether to
> authorise it. We can ascertain that generically by simply looking at
> whether we're currently attached to a translation domain or not.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> I don't have the means to test this, but I'm at least 80% confident
> in my unpicking of the structures to retrieve the correct device...
> 
>  drivers/thunderbolt/domain.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
> index 7018d959f775..5f5fc5f6a09b 100644
> --- a/drivers/thunderbolt/domain.c
> +++ b/drivers/thunderbolt/domain.c
> @@ -257,13 +257,14 @@ static ssize_t iommu_dma_protection_show(struct device *dev,
>  					 struct device_attribute *attr,
>  					 char *buf)
>  {
> +	struct tb *tb = container_of(dev, struct tb, dev);
> +	struct iommu_domain *iod = iommu_get_domain_for_dev(&tb->nhi->pdev->dev);

I wonder if this is the correct "domain"? I mean it's typically no the
Thunderbolt controller (here tb->nhi->pdev->dev) that needs the
protection (although in discrete controllers it does get it too) but
it's the tunneled PCIe topology that we need to check here.

For instance in Intel with intergrated Thunderbolt we have topology like
this:

  Host bridge
      |
      +--- Tunneled PCIe root port #1
      +--- Tunneled PCIe root port #2
      +--- Thunderbolt host controller (the NHI above)
      +--- xHCI

and In case of discrete controllers it looks like this:

  Host bridge
      |
      +--- PCIe root port #x
                |
                |
           PCIe switch upstream port
                |
	        +--- Tunneled PCIe switch downstream port #1
	        +--- Tunneled PCIe switch downstream port #2
        	+--- Thunderbolt host controller (the NHI above)
        	+--- xHCI

What we want is to make sure the Tunneled PCIe ports get the full IOMMU
protection. In case of the discrete above it is also fine if all the
devices behind the PCIe root port get the full IOMMU protection. Note in
the integrated all the devices are "siblings".