Messages in this thread | | | Date | Fri, 18 Mar 2022 12:01:42 +0000 | Subject | Re: [PATCH] thunderbolt: Make iommu_dma_protection more accurate | From | Robin Murphy <> |
| |
On 2022-03-18 11:38, mika.westerberg@linux.intel.com wrote: > Hi Mario, > > On Thu, Mar 17, 2022 at 08:36:13PM +0000, Limonciello, Mario wrote: >> Here is a proposal on top of what you did for this. >> The idea being check the ports right when the links are made if they exist >> (all the new USB4 stuff) and then check all siblings on TBT3 stuff. >> >> diff --git a/drivers/thunderbolt/acpi.c b/drivers/thunderbolt/acpi.c >> index 79b5abf9d042..89432456dbea 100644 >> --- a/drivers/thunderbolt/acpi.c >> +++ b/drivers/thunderbolt/acpi.c >> @@ -14,6 +14,7 @@ >> static acpi_status tb_acpi_add_link(acpi_handle handle, u32 level, void *data, >> void **return_value) >> { >> + enum nhi_iommu_status iommu_status = IOMMU_UNKNOWN; >> struct fwnode_reference_args args; >> struct fwnode_handle *fwnode; >> struct tb_nhi *nhi = data; >> @@ -91,6 +92,8 @@ static acpi_status tb_acpi_add_link(acpi_handle handle, u32 level, void *data, >> if (link) { >> dev_dbg(&nhi->pdev->dev, "created link from %s\n", >> dev_name(&pdev->dev)); >> + if (iommu_status != IOMMU_DISABLED) >> + iommu_status = nhi_check_iommu_for_port(pdev); >> } else { >> dev_warn(&nhi->pdev->dev, "device link creation from %s failed\n", >> dev_name(&pdev->dev)); >> @@ -101,6 +104,7 @@ static acpi_status tb_acpi_add_link(acpi_handle handle, u32 level, void *data, >> >> out_put: >> fwnode_handle_put(args.fwnode); >> + nhi->iommu_dma_protection = (iommu_status == IOMMU_ENABLED); >> return AE_OK; >> } >> >> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c >> index e12c2e266741..b5eb0cab392f 100644 >> --- a/drivers/thunderbolt/nhi.c >> +++ b/drivers/thunderbolt/nhi.c >> @@ -1103,10 +1103,30 @@ static void nhi_check_quirks(struct tb_nhi *nhi) >> nhi->quirks |= QUIRK_AUTO_CLEAR_INT; >> } >> >> +enum nhi_iommu_status nhi_check_iommu_for_port(struct pci_dev *pdev) >> +{ >> + if (!pci_is_pcie(pdev) || >> + !(pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT || >> + pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)) { >> + return IOMMU_UNKNOWN; >> + } >> + >> + if (!device_iommu_mapped(&pdev->dev)) { >> + return IOMMU_DISABLED; >> + } >> + >> + if (!pdev->untrusted) { >> + dev_info(&pdev->dev, >> + "Assuming unreliable Kernel DMA protection\n"); >> + return IOMMU_DISABLED; >> + } >> + return IOMMU_ENABLED; >> +} >> + >> static void nhi_check_iommu(struct tb_nhi *nhi) >> { >> - struct pci_dev *pdev; >> - bool port_ok = false; >> + enum nhi_iommu_status iommu_status = nhi->iommu_dma_protection ? >> + IOMMU_ENABLED : IOMMU_UNKNOWN; >> >> /* >> * Check for sibling devices that look like they should be our >> @@ -1117,23 +1137,13 @@ static void nhi_check_iommu(struct tb_nhi *nhi) >> * otherwise even if translation is enabled for existing devices it >> * may potentially be overridden for a future tunnelled endpoint. >> */ >> - for_each_pci_bridge(pdev, nhi->pdev->bus) { >> - if (!pci_is_pcie(pdev) || >> - !(pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT || >> - pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)) >> - continue; >> - >> - if (!device_iommu_mapped(&pdev->dev)) >> - return; >> - >> - if (!pdev->untrusted) { >> - dev_info(&nhi->pdev->dev, >> - "Assuming unreliable Kernel DMA protection\n"); >> - return; >> - } >> - port_ok = true; >> + if (iommu_status == IOMMU_UNKNOWN) { >> + struct pci_dev *pdev; >> + for_each_pci_bridge(pdev, nhi->pdev->bus) >> + if (iommu_status != IOMMU_DISABLED) >> + iommu_status = nhi_check_iommu_for_port(pdev); >> } >> - nhi->iommu_dma_protection = port_ok; >> + nhi->iommu_dma_protection = (iommu_status == IOMMU_ENABLED); >> } >> >> static int nhi_init_msi(struct tb_nhi *nhi) >> >> diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h >> index 69083aab2736..1622d49b1763 100644 >> --- a/drivers/thunderbolt/nhi.h >> +++ b/drivers/thunderbolt/nhi.h >> @@ -11,6 +11,13 @@ >> >> #include <linux/thunderbolt.h> >> >> +enum nhi_iommu_status { >> + IOMMU_UNKNOWN, >> + IOMMU_DISABLED, >> + IOMMU_ENABLED, >> +}; >> +enum nhi_iommu_status nhi_check_iommu_for_port(struct pci_dev *pdev); >> + > > This adds quite a lot code and complexity, and honestly I would like to > keep it as simple as possible (and this is not enough because we need to > make sure the DMAR bit is there so that none of the possible connected > devices were able to overwrite our memory already).
Shall we forget the standalone sibling check and just make the pdev->untrusted check directly in tb_acpi_add_link() then? On reflection I guess the DMAR bit makes iommu_dma_protection functionally dependent on ACPI already, so we don't actually lose anything (and anyone can come back and revisit firmware-agnostic methods later if a need appears).
Robin.
| |