Re: [PATCH] thunderbolt: Make iommu_dma_protection more accurate

[Robin Murphy <robin.murphy@arm.com>]

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.