| Message ID | 20200707224604.3737893-4-rajatja@google.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v4,1/4] PCI: Move pci_enable_acs() and its dependencies up in pci.c | expand |
On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote: > When enabling ACS, enable translation blocking for external facing ports > and untrusted devices. > > Signed-off-by: Rajat Jain <rajatja@google.com> > --- > v4: Add braces to avoid warning from kernel robot > print warning for only external-facing devices. > v3: print warning if ACS_TB not supported on external-facing/untrusted ports. > Minor code comments fixes. > v2: Commit log change > > drivers/pci/pci.c | 8 ++++++++ > drivers/pci/quirks.c | 15 +++++++++++++++ > 2 files changed, 23 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 73a8627822140..a5a6bea7af7ce 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev) > /* Upstream Forwarding */ > ctrl |= (cap & PCI_ACS_UF); > > + /* Enable Translation Blocking for external devices */ > + if (dev->external_facing || dev->untrusted) { > + if (cap & PCI_ACS_TB) > + ctrl |= PCI_ACS_TB; > + else if (dev->external_facing) > + pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n"); > + } IIUC, this means that external devices can *never* use ATS and can never cache translations. And (I guess, I'm not an expert) it can also never use the Page Request Services? Is this what we want? Do we have any idea how many external devices this will affect or how much of a performance impact they will see? Do we need some kind of override or mechanism to authenticate certain devices so they can use ATS and PRI? If we do decide this is the right thing to do, I think we need to expand the commit log a bit, because this is potentially a significant user-visible change. > pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl); > } > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index b341628e47527..bb22b46c1d719 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev) > } > } > > +/* > + * Currently this quirk does the equivalent of > + * PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF > + * > + * TODO: This quirk also needs to do equivalent of PCI_ACS_TB, > + * if dev->external_facing || dev->untrusted > + */ > static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev) > { > if (!pci_quirk_intel_pch_acs_match(dev)) > @@ -4973,6 +4980,14 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev) > ctrl |= (cap & PCI_ACS_CR); > ctrl |= (cap & PCI_ACS_UF); > > + /* Enable Translation Blocking for external devices */ > + if (dev->external_facing || dev->untrusted) { > + if (cap & PCI_ACS_TB) > + ctrl |= PCI_ACS_TB; > + else if (dev->external_facing) > + pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n"); > + } > + > pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl); > > pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n"); > -- > 2.27.0.212.ge8ba1cc988-goog >
Hi Bjorn On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote: > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote: > > When enabling ACS, enable translation blocking for external facing ports > > and untrusted devices. > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > --- > > v4: Add braces to avoid warning from kernel robot > > print warning for only external-facing devices. > > v3: print warning if ACS_TB not supported on external-facing/untrusted ports. > > Minor code comments fixes. > > v2: Commit log change > > > > drivers/pci/pci.c | 8 ++++++++ > > drivers/pci/quirks.c | 15 +++++++++++++++ > > 2 files changed, 23 insertions(+) > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index 73a8627822140..a5a6bea7af7ce 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev) > > /* Upstream Forwarding */ > > ctrl |= (cap & PCI_ACS_UF); > > > > + /* Enable Translation Blocking for external devices */ > > + if (dev->external_facing || dev->untrusted) { > > + if (cap & PCI_ACS_TB) > > + ctrl |= PCI_ACS_TB; > > + else if (dev->external_facing) > > + pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n"); > > + } > > IIUC, this means that external devices can *never* use ATS and can > never cache translations. And (I guess, I'm not an expert) it can > also never use the Page Request Services? Yep, sounds like it. > > Is this what we want? Do we have any idea how many external devices > this will affect or how much of a performance impact they will see? > > Do we need some kind of override or mechanism to authenticate certain > devices so they can use ATS and PRI? Sounds like we would need some form of an allow-list to start with so we can have something in the interim. I suppose a future platform might have a facilty to ensure ATS is secure and authenticated we could enable for all of devices in the system, in addition to PCI CMA/IDE. I think having a global override to enable all devices so platform can switch to current behavior, or maybe via a cmdline switch.. as much as we have a billion of those, it still gives an option in case someone needs it. > > If we do decide this is the right thing to do, I think we need to > expand the commit log a bit, because this is potentially a significant > user-visible change. > > > pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl); > > } > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > index b341628e47527..bb22b46c1d719 100644 > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev) > > } > > } > > > > +/* > > + * Currently this quirk does the equivalent of > > + * PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF > > + * > > + * TODO: This quirk also needs to do equivalent of PCI_ACS_TB, > > + * if dev->external_facing || dev->untrusted > > + */ > > static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev) > > { > > if (!pci_quirk_intel_pch_acs_match(dev)) > > @@ -4973,6 +4980,14 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev) > > ctrl |= (cap & PCI_ACS_CR); > > ctrl |= (cap & PCI_ACS_UF); > > > > + /* Enable Translation Blocking for external devices */ > > + if (dev->external_facing || dev->untrusted) { > > + if (cap & PCI_ACS_TB) > > + ctrl |= PCI_ACS_TB; > > + else if (dev->external_facing) > > + pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n"); > > + } > > + > > pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl); > > > > pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n"); > > -- > > 2.27.0.212.ge8ba1cc988-goog > >
Hello, On Fri, Jul 10, 2020 at 2:29 PM Raj, Ashok <ashok.raj@intel.com> wrote: > > Hi Bjorn > > > On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote: > > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote: > > > When enabling ACS, enable translation blocking for external facing ports > > > and untrusted devices. > > > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > > --- > > > v4: Add braces to avoid warning from kernel robot > > > print warning for only external-facing devices. > > > v3: print warning if ACS_TB not supported on external-facing/untrusted ports. > > > Minor code comments fixes. > > > v2: Commit log change > > > > > > drivers/pci/pci.c | 8 ++++++++ > > > drivers/pci/quirks.c | 15 +++++++++++++++ > > > 2 files changed, 23 insertions(+) > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > index 73a8627822140..a5a6bea7af7ce 100644 > > > --- a/drivers/pci/pci.c > > > +++ b/drivers/pci/pci.c > > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev) > > > /* Upstream Forwarding */ > > > ctrl |= (cap & PCI_ACS_UF); > > > > > > + /* Enable Translation Blocking for external devices */ > > > + if (dev->external_facing || dev->untrusted) { > > > + if (cap & PCI_ACS_TB) > > > + ctrl |= PCI_ACS_TB; > > > + else if (dev->external_facing) > > > + pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n"); > > > + } > > > > IIUC, this means that external devices can *never* use ATS > and can > > never cache translations. Yes, but it already exists today (and this patch doesn't change that): 521376741b2c2 "PCI/ATS: Only enable ATS for trusted devices" IMHO any external device trying to send ATS traffic despite having ATS disabled should count as a bad intent. And this patch is trying to plug that loophole, by blocking the AT traffic from devices that we do not expect to see AT from anyway. Do you see any case where this is not true? > And (I guess, I'm not an expert) it can > > also never use the Page Request Services? > > Yep, sounds like it. Yes, from spec "Address Translation Services" Rev 1.1: "...a device that supports ATS need not support PRI, but PRI is dependent on ATS’s capabilities." (So no ATS = No PRI). > > > > > Is this what we want? Do we have any idea how many external devices > > this will affect or how much of a performance impact they will see? > > > > Do we need some kind of override or mechanism to authenticate certain > > devices so they can use ATS and PRI? > > Sounds like we would need some form of an allow-list to start with so we > can have something in the interim. I assume what is being referred to, is an escape hatch to enable ATS on certain given "external-facing" ports (and devices downstream on that port). Do we really think a *per-port* control for ATS may be needed? I can add if there is consensus about this. > > I suppose a future platform might have a facilty to ensure ATS is secure and > authenticated we could enable for all of devices in the system, in addition > to PCI CMA/IDE. > > I think having a global override to enable all devices so platform can > switch to current behavior, or maybe via a cmdline switch.. as much as we > have a billion of those, it still gives an option in case someone needs it. Currently: pci.noats => No ATS on all PCI devices. (Absense of pci.noats): ATS on all PCI devices, EXCEPT external devices. I can look to add another parameter that is synonymous to "trust-external-pci-devices" that can keep ATS enabled on external ports as well. I think this is better than an allow-list of only certain ports, because most likely an admin will trust all its external ports, or not. Also, we can add this global override and may be add a more granular control later, if and when really needed. Thanks, Rajat > > > > > > > If we do decide this is the right thing to do, I think we need to > > expand the commit log a bit, because this is potentially a significant > > user-visible change. > > > > > pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl); > > > } > > > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > > index b341628e47527..bb22b46c1d719 100644 > > > --- a/drivers/pci/quirks.c > > > +++ b/drivers/pci/quirks.c > > > @@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev) > > > } > > > } > > > > > > +/* > > > + * Currently this quirk does the equivalent of > > > + * PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF > > > + * > > > + * TODO: This quirk also needs to do equivalent of PCI_ACS_TB, > > > + * if dev->external_facing || dev->untrusted > > > + */ > > > static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev) > > > { > > > if (!pci_quirk_intel_pch_acs_match(dev)) > > > @@ -4973,6 +4980,14 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev) > > > ctrl |= (cap & PCI_ACS_CR); > > > ctrl |= (cap & PCI_ACS_UF); > > > > > > + /* Enable Translation Blocking for external devices */ > > > + if (dev->external_facing || dev->untrusted) { > > > + if (cap & PCI_ACS_TB) > > > + ctrl |= PCI_ACS_TB; > > > + else if (dev->external_facing) > > > + pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n"); > > > + } > > > + > > > pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl); > > > > > > pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n"); > > > -- > > > 2.27.0.212.ge8ba1cc988-goog > > >
On Fri, Jul 10, 2020 at 03:53:59PM -0700, Rajat Jain wrote: > On Fri, Jul 10, 2020 at 2:29 PM Raj, Ashok <ashok.raj@intel.com> wrote: > > On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote: > > > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote: > > > > When enabling ACS, enable translation blocking for external facing ports > > > > and untrusted devices. > > > > > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > > > --- > > > > v4: Add braces to avoid warning from kernel robot > > > > print warning for only external-facing devices. > > > > v3: print warning if ACS_TB not supported on external-facing/untrusted ports. > > > > Minor code comments fixes. > > > > v2: Commit log change > > > > > > > > drivers/pci/pci.c | 8 ++++++++ > > > > drivers/pci/quirks.c | 15 +++++++++++++++ > > > > 2 files changed, 23 insertions(+) > > > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > index 73a8627822140..a5a6bea7af7ce 100644 > > > > --- a/drivers/pci/pci.c > > > > +++ b/drivers/pci/pci.c > > > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev) > > > > /* Upstream Forwarding */ > > > > ctrl |= (cap & PCI_ACS_UF); > > > > > > > > + /* Enable Translation Blocking for external devices */ > > > > + if (dev->external_facing || dev->untrusted) { > > > > + if (cap & PCI_ACS_TB) > > > > + ctrl |= PCI_ACS_TB; > > > > + else if (dev->external_facing) > > > > + pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n"); > > > > + } > > > > > > IIUC, this means that external devices can *never* use ATS > > and can > > > never cache translations. > > Yes, but it already exists today (and this patch doesn't change that): > 521376741b2c2 "PCI/ATS: Only enable ATS for trusted devices" If you get in the habit of using the commit reference style from Documentation/process/submitting-patches.rst it saves me the trouble of fixing them. I use this: gsr is aliased to `git --no-pager show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"' > IMHO any external device trying to send ATS traffic despite having > ATS disabled should count as a bad intent. And this patch is trying > to plug that loophole, by blocking the AT traffic from devices that > we do not expect to see AT from anyway. That's exactly the sort of assertion I was looking for. If we can get something like this explanation into the commit log, and if Ashok and Alex are OK with this, we'll be much closer. It sounds like this is just enforcing a restriction we already have, i.e., enabling PCI_ACS_TB blocks translated requests from devices that aren't supposed to be generating them. > Do you see any case where this is not true? > > > And (I guess, I'm not an expert) it can > > > also never use the Page Request Services? > > > > Yep, sounds like it. > > Yes, from spec "Address Translation Services" Rev 1.1: > "...a device that supports ATS need not support PRI, but PRI is > dependent on ATS’s capabilities." > (So no ATS = No PRI). > > > > Is this what we want? Do we have any idea how many external > > > devices this will affect or how much of a performance impact > > > they will see? > > > > > > Do we need some kind of override or mechanism to authenticate > > > certain devices so they can use ATS and PRI? > > > > Sounds like we would need some form of an allow-list to start with > > so we can have something in the interim. > > I assume what is being referred to, is an escape hatch to enable ATS > on certain given "external-facing" ports (and devices downstream on > that port). Do we really think a *per-port* control for ATS may be > needed? I can add if there is consensus about this. > > > I suppose a future platform might have a facilty to ensure ATS is > > secure and authenticated we could enable for all of devices in the > > system, in addition to PCI CMA/IDE. > > > > I think having a global override to enable all devices so platform > > can switch to current behavior, or maybe via a cmdline switch.. as > > much as we have a billion of those, it still gives an option in > > case someone needs it. > > Currently: > > pci.noats => No ATS on all PCI devices. > (Absense of pci.noats): ATS on all PCI devices, EXCEPT external devices. You mean the "pci=noats" kernel command line parameter, right? > I can look to add another parameter that is synonymous to > "trust-external-pci-devices" that can keep ATS enabled on external > ports as well. I think this is better than an allow-list of only > certain ports, because most likely an admin will trust all its > external ports, or not. Also, we can add this global override and > may be add a more granular control later, if and when really needed. I think this would be new functionality that we don't have today, and we don't have anything that actually *needs* it AFAIK, so I wouldn't bother. Bjorn
On Fri, Jul 10, 2020 at 03:53:59PM -0700, Rajat Jain wrote: > On Fri, Jul 10, 2020 at 2:29 PM Raj, Ashok <ashok.raj@intel.com> wrote: > > On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote: > > > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote: > > > > When enabling ACS, enable translation blocking for external facing ports > > > > and untrusted devices. > > > > > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > > > --- > > > > v4: Add braces to avoid warning from kernel robot > > > > print warning for only external-facing devices. > > > > v3: print warning if ACS_TB not supported on external-facing/untrusted ports. > > > > Minor code comments fixes. > > > > v2: Commit log change > > > > > > > > drivers/pci/pci.c | 8 ++++++++ > > > > drivers/pci/quirks.c | 15 +++++++++++++++ > > > > 2 files changed, 23 insertions(+) > > > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > index 73a8627822140..a5a6bea7af7ce 100644 > > > > --- a/drivers/pci/pci.c > > > > +++ b/drivers/pci/pci.c > > > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev) > > > > /* Upstream Forwarding */ > > > > ctrl |= (cap & PCI_ACS_UF); > > > > > > > > + /* Enable Translation Blocking for external devices */ > > > > + if (dev->external_facing || dev->untrusted) { > > > > + if (cap & PCI_ACS_TB) > > > > + ctrl |= PCI_ACS_TB; > > > > + else if (dev->external_facing) > > > > + pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n"); > > > > + } > > > > > > IIUC, this means that external devices can *never* use ATS and > > > can never cache translations. > > Yes, but it already exists today (and this patch doesn't change that): > 521376741b2c2 "PCI/ATS: Only enable ATS for trusted devices" > > IMHO any external device trying to send ATS traffic despite having ATS > disabled should count as a bad intent. And this patch is trying to > plug that loophole, by blocking the AT traffic from devices that we do > not expect to see AT from anyway. Thinking about this some more, I wonder if Linux should: - Explicitly disable ATS for every device at enumeration-time, e.g., in pci_init_capabilities(), - Enable PCI_ACS_TB for every device (not just external-facing or untrusted ones), - Disable PCI_ACS_TB for the relevant devices along the path only when enabling ATS. One nice thing about doing that is that the "untrusted" test would be only in pci_enable_ats(), and we wouldn't need one in pci_std_enable_acs(). It's possible BIOS gives us devices with ATS enabled, and this might break them, but that seems like something we'd want to find out about. Bjorn
On Sat, Jul 11, 2020 at 12:53 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Jul 10, 2020 at 03:53:59PM -0700, Rajat Jain wrote: > > On Fri, Jul 10, 2020 at 2:29 PM Raj, Ashok <ashok.raj@intel.com> wrote: > > > On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote: > > > > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote: > > > > > When enabling ACS, enable translation blocking for external facing ports > > > > > and untrusted devices. > > > > > > > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > > > > --- > > > > > v4: Add braces to avoid warning from kernel robot > > > > > print warning for only external-facing devices. > > > > > v3: print warning if ACS_TB not supported on external-facing/untrusted ports. > > > > > Minor code comments fixes. > > > > > v2: Commit log change > > > > > > > > > > drivers/pci/pci.c | 8 ++++++++ > > > > > drivers/pci/quirks.c | 15 +++++++++++++++ > > > > > 2 files changed, 23 insertions(+) > > > > > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > > index 73a8627822140..a5a6bea7af7ce 100644 > > > > > --- a/drivers/pci/pci.c > > > > > +++ b/drivers/pci/pci.c > > > > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev) > > > > > /* Upstream Forwarding */ > > > > > ctrl |= (cap & PCI_ACS_UF); > > > > > > > > > > + /* Enable Translation Blocking for external devices */ > > > > > + if (dev->external_facing || dev->untrusted) { > > > > > + if (cap & PCI_ACS_TB) > > > > > + ctrl |= PCI_ACS_TB; > > > > > + else if (dev->external_facing) > > > > > + pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n"); > > > > > + } > > > > > > > > IIUC, this means that external devices can *never* use ATS and > > > > can never cache translations. > > > > Yes, but it already exists today (and this patch doesn't change that): > > 521376741b2c2 "PCI/ATS: Only enable ATS for trusted devices" > > > > IMHO any external device trying to send ATS traffic despite having ATS > > disabled should count as a bad intent. And this patch is trying to > > plug that loophole, by blocking the AT traffic from devices that we do > > not expect to see AT from anyway. > > Thinking about this some more, I wonder if Linux should: > > - Explicitly disable ATS for every device at enumeration-time, e.g., > in pci_init_capabilities(), > > - Enable PCI_ACS_TB for every device (not just external-facing or > untrusted ones), > > - Disable PCI_ACS_TB for the relevant devices along the path only > when enabling ATS. > > One nice thing about doing that is that the "untrusted" test would be > only in pci_enable_ats(), and we wouldn't need one in > pci_std_enable_acs(). Yes, this could work. I think I had thought about this but I'm blanking out on why I had given it up. I think it was because of the possibility that some bridges may have "Translation blocking" disabled, even if not all their descendents were trusted enough to enable ATS on them. But now thinking about this again, as long as we retain the policy of not enabling ATS on external devices (and thus enable TB for sure on them), this should not be a problem. WDYT? > > It's possible BIOS gives us devices with ATS enabled, and this might > break them, but that seems like something we'd want to find out about. > Why would they break? We'd disable ATS on each device as we enumerate them, so they'd be functional, just with ATS disabled until it is enabled again on internal devices as needed. Which would be WAI behavior? Thanks, , Rajat > Bjorn
On Sat, Jul 11, 2020 at 05:08:51PM -0700, Rajat Jain wrote: > On Sat, Jul 11, 2020 at 12:53 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Jul 10, 2020 at 03:53:59PM -0700, Rajat Jain wrote: > > > On Fri, Jul 10, 2020 at 2:29 PM Raj, Ashok <ashok.raj@intel.com> wrote: > > > > On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote: > > > > > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote: > > > > > > When enabling ACS, enable translation blocking for external facing ports > > > > > > and untrusted devices. > > > > > > > > > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > > > > > --- > > > > > > v4: Add braces to avoid warning from kernel robot > > > > > > print warning for only external-facing devices. > > > > > > v3: print warning if ACS_TB not supported on external-facing/untrusted ports. > > > > > > Minor code comments fixes. > > > > > > v2: Commit log change > > > > > > > > > > > > drivers/pci/pci.c | 8 ++++++++ > > > > > > drivers/pci/quirks.c | 15 +++++++++++++++ > > > > > > 2 files changed, 23 insertions(+) > > > > > > > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > > > index 73a8627822140..a5a6bea7af7ce 100644 > > > > > > --- a/drivers/pci/pci.c > > > > > > +++ b/drivers/pci/pci.c > > > > > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev) > > > > > > /* Upstream Forwarding */ > > > > > > ctrl |= (cap & PCI_ACS_UF); > > > > > > > > > > > > + /* Enable Translation Blocking for external devices */ > > > > > > + if (dev->external_facing || dev->untrusted) { > > > > > > + if (cap & PCI_ACS_TB) > > > > > > + ctrl |= PCI_ACS_TB; > > > > > > + else if (dev->external_facing) > > > > > > + pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n"); > > > > > > + } > > > > > > > > > > IIUC, this means that external devices can *never* use ATS and > > > > > can never cache translations. > > > > > > Yes, but it already exists today (and this patch doesn't change that): > > > 521376741b2c2 "PCI/ATS: Only enable ATS for trusted devices" > > > > > > IMHO any external device trying to send ATS traffic despite having ATS > > > disabled should count as a bad intent. And this patch is trying to > > > plug that loophole, by blocking the AT traffic from devices that we do > > > not expect to see AT from anyway. > > > > Thinking about this some more, I wonder if Linux should: > > > > - Explicitly disable ATS for every device at enumeration-time, e.g., > > in pci_init_capabilities(), > > > > - Enable PCI_ACS_TB for every device (not just external-facing or > > untrusted ones), > > > > - Disable PCI_ACS_TB for the relevant devices along the path only > > when enabling ATS. > > > > One nice thing about doing that is that the "untrusted" test would be > > only in pci_enable_ats(), and we wouldn't need one in > > pci_std_enable_acs(). > > Yes, this could work. > > I think I had thought about this but I'm blanking out on why I had > given it up. I think it was because of the possibility that some > bridges may have "Translation blocking" disabled, even if not all > their descendents were trusted enough to enable ATS on them. But now > thinking about this again, as long as we retain the policy of not > enabling ATS on external devices (and thus enable TB for sure on > them), this should not be a problem. WDYT? I think I would feel better if we always enabled Translation Blocking except when we actually need it for ATS. But I'm not confident about how all the pieces of ATS work, so I could be missing something. > > It's possible BIOS gives us devices with ATS enabled, and this > > might break them, but that seems like something we'd want to find > > out about. > > Why would they break? We'd disable ATS on each device as we > enumerate them, so they'd be functional, just with ATS disabled > until it is enabled again on internal devices as needed. Which would > be WAI behavior? If BIOS handed off with ATS enabled and we somehow relied on it being already enabled, something might break if we start disabling ATS. Just a theoretical possibility, doesn't seem likely to me. Bjorn
On Sat, Jul 11, 2020 at 09:58:38PM -0500, Bjorn Helgaas wrote: > If BIOS handed off with ATS enabled and we somehow relied on it being > already enabled, something might break if we start disabling ATS. > Just a theoretical possibility, doesn't seem likely to me. I don't think this will be a problem. When the BIOS enables ATS for a device it also needs to enable the IOMMU already, an we are not handling an already enabled IOMMU (outside of kdump kernels) very well. Regards, Joerg
On Sat, Jul 11, 2020 at 12:53 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Jul 10, 2020 at 03:53:59PM -0700, Rajat Jain wrote: > > On Fri, Jul 10, 2020 at 2:29 PM Raj, Ashok <ashok.raj@intel.com> wrote: > > > On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote: > > > > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote: > > > > > When enabling ACS, enable translation blocking for external facing ports > > > > > and untrusted devices. > > > > > > > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > > > > --- > > > > > v4: Add braces to avoid warning from kernel robot > > > > > print warning for only external-facing devices. > > > > > v3: print warning if ACS_TB not supported on external-facing/untrusted ports. > > > > > Minor code comments fixes. > > > > > v2: Commit log change > > > > > > > > > > drivers/pci/pci.c | 8 ++++++++ > > > > > drivers/pci/quirks.c | 15 +++++++++++++++ > > > > > 2 files changed, 23 insertions(+) > > > > > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > > index 73a8627822140..a5a6bea7af7ce 100644 > > > > > --- a/drivers/pci/pci.c > > > > > +++ b/drivers/pci/pci.c > > > > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev) > > > > > /* Upstream Forwarding */ > > > > > ctrl |= (cap & PCI_ACS_UF); > > > > > > > > > > + /* Enable Translation Blocking for external devices */ > > > > > + if (dev->external_facing || dev->untrusted) { > > > > > + if (cap & PCI_ACS_TB) > > > > > + ctrl |= PCI_ACS_TB; > > > > > + else if (dev->external_facing) > > > > > + pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n"); > > > > > + } > > > > > > > > IIUC, this means that external devices can *never* use ATS and > > > > can never cache translations. > > > > Yes, but it already exists today (and this patch doesn't change that): > > 521376741b2c2 "PCI/ATS: Only enable ATS for trusted devices" > > > > IMHO any external device trying to send ATS traffic despite having ATS > > disabled should count as a bad intent. And this patch is trying to > > plug that loophole, by blocking the AT traffic from devices that we do > > not expect to see AT from anyway. > > Thinking about this some more, I wonder if Linux should: > > - Explicitly disable ATS for every device at enumeration-time, e.g., > in pci_init_capabilities(), > > - Enable PCI_ACS_TB for every device (not just external-facing or > untrusted ones), > > - Disable PCI_ACS_TB for the relevant devices along the path only > when enabling ATS. > > One nice thing about doing that is that the "untrusted" test would be > only in pci_enable_ats(), and we wouldn't need one in > pci_std_enable_acs(). Sent out v5 with this approach here: https://patchwork.kernel.org/patch/11663515/ Thanks, Rajat > > > It's possible BIOS gives us devices with ATS enabled, and this might > break them, but that seems like something we'd want to find out about. > > Bjorn
Hello Bjorn, On Tue, Jul 14, 2020 at 1:19 PM Rajat Jain <rajatja@google.com> wrote: > > On Sat, Jul 11, 2020 at 12:53 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > On Fri, Jul 10, 2020 at 03:53:59PM -0700, Rajat Jain wrote: > > > On Fri, Jul 10, 2020 at 2:29 PM Raj, Ashok <ashok.raj@intel.com> wrote: > > > > On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote: > > > > > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote: > > > > > > When enabling ACS, enable translation blocking for external facing ports > > > > > > and untrusted devices. > > > > > > > > > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > > > > > --- > > > > > > v4: Add braces to avoid warning from kernel robot > > > > > > print warning for only external-facing devices. > > > > > > v3: print warning if ACS_TB not supported on external-facing/untrusted ports. > > > > > > Minor code comments fixes. > > > > > > v2: Commit log change > > > > > > > > > > > > drivers/pci/pci.c | 8 ++++++++ > > > > > > drivers/pci/quirks.c | 15 +++++++++++++++ > > > > > > 2 files changed, 23 insertions(+) > > > > > > > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > > > index 73a8627822140..a5a6bea7af7ce 100644 > > > > > > --- a/drivers/pci/pci.c > > > > > > +++ b/drivers/pci/pci.c > > > > > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev) > > > > > > /* Upstream Forwarding */ > > > > > > ctrl |= (cap & PCI_ACS_UF); > > > > > > > > > > > > + /* Enable Translation Blocking for external devices */ > > > > > > + if (dev->external_facing || dev->untrusted) { > > > > > > + if (cap & PCI_ACS_TB) > > > > > > + ctrl |= PCI_ACS_TB; > > > > > > + else if (dev->external_facing) > > > > > > + pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n"); > > > > > > + } > > > > > > > > > > IIUC, this means that external devices can *never* use ATS and > > > > > can never cache translations. > > > > > > Yes, but it already exists today (and this patch doesn't change that): > > > 521376741b2c2 "PCI/ATS: Only enable ATS for trusted devices" > > > > > > IMHO any external device trying to send ATS traffic despite having ATS > > > disabled should count as a bad intent. And this patch is trying to > > > plug that loophole, by blocking the AT traffic from devices that we do > > > not expect to see AT from anyway. > > > > Thinking about this some more, I wonder if Linux should: > > > > - Explicitly disable ATS for every device at enumeration-time, e.g., > > in pci_init_capabilities(), > > > > - Enable PCI_ACS_TB for every device (not just external-facing or > > untrusted ones), > > > > - Disable PCI_ACS_TB for the relevant devices along the path only > > when enabling ATS. > > > > One nice thing about doing that is that the "untrusted" test would be > > only in pci_enable_ats(), and we wouldn't need one in > > pci_std_enable_acs(). > > Sent out v5 with this approach here: > https://patchwork.kernel.org/patch/11663515/ Any feedback on the patch above? It has been waiting for feedback.... Thanks & Best Regards,, Rajat > > Thanks, > > Rajat > > > > > > > It's possible BIOS gives us devices with ATS enabled, and this might > > break them, but that seems like something we'd want to find out about. > > > > Bjorn
On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote: > When enabling ACS, enable translation blocking for external facing ports > and untrusted devices. > > Signed-off-by: Rajat Jain <rajatja@google.com> Applied (slightly modified) to pci/acs for v5.10, thanks! I think the warning is superfluous because every external_facing device is a Root Port or Switch Downstream Port, and if those support ACS at all, they are required to support Translation Blocking. So we should only see the warning if the device is defective, and I don't think we need to go out of our way to look for those. > --- > v4: Add braces to avoid warning from kernel robot > print warning for only external-facing devices. > v3: print warning if ACS_TB not supported on external-facing/untrusted ports. > Minor code comments fixes. > v2: Commit log change > > drivers/pci/pci.c | 8 ++++++++ > drivers/pci/quirks.c | 15 +++++++++++++++ > 2 files changed, 23 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 73a8627822140..a5a6bea7af7ce 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev) > /* Upstream Forwarding */ > ctrl |= (cap & PCI_ACS_UF); > > + /* Enable Translation Blocking for external devices */ > + if (dev->external_facing || dev->untrusted) { > + if (cap & PCI_ACS_TB) > + ctrl |= PCI_ACS_TB; > + else if (dev->external_facing) > + pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n"); > + } > + > pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl); > } > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index b341628e47527..bb22b46c1d719 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev) > } > } > > +/* > + * Currently this quirk does the equivalent of > + * PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF > + * > + * TODO: This quirk also needs to do equivalent of PCI_ACS_TB, > + * if dev->external_facing || dev->untrusted > + */ > static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev) > { > if (!pci_quirk_intel_pch_acs_match(dev)) > @@ -4973,6 +4980,14 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev) > ctrl |= (cap & PCI_ACS_CR); > ctrl |= (cap & PCI_ACS_UF); > > + /* Enable Translation Blocking for external devices */ > + if (dev->external_facing || dev->untrusted) { > + if (cap & PCI_ACS_TB) > + ctrl |= PCI_ACS_TB; > + else if (dev->external_facing) > + pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n"); > + } > + > pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl); > > pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n"); > -- > 2.27.0.212.ge8ba1cc988-goog >
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 73a8627822140..a5a6bea7af7ce 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev) /* Upstream Forwarding */ ctrl |= (cap & PCI_ACS_UF); + /* Enable Translation Blocking for external devices */ + if (dev->external_facing || dev->untrusted) { + if (cap & PCI_ACS_TB) + ctrl |= PCI_ACS_TB; + else if (dev->external_facing) + pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n"); + } + pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl); } diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index b341628e47527..bb22b46c1d719 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev) } } +/* + * Currently this quirk does the equivalent of + * PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF + * + * TODO: This quirk also needs to do equivalent of PCI_ACS_TB, + * if dev->external_facing || dev->untrusted + */ static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev) { if (!pci_quirk_intel_pch_acs_match(dev)) @@ -4973,6 +4980,14 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev) ctrl |= (cap & PCI_ACS_CR); ctrl |= (cap & PCI_ACS_UF); + /* Enable Translation Blocking for external devices */ + if (dev->external_facing || dev->untrusted) { + if (cap & PCI_ACS_TB) + ctrl |= PCI_ACS_TB; + else if (dev->external_facing) + pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n"); + } + pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl); pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n");
When enabling ACS, enable translation blocking for external facing ports and untrusted devices. Signed-off-by: Rajat Jain <rajatja@google.com> --- v4: Add braces to avoid warning from kernel robot print warning for only external-facing devices. v3: print warning if ACS_TB not supported on external-facing/untrusted ports. Minor code comments fixes. v2: Commit log change drivers/pci/pci.c | 8 ++++++++ drivers/pci/quirks.c | 15 +++++++++++++++ 2 files changed, 23 insertions(+)