Messages in this thread Patch in this message | | | From | "Limonciello, Mario" <> | Subject | RE: [PATCH] thunderbolt: Stop using iommu_present() | Date | Wed, 16 Mar 2022 14:49:45 +0000 |
| |
[Public]
> -----Original Message----- > From: Mika Westerberg <mika.westerberg@linux.intel.com> > Sent: Wednesday, March 16, 2022 07:45 > 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; Limonciello, > Mario <Mario.Limonciello@amd.com>; hch@lst.de > Subject: Re: [PATCH] thunderbolt: Stop using iommu_present() > > 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. > >
Suggest you include a link to the discussion(s) that spurred this too in commit message.
> > 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...
I did check that as a result of this: * Turning IOMMU to pass through leads to iommu_dma_protection of 0 * Leaving IOMMU enabled leads to iommu_dma_protection of 1
I suspect you'll respin this on the below comment, but if you do keep it: Tested-by: Mario Limonciello <mario.limonciello@amd.com>
> > > > 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".
I think below is what you are looking for (on top of your patch). This checks the NHI, and then also checks all those siblings Mika referred to.
diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c index 5f5fc5f6a09b..b17961ba1396 100644 --- a/drivers/thunderbolt/domain.c +++ b/drivers/thunderbolt/domain.c @@ -259,12 +259,25 @@ static ssize_t iommu_dma_protection_show(struct device *dev, { struct tb *tb = container_of(dev, struct tb, dev); struct iommu_domain *iod = iommu_get_domain_for_dev(&tb->nhi->pdev->dev); + struct device_link *link; + bool protected; + /* * Kernel DMA protection is a feature where Thunderbolt security is * handled natively using IOMMU. It is enabled when IOMMU is * enabled and actively enforcing translation. */ - return sprintf(buf, "%d\n", iod && iod->type != IOMMU_DOMAIN_IDENTITY); + protected = iod && iod->type != IOMMU_DOMAIN_IDENTITY; + if (protected) { + list_for_each_entry(link, &tb->nhi->pdev->dev.links.consumers, s_node) { + if (protected && pci_pcie_type(to_pci_dev(link->consumer)) == PCI_EXP_TYPE_ROOT_PORT) { + iod = iommu_get_domain_for_dev(link->consumer); + if (!iod || iod->type == IOMMU_DOMAIN_IDENTITY) + protected = false; + } + } + } + return sprintf(buf, "%d\n", protected); } static DEVICE_ATTR_RO(iommu_dma_protection);
| |