RE: [PATCH v2 1/2] iommu/amd: Enable swiotlb in all cases
[AMD Official Use Only] > On Wed, Apr 06, 2022 at 05:04:52PM +0000, Limonciello, Mario wrote: > > Considering before this fix effectively swiotlb was turned off on most AMD > > systems, when this is picked up I think y'all should consider to add a: > > > > Cc: sta...@vger.kernel.org # 5.11+ > > Agreed. I think this is for Joerg to pick up, and I'd love to see it > picked up soon as I'll have to rebase my > > "cleanup swiotlb initialization" series on top of it. Joerg, Just want to double check this wasn't lost the last few weeks. I was just checking iommu.git/next and didn't notice it still. Thanks, ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2 1/2] iommu/amd: Enable swiotlb in all cases
[Public] > -Original Message- > From: Christoph Hellwig > Sent: Monday, April 4, 2022 23:34 > To: Limonciello, Mario > Cc: Joerg Roedel ; Will Deacon ; > Christoph Hellwig ; Marek Szyprowski > ; Robin Murphy ; > open list:IOMMU DRIVERS ; > Suthikulpanit, Suravee ; Hegde, Vasant > ; open list > Subject: Re: [PATCH v2 1/2] iommu/amd: Enable swiotlb in all cases > > Looks good: > > Reviewed-by: Christoph Hellwig Thanks! Considering before this fix effectively swiotlb was turned off on most AMD systems, when this is picked up I think y'all should consider to add a: Cc: sta...@vger.kernel.org # 5.11+ As well. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 0/2] Fix issues with untrusted devices and AMD IOMMU
[AMD Official Use Only]
> On Mon, Apr 04, 2022 at 11:47:05AM -0500, Mario Limonciello wrote:
> > The bounce buffers were originally set up, but torn down during
> > the boot process.
> > * This happens because as part of IOMMU initialization
> > `amd_iommu_init_dma_ops` gets called and resets the global swiotlb to 0.
> > * When late_init gets called `pci_swiotlb_late_init` `swiotlb_exit` is
> > called and the buffers are torn down.
>
> I think the proper thing is to do this:
>
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index a1ada7bff44e6..079694f894b85 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -1838,17 +1838,10 @@ void amd_iommu_domain_update(struct
> protection_domain *domain)
> amd_iommu_domain_flush_complete(domain);
> }
>
> -static void __init amd_iommu_init_dma_ops(void)
> -{
> - swiotlb = (iommu_default_passthrough() || sme_me_mask) ? 1 : 0;
> -}
> -
> int __init amd_iommu_init_api(void)
> {
> int err;
>
> - amd_iommu_init_dma_ops();
> -
> err = bus_set_iommu(_bus_type, _iommu_ops);
> if (err)
> return err;
I do expect that solves it as well. The reason I submitted the way I did is
that there seemed to be a strong affinity for having swiotlb disabled when IOMMU
is enabled on AMD IOMMU. The original code that disabled SWIOTLB in AMD IOMMU
dates all the way back to 2.6.33 (commit
75f1cdf1dda92cae037ec848ae63690d91913eac)
and it has ping ponged around since then to add more criteria that it would be
or wouldn't
be disabled, but was never actually dropped until your suggestion.
If the consensus is that it should be dropped, I'll validate it does help and
then send that out
as a v2. I do think that my messaging patch (1/2) may still be useful for
debugging in the
future if for another reason SWIOTLB is disabled.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2 2/2] thunderbolt: Make iommu_dma_protection more accurate
[Public] > -Original Message- > From: Robin Murphy > Sent: Monday, March 21, 2022 06:12 > To: mika.westerb...@linux.intel.com; Limonciello, Mario > > Cc: j...@8bytes.org; baolu...@linux.intel.com; andreas.noe...@gmail.com; > michael.ja...@intel.com; yehezkel...@gmail.com; iommu@lists.linux- > foundation.org; linux-...@vger.kernel.org; linux-ker...@vger.kernel.org; > h...@lst.de > Subject: Re: [PATCH v2 2/2] thunderbolt: Make iommu_dma_protection > more accurate > > On 2022-03-21 10:58, mika.westerb...@linux.intel.com wrote: > > Hi Mario, > > > > On Fri, Mar 18, 2022 at 10:29:59PM +, Limonciello, Mario wrote: > >> [Public] > >> > >>> Between me trying to get rid of iommu_present() and Mario wanting to > >>> support the AMD equivalent of DMAR_PLATFORM_OPT_IN, scrutiny has > >>> shown > >>> that the iommu_dma_protection attribute is being far too optimistic. > >>> 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(s) > >>> we care about. Furthermore, all that DMAR_PLATFORM_OPT_IN really > does > >>> is tell us that memory was protected before the kernel was loaded, and > >>> prevent the user from disabling the intel-iommu driver entirely. While > >>> that lets us assume kernel integrity, what matters for actual runtime > >>> DMA protection is whether we trust individual devices, based on the > >>> "external facing" property that we expect firmware to describe for > >>> Thunderbolt ports. > >>> > >>> It's proven challenging to determine the appropriate ports accurately > >>> given the variety of possible topologies, so while still not getting a > >>> perfect answer, by putting enough faith in firmware we can at least get > >>> a good bit closer. If we can see that any device near a Thunderbolt NHI > >>> has all the requisites for Kernel DMA Protection, chances are that it > >>> *is* a relevant port, but moreover that implies that firmware is playing > >>> the game overall, so we'll use that to assume that all Thunderbolt ports > >>> should be correctly marked and thus will end up fully protected. > >>> > >> > >> This approach looks generally good to me. I do worry a little bit about > older > >> systems that didn't set ExternalFacingPort in the FW but were previously > setting > >> iommu_dma_protection, but I think that those could be treated on a > quirk > >> basis to set PCI IDs for those root ports as external facing if/when they > come > >> up. > > > > There are no such systems out there AFAICT. > > And even if there are, as above they've never actually been fully > protected and still won't be, so it's arguably a good thing for them to > stop thinking so. I was meaning that if this situation comes up that we could tag the PCI IDs for those root ports as ExternalFacing in drivers/pci/quirks.c so that the protection "is" enacted for them even if it was missing from the firmware. > > >> I'll send up a follow up patch that adds the AMD ACPI table check. > >> If it looks good can roll it into your series for v3, or if this series > >> goes > >> as is for v2 it can come on its own. > >> > >>> CC: Mario Limonciello > >>> Signed-off-by: Robin Murphy > >>> --- > >>> > >>> v2: Give up trying to look for specific devices, just look for evidence > >>> that firmware cares at all. > >> > >> I still do think you could know exactly which devices to use if you're in > >> SW CM mode, but I guess the consensus is to not bifurcate the way this > >> can be checked. > > > > Indeed. > > > > The patch looks good to me now. I will give it a try on a couple of > > systems later today or tomorrow and let you guys know how it went. I > > don't expect any problems but let's see. > > > > Thanks a lot Robin for working on this :) > > Heh, let's just hope the other half-dozen or so subsystems I need to > touch for this IOMMU cleanup aren't all quite as involved as this turned > out to be :) Thanks a lot for this effort! > > Cheers, > Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2 2/2] thunderbolt: Make iommu_dma_protection more accurate
[Public]
> Between me trying to get rid of iommu_present() and Mario wanting to
> support the AMD equivalent of DMAR_PLATFORM_OPT_IN, scrutiny has
> shown
> that the iommu_dma_protection attribute is being far too optimistic.
> 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(s)
> we care about. Furthermore, all that DMAR_PLATFORM_OPT_IN really does
> is tell us that memory was protected before the kernel was loaded, and
> prevent the user from disabling the intel-iommu driver entirely. While
> that lets us assume kernel integrity, what matters for actual runtime
> DMA protection is whether we trust individual devices, based on the
> "external facing" property that we expect firmware to describe for
> Thunderbolt ports.
>
> It's proven challenging to determine the appropriate ports accurately
> given the variety of possible topologies, so while still not getting a
> perfect answer, by putting enough faith in firmware we can at least get
> a good bit closer. If we can see that any device near a Thunderbolt NHI
> has all the requisites for Kernel DMA Protection, chances are that it
> *is* a relevant port, but moreover that implies that firmware is playing
> the game overall, so we'll use that to assume that all Thunderbolt ports
> should be correctly marked and thus will end up fully protected.
>
This approach looks generally good to me. I do worry a little bit about older
systems that didn't set ExternalFacingPort in the FW but were previously setting
iommu_dma_protection, but I think that those could be treated on a quirk
basis to set PCI IDs for those root ports as external facing if/when they come
up.
I'll send up a follow up patch that adds the AMD ACPI table check.
If it looks good can roll it into your series for v3, or if this series goes
as is for v2 it can come on its own.
> CC: Mario Limonciello
> Signed-off-by: Robin Murphy
> ---
>
> v2: Give up trying to look for specific devices, just look for evidence
> that firmware cares at all.
I still do think you could know exactly which devices to use if you're in
SW CM mode, but I guess the consensus is to not bifurcate the way this
can be checked.
>
> drivers/thunderbolt/domain.c | 12 +++
> drivers/thunderbolt/nhi.c| 41
>
> include/linux/thunderbolt.h | 2 ++
> 3 files changed, 46 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
> index 7018d959f775..2889a214dadc 100644
> --- a/drivers/thunderbolt/domain.c
> +++ b/drivers/thunderbolt/domain.c
> @@ -7,9 +7,7 @@
> */
>
> #include
> -#include
> #include
> -#include
> #include
> #include
> #include
> @@ -257,13 +255,9 @@ static ssize_t iommu_dma_protection_show(struct
> device *dev,
>struct device_attribute *attr,
>char *buf)
> {
> - /*
> - * Kernel DMA protection is a feature where Thunderbolt security is
> - * handled natively using IOMMU. It is enabled when IOMMU is
> - * enabled and ACPI DMAR table has DMAR_PLATFORM_OPT_IN set.
> - */
> - return sprintf(buf, "%d\n",
> -iommu_present(_bus_type) &&
> dmar_platform_optin());
> + struct tb *tb = container_of(dev, struct tb, dev);
> +
> + return sysfs_emit(buf, "%d\n", tb->nhi->iommu_dma_protection);
> }
> static DEVICE_ATTR_RO(iommu_dma_protection);
>
> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index c73da0532be4..9e396e283792 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -14,6 +14,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -1102,6 +1103,45 @@ static void nhi_check_quirks(struct tb_nhi *nhi)
> nhi->quirks |= QUIRK_AUTO_CLEAR_INT;
> }
>
> +static int nhi_check_iommu_pdev(struct pci_dev *pdev, void *data)
> +{
> + if (!pdev->untrusted ||
> + !dev_iommu_capable(>dev,
> IOMMU_CAP_PRE_BOOT_PROTECTION))
> + return 0;
> + *(bool *)data = true;
> + return 1; /* Stop walking */
> +}
> +
> +static void nhi_check_iommu(struct tb_nhi *nhi)
> +{
> + struct pci_bus *bus = nhi->pdev->bus;
> + bool port_ok = false;
> +
> + /*
> + * Ideally what we'd do here is grab every PCI device that
> + * represents a tunnelling adapter for this NHI and check their
> + * status directly, but unfortunately USB4 seems to make it
> + * obnoxiously difficult to reliably make any correlation.
> + *
> + * So for now we'll have to bodge it... Hoping that the system
> + * is at least sane enough that an adapter is in the same PCI
> + * segment as its NHI, if we can find *something* on that segment
> + * which meets the requirements for Kernel DMA Protection, we'll
> + * take that to imply that
RE: [PATCH] thunderbolt: Make iommu_dma_protection more accurate
[Public] > -Original Message- > From: Robin Murphy > Sent: Friday, March 18, 2022 09:08 > To: mika.westerb...@linux.intel.com > Cc: Limonciello, Mario ; > andreas.noe...@gmail.com; michael.ja...@intel.com; > yehezkel...@gmail.com; linux-...@vger.kernel.org; linux- > ker...@vger.kernel.org; iommu@lists.linux-foundation.org; linux- > p...@vger.kernel.org > Subject: Re: [PATCH] thunderbolt: Make iommu_dma_protection more > accurate > > On 2022-03-18 13:25, mika.westerb...@linux.intel.com wrote: > > Hi Robin, > > > > On Fri, Mar 18, 2022 at 12:01:42PM +, Robin Murphy wrote: > >>> 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? > > > > I think we should leave tb_acpi_add_link() untouched if possible ;-) > > This is because it is used to add the device links from firmware > > description that we need for proper power management of the tunneled > > devices. It has little to do with the identification of the external > > facing DMA-capable PCIe ports. > > > > Furthermore these links only exists in USB4 software connection manager > > systems so we do not have those in the existing Thunderbolt 3/4 systems > > that use firmware based connection manager (pretty much all out there). > > > >> 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). > > > > I agree. > > OK, so do we have any realistic options for identifying the correct PCI > devices, if USB4 PCIe adapters might be anywhere relative to their > associated NHI? Short of maintaining a list of known IDs, the only > thought I have left is that if we walk the whole PCI segment looking > specifically for hotplug-capable Gen1 ports, any system modern enough to > have Thunderbolt is *probably* not going to have any real PCIe Gen1 > hotplug slots, so maybe false negatives might be tolerable, but it still > feels like a bit of a sketchy heuristic. > > I suppose we could just look to see if any device anywhere is marked as > external-facing, and hope that if firmware's done that much then it's > done everything right. That's still at least slightly better than what > we have today, but AFAICS still carries significant risk of a false > positive for an add-in card that firmware didn't recognise. > > I'm satisfied that we've come round to the right conclusion on the DMAR > opt-in - I'm in the middle or writing up patches for that now - but even > Microsoft's spec gives that as a separate requirement from the flagging > of external ports, with both being necessary for Kernel DMA Protection. > > Robin. The thunderbolt driver already has a good idea whether it's using software CM or firmware CM. How about if we use that to make a decision? If running firmware CM presumably that is a fixed quantity of machines that will dwindle over time as OEMs release new HW with SW CM designs. So maybe leave things "as is" for those - opt in bit is sufficient for this check. And then if running software CM then make it mandatory that any created links are set untrusted AND some pre-boot bits are set to get iommu dma protection set. We effectively end up with the same requirements as Microsoft has in their spec for new hardware then, and don't break any old hardware that doesn't have the links made and relies on firmware for the CM. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH] thunderbolt: Make iommu_dma_protection more accurate
[Public]
> -Original Message-
> From: Limonciello, Mario
> Sent: Thursday, March 17, 2022 12:09
> To: Robin Murphy ; andreas.noe...@gmail.com;
> michael.ja...@intel.com; mika.westerb...@linux.intel.com;
> yehezkel...@gmail.com
> Cc: linux-...@vger.kernel.org; linux-ker...@vger.kernel.org;
> iommu@lists.linux-foundation.org; linux-...@vger.kernel.org
> Subject: RE: [PATCH] thunderbolt: Make iommu_dma_protection more
> accurate
>
> [Public]
>
>
>
> > -Original Message-
> > From: Robin Murphy
> > Sent: Thursday, March 17, 2022 11:17
> > To: andreas.noe...@gmail.com; michael.ja...@intel.com;
> > mika.westerb...@linux.intel.com; yehezkel...@gmail.com
> > Cc: linux-...@vger.kernel.org; linux-ker...@vger.kernel.org;
> > iommu@lists.linux-foundation.org; linux-...@vger.kernel.org; Limonciello,
> > Mario
> > Subject: [PATCH] thunderbolt: Make iommu_dma_protection more
> accurate
> >
> > Between me trying to get rid of iommu_present() and Mario wanting to
> > support the AMD equivalent of DMAR_PLATFORM_OPT_IN, scrutiny has
> > shown
> > that the iommu_dma_protection attribute is being far too optimistic.
> > 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(s)
> > we care about. Furthermore, all that DMAR_PLATFORM_OPT_IN really
> does
> > is tell us that memory was protected before the kernel was loaded, and
> > prevent the user from disabling the intel-iommu driver entirely. What
> > actually matters is whether we trust individual devices, based on the
> > "external facing" property that we expect firmware to describe for
> > Thunderbolt ports.
> >
> > Avoid false positives by looking as close as possible to the same PCI
> > topology that the IOMMU layer will consider once a Thunderbolt endpoint
> > appears. Crucially, we can't assume that IOMMU translation being enabled
> > for any reason is sufficient on its own; full (expensive) DMA protection
> > will still only be imposed on untrusted devices.
> >
> > CC: Mario Limonciello
> > Signed-off-by: Robin Murphy
> > ---
> >
> > This supersedes my previous attempt just trying to replace
> > iommu_present() at [1], further to the original discussion at [2].
> >
> > [1]
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
> > kernel.org%2Flinux-
> >
> iommu%2FBL1PR12MB515799C0BE396377DBBEF055E2119%40BL1PR12MB515
> >
> 7.namprd12.prod.outlook.com%2FT%2Fdata=04%7C01%7Cmario.limo
> >
> nciello%40amd.com%7C14f5afbba9624b4d0ef508da08319b2a%7C3dd8961fe4
> >
> 884e608e11a82d994e183d%7C0%7C0%7C637831306409535091%7CUnknown
> > %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> >
> WwiLCJXVCI6Mn0%3D%7C3000sdata=9xJ9bNT3pR3YhqOOqiJtGv94ln2
> > IJSvrXllbPZjTI6M%3Dreserved=0
> > [2]
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
> > kernel.org%2Flinux-iommu%2F202203160844.lKviWR1Q-
> >
> lkp%40intel.com%2FT%2Fdata=04%7C01%7Cmario.limonciello%40amd
> >
> .com%7C14f5afbba9624b4d0ef508da08319b2a%7C3dd8961fe4884e608e11a8
> >
> 2d994e183d%7C0%7C0%7C637831306409535091%7CUnknown%7CTWFpbGZs
> >
> b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> >
> 0%3D%7C3000sdata=wSbpjpPQk8ulX8ifOTt%2BNMdO5svwQceQthyca
> > txzScI%3Dreserved=0
> >
> > drivers/thunderbolt/domain.c | 12 +++-
> > drivers/thunderbolt/nhi.c| 35
> > +++
> > include/linux/thunderbolt.h | 2 ++
> > 3 files changed, 40 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
> > index 7018d959f775..d5c825e84ac8 100644
> > --- a/drivers/thunderbolt/domain.c
> > +++ b/drivers/thunderbolt/domain.c
> > @@ -7,9 +7,7 @@
> > */
> >
> > #include
> > -#include
> > #include
> > -#include
> > #include
> > #include
> > #include
> > @@ -257,13 +255,9 @@ static ssize_t iommu_dma_protection_show(struct
> > device *dev,
> > struct device_attribute *attr,
> > char *buf)
> > {
> > - /*
> > -* Kernel DMA protection is a feature where Thunderbolt security is
> > -* handled natively using IOMMU. It is enabled when IOMMU is
> > -* enabled and ACPI DMAR table has DMAR_PLATFORM_OPT_IN set.
> > -*/
> > - return sprintf(buf, "%d\
RE: [PATCH] thunderbolt: Make iommu_dma_protection more accurate
[Public]
> -Original Message-
> From: Robin Murphy
> Sent: Thursday, March 17, 2022 11:17
> To: andreas.noe...@gmail.com; michael.ja...@intel.com;
> mika.westerb...@linux.intel.com; yehezkel...@gmail.com
> Cc: linux-...@vger.kernel.org; linux-ker...@vger.kernel.org;
> iommu@lists.linux-foundation.org; linux-...@vger.kernel.org; Limonciello,
> Mario
> Subject: [PATCH] thunderbolt: Make iommu_dma_protection more accurate
>
> Between me trying to get rid of iommu_present() and Mario wanting to
> support the AMD equivalent of DMAR_PLATFORM_OPT_IN, scrutiny has
> shown
> that the iommu_dma_protection attribute is being far too optimistic.
> 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(s)
> we care about. Furthermore, all that DMAR_PLATFORM_OPT_IN really does
> is tell us that memory was protected before the kernel was loaded, and
> prevent the user from disabling the intel-iommu driver entirely. What
> actually matters is whether we trust individual devices, based on the
> "external facing" property that we expect firmware to describe for
> Thunderbolt ports.
>
> Avoid false positives by looking as close as possible to the same PCI
> topology that the IOMMU layer will consider once a Thunderbolt endpoint
> appears. Crucially, we can't assume that IOMMU translation being enabled
> for any reason is sufficient on its own; full (expensive) DMA protection
> will still only be imposed on untrusted devices.
>
> CC: Mario Limonciello
> Signed-off-by: Robin Murphy
> ---
>
> This supersedes my previous attempt just trying to replace
> iommu_present() at [1], further to the original discussion at [2].
>
> [1]
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
> kernel.org%2Flinux-
> iommu%2FBL1PR12MB515799C0BE396377DBBEF055E2119%40BL1PR12MB515
> 7.namprd12.prod.outlook.com%2FT%2Fdata=04%7C01%7Cmario.limo
> nciello%40amd.com%7C14f5afbba9624b4d0ef508da08319b2a%7C3dd8961fe4
> 884e608e11a82d994e183d%7C0%7C0%7C637831306409535091%7CUnknown
> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> WwiLCJXVCI6Mn0%3D%7C3000sdata=9xJ9bNT3pR3YhqOOqiJtGv94ln2
> IJSvrXllbPZjTI6M%3Dreserved=0
> [2]
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
> kernel.org%2Flinux-iommu%2F202203160844.lKviWR1Q-
> lkp%40intel.com%2FT%2Fdata=04%7C01%7Cmario.limonciello%40amd
> .com%7C14f5afbba9624b4d0ef508da08319b2a%7C3dd8961fe4884e608e11a8
> 2d994e183d%7C0%7C0%7C637831306409535091%7CUnknown%7CTWFpbGZs
> b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D%7C3000sdata=wSbpjpPQk8ulX8ifOTt%2BNMdO5svwQceQthyca
> txzScI%3Dreserved=0
>
> drivers/thunderbolt/domain.c | 12 +++-
> drivers/thunderbolt/nhi.c| 35
> +++
> include/linux/thunderbolt.h | 2 ++
> 3 files changed, 40 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
> index 7018d959f775..d5c825e84ac8 100644
> --- a/drivers/thunderbolt/domain.c
> +++ b/drivers/thunderbolt/domain.c
> @@ -7,9 +7,7 @@
> */
>
> #include
> -#include
> #include
> -#include
> #include
> #include
> #include
> @@ -257,13 +255,9 @@ static ssize_t iommu_dma_protection_show(struct
> device *dev,
>struct device_attribute *attr,
>char *buf)
> {
> - /*
> - * Kernel DMA protection is a feature where Thunderbolt security is
> - * handled natively using IOMMU. It is enabled when IOMMU is
> - * enabled and ACPI DMAR table has DMAR_PLATFORM_OPT_IN set.
> - */
> - return sprintf(buf, "%d\n",
> -iommu_present(_bus_type) &&
> dmar_platform_optin());
> + struct tb *tb = container_of(dev, struct tb, dev);
> +
> + return sprintf(buf, "%d\n", tb->nhi->iommu_dma_protection);
> }
> static DEVICE_ATTR_RO(iommu_dma_protection);
>
> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index c73da0532be4..e12c2e266741 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -14,6 +14,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -1102,6 +1103,39 @@ static void nhi_check_quirks(struct tb_nhi *nhi)
> nhi->quirks |= QUIRK_AUTO_CLEAR_INT;
> }
>
> +static void nhi_check_iommu(struct tb_nhi *nhi)
> +{
> + struct pci_dev *pdev;
> + bool port_ok = false;
> +
> + /*
> + * Check for sibling
RE: [PATCH] thunderbolt: Stop using iommu_present()
[Public] > -Original Message- > From: Robin Murphy > Sent: Wednesday, March 16, 2022 14:18 > To: Limonciello, Mario ; Mika Westerberg > > Cc: michael.ja...@intel.com; linux-...@vger.kernel.org; linux- > ker...@vger.kernel.org; yehezkel...@gmail.com; iommu@lists.linux- > foundation.org; andreas.noe...@gmail.com; h...@lst.de > Subject: Re: [PATCH] thunderbolt: Stop using iommu_present() > > On 2022-03-16 18:34, Limonciello, Mario wrote: > > [Public] > > > >>> Can the USB4 CM make the device links in the DVSEC case perhaps too? > I > >> would > >>> think we want that anyway to control device suspend ordering. > >>> > >>> If I had something discrete to try I'd dust off the DVSEC patch I wrote > >> before to > >>> try it, but alas all I have is integrated stuff on my hand. > >>> > >>>>>> Mika, you might not have seen it yet, but I sent a follow up diff in > this > >>>> thread > >>>>>> to Robin's patch. If that looks good Robin can submit a v2 (or I'm > happy > >> to > >>>> do > >>>>>> so as well as I confirmed it helps my original intent too). > >>>>> > >>>>> I saw it now and I'm thinking are we making this unnecessary > complex? I > >>>>> mean Microsoft solely depends on the DMAR platform opt-in flag: > >>>>> > >>>>> > >>>> > >>> > >>> I think Microsoft doesn't allow you to turn off the IOMMU though or put > it > >> in > >>> passthrough through on the kernel command line. > >>> > >>>>> We also do turn on full IOMMU mappings in that case for devices that > >> are > >>>>> marked as external facing by the same firmware that provided the > >> DMAR > >>>>> bit. If the user decides to disable IOMMU from command line for > >> instance > >>>>> then we expect she knows what she is doing. > >>>> > >>>> Yeah, if external_facing is set correctly then we can safely expect the > >>>> the IOMMU layer to do the right thing, so in that case it probably is OK > >>>> to infer that if an IOMMU is present for the NHI then it'll be managing > >>>> that whole bus hierarchy. What I'm really thinking about here is > whether > >>>> we can defend against a case when external_facing *isn't* set, so we > >>>> treat the tunnelled ports as normal PCI buses, assume it's OK since > >>>> we've got an IOMMU and everything else is getting translation domains > >> by > >>>> default, but then a Thunderbolt device shows up masquerading the > >> VID:DID > >>>> of something that gets a passthrough quirk, and thus tricks its way > >>>> through the perceived protection. > >>>> > >>>> Robin. > >>> > >>> Unless it happened after 5.17-rc8 looking at the code I think that's Intel > >>> specific behavior though at the moment (has_external_pci). I don't see > it > >>> in a generic layer. > >> > >> Ah, it's not necessarily the most obvious thing - > >> pci_dev->external_facing gets propagated through to pci_dev- > >untrusted > >> by set_pcie_untrusted(), and it's that that's then checked by > >> iommu_get_def_domain_type() to enforce a translation domain > regardless > >> of default passthrough or quirks. It's then further checked by > >> iommu-dma's dev_is_untrusted() to enforce bounce-buffering to avoid > data > >> leakage in sub-page mappings too. > >> > > > > Ah thanks for explaining it, that was immediately obvious to me. > > > >>> In addition to the point Robin said about firmware not setting external > >> facing > >>> if the IOMMU was disabled on command line then > iommu_dma_protection > >>> would be showing the wrong values meaning userspace may choose to > >>> authorize the device automatically in a potentially unsafe scenario. > >>> > >>> Even if the user "knew what they were doing", I would expect that we > still > >>> do our best to protect them from themselves and not advertise > something > >>> that will cause automatic authorization. > >> > >> Might it be reasonable for the Thunderbolt core to check early on if any > >> tunnelled ports are not marked as external facing, and if so just tell > >&g
RE: [PATCH] thunderbolt: Stop using iommu_present()
[Public] > > Can the USB4 CM make the device links in the DVSEC case perhaps too? I > would > > think we want that anyway to control device suspend ordering. > > > > If I had something discrete to try I'd dust off the DVSEC patch I wrote > before to > > try it, but alas all I have is integrated stuff on my hand. > > > Mika, you might not have seen it yet, but I sent a follow up diff in this > >> thread > to Robin's patch. If that looks good Robin can submit a v2 (or I'm happy > to > >> do > so as well as I confirmed it helps my original intent too). > >>> > >>> I saw it now and I'm thinking are we making this unnecessary complex? I > >>> mean Microsoft solely depends on the DMAR platform opt-in flag: > >>> > >>> > >> > > > > I think Microsoft doesn't allow you to turn off the IOMMU though or put it > in > > passthrough through on the kernel command line. > > > >>> We also do turn on full IOMMU mappings in that case for devices that > are > >>> marked as external facing by the same firmware that provided the > DMAR > >>> bit. If the user decides to disable IOMMU from command line for > instance > >>> then we expect she knows what she is doing. > >> > >> Yeah, if external_facing is set correctly then we can safely expect the > >> the IOMMU layer to do the right thing, so in that case it probably is OK > >> to infer that if an IOMMU is present for the NHI then it'll be managing > >> that whole bus hierarchy. What I'm really thinking about here is whether > >> we can defend against a case when external_facing *isn't* set, so we > >> treat the tunnelled ports as normal PCI buses, assume it's OK since > >> we've got an IOMMU and everything else is getting translation domains > by > >> default, but then a Thunderbolt device shows up masquerading the > VID:DID > >> of something that gets a passthrough quirk, and thus tricks its way > >> through the perceived protection. > >> > >> Robin. > > > > Unless it happened after 5.17-rc8 looking at the code I think that's Intel > > specific behavior though at the moment (has_external_pci). I don't see it > > in a generic layer. > > Ah, it's not necessarily the most obvious thing - > pci_dev->external_facing gets propagated through to pci_dev->untrusted > by set_pcie_untrusted(), and it's that that's then checked by > iommu_get_def_domain_type() to enforce a translation domain regardless > of default passthrough or quirks. It's then further checked by > iommu-dma's dev_is_untrusted() to enforce bounce-buffering to avoid data > leakage in sub-page mappings too. > Ah thanks for explaining it, that was immediately obvious to me. > > In addition to the point Robin said about firmware not setting external > facing > > if the IOMMU was disabled on command line then iommu_dma_protection > > would be showing the wrong values meaning userspace may choose to > > authorize the device automatically in a potentially unsafe scenario. > > > > Even if the user "knew what they were doing", I would expect that we still > > do our best to protect them from themselves and not advertise something > > that will cause automatic authorization. > > Might it be reasonable for the Thunderbolt core to check early on if any > tunnelled ports are not marked as external facing, and if so just tell > the user that iommu_dma_protection is off the table and anything they > authorise is at their own risk? > > Robin. How about in iommu_dma_protection_show to just check that all the device links to the NHI are marked as untrusted? Then if there are device links missing we solve that separately (discrete USB4 DVSEC case we just need to make those device links). ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH] thunderbolt: Stop using iommu_present()
[Public] > -Original Message- > From: Limonciello, Mario > Sent: Wednesday, March 16, 2022 12:54 > To: Robin Murphy ; Mika Westerberg > > Cc: michael.ja...@intel.com; linux-...@vger.kernel.org; linux- > ker...@vger.kernel.org; yehezkel...@gmail.com; iommu@lists.linux- > foundation.org; andreas.noe...@gmail.com; h...@lst.de > Subject: RE: [PATCH] thunderbolt: Stop using iommu_present() > > [Public] > > > >>> > > >>> There is a way to figure out the "tunneled" PCIe ports by looking at > > >>> certain properties and we do that already actually. The BIOS has the > > >>> following under these ports: > > >>> > > >>> > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs > > >>> .microsoft.com%2Fen-us%2Fwindows- > > hardware%2Fdrivers%2Fpci%2Fdsd- > > >>> for-pcie-root-ports%23identifying-externally-exposed-pcie-root- > > >>> > > > portsdata=04%7C01%7Cmario.limonciello%40amd.com%7C0465d319a > > >>> > > > 6684335d9c208da07710e7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7 > > >>> > > > C0%7C637830479402895833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w > > >>> > > > LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000 > > >>> > > > p;sdata=z6hpYGpj%2B%2BVvz9d6MXiO4N66PUm4zwhOdI%2Br6l3PjhQ%3D > > >>> reserved=0 > > >>> > > >>> and the ports will have dev->external_facing set to 1. Perhaps looking > > >>> at that field helps here? > > >> > > >> External facing isn't a guarantee from the firmware though. It's > > something we > > >> all expect in practice, but I think it's better to look at the ones that > > >> are > > from > > >> the _DSD usb4-host-interface to be safer. > > > > > > Right but then we have the discrete ones with the DVSEC that exposes > the > > > tunneled ports :( > > > > > Can the USB4 CM make the device links in the DVSEC case perhaps too? I > would > think we want that anyway to control device suspend ordering. > > If I had something discrete to try I'd dust off the DVSEC patch I wrote before > to > try it, but alas all I have is integrated stuff on my hand. > > > >> Mika, you might not have seen it yet, but I sent a follow up diff in this > > thread > > >> to Robin's patch. If that looks good Robin can submit a v2 (or I'm happy > to > > do > > >> so as well as I confirmed it helps my original intent too). > > > > > > I saw it now and I'm thinking are we making this unnecessary complex? I > > > mean Microsoft solely depends on the DMAR platform opt-in flag: > > > > > > > > > > I think Microsoft doesn't allow you to turn off the IOMMU though or put it in > passthrough through on the kernel command line. > > > > We also do turn on full IOMMU mappings in that case for devices that are > > > marked as external facing by the same firmware that provided the DMAR > > > bit. If the user decides to disable IOMMU from command line for instance > > > then we expect she knows what she is doing. > > > > Yeah, if external_facing is set correctly then we can safely expect the > > the IOMMU layer to do the right thing, so in that case it probably is OK > > to infer that if an IOMMU is present for the NHI then it'll be managing > > that whole bus hierarchy. What I'm really thinking about here is whether > > we can defend against a case when external_facing *isn't* set, so we > > treat the tunnelled ports as normal PCI buses, assume it's OK since > > we've got an IOMMU and everything else is getting translation domains by > > default, but then a Thunderbolt device shows up masquerading the > VID:DID > > of something that gets a passthrough quirk, and thus tricks its way > > through the perceived protection. > > > > Robin. > > Unless it happened after 5.17-rc8 looking at the code I think that's Intel > specific behavior though at the moment (has_external_pci). I don't see it > in a generic layer. Oh it's via dev_is_untrusted. A few layers through external facing translates to untrusted and then dev_use_swiotlb is used, got it. > > In addition to the point Robin said about firmware not setting external facing > if the IOMMU was disabled on command line then iommu_dma_protection > would be showing the wrong values meaning userspace may choose to > authorize the device automatically in a potentially unsafe scenario. > > Even if the user "knew what they were doing", I would expect that we still > do our best to protect them from themselves and not advertise something > that will cause automatic authorization. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH] thunderbolt: Stop using iommu_present()
[Public] > >>> > >>> There is a way to figure out the "tunneled" PCIe ports by looking at > >>> certain properties and we do that already actually. The BIOS has the > >>> following under these ports: > >>> > >>> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs > >>> .microsoft.com%2Fen-us%2Fwindows- > hardware%2Fdrivers%2Fpci%2Fdsd- > >>> for-pcie-root-ports%23identifying-externally-exposed-pcie-root- > >>> > portsdata=04%7C01%7Cmario.limonciello%40amd.com%7C0465d319a > >>> > 6684335d9c208da07710e7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7 > >>> > C0%7C637830479402895833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w > >>> > LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000 > >>> > p;sdata=z6hpYGpj%2B%2BVvz9d6MXiO4N66PUm4zwhOdI%2Br6l3PjhQ%3D > >>> reserved=0 > >>> > >>> and the ports will have dev->external_facing set to 1. Perhaps looking > >>> at that field helps here? > >> > >> External facing isn't a guarantee from the firmware though. It's > something we > >> all expect in practice, but I think it's better to look at the ones that > >> are > from > >> the _DSD usb4-host-interface to be safer. > > > > Right but then we have the discrete ones with the DVSEC that exposes the > > tunneled ports :( > > Can the USB4 CM make the device links in the DVSEC case perhaps too? I would think we want that anyway to control device suspend ordering. If I had something discrete to try I'd dust off the DVSEC patch I wrote before to try it, but alas all I have is integrated stuff on my hand. > >> Mika, you might not have seen it yet, but I sent a follow up diff in this > thread > >> to Robin's patch. If that looks good Robin can submit a v2 (or I'm happy > >> to > do > >> so as well as I confirmed it helps my original intent too). > > > > I saw it now and I'm thinking are we making this unnecessary complex? I > > mean Microsoft solely depends on the DMAR platform opt-in flag: > > > > > I think Microsoft doesn't allow you to turn off the IOMMU though or put it in passthrough through on the kernel command line. > > We also do turn on full IOMMU mappings in that case for devices that are > > marked as external facing by the same firmware that provided the DMAR > > bit. If the user decides to disable IOMMU from command line for instance > > then we expect she knows what she is doing. > > Yeah, if external_facing is set correctly then we can safely expect the > the IOMMU layer to do the right thing, so in that case it probably is OK > to infer that if an IOMMU is present for the NHI then it'll be managing > that whole bus hierarchy. What I'm really thinking about here is whether > we can defend against a case when external_facing *isn't* set, so we > treat the tunnelled ports as normal PCI buses, assume it's OK since > we've got an IOMMU and everything else is getting translation domains by > default, but then a Thunderbolt device shows up masquerading the VID:DID > of something that gets a passthrough quirk, and thus tricks its way > through the perceived protection. > > Robin. Unless it happened after 5.17-rc8 looking at the code I think that's Intel specific behavior though at the moment (has_external_pci). I don't see it in a generic layer. In addition to the point Robin said about firmware not setting external facing if the IOMMU was disabled on command line then iommu_dma_protection would be showing the wrong values meaning userspace may choose to authorize the device automatically in a potentially unsafe scenario. Even if the user "knew what they were doing", I would expect that we still do our best to protect them from themselves and not advertise something that will cause automatic authorization. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH] thunderbolt: Stop using iommu_present()
[Public] > On Wed, Mar 16, 2022 at 02:49:09PM +, Robin Murphy wrote: > > > 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". > > > > Ah, OK, I wasn't aware that the NHI isn't even the right thing in the first > > place :( > > > > Is there an easy way to get from the struct tb to a PCI device representing > > the end of its relevant tunnel, or do we have a circular dependency > problem > > where the latter won't appear until we've authorised it (and thus the > IOMMU > > layer won't know about it yet either)? > > The PCIe root ports (and the PCIe downstream ports) are there already > even without "authorization". > > There is a way to figure out the "tunneled" PCIe ports by looking at > certain properties and we do that already actually. The BIOS has the > following under these ports: > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs > .microsoft.com%2Fen-us%2Fwindows-hardware%2Fdrivers%2Fpci%2Fdsd- > for-pcie-root-ports%23identifying-externally-exposed-pcie-root- > portsdata=04%7C01%7Cmario.limonciello%40amd.com%7C0465d319a > 6684335d9c208da07710e7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7 > C0%7C637830479402895833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w > LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000 > p;sdata=z6hpYGpj%2B%2BVvz9d6MXiO4N66PUm4zwhOdI%2Br6l3PjhQ%3D > reserved=0 > > and the ports will have dev->external_facing set to 1. Perhaps looking > at that field helps here? External facing isn't a guarantee from the firmware though. It's something we all expect in practice, but I think it's better to look at the ones that are from the _DSD usb4-host-interface to be safer. Mika, you might not have seen it yet, but I sent a follow up diff in this thread to Robin's patch. If that looks good Robin can submit a v2 (or I'm happy to do so as well as I confirmed it helps my original intent too). ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH] thunderbolt: Stop using iommu_present()
[Public]
> -Original Message-
> From: Mika Westerberg
> Sent: Wednesday, March 16, 2022 07:45
> To: Robin Murphy
> Cc: andreas.noe...@gmail.com; michael.ja...@intel.com;
> yehezkel...@gmail.com; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; iommu@lists.linux-foundation.org; Limonciello,
> Mario ; h...@lst.de
> Subject: Re: [PATCH] thunderbolt: Stop using iommu_present()
>
> Hi Robin,
>
> On Wed, Mar 16, 2022 at 11:25:51AM +, 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
> > ---
> >
> > 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
> >
> > 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(
> >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(>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->t
RE: [PATCH 2/2] thunderbolt: Use pre-boot DMA protection on AMD systems
[Public] > -Original Message- > From: Limonciello, Mario > Sent: Tuesday, March 15, 2022 13:36 > To: Robin Murphy ; Christoph Hellwig > ; christ...@kellner.me; Mika Westerberg > > Cc: Michael Jamet ; open list:THUNDERBOLT > DRIVER ; open list ker...@vger.kernel.org>; Yehezkel Bernat ; > open list:AMD IOMMU (AMD-VI) ; > Andreas Noever ; Will Deacon > > Subject: Re: [PATCH 2/2] thunderbolt: Use pre-boot DMA protection on AMD > systems > > + Christian Kellner (Bolt userspace maintainer) > > On 3/15/2022 13:07, Robin Murphy wrote: > > On 2022-03-15 16:54, Limonciello, Mario via iommu wrote: > >> [Public] > >> > >> > >>> On Tue, Mar 15, 2022 at 11:24:55AM -0500, Mario Limonciello wrote: > >>>> - * handled natively using IOMMU. It is enabled when IOMMU is > >>>> - * enabled and ACPI DMAR table has DMAR_PLATFORM_OPT_IN > set. > >>>> + * handled natively using IOMMU. It is enabled when the IOMMU is > >>>> + * enabled and either: > >>>> + * ACPI DMAR table has DMAR_PLATFORM_OPT_IN set > >>>> + * or > >>>> + * ACPI IVRS table has DMA_REMAP bitset > >>>> */ > >>>> return sprintf(buf, "%d\n", > >>>> - iommu_present(_bus_type) && > >>> dmar_platform_optin()); > >>>> + iommu_present(_bus_type) && > >>>> + (dmar_platform_optin() || amd_ivrs_remap_support())); > >>> > >>> Yikes. No, the thunderbot code does not have any business poking into > >>> either dmar_platform_optin or amd_ivrs_remap_support. This needs > >>> a proper abstration from the IOMMU code. > >> > >> To make sure I follow your ask - it's to make a new generic iommu > >> function > >> That would check dmar/ivrs, and switch out thunderbolt domain.c to use > >> the > >> symbol? > >> > >> I'm happy to rework that if that is what you want. > >> Do you have a preferred proposed function name for that? > > > > But why? Either IOMMU translation is enabled or it isn't, and if it is, > > what's to gain from guessing at *why* it might have been? And even if > > the IOMMU's firmware table did tell the IOMMU driver to enable the > > IOMMU, why should that be Thunderbolt's business? > A lot of this comes from baggage from early Thunderbolt 3 implementation > on systems with ICM (Intel's FW CM). On those systems there was a > concept of "Security Levels". This meant that downstream PCIe devices > were not automatically authorized when a TBT3 device was plugged in. In > those cases there was no guarantee that the IOMMU was in use and so the > security was passed on to the user to make a decision. > > In Linux this was accomplished using the 'authorized' attribute in > /sys/bus/thunderbolt/devices/$NUM/authorized. When this was set to 1 > then the TBT3 device and PCIe topology behind it would be enumerated. > > Further documentation explaining how this works is available here: > https://www.kernel.org/doc/html/latest/admin- > guide/thunderbolt.html#security-levels-and-how-to-use-them > > (Intel based) Platforms from 2018+ w/ TBT3 started to use the IOMMU > consistently at runtime but had this existing implementation of security > levels to worry about. Furthermore tunnels could be created pre-boot, > and so the thunderbolt driver may or may not re-create them based on > policy. > > So a new attribute was created "iommu_dma_protection" that userspace > could use as part of a policy decision to automatically authorize > devices. Exporting this attribute is very similar to what Microsoft > does to let the user see the security of the system. > > https://docs.microsoft.com/en-us/windows-hardware/design/device- > experiences/oem-kernel-dma-protection > > In Linux today some userspace software "bolt" has a policy included by > default that will automatically authorize TBT3 and USB4 (w/ PCIe) > devices when iommu_dma_protection is set to 1. > > > > > Furthermore, looking at patch #1 I can only conclude that this is > > entirely meaningless anyway. AFAICS it's literally reporting whether the > > firmware flag was set or not. Not whether it's actually been honoured > > and the IOMMU is enforcing any kind of DMA protection at all. Even on > > Intel where the flag does at least have some effect on the IOMMU driver, > > that can still be overridden. > > Take a look at the Microsoft link I shared above. They also m
Re: [PATCH 2/2] thunderbolt: Use pre-boot DMA protection on AMD systems
+ Christian Kellner (Bolt userspace maintainer) On 3/15/2022 13:07, Robin Murphy wrote: On 2022-03-15 16:54, Limonciello, Mario via iommu wrote: [Public] On Tue, Mar 15, 2022 at 11:24:55AM -0500, Mario Limonciello wrote: - * handled natively using IOMMU. It is enabled when IOMMU is - * enabled and ACPI DMAR table has DMAR_PLATFORM_OPT_IN set. + * handled natively using IOMMU. It is enabled when the IOMMU is + * enabled and either: + * ACPI DMAR table has DMAR_PLATFORM_OPT_IN set + * or + * ACPI IVRS table has DMA_REMAP bitset */ return sprintf(buf, "%d\n", - iommu_present(_bus_type) && dmar_platform_optin()); + iommu_present(_bus_type) && + (dmar_platform_optin() || amd_ivrs_remap_support())); Yikes. No, the thunderbot code does not have any business poking into either dmar_platform_optin or amd_ivrs_remap_support. This needs a proper abstration from the IOMMU code. To make sure I follow your ask - it's to make a new generic iommu function That would check dmar/ivrs, and switch out thunderbolt domain.c to use the symbol? I'm happy to rework that if that is what you want. Do you have a preferred proposed function name for that? But why? Either IOMMU translation is enabled or it isn't, and if it is, what's to gain from guessing at *why* it might have been? And even if the IOMMU's firmware table did tell the IOMMU driver to enable the IOMMU, why should that be Thunderbolt's business? A lot of this comes from baggage from early Thunderbolt 3 implementation on systems with ICM (Intel's FW CM). On those systems there was a concept of "Security Levels". This meant that downstream PCIe devices were not automatically authorized when a TBT3 device was plugged in. In those cases there was no guarantee that the IOMMU was in use and so the security was passed on to the user to make a decision. In Linux this was accomplished using the 'authorized' attribute in /sys/bus/thunderbolt/devices/$NUM/authorized. When this was set to 1 then the TBT3 device and PCIe topology behind it would be enumerated. Further documentation explaining how this works is available here: https://www.kernel.org/doc/html/latest/admin-guide/thunderbolt.html#security-levels-and-how-to-use-them (Intel based) Platforms from 2018+ w/ TBT3 started to use the IOMMU consistently at runtime but had this existing implementation of security levels to worry about. Furthermore tunnels could be created pre-boot, and so the thunderbolt driver may or may not re-create them based on policy. So a new attribute was created "iommu_dma_protection" that userspace could use as part of a policy decision to automatically authorize devices. Exporting this attribute is very similar to what Microsoft does to let the user see the security of the system. https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/oem-kernel-dma-protection In Linux today some userspace software "bolt" has a policy included by default that will automatically authorize TBT3 and USB4 (w/ PCIe) devices when iommu_dma_protection is set to 1. Furthermore, looking at patch #1 I can only conclude that this is entirely meaningless anyway. AFAICS it's literally reporting whether the firmware flag was set or not. Not whether it's actually been honoured and the IOMMU is enforcing any kind of DMA protection at all. Even on Intel where the flag does at least have some effect on the IOMMU driver, that can still be overridden. Take a look at the Microsoft link I shared above. They also make policy decisions based on the information in these tables. I already have a patch refactoring this to get rid of iommu_present(), but at the time I wasn't looking to closely at what it's trying to *do* with the information. If it's supposed to accurately reflect whether the Thunderbolt device is subject to IOMMU translation and not bypassed, I can fix that too (and unexport dmar_platform_optin() in the process...) Robin. This patch series stems from that history. To give the best experience to end users you want hotplugged devices to be automatically authorized when software says it's safe to do so. To summarize the flow: * User plugs in device * USB4 CM will query supported tunnels * USB4 CM will create devices in /sys/bus/thunderbolt/devices for new plugged in TBT3/USB4 device * "authorized" attribute will default to "0" and PCIe tunnels are not created * Userspace gets a uevent that the device was added * Userspace (bolt) reacts by reading /sys/bus/thunderbolt/devices/domainX/iommu_dma_protection * If that is set to "1", bolt will write "1" to "authorized" and USB4 CM will create PCIe tunnels * If that is set to "0", bolt will send an event to GUI to show a popup asking to authorize the device * After user acks the au
RE: [PATCH 2/2] thunderbolt: Use pre-boot DMA protection on AMD systems
[Public] > On Tue, Mar 15, 2022 at 11:24:55AM -0500, Mario Limonciello wrote: > > -* handled natively using IOMMU. It is enabled when IOMMU is > > -* enabled and ACPI DMAR table has DMAR_PLATFORM_OPT_IN set. > > +* handled natively using IOMMU. It is enabled when the IOMMU is > > +* enabled and either: > > +* ACPI DMAR table has DMAR_PLATFORM_OPT_IN set > > +* or > > +* ACPI IVRS table has DMA_REMAP bitset > > */ > > return sprintf(buf, "%d\n", > > - iommu_present(_bus_type) && > dmar_platform_optin()); > > + iommu_present(_bus_type) && > > + (dmar_platform_optin() || amd_ivrs_remap_support())); > > Yikes. No, the thunderbot code does not have any business poking into > either dmar_platform_optin or amd_ivrs_remap_support. This needs > a proper abstration from the IOMMU code. To make sure I follow your ask - it's to make a new generic iommu function That would check dmar/ivrs, and switch out thunderbolt domain.c to use the symbol? I'm happy to rework that if that is what you want. Do you have a preferred proposed function name for that? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 1/1] iommu/vt-d: Skip TE disabling on quirky gfx dedicated iommu
> -Original Message- > From: Lu Baolu > Sent: Tuesday, July 21, 2020 6:07 PM > To: Limonciello, Mario; Joerg Roedel > Cc: baolu...@linux.intel.com; Ashok Raj; linux-ker...@vger.kernel.org; > sta...@vger.kernel.org; Koba Ko; iommu@lists.linux-foundation.org > Subject: Re: [PATCH 1/1] iommu/vt-d: Skip TE disabling on quirky gfx dedicated > iommu > > > [EXTERNAL EMAIL] > > Hi Limonciello, > > On 7/21/20 10:44 PM, Limonciello, Mario wrote: > >> -Original Message- > >> From: iommu On Behalf Of Lu > >> Baolu > >> Sent: Monday, July 20, 2020 7:17 PM > >> To: Joerg Roedel > >> Cc: Ashok Raj;linux-ker...@vger.kernel.org;sta...@vger.kernel.org; Koba > >> Ko;iommu@lists.linux-foundation.org > >> Subject: [PATCH 1/1] iommu/vt-d: Skip TE disabling on quirky gfx dedicated > >> iommu > >> > >> The VT-d spec requires (10.4.4 Global Command Register, TE field) that: > >> > >> Hardware implementations supporting DMA draining must drain any in-flight > >> DMA read/write requests queued within the Root-Complex before completing > >> the translation enable command and reflecting the status of the command > >> through the TES field in the Global Status register. > >> > >> Unfortunately, some integrated graphic devices fail to do so after some > >> kind of power state transition. As the result, the system might stuck in > >> iommu_disable_translation(), waiting for the completion of TE transition. > >> > >> This provides a quirk list for those devices and skips TE disabling if > >> the qurik hits. > >> > >> Fixes:https://bugzilla.kernel.org/show_bug.cgi?id=208363 > > That one is for TGL. > > > > I think you also want to add this one for ICL: > > Fixes:https://bugzilla.kernel.org/show_bug.cgi?id=206571 > > > > Do you mean someone have tested that this patch also fixes the problem > described in 206571? > Yes, confusingly https://bugzilla.kernel.org/show_bug.cgi?id=208363#c31 actually is the XPS 9300 ICL system and issue. I also have a private confirmation from another person that it resolves it for them on another ICL platform. Christian, maybe you can add a tested by clause for the ICL testing. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 1/1] iommu/vt-d: Skip TE disabling on quirky gfx dedicated iommu
> -Original Message-
> From: iommu On Behalf Of Lu
> Baolu
> Sent: Monday, July 20, 2020 7:17 PM
> To: Joerg Roedel
> Cc: Ashok Raj; linux-ker...@vger.kernel.org; sta...@vger.kernel.org; Koba
> Ko; iommu@lists.linux-foundation.org
> Subject: [PATCH 1/1] iommu/vt-d: Skip TE disabling on quirky gfx dedicated
> iommu
>
> The VT-d spec requires (10.4.4 Global Command Register, TE field) that:
>
> Hardware implementations supporting DMA draining must drain any in-flight
> DMA read/write requests queued within the Root-Complex before completing
> the translation enable command and reflecting the status of the command
> through the TES field in the Global Status register.
>
> Unfortunately, some integrated graphic devices fail to do so after some
> kind of power state transition. As the result, the system might stuck in
> iommu_disable_translation(), waiting for the completion of TE transition.
>
> This provides a quirk list for those devices and skips TE disabling if
> the qurik hits.
>
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=208363
That one is for TGL.
I think you also want to add this one for ICL:
Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=206571
> Tested-by: Koba Ko
> Cc: Ashok Raj
> Cc: sta...@vger.kernel.org
> Signed-off-by: Lu Baolu
> ---
> drivers/iommu/intel/dmar.c | 1 +
> drivers/iommu/intel/iommu.c | 27 +++
> include/linux/dmar.h| 1 +
> include/linux/intel-iommu.h | 2 ++
> 4 files changed, 31 insertions(+)
>
> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
> index 683b812c5c47..16f47041f1bf 100644
> --- a/drivers/iommu/intel/dmar.c
> +++ b/drivers/iommu/intel/dmar.c
> @@ -1102,6 +1102,7 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
> }
>
> drhd->iommu = iommu;
> + iommu->drhd = drhd;
>
> return 0;
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 98390a6d8113..11418b14cc3f 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -356,6 +356,7 @@ static int intel_iommu_strict;
> static int intel_iommu_superpage = 1;
> static int iommu_identity_mapping;
> static int intel_no_bounce;
> +static int iommu_skip_te_disable;
>
> #define IDENTMAP_GFX 2
> #define IDENTMAP_AZALIA 4
> @@ -1633,6 +1634,10 @@ static void iommu_disable_translation(struct
> intel_iommu *iommu)
> u32 sts;
> unsigned long flag;
>
> + if (iommu_skip_te_disable && iommu->drhd->gfx_dedicated &&
> + (cap_read_drain(iommu->cap) || cap_write_drain(iommu->cap)))
> + return;
> +
> raw_spin_lock_irqsave(>register_lock, flag);
> iommu->gcmd &= ~DMA_GCMD_TE;
> writel(iommu->gcmd, iommu->reg + DMAR_GCMD_REG);
> @@ -4043,6 +4048,7 @@ static void __init init_no_remapping_devices(void)
>
> /* This IOMMU has *only* gfx devices. Either bypass it or
> set the gfx_mapped flag, as appropriate */
> + drhd->gfx_dedicated = 1;
> if (!dmar_map_gfx) {
> drhd->ignored = 1;
> for_each_active_dev_scope(drhd->devices,
> @@ -6160,6 +6166,27 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,
> 0x0044, quirk_calpella_no_shadow_g
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0062,
> quirk_calpella_no_shadow_gtt);
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x006a,
> quirk_calpella_no_shadow_gtt);
>
> +static void quirk_igfx_skip_te_disable(struct pci_dev *dev)
> +{
> + unsigned short ver;
> +
> + if (!IS_GFX_DEVICE(dev))
> + return;
> +
> + ver = (dev->device >> 8) & 0xff;
> + if (ver != 0x45 && ver != 0x46 && ver != 0x4c &&
> + ver != 0x4e && ver != 0x8a && ver != 0x98 &&
> + ver != 0x9a)
> + return;
> +
> + if (risky_device(dev))
> + return;
> +
> + pci_info(dev, "Skip IOMMU disabling for graphics\n");
> + iommu_skip_te_disable = 1;
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
> quirk_igfx_skip_te_disable);
> +
> /* On Tylersburg chipsets, some BIOSes have been known to enable the
> ISOCH DMAR unit for the Azalia sound device, but not give it any
> TLB entries, which causes it to deadlock. Check for that. We do
> diff --git a/include/linux/dmar.h b/include/linux/dmar.h
> index d7bf029df737..65565820328a 100644
> --- a/include/linux/dmar.h
> +++ b/include/linux/dmar.h
> @@ -48,6 +48,7 @@ struct dmar_drhd_unit {
> u16 segment;/* PCI domain */
> u8 ignored:1; /* ignore drhd */
> u8 include_all:1;
> + u8 gfx_dedicated:1;/* graphic dedicated*/
> struct intel_iommu *iommu;
> };
>
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index bf6009a344f5..329629e1e9de 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@
RE: [Issue]platform/x86: iommu: System can't shutdown because iommu driver keeps checking the status of DMA_GSTS_TES
> -Original Message- > From: iommu On Behalf Of Koba Ko > Sent: Sunday, June 14, 2020 10:47 PM > To: David Woodhouse; Lu Baolu; Joerg Roedel > Cc: iommu@lists.linux-foundation.org; Kai Heng Feng; Linux Kernel Mailing > List > Subject: [Issue]platform/x86: iommu: System can't shutdown because iommu > driver keeps checking the status of DMA_GSTS_TES > > hi All, > I have a machine and there's only intel gpu. > the secureboot and vt-d is enabled in BIOS. > On the Ubuntu desktop, I do s2idle first and restart the machine. > The machine can't restart successfully, so I need to press the power > button to shutdown. > I tried each of the following and the issue can't be triggered. > 1. disable secure boot in BIOS. Just to explain why this happens, on many of Dell's systems VT-d is only enabled when secure boot is enabled. > 2. intel_iommu=off. > 3. intel_iomm=igfx_off. > 4. nomodeset > 5. i915.modeset=0. > > After I investigate further, find inte_iommu keeps checking the status > of DMA_GSTS_TES. > During the procedure of restart, the driver would disable iommu translation > and > check the status of DMA_GSTS_TES until status of DMA_GSTS_TES is 0. > > If you need more information, I can provide it. > > Thanks > Koba Ko > ___ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu This is reported on TGL pre-production system, but actually same symptom is also happening in ICL production systems such as XPS 7390 2-in-1 and XPS 9300. Details for the ICL issue are available here: https://bugzilla.kernel.org/show_bug.cgi?id=206571 A user did bisect it back to commit 6c3a44ed3c553c324845744f30bcd1d3b07d61fd in 5.5. Glancing through the code and comparing the suspend case, I would ask is it just a case of missing a flush at shutdown (IE iommu_flush_all)? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
