* [PATCH v5] PCI/ACS: Enable PCI_ACS_TB and disable only when needed for ATS
@ 2020-07-14 20:15 Rajat Jain
2020-07-14 20:24 ` Rajat Jain
2020-09-16 21:49 ` Bjorn Helgaas
0 siblings, 2 replies; 6+ messages in thread
From: Rajat Jain @ 2020-07-14 20:15 UTC (permalink / raw)
To: David Woodhouse, Lu Baolu, Joerg Roedel, Bjorn Helgaas,
Rafael J. Wysocki, Len Brown, iommu, linux-kernel, linux-pci,
linux-acpi, Raj Ashok, lalithambika.krishnakumar,
Mika Westerberg, Jean-Philippe Brucker, Prashant Malani,
Benson Leung, Todd Broch, Alex Levin, Mattias Nissler,
Rajat Jain, Bernie Keany, Aaron Durbin, Diego Rivas,
Duncan Laurie, Furquan Shaikh, Jesse Barnes, Christian Kellner,
Alex Williamson, Greg Kroah-Hartman, oohall, Saravana Kannan,
Suzuki K Poulose, Arnd Bergmann, Heikki Krogerus
Cc: Rajat Jain
The ACS "Translation Blocking" bit blocks the translated addresses from
the devices. We don't expect such traffic from devices unless ATS is
enabled on them. A device sending such traffic without ATS enabled,
indicates malicious intent, and thus should be blocked.
Enable PCI_ACS_TB by default for all devices, and it stays enabled until
atleast one of the devices downstream wants to enable ATS. It gets
disabled to enable ATS on a device downstream it, and then gets enabled
back on once all the downstream devices don't need ATS.
Signed-off-by: Rajat Jain <rajatja@google.com>
---
Note that I'm ignoring the devices that require quirks to enable or
disable ACS, instead of using the standard way for ACS configuration.
The reason is that it would require adding yet another quirk table or
quirk function pointer, that I don't know how to implement for those
devices, and will neither have the devices to test that code.
v5: Enable TB and disable ATS for all devices on boot. Disable TB later
only if needed to enable ATS on downstream devices.
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/ats.c | 5 ++++
drivers/pci/pci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++
drivers/pci/pci.h | 2 ++
drivers/pci/probe.c | 2 +-
include/linux/pci.h | 2 ++
5 files changed, 67 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index b761c1f72f67..e2ea9083f30f 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -28,6 +28,9 @@ void pci_ats_init(struct pci_dev *dev)
return;
dev->ats_cap = pos;
+
+ dev->ats_enabled = 1; /* To avoid WARN_ON from pci_disable_ats() */
+ pci_disable_ats(dev);
}
/**
@@ -82,6 +85,7 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
}
pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
+ pci_disable_acs_trans_blocking(dev);
dev->ats_enabled = 1;
return 0;
}
@@ -102,6 +106,7 @@ void pci_disable_ats(struct pci_dev *dev)
ctrl &= ~PCI_ATS_CTRL_ENABLE;
pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
+ pci_enable_acs_trans_blocking(dev);
dev->ats_enabled = 0;
}
EXPORT_SYMBOL_GPL(pci_disable_ats);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 73a862782214..614e3c1e8c56 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -876,6 +876,9 @@ static void pci_std_enable_acs(struct pci_dev *dev)
/* Upstream Forwarding */
ctrl |= (cap & PCI_ACS_UF);
+ /* Translation Blocking */
+ ctrl |= (cap & PCI_ACS_TB);
+
pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
}
@@ -904,6 +907,60 @@ static void pci_enable_acs(struct pci_dev *dev)
pci_disable_acs_redir(dev);
}
+void pci_disable_acs_trans_blocking(struct pci_dev *pdev)
+{
+ u16 cap, ctrl, pos;
+ struct pci_dev *dev;
+
+ if (!pci_acs_enable)
+ return;
+
+ for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
+
+ pos = dev->acs_cap;
+ if (!pos)
+ continue;
+
+ /*
+ * Disable translation blocking when first downstream
+ * device that needs it (for ATS) wants to enable ATS
+ */
+ if (++dev->ats_dependencies == 1) {
+ pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
+ pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
+ ctrl &= ~(cap & PCI_ACS_TB);
+ pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+ }
+ }
+}
+
+void pci_enable_acs_trans_blocking(struct pci_dev *pdev)
+{
+ u16 cap, ctrl, pos;
+ struct pci_dev *dev;
+
+ if (!pci_acs_enable)
+ return;
+
+ for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
+
+ pos = dev->acs_cap;
+ if (!pos)
+ continue;
+
+ /*
+ * Enable translation blocking when last downstream device
+ * that depends on it (for ATS), doesn't need ATS anymore
+ */
+ if (--dev->ats_dependencies == 0) {
+ pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
+ pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
+ ctrl |= (cap & PCI_ACS_TB);
+ pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+ }
+ }
+}
+
/**
* pci_restore_bars - restore a device's BAR values (e.g. after wake-up)
* @dev: PCI device to have its BARs restored
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 12fb79fbe29d..f5d8ecb6ba96 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -552,6 +552,8 @@ static inline int pci_dev_specific_disable_acs_redir(struct pci_dev *dev)
return -ENOTTY;
}
#endif
+void pci_disable_acs_trans_blocking(struct pci_dev *dev);
+void pci_enable_acs_trans_blocking(struct pci_dev *dev);
/* PCI error reporting and recovery */
pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8c40c00413e7..e2ff3a94e621 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2387,10 +2387,10 @@ static void pci_init_capabilities(struct pci_dev *dev)
pci_vpd_init(dev); /* Vital Product Data */
pci_configure_ari(dev); /* Alternative Routing-ID Forwarding */
pci_iov_init(dev); /* Single Root I/O Virtualization */
+ pci_acs_init(dev); /* Access Control Services */
pci_ats_init(dev); /* Address Translation Services */
pci_pri_init(dev); /* Page Request Interface */
pci_pasid_init(dev); /* Process Address Space ID */
- pci_acs_init(dev); /* Access Control Services */
pci_ptm_init(dev); /* Precision Time Measurement */
pci_aer_init(dev); /* Advanced Error Reporting */
pci_dpc_init(dev); /* Downstream Port Containment */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 7a40cd5caed0..31da4355f0fd 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -480,6 +480,8 @@ struct pci_dev {
u16 ats_cap; /* ATS Capability offset */
u8 ats_stu; /* ATS Smallest Translation Unit */
#endif
+ /* Total number of downstream devices below a bridge that need ATS */
+ u8 ats_dependencies;
#ifdef CONFIG_PCI_PRI
u16 pri_cap; /* PRI Capability offset */
u32 pri_reqs_alloc; /* Number of PRI requests allocated */
--
2.27.0.389.gc38d7665816-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v5] PCI/ACS: Enable PCI_ACS_TB and disable only when needed for ATS
2020-07-14 20:15 [PATCH v5] PCI/ACS: Enable PCI_ACS_TB and disable only when needed for ATS Rajat Jain
@ 2020-07-14 20:24 ` Rajat Jain
2020-08-02 0:30 ` Rajat Jain
2020-09-16 21:49 ` Bjorn Helgaas
1 sibling, 1 reply; 6+ messages in thread
From: Rajat Jain @ 2020-07-14 20:24 UTC (permalink / raw)
To: Rajat Jain
Cc: David Woodhouse, Lu Baolu, Joerg Roedel, Bjorn Helgaas,
Rafael J. Wysocki, Len Brown, open list:AMD IOMMU (AMD-VI),
Linux Kernel Mailing List, linux-pci, ACPI Devel Maling List,
Raj Ashok, Krishnakumar, Lalithambika, Mika Westerberg,
Jean-Philippe Brucker, Prashant Malani, Benson Leung, Todd Broch,
Alex Levin, Mattias Nissler, Bernie Keany, Aaron Durbin,
Diego Rivas, Duncan Laurie, Furquan Shaikh, Jesse Barnes,
Christian Kellner, Alex Williamson, Greg Kroah-Hartman,
Oliver O'Halloran, Saravana Kannan, Suzuki K Poulose,
Arnd Bergmann, Heikki Krogerus
On Tue, Jul 14, 2020 at 1:15 PM Rajat Jain <rajatja@google.com> wrote:
>
> The ACS "Translation Blocking" bit blocks the translated addresses from
> the devices. We don't expect such traffic from devices unless ATS is
> enabled on them. A device sending such traffic without ATS enabled,
> indicates malicious intent, and thus should be blocked.
>
> Enable PCI_ACS_TB by default for all devices, and it stays enabled until
> atleast one of the devices downstream wants to enable ATS. It gets
> disabled to enable ATS on a device downstream it, and then gets enabled
> back on once all the downstream devices don't need ATS.
>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> Note that I'm ignoring the devices that require quirks to enable or
> disable ACS, instead of using the standard way for ACS configuration.
> The reason is that it would require adding yet another quirk table or
> quirk function pointer, that I don't know how to implement for those
> devices, and will neither have the devices to test that code.
>
> v5: Enable TB and disable ATS for all devices on boot. Disable TB later
> only if needed to enable ATS on downstream devices.
> 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/ats.c | 5 ++++
> drivers/pci/pci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++
> drivers/pci/pci.h | 2 ++
> drivers/pci/probe.c | 2 +-
> include/linux/pci.h | 2 ++
> 5 files changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index b761c1f72f67..e2ea9083f30f 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -28,6 +28,9 @@ void pci_ats_init(struct pci_dev *dev)
> return;
>
> dev->ats_cap = pos;
> +
> + dev->ats_enabled = 1; /* To avoid WARN_ON from pci_disable_ats() */
> + pci_disable_ats(dev);
> }
>
> /**
> @@ -82,6 +85,7 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
> }
> pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
>
> + pci_disable_acs_trans_blocking(dev);
> dev->ats_enabled = 1;
> return 0;
> }
> @@ -102,6 +106,7 @@ void pci_disable_ats(struct pci_dev *dev)
> ctrl &= ~PCI_ATS_CTRL_ENABLE;
> pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
>
> + pci_enable_acs_trans_blocking(dev);
> dev->ats_enabled = 0;
> }
> EXPORT_SYMBOL_GPL(pci_disable_ats);
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 73a862782214..614e3c1e8c56 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -876,6 +876,9 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> /* Upstream Forwarding */
> ctrl |= (cap & PCI_ACS_UF);
>
> + /* Translation Blocking */
> + ctrl |= (cap & PCI_ACS_TB);
> +
> pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> }
>
> @@ -904,6 +907,60 @@ static void pci_enable_acs(struct pci_dev *dev)
> pci_disable_acs_redir(dev);
> }
>
> +void pci_disable_acs_trans_blocking(struct pci_dev *pdev)
> +{
> + u16 cap, ctrl, pos;
> + struct pci_dev *dev;
> +
> + if (!pci_acs_enable)
> + return;
> +
> + for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
> +
> + pos = dev->acs_cap;
> + if (!pos)
> + continue;
> +
> + /*
> + * Disable translation blocking when first downstream
> + * device that needs it (for ATS) wants to enable ATS
> + */
> + if (++dev->ats_dependencies == 1) {
I am a little worried about a potential race condition here. I know
that 2 PCI devices cannot be enumerating at the same time. Do we know
if multiple pci_enable_ats() and pci_disable_ats() function calls can
be simultaneously executing (even for different devices)? If so, we
may need an atomic_t variable for ats_dependencies.
Thanks,
Rajat
> + pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
> + pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
> + ctrl &= ~(cap & PCI_ACS_TB);
> + pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> + }
> + }
> +}
> +
> +void pci_enable_acs_trans_blocking(struct pci_dev *pdev)
> +{
> + u16 cap, ctrl, pos;
> + struct pci_dev *dev;
> +
> + if (!pci_acs_enable)
> + return;
> +
> + for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
> +
> + pos = dev->acs_cap;
> + if (!pos)
> + continue;
> +
> + /*
> + * Enable translation blocking when last downstream device
> + * that depends on it (for ATS), doesn't need ATS anymore
> + */
> + if (--dev->ats_dependencies == 0) {
> + pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
> + pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
> + ctrl |= (cap & PCI_ACS_TB);
> + pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> + }
> + }
> +}
> +
> /**
> * pci_restore_bars - restore a device's BAR values (e.g. after wake-up)
> * @dev: PCI device to have its BARs restored
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 12fb79fbe29d..f5d8ecb6ba96 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -552,6 +552,8 @@ static inline int pci_dev_specific_disable_acs_redir(struct pci_dev *dev)
> return -ENOTTY;
> }
> #endif
> +void pci_disable_acs_trans_blocking(struct pci_dev *dev);
> +void pci_enable_acs_trans_blocking(struct pci_dev *dev);
>
> /* PCI error reporting and recovery */
> pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 8c40c00413e7..e2ff3a94e621 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2387,10 +2387,10 @@ static void pci_init_capabilities(struct pci_dev *dev)
> pci_vpd_init(dev); /* Vital Product Data */
> pci_configure_ari(dev); /* Alternative Routing-ID Forwarding */
> pci_iov_init(dev); /* Single Root I/O Virtualization */
> + pci_acs_init(dev); /* Access Control Services */
> pci_ats_init(dev); /* Address Translation Services */
> pci_pri_init(dev); /* Page Request Interface */
> pci_pasid_init(dev); /* Process Address Space ID */
> - pci_acs_init(dev); /* Access Control Services */
> pci_ptm_init(dev); /* Precision Time Measurement */
> pci_aer_init(dev); /* Advanced Error Reporting */
> pci_dpc_init(dev); /* Downstream Port Containment */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 7a40cd5caed0..31da4355f0fd 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -480,6 +480,8 @@ struct pci_dev {
> u16 ats_cap; /* ATS Capability offset */
> u8 ats_stu; /* ATS Smallest Translation Unit */
> #endif
> + /* Total number of downstream devices below a bridge that need ATS */
> + u8 ats_dependencies;
> #ifdef CONFIG_PCI_PRI
> u16 pri_cap; /* PRI Capability offset */
> u32 pri_reqs_alloc; /* Number of PRI requests allocated */
> --
> 2.27.0.389.gc38d7665816-goog
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v5] PCI/ACS: Enable PCI_ACS_TB and disable only when needed for ATS
2020-07-14 20:24 ` Rajat Jain
@ 2020-08-02 0:30 ` Rajat Jain
2020-08-17 22:50 ` Rajat Jain
0 siblings, 1 reply; 6+ messages in thread
From: Rajat Jain @ 2020-08-02 0:30 UTC (permalink / raw)
To: Rajat Jain
Cc: David Woodhouse, Lu Baolu, Joerg Roedel, Bjorn Helgaas,
Rafael J. Wysocki, Len Brown, open list:AMD IOMMU (AMD-VI),
Linux Kernel Mailing List, linux-pci, ACPI Devel Maling List,
Raj Ashok, Krishnakumar, Lalithambika, Mika Westerberg,
Jean-Philippe Brucker, Prashant Malani, Benson Leung, Todd Broch,
Alex Levin, Mattias Nissler, Bernie Keany, Aaron Durbin,
Diego Rivas, Duncan Laurie, Furquan Shaikh, Jesse Barnes,
Christian Kellner, Alex Williamson, Greg Kroah-Hartman,
Oliver O'Halloran, Saravana Kannan, Suzuki K Poulose,
Arnd Bergmann, Heikki Krogerus
Hi Bjorn,
On Tue, Jul 14, 2020 at 1:24 PM Rajat Jain <rajatxjain@gmail.com> wrote:
>
> On Tue, Jul 14, 2020 at 1:15 PM Rajat Jain <rajatja@google.com> wrote:
> >
> > The ACS "Translation Blocking" bit blocks the translated addresses from
> > the devices. We don't expect such traffic from devices unless ATS is
> > enabled on them. A device sending such traffic without ATS enabled,
> > indicates malicious intent, and thus should be blocked.
> >
> > Enable PCI_ACS_TB by default for all devices, and it stays enabled until
> > atleast one of the devices downstream wants to enable ATS. It gets
> > disabled to enable ATS on a device downstream it, and then gets enabled
> > back on once all the downstream devices don't need ATS.
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
Just checking to see if you got a chance to look at this V5 patch.
Thanks & Best Regards,
Rajat
> > ---
> > Note that I'm ignoring the devices that require quirks to enable or
> > disable ACS, instead of using the standard way for ACS configuration.
> > The reason is that it would require adding yet another quirk table or
> > quirk function pointer, that I don't know how to implement for those
> > devices, and will neither have the devices to test that code.
> >
> > v5: Enable TB and disable ATS for all devices on boot. Disable TB later
> > only if needed to enable ATS on downstream devices.
> > 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/ats.c | 5 ++++
> > drivers/pci/pci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++
> > drivers/pci/pci.h | 2 ++
> > drivers/pci/probe.c | 2 +-
> > include/linux/pci.h | 2 ++
> > 5 files changed, 67 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> > index b761c1f72f67..e2ea9083f30f 100644
> > --- a/drivers/pci/ats.c
> > +++ b/drivers/pci/ats.c
> > @@ -28,6 +28,9 @@ void pci_ats_init(struct pci_dev *dev)
> > return;
> >
> > dev->ats_cap = pos;
> > +
> > + dev->ats_enabled = 1; /* To avoid WARN_ON from pci_disable_ats() */
> > + pci_disable_ats(dev);
> > }
> >
> > /**
> > @@ -82,6 +85,7 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
> > }
> > pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
> >
> > + pci_disable_acs_trans_blocking(dev);
> > dev->ats_enabled = 1;
> > return 0;
> > }
> > @@ -102,6 +106,7 @@ void pci_disable_ats(struct pci_dev *dev)
> > ctrl &= ~PCI_ATS_CTRL_ENABLE;
> > pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
> >
> > + pci_enable_acs_trans_blocking(dev);
> > dev->ats_enabled = 0;
> > }
> > EXPORT_SYMBOL_GPL(pci_disable_ats);
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 73a862782214..614e3c1e8c56 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -876,6 +876,9 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> > /* Upstream Forwarding */
> > ctrl |= (cap & PCI_ACS_UF);
> >
> > + /* Translation Blocking */
> > + ctrl |= (cap & PCI_ACS_TB);
> > +
> > pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> > }
> >
> > @@ -904,6 +907,60 @@ static void pci_enable_acs(struct pci_dev *dev)
> > pci_disable_acs_redir(dev);
> > }
> >
> > +void pci_disable_acs_trans_blocking(struct pci_dev *pdev)
> > +{
> > + u16 cap, ctrl, pos;
> > + struct pci_dev *dev;
> > +
> > + if (!pci_acs_enable)
> > + return;
> > +
> > + for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
> > +
> > + pos = dev->acs_cap;
> > + if (!pos)
> > + continue;
> > +
> > + /*
> > + * Disable translation blocking when first downstream
> > + * device that needs it (for ATS) wants to enable ATS
> > + */
> > + if (++dev->ats_dependencies == 1) {
>
> I am a little worried about a potential race condition here. I know
> that 2 PCI devices cannot be enumerating at the same time. Do we know
> if multiple pci_enable_ats() and pci_disable_ats() function calls can
> be simultaneously executing (even for different devices)? If so, we
> may need an atomic_t variable for ats_dependencies.
>
> Thanks,
>
> Rajat
>
>
> > + pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
> > + pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
> > + ctrl &= ~(cap & PCI_ACS_TB);
> > + pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> > + }
> > + }
> > +}
> > +
> > +void pci_enable_acs_trans_blocking(struct pci_dev *pdev)
> > +{
> > + u16 cap, ctrl, pos;
> > + struct pci_dev *dev;
> > +
> > + if (!pci_acs_enable)
> > + return;
> > +
> > + for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
> > +
> > + pos = dev->acs_cap;
> > + if (!pos)
> > + continue;
> > +
> > + /*
> > + * Enable translation blocking when last downstream device
> > + * that depends on it (for ATS), doesn't need ATS anymore
> > + */
> > + if (--dev->ats_dependencies == 0) {
> > + pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
> > + pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
> > + ctrl |= (cap & PCI_ACS_TB);
> > + pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> > + }
> > + }
> > +}
> > +
> > /**
> > * pci_restore_bars - restore a device's BAR values (e.g. after wake-up)
> > * @dev: PCI device to have its BARs restored
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 12fb79fbe29d..f5d8ecb6ba96 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -552,6 +552,8 @@ static inline int pci_dev_specific_disable_acs_redir(struct pci_dev *dev)
> > return -ENOTTY;
> > }
> > #endif
> > +void pci_disable_acs_trans_blocking(struct pci_dev *dev);
> > +void pci_enable_acs_trans_blocking(struct pci_dev *dev);
> >
> > /* PCI error reporting and recovery */
> > pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 8c40c00413e7..e2ff3a94e621 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -2387,10 +2387,10 @@ static void pci_init_capabilities(struct pci_dev *dev)
> > pci_vpd_init(dev); /* Vital Product Data */
> > pci_configure_ari(dev); /* Alternative Routing-ID Forwarding */
> > pci_iov_init(dev); /* Single Root I/O Virtualization */
> > + pci_acs_init(dev); /* Access Control Services */
> > pci_ats_init(dev); /* Address Translation Services */
> > pci_pri_init(dev); /* Page Request Interface */
> > pci_pasid_init(dev); /* Process Address Space ID */
> > - pci_acs_init(dev); /* Access Control Services */
> > pci_ptm_init(dev); /* Precision Time Measurement */
> > pci_aer_init(dev); /* Advanced Error Reporting */
> > pci_dpc_init(dev); /* Downstream Port Containment */
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 7a40cd5caed0..31da4355f0fd 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -480,6 +480,8 @@ struct pci_dev {
> > u16 ats_cap; /* ATS Capability offset */
> > u8 ats_stu; /* ATS Smallest Translation Unit */
> > #endif
> > + /* Total number of downstream devices below a bridge that need ATS */
> > + u8 ats_dependencies;
> > #ifdef CONFIG_PCI_PRI
> > u16 pri_cap; /* PRI Capability offset */
> > u32 pri_reqs_alloc; /* Number of PRI requests allocated */
> > --
> > 2.27.0.389.gc38d7665816-goog
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v5] PCI/ACS: Enable PCI_ACS_TB and disable only when needed for ATS
2020-08-02 0:30 ` Rajat Jain
@ 2020-08-17 22:50 ` Rajat Jain
2020-09-10 21:31 ` Rajat Jain
0 siblings, 1 reply; 6+ messages in thread
From: Rajat Jain @ 2020-08-17 22:50 UTC (permalink / raw)
To: Rajat Jain
Cc: David Woodhouse, Lu Baolu, Joerg Roedel, Bjorn Helgaas,
Rafael J. Wysocki, Len Brown, open list:AMD IOMMU (AMD-VI),
Linux Kernel Mailing List, linux-pci, ACPI Devel Maling List,
Raj Ashok, Krishnakumar, Lalithambika, Mika Westerberg,
Jean-Philippe Brucker, Prashant Malani, Benson Leung, Todd Broch,
Alex Levin, Mattias Nissler, Bernie Keany, Aaron Durbin,
Diego Rivas, Duncan Laurie, Furquan Shaikh, Jesse Barnes,
Christian Kellner, Alex Williamson, Greg Kroah-Hartman,
Oliver O'Halloran, Saravana Kannan, Suzuki K Poulose,
Arnd Bergmann, Heikki Krogerus
Hello Bjorn,
On Sat, Aug 1, 2020 at 5:30 PM Rajat Jain <rajatja@google.com> wrote:
>
> Hi Bjorn,
>
>
> On Tue, Jul 14, 2020 at 1:24 PM Rajat Jain <rajatxjain@gmail.com> wrote:
> >
> > On Tue, Jul 14, 2020 at 1:15 PM Rajat Jain <rajatja@google.com> wrote:
> > >
> > > The ACS "Translation Blocking" bit blocks the translated addresses from
> > > the devices. We don't expect such traffic from devices unless ATS is
> > > enabled on them. A device sending such traffic without ATS enabled,
> > > indicates malicious intent, and thus should be blocked.
> > >
> > > Enable PCI_ACS_TB by default for all devices, and it stays enabled until
> > > atleast one of the devices downstream wants to enable ATS. It gets
> > > disabled to enable ATS on a device downstream it, and then gets enabled
> > > back on once all the downstream devices don't need ATS.
> > >
> > > Signed-off-by: Rajat Jain <rajatja@google.com>
>
> Just checking to see if you got a chance to look at this V5 patch.
Any feedback on this patch?
Thanks & Best Regards,
Rajat
>
> Thanks & Best Regards,
>
> Rajat
>
> > > ---
> > > Note that I'm ignoring the devices that require quirks to enable or
> > > disable ACS, instead of using the standard way for ACS configuration.
> > > The reason is that it would require adding yet another quirk table or
> > > quirk function pointer, that I don't know how to implement for those
> > > devices, and will neither have the devices to test that code.
> > >
> > > v5: Enable TB and disable ATS for all devices on boot. Disable TB later
> > > only if needed to enable ATS on downstream devices.
> > > 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/ats.c | 5 ++++
> > > drivers/pci/pci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++
> > > drivers/pci/pci.h | 2 ++
> > > drivers/pci/probe.c | 2 +-
> > > include/linux/pci.h | 2 ++
> > > 5 files changed, 67 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> > > index b761c1f72f67..e2ea9083f30f 100644
> > > --- a/drivers/pci/ats.c
> > > +++ b/drivers/pci/ats.c
> > > @@ -28,6 +28,9 @@ void pci_ats_init(struct pci_dev *dev)
> > > return;
> > >
> > > dev->ats_cap = pos;
> > > +
> > > + dev->ats_enabled = 1; /* To avoid WARN_ON from pci_disable_ats() */
> > > + pci_disable_ats(dev);
> > > }
> > >
> > > /**
> > > @@ -82,6 +85,7 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
> > > }
> > > pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
> > >
> > > + pci_disable_acs_trans_blocking(dev);
> > > dev->ats_enabled = 1;
> > > return 0;
> > > }
> > > @@ -102,6 +106,7 @@ void pci_disable_ats(struct pci_dev *dev)
> > > ctrl &= ~PCI_ATS_CTRL_ENABLE;
> > > pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
> > >
> > > + pci_enable_acs_trans_blocking(dev);
> > > dev->ats_enabled = 0;
> > > }
> > > EXPORT_SYMBOL_GPL(pci_disable_ats);
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 73a862782214..614e3c1e8c56 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -876,6 +876,9 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> > > /* Upstream Forwarding */
> > > ctrl |= (cap & PCI_ACS_UF);
> > >
> > > + /* Translation Blocking */
> > > + ctrl |= (cap & PCI_ACS_TB);
> > > +
> > > pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> > > }
> > >
> > > @@ -904,6 +907,60 @@ static void pci_enable_acs(struct pci_dev *dev)
> > > pci_disable_acs_redir(dev);
> > > }
> > >
> > > +void pci_disable_acs_trans_blocking(struct pci_dev *pdev)
> > > +{
> > > + u16 cap, ctrl, pos;
> > > + struct pci_dev *dev;
> > > +
> > > + if (!pci_acs_enable)
> > > + return;
> > > +
> > > + for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
> > > +
> > > + pos = dev->acs_cap;
> > > + if (!pos)
> > > + continue;
> > > +
> > > + /*
> > > + * Disable translation blocking when first downstream
> > > + * device that needs it (for ATS) wants to enable ATS
> > > + */
> > > + if (++dev->ats_dependencies == 1) {
> >
> > I am a little worried about a potential race condition here. I know
> > that 2 PCI devices cannot be enumerating at the same time. Do we know
> > if multiple pci_enable_ats() and pci_disable_ats() function calls can
> > be simultaneously executing (even for different devices)? If so, we
> > may need an atomic_t variable for ats_dependencies.
> >
> > Thanks,
> >
> > Rajat
> >
> >
> > > + pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
> > > + pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
> > > + ctrl &= ~(cap & PCI_ACS_TB);
> > > + pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> > > + }
> > > + }
> > > +}
> > > +
> > > +void pci_enable_acs_trans_blocking(struct pci_dev *pdev)
> > > +{
> > > + u16 cap, ctrl, pos;
> > > + struct pci_dev *dev;
> > > +
> > > + if (!pci_acs_enable)
> > > + return;
> > > +
> > > + for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
> > > +
> > > + pos = dev->acs_cap;
> > > + if (!pos)
> > > + continue;
> > > +
> > > + /*
> > > + * Enable translation blocking when last downstream device
> > > + * that depends on it (for ATS), doesn't need ATS anymore
> > > + */
> > > + if (--dev->ats_dependencies == 0) {
> > > + pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
> > > + pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
> > > + ctrl |= (cap & PCI_ACS_TB);
> > > + pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> > > + }
> > > + }
> > > +}
> > > +
> > > /**
> > > * pci_restore_bars - restore a device's BAR values (e.g. after wake-up)
> > > * @dev: PCI device to have its BARs restored
> > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > index 12fb79fbe29d..f5d8ecb6ba96 100644
> > > --- a/drivers/pci/pci.h
> > > +++ b/drivers/pci/pci.h
> > > @@ -552,6 +552,8 @@ static inline int pci_dev_specific_disable_acs_redir(struct pci_dev *dev)
> > > return -ENOTTY;
> > > }
> > > #endif
> > > +void pci_disable_acs_trans_blocking(struct pci_dev *dev);
> > > +void pci_enable_acs_trans_blocking(struct pci_dev *dev);
> > >
> > > /* PCI error reporting and recovery */
> > > pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 8c40c00413e7..e2ff3a94e621 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -2387,10 +2387,10 @@ static void pci_init_capabilities(struct pci_dev *dev)
> > > pci_vpd_init(dev); /* Vital Product Data */
> > > pci_configure_ari(dev); /* Alternative Routing-ID Forwarding */
> > > pci_iov_init(dev); /* Single Root I/O Virtualization */
> > > + pci_acs_init(dev); /* Access Control Services */
> > > pci_ats_init(dev); /* Address Translation Services */
> > > pci_pri_init(dev); /* Page Request Interface */
> > > pci_pasid_init(dev); /* Process Address Space ID */
> > > - pci_acs_init(dev); /* Access Control Services */
> > > pci_ptm_init(dev); /* Precision Time Measurement */
> > > pci_aer_init(dev); /* Advanced Error Reporting */
> > > pci_dpc_init(dev); /* Downstream Port Containment */
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index 7a40cd5caed0..31da4355f0fd 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -480,6 +480,8 @@ struct pci_dev {
> > > u16 ats_cap; /* ATS Capability offset */
> > > u8 ats_stu; /* ATS Smallest Translation Unit */
> > > #endif
> > > + /* Total number of downstream devices below a bridge that need ATS */
> > > + u8 ats_dependencies;
> > > #ifdef CONFIG_PCI_PRI
> > > u16 pri_cap; /* PRI Capability offset */
> > > u32 pri_reqs_alloc; /* Number of PRI requests allocated */
> > > --
> > > 2.27.0.389.gc38d7665816-goog
> > >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v5] PCI/ACS: Enable PCI_ACS_TB and disable only when needed for ATS
2020-08-17 22:50 ` Rajat Jain
@ 2020-09-10 21:31 ` Rajat Jain
0 siblings, 0 replies; 6+ messages in thread
From: Rajat Jain @ 2020-09-10 21:31 UTC (permalink / raw)
To: Rajat Jain
Cc: David Woodhouse, Lu Baolu, Joerg Roedel, Bjorn Helgaas,
Rafael J. Wysocki, Len Brown, open list:AMD IOMMU (AMD-VI),
Linux Kernel Mailing List, linux-pci, ACPI Devel Maling List,
Raj Ashok, Krishnakumar, Lalithambika, Mika Westerberg,
Jean-Philippe Brucker, Prashant Malani, Benson Leung, Todd Broch,
Alex Levin, Mattias Nissler, Bernie Keany, Aaron Durbin,
Diego Rivas, Duncan Laurie, Furquan Shaikh, Jesse Barnes,
Christian Kellner, Alex Williamson, Greg Kroah-Hartman,
Oliver O'Halloran, Saravana Kannan, Suzuki K Poulose,
Arnd Bergmann, Heikki Krogerus
Hello Bjorn,
On Mon, Aug 17, 2020 at 3:50 PM Rajat Jain <rajatxjain@gmail.com> wrote:
>
> Hello Bjorn,
>
>
> On Sat, Aug 1, 2020 at 5:30 PM Rajat Jain <rajatja@google.com> wrote:
> >
> > Hi Bjorn,
> >
> >
> > On Tue, Jul 14, 2020 at 1:24 PM Rajat Jain <rajatxjain@gmail.com> wrote:
> > >
> > > On Tue, Jul 14, 2020 at 1:15 PM Rajat Jain <rajatja@google.com> wrote:
> > > >
> > > > The ACS "Translation Blocking" bit blocks the translated addresses from
> > > > the devices. We don't expect such traffic from devices unless ATS is
> > > > enabled on them. A device sending such traffic without ATS enabled,
> > > > indicates malicious intent, and thus should be blocked.
> > > >
> > > > Enable PCI_ACS_TB by default for all devices, and it stays enabled until
> > > > atleast one of the devices downstream wants to enable ATS. It gets
> > > > disabled to enable ATS on a device downstream it, and then gets enabled
> > > > back on once all the downstream devices don't need ATS.
> > > >
> > > > Signed-off-by: Rajat Jain <rajatja@google.com>
> >
> > Just checking to see if you got a chance to look at this V5 patch.
>
> Any feedback on this patch?
Gentle reminder?
Thanks & Best Regards,
Rajat
>
> Thanks & Best Regards,
>
> Rajat
>
> >
> > Thanks & Best Regards,
> >
> > Rajat
> >
> > > > ---
> > > > Note that I'm ignoring the devices that require quirks to enable or
> > > > disable ACS, instead of using the standard way for ACS configuration.
> > > > The reason is that it would require adding yet another quirk table or
> > > > quirk function pointer, that I don't know how to implement for those
> > > > devices, and will neither have the devices to test that code.
> > > >
> > > > v5: Enable TB and disable ATS for all devices on boot. Disable TB later
> > > > only if needed to enable ATS on downstream devices.
> > > > 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/ats.c | 5 ++++
> > > > drivers/pci/pci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++
> > > > drivers/pci/pci.h | 2 ++
> > > > drivers/pci/probe.c | 2 +-
> > > > include/linux/pci.h | 2 ++
> > > > 5 files changed, 67 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> > > > index b761c1f72f67..e2ea9083f30f 100644
> > > > --- a/drivers/pci/ats.c
> > > > +++ b/drivers/pci/ats.c
> > > > @@ -28,6 +28,9 @@ void pci_ats_init(struct pci_dev *dev)
> > > > return;
> > > >
> > > > dev->ats_cap = pos;
> > > > +
> > > > + dev->ats_enabled = 1; /* To avoid WARN_ON from pci_disable_ats() */
> > > > + pci_disable_ats(dev);
> > > > }
> > > >
> > > > /**
> > > > @@ -82,6 +85,7 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
> > > > }
> > > > pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
> > > >
> > > > + pci_disable_acs_trans_blocking(dev);
> > > > dev->ats_enabled = 1;
> > > > return 0;
> > > > }
> > > > @@ -102,6 +106,7 @@ void pci_disable_ats(struct pci_dev *dev)
> > > > ctrl &= ~PCI_ATS_CTRL_ENABLE;
> > > > pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
> > > >
> > > > + pci_enable_acs_trans_blocking(dev);
> > > > dev->ats_enabled = 0;
> > > > }
> > > > EXPORT_SYMBOL_GPL(pci_disable_ats);
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index 73a862782214..614e3c1e8c56 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -876,6 +876,9 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> > > > /* Upstream Forwarding */
> > > > ctrl |= (cap & PCI_ACS_UF);
> > > >
> > > > + /* Translation Blocking */
> > > > + ctrl |= (cap & PCI_ACS_TB);
> > > > +
> > > > pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> > > > }
> > > >
> > > > @@ -904,6 +907,60 @@ static void pci_enable_acs(struct pci_dev *dev)
> > > > pci_disable_acs_redir(dev);
> > > > }
> > > >
> > > > +void pci_disable_acs_trans_blocking(struct pci_dev *pdev)
> > > > +{
> > > > + u16 cap, ctrl, pos;
> > > > + struct pci_dev *dev;
> > > > +
> > > > + if (!pci_acs_enable)
> > > > + return;
> > > > +
> > > > + for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
> > > > +
> > > > + pos = dev->acs_cap;
> > > > + if (!pos)
> > > > + continue;
> > > > +
> > > > + /*
> > > > + * Disable translation blocking when first downstream
> > > > + * device that needs it (for ATS) wants to enable ATS
> > > > + */
> > > > + if (++dev->ats_dependencies == 1) {
> > >
> > > I am a little worried about a potential race condition here. I know
> > > that 2 PCI devices cannot be enumerating at the same time. Do we know
> > > if multiple pci_enable_ats() and pci_disable_ats() function calls can
> > > be simultaneously executing (even for different devices)? If so, we
> > > may need an atomic_t variable for ats_dependencies.
> > >
> > > Thanks,
> > >
> > > Rajat
> > >
> > >
> > > > + pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
> > > > + pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
> > > > + ctrl &= ~(cap & PCI_ACS_TB);
> > > > + pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> > > > + }
> > > > + }
> > > > +}
> > > > +
> > > > +void pci_enable_acs_trans_blocking(struct pci_dev *pdev)
> > > > +{
> > > > + u16 cap, ctrl, pos;
> > > > + struct pci_dev *dev;
> > > > +
> > > > + if (!pci_acs_enable)
> > > > + return;
> > > > +
> > > > + for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
> > > > +
> > > > + pos = dev->acs_cap;
> > > > + if (!pos)
> > > > + continue;
> > > > +
> > > > + /*
> > > > + * Enable translation blocking when last downstream device
> > > > + * that depends on it (for ATS), doesn't need ATS anymore
> > > > + */
> > > > + if (--dev->ats_dependencies == 0) {
> > > > + pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
> > > > + pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
> > > > + ctrl |= (cap & PCI_ACS_TB);
> > > > + pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> > > > + }
> > > > + }
> > > > +}
> > > > +
> > > > /**
> > > > * pci_restore_bars - restore a device's BAR values (e.g. after wake-up)
> > > > * @dev: PCI device to have its BARs restored
> > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > > index 12fb79fbe29d..f5d8ecb6ba96 100644
> > > > --- a/drivers/pci/pci.h
> > > > +++ b/drivers/pci/pci.h
> > > > @@ -552,6 +552,8 @@ static inline int pci_dev_specific_disable_acs_redir(struct pci_dev *dev)
> > > > return -ENOTTY;
> > > > }
> > > > #endif
> > > > +void pci_disable_acs_trans_blocking(struct pci_dev *dev);
> > > > +void pci_enable_acs_trans_blocking(struct pci_dev *dev);
> > > >
> > > > /* PCI error reporting and recovery */
> > > > pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > > index 8c40c00413e7..e2ff3a94e621 100644
> > > > --- a/drivers/pci/probe.c
> > > > +++ b/drivers/pci/probe.c
> > > > @@ -2387,10 +2387,10 @@ static void pci_init_capabilities(struct pci_dev *dev)
> > > > pci_vpd_init(dev); /* Vital Product Data */
> > > > pci_configure_ari(dev); /* Alternative Routing-ID Forwarding */
> > > > pci_iov_init(dev); /* Single Root I/O Virtualization */
> > > > + pci_acs_init(dev); /* Access Control Services */
> > > > pci_ats_init(dev); /* Address Translation Services */
> > > > pci_pri_init(dev); /* Page Request Interface */
> > > > pci_pasid_init(dev); /* Process Address Space ID */
> > > > - pci_acs_init(dev); /* Access Control Services */
> > > > pci_ptm_init(dev); /* Precision Time Measurement */
> > > > pci_aer_init(dev); /* Advanced Error Reporting */
> > > > pci_dpc_init(dev); /* Downstream Port Containment */
> > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > index 7a40cd5caed0..31da4355f0fd 100644
> > > > --- a/include/linux/pci.h
> > > > +++ b/include/linux/pci.h
> > > > @@ -480,6 +480,8 @@ struct pci_dev {
> > > > u16 ats_cap; /* ATS Capability offset */
> > > > u8 ats_stu; /* ATS Smallest Translation Unit */
> > > > #endif
> > > > + /* Total number of downstream devices below a bridge that need ATS */
> > > > + u8 ats_dependencies;
> > > > #ifdef CONFIG_PCI_PRI
> > > > u16 pri_cap; /* PRI Capability offset */
> > > > u32 pri_reqs_alloc; /* Number of PRI requests allocated */
> > > > --
> > > > 2.27.0.389.gc38d7665816-goog
> > > >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v5] PCI/ACS: Enable PCI_ACS_TB and disable only when needed for ATS
2020-07-14 20:15 [PATCH v5] PCI/ACS: Enable PCI_ACS_TB and disable only when needed for ATS Rajat Jain
2020-07-14 20:24 ` Rajat Jain
@ 2020-09-16 21:49 ` Bjorn Helgaas
1 sibling, 0 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2020-09-16 21:49 UTC (permalink / raw)
To: Rajat Jain
Cc: David Woodhouse, Lu Baolu, Joerg Roedel, Bjorn Helgaas,
Rafael J. Wysocki, Len Brown, iommu, linux-kernel, linux-pci,
linux-acpi, Raj Ashok, lalithambika.krishnakumar,
Mika Westerberg, Jean-Philippe Brucker, Prashant Malani,
Benson Leung, Todd Broch, Alex Levin, Mattias Nissler,
Rajat Jain, Bernie Keany, Aaron Durbin, Diego Rivas,
Duncan Laurie, Furquan Shaikh, Jesse Barnes, Christian Kellner,
Alex Williamson, Greg Kroah-Hartman, oohall, Saravana Kannan,
Suzuki K Poulose, Arnd Bergmann, Heikki Krogerus
On Tue, Jul 14, 2020 at 01:15:40PM -0700, Rajat Jain wrote:
> The ACS "Translation Blocking" bit blocks the translated addresses from
> the devices. We don't expect such traffic from devices unless ATS is
> enabled on them. A device sending such traffic without ATS enabled,
> indicates malicious intent, and thus should be blocked.
>
> Enable PCI_ACS_TB by default for all devices, and it stays enabled until
> atleast one of the devices downstream wants to enable ATS. It gets
> disabled to enable ATS on a device downstream it, and then gets enabled
> back on once all the downstream devices don't need ATS.
>
> Signed-off-by: Rajat Jain <rajatja@google.com>
I applied v4 of this patch instead because I think the complexity of
this one, where we have to walk up the tree and disable TB in upstream
bridges, is too high. It's always tricky to modify the state of
device Y when we're doing something for device X.
> ---
> Note that I'm ignoring the devices that require quirks to enable or
> disable ACS, instead of using the standard way for ACS configuration.
> The reason is that it would require adding yet another quirk table or
> quirk function pointer, that I don't know how to implement for those
> devices, and will neither have the devices to test that code.
>
> v5: Enable TB and disable ATS for all devices on boot. Disable TB later
> only if needed to enable ATS on downstream devices.
> 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/ats.c | 5 ++++
> drivers/pci/pci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++
> drivers/pci/pci.h | 2 ++
> drivers/pci/probe.c | 2 +-
> include/linux/pci.h | 2 ++
> 5 files changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index b761c1f72f67..e2ea9083f30f 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -28,6 +28,9 @@ void pci_ats_init(struct pci_dev *dev)
> return;
>
> dev->ats_cap = pos;
> +
> + dev->ats_enabled = 1; /* To avoid WARN_ON from pci_disable_ats() */
> + pci_disable_ats(dev);
> }
>
> /**
> @@ -82,6 +85,7 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
> }
> pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
>
> + pci_disable_acs_trans_blocking(dev);
> dev->ats_enabled = 1;
> return 0;
> }
> @@ -102,6 +106,7 @@ void pci_disable_ats(struct pci_dev *dev)
> ctrl &= ~PCI_ATS_CTRL_ENABLE;
> pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
>
> + pci_enable_acs_trans_blocking(dev);
> dev->ats_enabled = 0;
> }
> EXPORT_SYMBOL_GPL(pci_disable_ats);
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 73a862782214..614e3c1e8c56 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -876,6 +876,9 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> /* Upstream Forwarding */
> ctrl |= (cap & PCI_ACS_UF);
>
> + /* Translation Blocking */
> + ctrl |= (cap & PCI_ACS_TB);
> +
> pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> }
>
> @@ -904,6 +907,60 @@ static void pci_enable_acs(struct pci_dev *dev)
> pci_disable_acs_redir(dev);
> }
>
> +void pci_disable_acs_trans_blocking(struct pci_dev *pdev)
> +{
> + u16 cap, ctrl, pos;
> + struct pci_dev *dev;
> +
> + if (!pci_acs_enable)
> + return;
> +
> + for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
> +
> + pos = dev->acs_cap;
> + if (!pos)
> + continue;
> +
> + /*
> + * Disable translation blocking when first downstream
> + * device that needs it (for ATS) wants to enable ATS
> + */
> + if (++dev->ats_dependencies == 1) {
> + pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
> + pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
> + ctrl &= ~(cap & PCI_ACS_TB);
> + pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> + }
> + }
> +}
> +
> +void pci_enable_acs_trans_blocking(struct pci_dev *pdev)
> +{
> + u16 cap, ctrl, pos;
> + struct pci_dev *dev;
> +
> + if (!pci_acs_enable)
> + return;
> +
> + for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
> +
> + pos = dev->acs_cap;
> + if (!pos)
> + continue;
> +
> + /*
> + * Enable translation blocking when last downstream device
> + * that depends on it (for ATS), doesn't need ATS anymore
> + */
> + if (--dev->ats_dependencies == 0) {
> + pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
> + pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
> + ctrl |= (cap & PCI_ACS_TB);
> + pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> + }
> + }
> +}
> +
> /**
> * pci_restore_bars - restore a device's BAR values (e.g. after wake-up)
> * @dev: PCI device to have its BARs restored
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 12fb79fbe29d..f5d8ecb6ba96 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -552,6 +552,8 @@ static inline int pci_dev_specific_disable_acs_redir(struct pci_dev *dev)
> return -ENOTTY;
> }
> #endif
> +void pci_disable_acs_trans_blocking(struct pci_dev *dev);
> +void pci_enable_acs_trans_blocking(struct pci_dev *dev);
>
> /* PCI error reporting and recovery */
> pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 8c40c00413e7..e2ff3a94e621 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2387,10 +2387,10 @@ static void pci_init_capabilities(struct pci_dev *dev)
> pci_vpd_init(dev); /* Vital Product Data */
> pci_configure_ari(dev); /* Alternative Routing-ID Forwarding */
> pci_iov_init(dev); /* Single Root I/O Virtualization */
> + pci_acs_init(dev); /* Access Control Services */
> pci_ats_init(dev); /* Address Translation Services */
> pci_pri_init(dev); /* Page Request Interface */
> pci_pasid_init(dev); /* Process Address Space ID */
> - pci_acs_init(dev); /* Access Control Services */
> pci_ptm_init(dev); /* Precision Time Measurement */
> pci_aer_init(dev); /* Advanced Error Reporting */
> pci_dpc_init(dev); /* Downstream Port Containment */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 7a40cd5caed0..31da4355f0fd 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -480,6 +480,8 @@ struct pci_dev {
> u16 ats_cap; /* ATS Capability offset */
> u8 ats_stu; /* ATS Smallest Translation Unit */
> #endif
> + /* Total number of downstream devices below a bridge that need ATS */
> + u8 ats_dependencies;
> #ifdef CONFIG_PCI_PRI
> u16 pri_cap; /* PRI Capability offset */
> u32 pri_reqs_alloc; /* Number of PRI requests allocated */
> --
> 2.27.0.389.gc38d7665816-goog
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-16 22:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-14 20:15 [PATCH v5] PCI/ACS: Enable PCI_ACS_TB and disable only when needed for ATS Rajat Jain
2020-07-14 20:24 ` Rajat Jain
2020-08-02 0:30 ` Rajat Jain
2020-08-17 22:50 ` Rajat Jain
2020-09-10 21:31 ` Rajat Jain
2020-09-16 21:49 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).