Re: [PATCH] thunderbolt: Make iommu_dma_protection more accurate

[Robin Murphy <robin.murphy@xxxxxxx>]

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.