[v4,4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
diff mbox series

Message ID 20200707224604.3737893-4-rajatja@google.com
State New
Headers show
Series
  • [v4,1/4] PCI: Move pci_enable_acs() and its dependencies up in pci.c
Related show

Commit Message

Rajat Jain July 7, 2020, 10:46 p.m. UTC
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(+)

Comments

Bjorn Helgaas July 10, 2020, 8:29 p.m. UTC | #1
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
>
Ashok Raj July 10, 2020, 9:28 p.m. UTC | #2
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
> >
Rajat Jain July 10, 2020, 10:53 p.m. UTC | #3
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
> > >
Bjorn Helgaas July 10, 2020, 11:32 p.m. UTC | #4
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
Bjorn Helgaas July 11, 2020, 7:53 p.m. UTC | #5
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
Rajat Jain July 12, 2020, 12:08 a.m. UTC | #6
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
Bjorn Helgaas July 12, 2020, 2:58 a.m. UTC | #7
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
Joerg Roedel July 13, 2020, 8:01 a.m. UTC | #8
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
Rajat Jain July 14, 2020, 8:19 p.m. UTC | #9
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

Patch
diff mbox series

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");