| Message ID | 20200213165049.508908-3-jean-philippe@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI/ATS: Device-tree support and other improvements | expand |
On Thu, Feb 13, 2020 at 05:50:40PM +0100, Jean-Philippe Brucker wrote: > Each vendor has their own way of describing whether a host bridge > supports ATS. The Intel and AMD ACPI tables selectively enable or > disable ATS per device or sub-tree, while Arm has a single bit for each > host bridge. For those that need it, add an ats_supported bit to the > host bridge structure. > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > --- > drivers/pci/probe.c | 7 +++++++ > include/linux/pci.h | 1 + > 2 files changed, 8 insertions(+) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 512cb4312ddd..75c0a25af44e 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -598,6 +598,13 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge) > bridge->native_shpc_hotplug = 1; > bridge->native_pme = 1; > bridge->native_ltr = 1; > + > + /* > + * Some systems may disable ATS at the host bridge (ACPI IORT, > + * device-tree), other filter it with a smaller granularity (ACPI DMAR > + * and IVRS). > + */ > + bridge->ats_supported = 1; The cover letter says it's important to enable ATS only if the host bridge supports it. From the other patches, it looks like we learn if the host bridge supports ATS from either a DT "ats-supported" property or an ACPI IORT table. If that's the case, shouldn't the default here be "ATS is *not* supported"? > } > > struct pci_host_bridge *pci_alloc_host_bridge(size_t priv) > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 3840a541a9de..9fe2e84d74d7 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -511,6 +511,7 @@ struct pci_host_bridge { > unsigned int native_pme:1; /* OS may use PCIe PME */ > unsigned int native_ltr:1; /* OS may use PCIe LTR */ > unsigned int preserve_config:1; /* Preserve FW resource setup */ > + unsigned int ats_supported:1; > > /* Resource alignment requirements */ > resource_size_t (*align_resource)(struct pci_dev *dev, > -- > 2.25.0 >
On Sat, Feb 15, 2020 at 03:10:47PM -0600, Bjorn Helgaas wrote: > On Thu, Feb 13, 2020 at 05:50:40PM +0100, Jean-Philippe Brucker wrote: > > Each vendor has their own way of describing whether a host bridge > > supports ATS. The Intel and AMD ACPI tables selectively enable or > > disable ATS per device or sub-tree, while Arm has a single bit for each > > host bridge. For those that need it, add an ats_supported bit to the > > host bridge structure. > > > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > > --- > > drivers/pci/probe.c | 7 +++++++ > > include/linux/pci.h | 1 + > > 2 files changed, 8 insertions(+) > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > index 512cb4312ddd..75c0a25af44e 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -598,6 +598,13 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge) > > bridge->native_shpc_hotplug = 1; > > bridge->native_pme = 1; > > bridge->native_ltr = 1; > > + > > + /* > > + * Some systems may disable ATS at the host bridge (ACPI IORT, > > + * device-tree), other filter it with a smaller granularity (ACPI DMAR > > + * and IVRS). > > + */ > > + bridge->ats_supported = 1; > > The cover letter says it's important to enable ATS only if the host > bridge supports it. From the other patches, it looks like we learn if > the host bridge supports ATS from either a DT "ats-supported" property > or an ACPI IORT table. If that's the case, shouldn't the default here > be "ATS is *not* supported"? The ACPI IVRS table (AMD) doesn't have a property for the host bridge, it can only deselect ATS for a sub-range of devices. Similarly the DMAR table (Intel) declares that ATS is supported either by the whole PCIe domain or for sub-ranges of devices. I selected ats_supported at the bridge by default since IVRS needs it and DMAR has its own fine-grained ATS support configuration. I'm still not sure this is the right approach, given that the ats_supported bridge property doesn't exactly correspond to a firmware property on all platforms. Maybe the device-tree implementation should follow the IORT one where each device carries a fwspec property stating "root-complex supports ATS". But it isn't nice either so I tried a cleaner implementation (as discussed with Robin back on the ATS-with-SMMUv3 series [1]). Thanks, Jean [1] https://lore.kernel.org/linux-iommu/c10c7adb-c7f6-f8c6-05cc-f4f143427a2d@arm.com/
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 512cb4312ddd..75c0a25af44e 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -598,6 +598,13 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge) bridge->native_shpc_hotplug = 1; bridge->native_pme = 1; bridge->native_ltr = 1; + + /* + * Some systems may disable ATS at the host bridge (ACPI IORT, + * device-tree), other filter it with a smaller granularity (ACPI DMAR + * and IVRS). + */ + bridge->ats_supported = 1; } struct pci_host_bridge *pci_alloc_host_bridge(size_t priv) diff --git a/include/linux/pci.h b/include/linux/pci.h index 3840a541a9de..9fe2e84d74d7 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -511,6 +511,7 @@ struct pci_host_bridge { unsigned int native_pme:1; /* OS may use PCIe PME */ unsigned int native_ltr:1; /* OS may use PCIe LTR */ unsigned int preserve_config:1; /* Preserve FW resource setup */ + unsigned int ats_supported:1; /* Resource alignment requirements */ resource_size_t (*align_resource)(struct pci_dev *dev,
Each vendor has their own way of describing whether a host bridge supports ATS. The Intel and AMD ACPI tables selectively enable or disable ATS per device or sub-tree, while Arm has a single bit for each host bridge. For those that need it, add an ats_supported bit to the host bridge structure. Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> --- drivers/pci/probe.c | 7 +++++++ include/linux/pci.h | 1 + 2 files changed, 8 insertions(+)