Re: [PATCH] thunderbolt: Make iommu_dma_protection more accurate

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2022-03-18 11:38, mika.westerberg@xxxxxxxxxxxxxxx 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.



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux