| Message ID | 20181112160628.86620-2-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI / iommu / thunderbolt: IOMMU based DMA protection | expand |
On Mon, Nov 12, 2018 at 07:06:25PM +0300, Mika Westerberg wrote: > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1378,6 +1378,27 @@ static void set_pcie_thunderbolt(struct pci_dev *dev) > } > } > > +static void set_pcie_external(struct pci_dev *dev) > +{ > + struct pci_dev *parent; > + > + /* > + * Walk up the device hierarchy and check for any upstream > + * bridge that has is_external_facing set to true. This means > + * the hierarchy is below PCIe port that is exposed externally > + * (such as Thunderbolt). > + */ > + parent = pci_upstream_bridge(dev); > + while (parent) { > + if (parent->is_external) { > + dev->is_external = true; > + break; > + } > + > + parent = pci_upstream_bridge(parent); > + } > +} > + This looks pretty much like a duplication of the is_thunderbolt bit in struct pci_dev and the pci_is_thunderbolt_attached() helper. Why constrain the functionality to ports with the _DSD property instead of making it available for *any* Thunderbolt port? Thanks, Lukas
On Mon, Nov 12, 2018 at 07:02:03PM +0100, Lukas Wunner wrote: > On Mon, Nov 12, 2018 at 07:06:25PM +0300, Mika Westerberg wrote: > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -1378,6 +1378,27 @@ static void set_pcie_thunderbolt(struct pci_dev *dev) > > } > > } > > > > +static void set_pcie_external(struct pci_dev *dev) > > +{ > > + struct pci_dev *parent; > > + > > + /* > > + * Walk up the device hierarchy and check for any upstream > > + * bridge that has is_external_facing set to true. This means > > + * the hierarchy is below PCIe port that is exposed externally > > + * (such as Thunderbolt). > > + */ > > + parent = pci_upstream_bridge(dev); > > + while (parent) { > > + if (parent->is_external) { > > + dev->is_external = true; > > + break; > > + } > > + > > + parent = pci_upstream_bridge(parent); > > + } > > +} > > + > > This looks pretty much like a duplication of the is_thunderbolt bit > in struct pci_dev and the pci_is_thunderbolt_attached() helper. > > Why constrain the functionality to ports with the _DSD property > instead of making it available for *any* Thunderbolt port? I assume it is because this is just not needed for Thuderbolt ports but rather for all PCIe devices that are "external" (whatever that is supposed to mean), ie it is valid also for PCIe slots. To be frank the concept (and Microsoft _DSD bindings) seems a bit vague and not thoroughly defined and I would question its detection at PCI/ACPI core level, I would hope this can be clarified at ACPI specification level, at least. Thanks, Lorenzo
On Tue, Nov 13, 2018 at 10:56:36AM +0000, Lorenzo Pieralisi wrote: > On Mon, Nov 12, 2018 at 07:02:03PM +0100, Lukas Wunner wrote: > > On Mon, Nov 12, 2018 at 07:06:25PM +0300, Mika Westerberg wrote: > > > --- a/drivers/pci/probe.c > > > +++ b/drivers/pci/probe.c > > > @@ -1378,6 +1378,27 @@ static void set_pcie_thunderbolt(struct pci_dev *dev) > > > } > > > } > > > > > > +static void set_pcie_external(struct pci_dev *dev) > > > +{ > > > + struct pci_dev *parent; > > > + > > > + /* > > > + * Walk up the device hierarchy and check for any upstream > > > + * bridge that has is_external_facing set to true. This means > > > + * the hierarchy is below PCIe port that is exposed externally > > > + * (such as Thunderbolt). > > > + */ > > > + parent = pci_upstream_bridge(dev); > > > + while (parent) { > > > + if (parent->is_external) { > > > + dev->is_external = true; > > > + break; > > > + } > > > + > > > + parent = pci_upstream_bridge(parent); > > > + } > > > +} > > > + > > > > This looks pretty much like a duplication of the is_thunderbolt bit > > in struct pci_dev and the pci_is_thunderbolt_attached() helper. > > > > Why constrain the functionality to ports with the _DSD property > > instead of making it available for *any* Thunderbolt port? > > I assume it is because this is just not needed for Thuderbolt ports but > rather for all PCIe devices that are "external" (whatever that is > supposed to mean), ie it is valid also for PCIe slots. Yes, that was the idea. We could have other "external" devices that are not using Thunderbolt as the interconnect. We could do so that we automatically set "is_external" for devices with "is_thunderbolt" so it should cover those as well. > To be frank the concept (and Microsoft _DSD bindings) seems a bit vague > and not thoroughly defined and I would question its detection at > PCI/ACPI core level, I would hope this can be clarified at ACPI > specification level, at least. I guess that is the way they envision to use _DSD. Instead of having single UUID that covers all properties (like what we have with device properties) they have one UUID per property "class". I certainly hope we don't need to keep extending prp_guids[] array each time they invent another "class" of properties.
On Tue, Nov 13, 2018 at 01:27:00PM +0200, Mika Westerberg wrote: [...] > > To be frank the concept (and Microsoft _DSD bindings) seems a bit vague > > and not thoroughly defined and I would question its detection at > > PCI/ACPI core level, I would hope this can be clarified at ACPI > > specification level, at least. > > I guess that is the way they envision to use _DSD. Instead of having > single UUID that covers all properties (like what we have with device > properties) they have one UUID per property "class". I certainly hope we > don't need to keep extending prp_guids[] array each time they invent > another "class" of properties. It is even worse than that. This is a unilateral/obscure change that won't be part of ACPI specifications (I guess it was easier to add a UUID than add this to the ACPI specifications through the AWSG) but it is still supposed to be applicable to ACPI PCI bindings on any platforms/arches; this way of adding bindings does not work and it has to be rectified. Lorenzo
On Tue, Nov 13, 2018 at 11:45:36AM +0000, Lorenzo Pieralisi wrote: > On Tue, Nov 13, 2018 at 01:27:00PM +0200, Mika Westerberg wrote: > > [...] > > > > To be frank the concept (and Microsoft _DSD bindings) seems a bit vague > > > and not thoroughly defined and I would question its detection at > > > PCI/ACPI core level, I would hope this can be clarified at ACPI > > > specification level, at least. > > > > I guess that is the way they envision to use _DSD. Instead of having > > single UUID that covers all properties (like what we have with device > > properties) they have one UUID per property "class". I certainly hope we > > don't need to keep extending prp_guids[] array each time they invent > > another "class" of properties. > > It is even worse than that. This is a unilateral/obscure change that > won't be part of ACPI specifications (I guess it was easier to add a > UUID than add this to the ACPI specifications through the AWSG) but it > is still supposed to be applicable to ACPI PCI bindings on any > platforms/arches; this way of adding bindings does not work and it > has to be rectified. I agree. For the existing property "classes" such as the one here I don't think we can do anything. There are systems already with these included in their ACPI tables. I wonder if you have any objections regarding this patch?
On Thu, Nov 15, 2018 at 12:22:39PM +0200, Mika Westerberg wrote: > On Tue, Nov 13, 2018 at 11:45:36AM +0000, Lorenzo Pieralisi wrote: > > On Tue, Nov 13, 2018 at 01:27:00PM +0200, Mika Westerberg wrote: > > > > [...] > > > > > > To be frank the concept (and Microsoft _DSD bindings) seems a bit vague > > > > and not thoroughly defined and I would question its detection at > > > > PCI/ACPI core level, I would hope this can be clarified at ACPI > > > > specification level, at least. > > > > > > I guess that is the way they envision to use _DSD. Instead of having > > > single UUID that covers all properties (like what we have with device > > > properties) they have one UUID per property "class". I certainly hope we > > > don't need to keep extending prp_guids[] array each time they invent > > > another "class" of properties. > > > > It is even worse than that. This is a unilateral/obscure change that > > won't be part of ACPI specifications (I guess it was easier to add a > > UUID than add this to the ACPI specifications through the AWSG) but it > > is still supposed to be applicable to ACPI PCI bindings on any > > platforms/arches; this way of adding bindings does not work and it > > has to be rectified. > > I agree. > > For the existing property "classes" such as the one here I don't think > we can do anything. There are systems already with these included in > their ACPI tables. > > I wonder if you have any objections regarding this patch? I have strong objections to the way these bindings have been forced upon everybody; if that's the way *generic* ACPI bindings are specified I wonder why there still exists an ACPI specification and related working group. I personally (but that's Bjorn and Rafael choice) think that this is not a change that belongs in PCI core, ACPI bindings are ill-defined and device tree bindings are non-existing. At the very least Microsoft should be asked to publish and discuss these bindings within the ACPI and UEFI forums. Lorenzo
On Thu, Nov 15, 2018 at 11:13:56AM +0000, Lorenzo Pieralisi wrote: > I have strong objections to the way these bindings have been forced upon > everybody; if that's the way *generic* ACPI bindings are specified I > wonder why there still exists an ACPI specification and related working > group. > > I personally (but that's Bjorn and Rafael choice) think that this is > not a change that belongs in PCI core, ACPI bindings are ill-defined > and device tree bindings are non-existing. Any idea where should I put it then? These systems are already out there and we need to support them one way or another. > At the very least Microsoft should be asked to publish and discuss > these bindings within the ACPI and UEFI forums. These bindings are public, see here: https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports However, they are not part of the ACPI spec as you say.
On Thu, Nov 15, 2018 at 01:37:37PM +0200, Mika Westerberg wrote: > On Thu, Nov 15, 2018 at 11:13:56AM +0000, Lorenzo Pieralisi wrote: > > I have strong objections to the way these bindings have been forced upon > > everybody; if that's the way *generic* ACPI bindings are specified I > > wonder why there still exists an ACPI specification and related working > > group. > > > > I personally (but that's Bjorn and Rafael choice) think that this is > > not a change that belongs in PCI core, ACPI bindings are ill-defined > > and device tree bindings are non-existing. > > Any idea where should I put it then? These systems are already out there > and we need to support them one way or another. I suppose those are all Thunderbolt, so could be handled by the existing ->is_thunderbolt bit? It was said in this thread that ->is_external is more generic in that it could also be used on PCIe slots, however that use case doesn't appear to lend itself to the "plug in while laptop owner is getting coffee" attack. To access PCIe slots on a server you normally need access to a data center. On a desktop, you usually have to open the case, by which time the coffee may already have been fetched. So frankly the binding seems a bit over-engineered to me and yet another thing that BIOS writers may get wrong. Well, just my 2 cents anyway. Lukas
On Thu, Nov 15, 2018 at 01:07:36PM +0100, Lukas Wunner wrote: > On Thu, Nov 15, 2018 at 01:37:37PM +0200, Mika Westerberg wrote: > > On Thu, Nov 15, 2018 at 11:13:56AM +0000, Lorenzo Pieralisi wrote: > > > I have strong objections to the way these bindings have been forced upon > > > everybody; if that's the way *generic* ACPI bindings are specified I > > > wonder why there still exists an ACPI specification and related working > > > group. > > > > > > I personally (but that's Bjorn and Rafael choice) think that this is > > > not a change that belongs in PCI core, ACPI bindings are ill-defined > > > and device tree bindings are non-existing. > > > > Any idea where should I put it then? These systems are already out there > > and we need to support them one way or another. > > I suppose those are all Thunderbolt, so could be handled by the > existing ->is_thunderbolt bit? > > It was said in this thread that ->is_external is more generic in > that it could also be used on PCIe slots, however that use case > doesn't appear to lend itself to the "plug in while laptop owner > is getting coffee" attack. To access PCIe slots on a server you > normally need access to a data center. On a desktop, you usually > have to open the case, by which time the coffee may already have > been fetched. So frankly the binding seems a bit over-engineered > to me and yet another thing that BIOS writers may get wrong. I would not say it should include PCIe slots but there are other cables that carry PCIe and I was thinking we could make it to support those as well. I have no problem using is_thunderbolt here, though if we don't want to support non-Thunderbolt external devices this way. However, the question here is more that where I should put the _DSD parsing code if it is not suitable to be placed inside PCI/ACPI core as I've done in this patch? ;-)
On Thu, Nov 15, 2018 at 02:16:27PM +0200, Mika Westerberg wrote: > On Thu, Nov 15, 2018 at 01:07:36PM +0100, Lukas Wunner wrote: > > On Thu, Nov 15, 2018 at 01:37:37PM +0200, Mika Westerberg wrote: > > > On Thu, Nov 15, 2018 at 11:13:56AM +0000, Lorenzo Pieralisi wrote: > > > > I have strong objections to the way these bindings have been forced upon > > > > everybody; if that's the way *generic* ACPI bindings are specified I > > > > wonder why there still exists an ACPI specification and related working > > > > group. > > > > > > > > I personally (but that's Bjorn and Rafael choice) think that this is > > > > not a change that belongs in PCI core, ACPI bindings are ill-defined > > > > and device tree bindings are non-existing. > > > > > > Any idea where should I put it then? These systems are already out there > > > and we need to support them one way or another. > > > > I suppose those are all Thunderbolt, so could be handled by the > > existing ->is_thunderbolt bit? > > > > It was said in this thread that ->is_external is more generic in > > that it could also be used on PCIe slots, however that use case > > doesn't appear to lend itself to the "plug in while laptop owner > > is getting coffee" attack. To access PCIe slots on a server you > > normally need access to a data center. On a desktop, you usually > > have to open the case, by which time the coffee may already have > > been fetched. So frankly the binding seems a bit over-engineered > > to me and yet another thing that BIOS writers may get wrong. > > I would not say it should include PCIe slots but there are other cables > that carry PCIe and I was thinking we could make it to support those as > well. > > I have no problem using is_thunderbolt here, though if we don't want to > support non-Thunderbolt external devices this way. > > However, the question here is more that where I should put the _DSD > parsing code if it is not suitable to be placed inside PCI/ACPI core as > I've done in this patch? ;-) Do you really need to parse it if the dev->is_thunderbolt check is enough ? Thanks, Lorenzo
On Thu, Nov 15, 2018 at 7:46 PM Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Thu, Nov 15, 2018 at 02:16:27PM +0200, Mika Westerberg wrote: > > On Thu, Nov 15, 2018 at 01:07:36PM +0100, Lukas Wunner wrote: > > > On Thu, Nov 15, 2018 at 01:37:37PM +0200, Mika Westerberg wrote: > > > > On Thu, Nov 15, 2018 at 11:13:56AM +0000, Lorenzo Pieralisi wrote: > > > > > I have strong objections to the way these bindings have been forced upon > > > > > everybody; if that's the way *generic* ACPI bindings are specified I > > > > > wonder why there still exists an ACPI specification and related working > > > > > group. > > > > > > > > > > I personally (but that's Bjorn and Rafael choice) think that this is > > > > > not a change that belongs in PCI core, ACPI bindings are ill-defined > > > > > and device tree bindings are non-existing. > > > > > > > > Any idea where should I put it then? These systems are already out there > > > > and we need to support them one way or another. > > > > > > I suppose those are all Thunderbolt, so could be handled by the > > > existing ->is_thunderbolt bit? > > > > > > It was said in this thread that ->is_external is more generic in > > > that it could also be used on PCIe slots, however that use case > > > doesn't appear to lend itself to the "plug in while laptop owner > > > is getting coffee" attack. To access PCIe slots on a server you > > > normally need access to a data center. On a desktop, you usually > > > have to open the case, by which time the coffee may already have > > > been fetched. So frankly the binding seems a bit over-engineered > > > to me and yet another thing that BIOS writers may get wrong. > > > > I would not say it should include PCIe slots but there are other cables > > that carry PCIe and I was thinking we could make it to support those as > > well. > > > > I have no problem using is_thunderbolt here, though if we don't want to > > support non-Thunderbolt external devices this way. > > > > However, the question here is more that where I should put the _DSD > > parsing code if it is not suitable to be placed inside PCI/ACPI core as > > I've done in this patch? ;-) > > Do you really need to parse it if the dev->is_thunderbolt check is enough ? From what I know, there are more devices that suffer from similar security issues like Thunderbolt, e.g. FireWire [1]. My assumption is that the same protection may be applied to such devices too, even if currently it sounds like vendors care mostly about Thunderbolt (probably because it removes the need for user approval for device connection; it becames a simple plug-and-play experience). Thus, I don't think binding it with dev->is_thunderbolt is the correct thing to do. [1] https://github.com/carmaa/inception
On Thu, Nov 15, 2018 at 05:46:08PM +0000, Lorenzo Pieralisi wrote:
> Do you really need to parse it if the dev->is_thunderbolt check is enough ?
Yes, we need to parse it one way or another. dev->is_thunderbolt is
based on heuristics which do not apply anymore when the thing gets
integrated in the SoC.
The _DSD is there already (on existing systems) and is being used by
Windows so I don't understand why we cannot take advantage of it? Every
new system with Thunderbolt ports will have it.
On Thu, Nov 15, 2018 at 07:58:13PM +0200, Yehezkel Bernat wrote: > From what I know, there are more devices that suffer from similar security > issues like Thunderbolt, e.g. FireWire [1]. > My assumption is that the same protection may be applied to such devices too, > even if currently it sounds like vendors care mostly about Thunderbolt (probably > because it removes the need for user approval for device connection; it becames > a simple plug-and-play experience). FireWire is kind of different but there are connectors such as ExpressCard and NVMe (over U.2 connector) which carry PCIe and are relatively easy to access without need for a screwdriver. AFAIK some eGPUs are also using some other proprietary (non-TBT) connector that carries PCIe. I was thinking we could cover all these with is_external filling them based on the _DSD or some other means in the kernel. We would then deal all such devices as "untrusted" by default. > Thus, I don't think binding it with dev->is_thunderbolt is the correct > thing to do. One option that I suggested already is that we keep both and mark all is_thunderbolt devices as is_external as well. But I guess this is up to Bjorn and Rafael to decide :-)
On Thu, Nov 15, 2018 at 09:10:26PM +0200, Mika Westerberg wrote: > I was thinking we could cover all these with is_external filling them > based on the _DSD or some other means in the kernel. > > We would then deal all such devices as "untrusted" by default. Tinfoil hat on, even internal devices could be malicious. What's the downside of enabling the feature for everything?
On Thu, Nov 15, 2018 at 08:27:41PM +0100, Lukas Wunner wrote: > On Thu, Nov 15, 2018 at 09:10:26PM +0200, Mika Westerberg wrote: > > I was thinking we could cover all these with is_external filling them > > based on the _DSD or some other means in the kernel. > > > > We would then deal all such devices as "untrusted" by default. > > Tinfoil hat on, even internal devices could be malicious. > What's the downside of enabling the feature for everything? Mostly performance, I think. That's the main reason we put all non external devices to passthrough IOMMU mode.
> -----Original Message----- > From: Mika Westerberg <mika.westerberg@linux.intel.com> > Sent: Thursday, November 15, 2018 1:01 PM > To: Lorenzo Pieralisi > Cc: Lukas Wunner; iommu@lists.linux-foundation.org; Joerg Roedel; David > Woodhouse; Lu Baolu; Ashok Raj; Bjorn Helgaas; Rafael J. Wysocki; Jacob jun Pan; > Andreas Noever; Michael Jamet; Yehezkel Bernat; Christian Kellner; Limonciello, > Mario; Anthony Wong; linux-acpi@vger.kernel.org; linux-pci@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices > > > [EXTERNAL EMAIL] > > On Thu, Nov 15, 2018 at 05:46:08PM +0000, Lorenzo Pieralisi wrote: > > Do you really need to parse it if the dev->is_thunderbolt check is enough ? > > Yes, we need to parse it one way or another. dev->is_thunderbolt is > based on heuristics which do not apply anymore when the thing gets > integrated in the SoC. > > The _DSD is there already (on existing systems) and is being used by > Windows so I don't understand why we cannot take advantage of it? Every > new system with Thunderbolt ports will have it. Furthermore it's entirely in the BIOS writers best interest to do this correctly as it applies proper policy on Windows as well. I wouldn't be surprised if WHCK failed if it was done wrong.
On Thu, Nov 15, 2018 at 09:00:54PM +0200, Mika Westerberg wrote: > On Thu, Nov 15, 2018 at 05:46:08PM +0000, Lorenzo Pieralisi wrote: > > Do you really need to parse it if the dev->is_thunderbolt check is enough ? > > Yes, we need to parse it one way or another. dev->is_thunderbolt is > based on heuristics which do not apply anymore when the thing gets > integrated in the SoC. > > The _DSD is there already (on existing systems) and is being used by > Windows so I don't understand why we cannot take advantage of it? Every > new system with Thunderbolt ports will have it. Just to clarify a bit. We can use is_thunderbolt in place of is_external if we don't want to deal with other possible "external" devices right now. However, we still need to parse the _DSD and based on that fill the is_thunderbolt for these devices (same way we do for is_external in this series). So basically we just get rid of the is_external flag and use is_thunderbolt instead.
On Thu, Nov 15, 2018 at 09:10:26PM +0200, Mika Westerberg wrote: > FireWire is kind of different but there are connectors such as > ExpressCard and NVMe (over U.2 connector) which carry PCIe and are > relatively easy to access without need for a screwdriver. AFAIK some > eGPUs are also using some other proprietary (non-TBT) connector that > carries PCIe. U.2 is a data center internal form factor with hot plug capability. If you enable an iommu for that by default you will make a lot of people very unhappy. More importantly NVMe is now used for the current/next generation Compact Flash and SD cards, which contain full PCIe gen 3 links.
On Fri, Nov 16, 2018 at 01:18:04AM -0800, Christoph Hellwig wrote: > On Thu, Nov 15, 2018 at 09:10:26PM +0200, Mika Westerberg wrote: > > FireWire is kind of different but there are connectors such as > > ExpressCard and NVMe (over U.2 connector) which carry PCIe and are > > relatively easy to access without need for a screwdriver. AFAIK some > > eGPUs are also using some other proprietary (non-TBT) connector that > > carries PCIe. > > U.2 is a data center internal form factor with hot plug capability. If > you enable an iommu for that by default you will make a lot of people > very unhappy. Well, it needs the other bit in ACPI DMAR table to be enabled by default so I don't think anyone in data center domain will notice ;-) > More importantly NVMe is now used for the current/next generation > Compact Flash and SD cards, which contain full PCIe gen 3 links. OK, thanks for the information - I did not know that. I guess those belong to the "external" category as well.
On Thu, Nov 15, 2018 at 07:33:54PM +0000, Mario.Limonciello@dell.com wrote: > > > > -----Original Message----- > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > > Sent: Thursday, November 15, 2018 1:01 PM > > To: Lorenzo Pieralisi > > Cc: Lukas Wunner; iommu@lists.linux-foundation.org; Joerg Roedel; David > > Woodhouse; Lu Baolu; Ashok Raj; Bjorn Helgaas; Rafael J. Wysocki; Jacob jun Pan; > > Andreas Noever; Michael Jamet; Yehezkel Bernat; Christian Kellner; Limonciello, > > Mario; Anthony Wong; linux-acpi@vger.kernel.org; linux-pci@vger.kernel.org; linux- > > kernel@vger.kernel.org > > Subject: Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices > > > > > > [EXTERNAL EMAIL] > > > > On Thu, Nov 15, 2018 at 05:46:08PM +0000, Lorenzo Pieralisi wrote: > > > Do you really need to parse it if the dev->is_thunderbolt check is enough ? > > > > Yes, we need to parse it one way or another. dev->is_thunderbolt is > > based on heuristics which do not apply anymore when the thing gets > > integrated in the SoC. > > > > The _DSD is there already (on existing systems) and is being used by > > Windows so I don't understand why we cannot take advantage of it? Every > > new system with Thunderbolt ports will have it. We have different opinions on this, there is no point in me reiterating it over and over, I am against the approach taken to solve this problem first in defining the bindings outside the ACPI specifications and second by acquiescing to what has been done so that it will be done over and over again. I will raise the point in the appropriate forum, it is up to Bjorn and Rafael to decide on this patch. Lorenzo
On Friday, November 16, 2018 11:57:38 AM CET Lorenzo Pieralisi wrote: > On Thu, Nov 15, 2018 at 07:33:54PM +0000, Mario.Limonciello@dell.com wrote: > > > > > > > -----Original Message----- > > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > > > Sent: Thursday, November 15, 2018 1:01 PM > > > To: Lorenzo Pieralisi > > > Cc: Lukas Wunner; iommu@lists.linux-foundation.org; Joerg Roedel; David > > > Woodhouse; Lu Baolu; Ashok Raj; Bjorn Helgaas; Rafael J. Wysocki; Jacob jun Pan; > > > Andreas Noever; Michael Jamet; Yehezkel Bernat; Christian Kellner; Limonciello, > > > Mario; Anthony Wong; linux-acpi@vger.kernel.org; linux-pci@vger.kernel.org; linux- > > > kernel@vger.kernel.org > > > Subject: Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices > > > > > > > > > [EXTERNAL EMAIL] > > > > > > On Thu, Nov 15, 2018 at 05:46:08PM +0000, Lorenzo Pieralisi wrote: > > > > Do you really need to parse it if the dev->is_thunderbolt check is enough ? > > > > > > Yes, we need to parse it one way or another. dev->is_thunderbolt is > > > based on heuristics which do not apply anymore when the thing gets > > > integrated in the SoC. > > > > > > The _DSD is there already (on existing systems) and is being used by > > > Windows so I don't understand why we cannot take advantage of it? Every > > > new system with Thunderbolt ports will have it. > > We have different opinions on this, there is no point in me reiterating > it over and over, I am against the approach taken to solve this problem > first in defining the bindings outside the ACPI specifications and > second by acquiescing to what has been done so that it will be done > over and over again. Arguably, however, we are on the receiving end of things here and even if we don't use this binding, that won't buy us anything (but it will introduce a fair amount of disappointment among both users and OEMs). If Windows uses it, then systems will ship with it regardless of what Linux does with it, so your "acquiescing to what has been done" argument leads to nowhere in this particular case. It's just a matter of whether or not Linux will provide the same level of functionality as Windows with respect to this and IMO it would be impractical to refuse to do that for purely formal reasons. > I will raise the point in the appropriate forum, it is up to Bjorn > and Rafael to decide on this patch. For the record, my opinion is that there's a little choice on whether or not to use this extra information that firmware is willing to give us. It could be defined in a better way and so on, but since it is in use anyway, we really have nothing to gain by refusing to use it. Now, where the handling of it belongs to is a separate matter that should be decided on its own. Thanks, Rafael
On Tue, Nov 20, 2018 at 10:43:35PM +0100, Rafael J. Wysocki wrote: > On Friday, November 16, 2018 11:57:38 AM CET Lorenzo Pieralisi wrote: > > On Thu, Nov 15, 2018 at 07:33:54PM +0000, Mario.Limonciello@dell.com wrote: > > > > > > > > > > -----Original Message----- > > > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > Sent: Thursday, November 15, 2018 1:01 PM > > > > To: Lorenzo Pieralisi > > > > Cc: Lukas Wunner; iommu@lists.linux-foundation.org; Joerg Roedel; David > > > > Woodhouse; Lu Baolu; Ashok Raj; Bjorn Helgaas; Rafael J. Wysocki; Jacob jun Pan; > > > > Andreas Noever; Michael Jamet; Yehezkel Bernat; Christian Kellner; Limonciello, > > > > Mario; Anthony Wong; linux-acpi@vger.kernel.org; linux-pci@vger.kernel.org; linux- > > > > kernel@vger.kernel.org > > > > Subject: Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices > > > > > > > > > > > > [EXTERNAL EMAIL] > > > > > > > > On Thu, Nov 15, 2018 at 05:46:08PM +0000, Lorenzo Pieralisi wrote: > > > > > Do you really need to parse it if the dev->is_thunderbolt check is enough ? > > > > > > > > Yes, we need to parse it one way or another. dev->is_thunderbolt is > > > > based on heuristics which do not apply anymore when the thing gets > > > > integrated in the SoC. > > > > > > > > The _DSD is there already (on existing systems) and is being used by > > > > Windows so I don't understand why we cannot take advantage of it? Every > > > > new system with Thunderbolt ports will have it. > > > > We have different opinions on this, there is no point in me reiterating > > it over and over, I am against the approach taken to solve this problem > > first in defining the bindings outside the ACPI specifications and > > second by acquiescing to what has been done so that it will be done > > over and over again. > > Arguably, however, we are on the receiving end of things here and even if > we don't use this binding, that won't buy us anything (but it will introduce > a fair amount of disappointment among both users and OEMs). > > If Windows uses it, then systems will ship with it regardless of what Linux > does with it, so your "acquiescing to what has been done" argument leads to > nowhere in this particular case. It's just a matter of whether or not > Linux will provide the same level of functionality as Windows with respect > to this and IMO it would be impractical to refuse to do that for purely > formal reasons. > > > I will raise the point in the appropriate forum, it is up to Bjorn > > and Rafael to decide on this patch. > > For the record, my opinion is that there's a little choice on whether > or not to use this extra information that firmware is willing to give > us. It could be defined in a better way and so on, but since it is in > use anyway, we really have nothing to gain by refusing to use it. AFAIK PCI firmware bindings should go into PCI firmware specifications, not Microsoft webpages. If we deviate from this model there is no way to tell whether that extra information is right or wrong, it is not necessarily about this patch, it is about changing the way these bindings are deployed in future systems. > Now, where the handling of it belongs to is a separate matter that should be > decided on its own. I think that the way these bindings were deployed is wrong, I agree this is not the right forum to discuss that though. What you will do with this patch is not my call anyway, I just expressed my opinion. Thanks, Lorenzo
On Fri, Nov 16, 2018 at 11:32:10AM +0200, Mika Westerberg wrote: > On Fri, Nov 16, 2018 at 01:18:04AM -0800, Christoph Hellwig wrote: > > On Thu, Nov 15, 2018 at 09:10:26PM +0200, Mika Westerberg wrote: > > > FireWire is kind of different but there are connectors such as > > > ExpressCard and NVMe (over U.2 connector) which carry PCIe and are > > > relatively easy to access without need for a screwdriver. AFAIK some > > > eGPUs are also using some other proprietary (non-TBT) connector that > > > carries PCIe. > > > > U.2 is a data center internal form factor with hot plug capability. If > > you enable an iommu for that by default you will make a lot of people > > very unhappy. > > Well, it needs the other bit in ACPI DMAR table to be enabled by default > so I don't think anyone in data center domain will notice ;-) > > > More importantly NVMe is now used for the current/next generation > > Compact Flash and SD cards, which contain full PCIe gen 3 links. > > OK, thanks for the information - I did not know that. I guess those > belong to the "external" category as well. We had an internal discussion regarding this and it was suggested that the new flag is called "is_untrusted" instead of "is_external". This covers Thunderbolt devices currently but can be extend to any other PCIe device such as "SD express" ones. When IOMMU is turned on it will then make sure devices with "is_untrusted" set are always using full IOMMU protection. Any comments, objections? I was going to send v2 with this change included.
On Thu, Nov 22, 2018 at 12:48:40PM +0200, Mika Westerberg wrote: > We had an internal discussion regarding this and it was suggested that > the new flag is called "is_untrusted" instead of "is_external". This > covers Thunderbolt devices currently but can be extend to any other PCIe > device such as "SD express" ones. When IOMMU is turned on it will then > make sure devices with "is_untrusted" set are always using full IOMMU > protection. > > Any comments, objections? I was going to send v2 with this change > included. Sounds good to me as long as it goes along with a nice fat comment explaining it.
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c index 8c7c4583b52d..4bdad32f62c8 100644 --- a/drivers/acpi/property.c +++ b/drivers/acpi/property.c @@ -31,6 +31,9 @@ static const guid_t prp_guids[] = { /* Hotplug in D3 GUID: 6211e2c0-58a3-4af3-90e1-927a4e0c55a4 */ GUID_INIT(0x6211e2c0, 0x58a3, 0x4af3, 0x90, 0xe1, 0x92, 0x7a, 0x4e, 0x0c, 0x55, 0xa4), + /* External facing port GUID: efcc06cc-73ac-4bc3-bff0-76143807c389 */ + GUID_INIT(0xefcc06cc, 0x73ac, 0x4bc3, + 0xbf, 0xf0, 0x76, 0x14, 0x38, 0x07, 0xc3, 0x89), }; static const guid_t ads_guid = diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index 2a4aa6468579..fc193279d24d 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -789,6 +789,18 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev, ACPI_FREE(obj); } +static void pci_acpi_set_external(struct pci_dev *dev) +{ + u8 val; + + if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) + return; + if (device_property_read_u8(&dev->dev, "ExternalFacingPort", &val)) + return; + + dev->is_external = val == 1; +} + static void pci_acpi_setup(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); @@ -803,6 +815,7 @@ static void pci_acpi_setup(struct device *dev) set_dev_node(dev, node); pci_acpi_optimize_delay(pci_dev, adev->handle); + pci_acpi_set_external(pci_dev); pci_acpi_add_pm_notifier(adev, pci_dev); if (!adev->wakeup.flags.valid) diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index b1c05b5054a0..f1db195a4a90 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1378,6 +1378,27 @@ static void set_pcie_thunderbolt(struct pci_dev *dev) } } +static void set_pcie_external(struct pci_dev *dev) +{ + struct pci_dev *parent; + + /* + * Walk up the device hierarchy and check for any upstream + * bridge that has is_external_facing set to true. This means + * the hierarchy is below PCIe port that is exposed externally + * (such as Thunderbolt). + */ + parent = pci_upstream_bridge(dev); + while (parent) { + if (parent->is_external) { + dev->is_external = true; + break; + } + + parent = pci_upstream_bridge(parent); + } +} + /** * pci_ext_cfg_is_aliased - Is ext config space just an alias of std config? * @dev: PCI device @@ -1638,6 +1659,8 @@ int pci_setup_device(struct pci_dev *dev) /* Need to have dev->cfg_size ready */ set_pcie_thunderbolt(dev); + set_pcie_external(dev); + /* "Unknown power state" */ dev->current_state = PCI_UNKNOWN; diff --git a/include/linux/pci.h b/include/linux/pci.h index 11c71c4ecf75..e1c0e032da55 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -396,6 +396,7 @@ struct pci_dev { unsigned int is_hotplug_bridge:1; unsigned int shpc_managed:1; /* SHPC owned by shpchp */ unsigned int is_thunderbolt:1; /* Thunderbolt controller */ + unsigned int is_external:1; /* External PCIe device */ unsigned int __aer_firmware_first_valid:1; unsigned int __aer_firmware_first:1; unsigned int broken_intx_masking:1; /* INTx masking can't be used */
Recent systems shipping Windows 10 version 1803 or later may support IOMMU based DMA protection. This means that the platform utilizes IOMMU to prevent DMA attacks over externally exposed PCIe root ports (typically Thunderbolt ports) The system BIOS marks these PCIe root ports as being externally facing ports by implementing following ACPI _DSD under the root port in question: Name (_DSD, Package () { ToUUID ("efcc06cc-73ac-4bc3-bff0-76143807c389"), Package () { Package () {"ExternalFacingPort", 1}, Package () {"UID", 0 } } }) This is documented [1]. To make it possible for IOMMU code to identify these external devices, we look up for this property and mark all children devices (including the root port itself) with a new flag (->is_external). [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-externally-exposed-pcie-root-ports Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/acpi/property.c | 3 +++ drivers/pci/pci-acpi.c | 13 +++++++++++++ drivers/pci/probe.c | 23 +++++++++++++++++++++++ include/linux/pci.h | 1 + 4 files changed, 40 insertions(+)