arm64: SMMU-v2: Workaround for Cavium ThunderX erratum 28168
Subsystems (5)
PATCH v3 1/1 arm64: SMMU-v2: Workaround for Cavium ThunderX erratum 28168
From: Tirumalesh Chalamarla <[email protected]>
This patch implements Cavium ThunderX erratum 28168.
PCI requires stores complete in order. Due to erratum #28168 PCI-inbound MSI-X store to the interrupt controller are delivered to the interrupt controller before older PCI-inbound memory stores are committed.
Doing a sync on SMMU will make sure all prior data transfers are completed before invoking ISR.
changes from v2:
- added entry in Documentation/arm64/silicon-errata.txt
- moved registration of the preflow_handler into msi_domain_ops.msi_finish() handler.
- create linux/arm.smmu.h to expose smmu API.
--- Documentation/arm64/silicon-errata.txt | 1 + arch/arm64/Kconfig | 12 ++++++++ arch/arm64/include/asm/cpucaps.h | 3 +- arch/arm64/kernel/cpu_errata.c | 16 +++++++++++ drivers/iommu/arm-smmu.c | 22 +++++++++++++++ drivers/irqchip/irq-gic-v3-its-pci-msi.c | 48 +++++++++++++++++++++++++++++++- include/linux/arm-smmu.h | 8 ++++++ 7 files changed, 108 insertions(+), 2 deletions(-) create mode 100644 include/linux/arm-smmu.h
Documentation/arm64/silicon-errata.txt +1 -0
@@ -61,5 +61,6 @@ stable kernels. | Cavium | ThunderX GICv3 | #23154 | CAVIUM_ERRATUM_23154 | | Cavium | ThunderX Core | #27456 | CAVIUM_ERRATUM_27456 | | Cavium | ThunderX SMMUv2 | #27704 | N/A | +| Cavium | ThunderX PCI | #28168 | CAVIUM_ERRATUM_28168 | | | | | | | Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 |
arch/arm64/Kconfig +12 -0
@@ -474,6 +474,18 @@ config CAVIUM_ERRATUM_27456 If unsure, say Y. +config CAVIUM_ERRATUM_28168 + bool "Cavium erratum 28168: Make sure DMA data transfer is done before MSIX" + depends on ARM64 && ARM_SMMU + default y + select IRQ_PREFLOW_FASTEOI + help + Due to erratum #28168 PCI-inbound MSI-X store to the interrupt + controller are delivered to the interrupt controller before older + PCI-inbound memory stores are committed. Doing a sync on SMMU + will make sure all prior data transfers are done before invoking ISR. + + If unsure, say Y. endmenu
arch/arm64/include/asm/cpucaps.h +2 -1
@@ -34,7 +34,8 @@#define ARM64_HAS_32BIT_EL0 13#define ARM64_HYP_OFFSET_LOW 14#define ARM64_MISMATCHED_CACHE_LINE_SIZE 15 +#define ARM64_WORKAROUND_CAVIUM_28168 16 -#define ARM64_NCAPS 16 +#define ARM64_NCAPS 17#endif /* __ASM_CPUCAPS_H */
arch/arm64/kernel/cpu_errata.c +16 -0
@@ -123,6 +123,22 @@ static int cpu_enable_trap_ctr_access(void *__unused) MIDR_RANGE(MIDR_THUNDERX_81XX, 0x00, 0x00), },#endif +#ifdef CONFIG_CAVIUM_ERRATUM_28168 + { +/* Cavium ThunderX, T88 pass 1.x - 2.1 */ + .desc = "Cavium erratum 28168", + .capability =ARM64_WORKAROUND_CAVIUM_28168 , +MIDR_RANGE (MIDR_THUNDERX ,0x00 , + (1 << MIDR_VARIANT_SHIFT) | 1), + }, + { +/* Cavium ThunderX, T81 pass 1.0 */ + .desc ="Cavium erratum 28168" , + .capability = ARM64_WORKAROUND_CAVIUM_28168, + MIDR_RANGE(MIDR_THUNDERX_81XX, 0x00, 0x00), + }, +#endif + { .desc ="Mismatched cache line size" , .capability = ARM64_MISMATCHED_CACHE_LINE_SIZE,
drivers/iommu/arm-smmu.c +22 -0
@@ -49,6 +49,8 @@#include <linux/spinlock.h> #include <linux/amba/bus.h> +#include <linux/arm-smmu.h> +#include "io-pgtable.h"
@@ -577,6 +579,26 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu) } } +#ifdef CONFIG_CAVIUM_ERRATUM_28168 +/* +* Cavium ThunderX erratum 28168 +* +* Due to erratum #28168 PCI-inbound MSI-X store to the interrupt +* controller are delivered to the interrupt controller before older +* PCI-inbound memory stores are committed. Doing a sync on SMMU +* will make sure all prior data transfers are completed before +* invoking ISR. +* +*/ +void dev_smmu_tlb_sync (struct device *dev) +{ +struct iommu_fwspec *fwspec = dev->iommu_fwspec; +struct arm_smmu_device *smmu =fwspec_smmu (fwspec); + +if (smmu) +__arm_smmu_tlb_sync (smmu); +} +#endif static void arm_smmu_tlb_sync (void *cookie) {struct arm_smmu_domain *smmu_domain = cookie;
drivers/irqchip/irq-gic-v3-its-pci-msi.c +47 -1
70 lines not on this page · raw message
include/linux/arm-smmu.h new file +8 -0
@@ -0,0 +1,8 @@ +#ifndef _ARM_SMMU_H +#define _ARM_SMMU_H + +#include <linux/pci.h> + +extern void dev_smmu_tlb_sync (struct device *dev); + +#endif /* _ARM_SMMU_H */
Signature · 1 line
1.9.1
PATCH v3 1/1 arm64: SMMU-v2: Workaround for Cavium ThunderX erratum 28168
Geetha,
Quoted 11 lines · Geetha sowjanya
From: Tirumalesh Chalamarla <[email protected]>
This patch implements Cavium ThunderX erratum 28168.
PCI requires stores complete in order. Due to erratum #28168 PCI-inbound MSI-X store to the interrupt controller are delivered to the interrupt controller before older PCI-inbound memory stores are committed.
Doing a sync on SMMU will make sure all prior data transfers are completed before invoking ISR.
Quoted 7 lines
changes from v2:
- added entry inDocumentation/arm64/silicon-errata.txt
- moved registration of thepreflow_handlerintomsi_domain_ops.msi_finish()handler.
- createlinux/arm.smmu.hto expose smmu API.Signed-off-by Tirumalesh Chalamarla <[email protected]>
Where is your SoB? This is a requirement if you're picking a patch from someone else.
Quoted 113 lines
--- Documentation/arm64/silicon-errata.txt | 1 + arch/arm64/Kconfig | 12 ++++++++ arch/arm64/include/asm/cpucaps.h | 3 +- arch/arm64/kernel/cpu_errata.c | 16 +++++++++++ drivers/iommu/arm-smmu.c | 22 +++++++++++++++ drivers/irqchip/irq-gic-v3-its-pci-msi.c | 48 +++++++++++++++++++++++++++++++- include/linux/arm-smmu.h | 8 ++++++ 7 files changed, 108 insertions(+), 2 deletions(-) create mode 100644 include/linux/arm-smmu.hDocumentation/arm64/silicon-errata.txt +1
-0@@ -61,5 +61,6 @@ stable kernels. | Cavium | ThunderX GICv3 | #23154 | CAVIUM_ERRATUM_23154 | | Cavium | ThunderX Core | #27456 | CAVIUM_ERRATUM_27456 | | Cavium | ThunderX SMMUv2 | #27704 | N/A | +| Cavium | ThunderX PCI | #28168 | CAVIUM_ERRATUM_28168 | | | | | | | Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 |arch/arm64/Kconfig +12
-0@@ -474,6 +474,18 @@ config CAVIUM_ERRATUM_27456 If unsure, say Y. +config CAVIUM_ERRATUM_28168 + bool "Cavium erratum 28168: Make sure DMA data transfer is done before MSIX" + depends on ARM64 && ARM_SMMU + default y + select IRQ_PREFLOW_FASTEOI + help + Due to erratum #28168 PCI-inbound MSI-X store to the interrupt + controller are delivered to the interrupt controller before older + PCI-inbound memory stores are committed. Doing a sync on SMMU + will make sure all prior data transfers are done before invoking ISR. + + If unsure, say Y. endmenuarch/arm64/include/asm/cpucaps.h +2
-1@@ -34,7 +34,8 @@#define ARM64_HAS_32BIT_EL0 13#define ARM64_HYP_OFFSET_LOW 14#define ARM64_MISMATCHED_CACHE_LINE_SIZE 15 +#define ARM64_WORKAROUND_CAVIUM_28168 16 -#define ARM64_NCAPS 16 +#define ARM64_NCAPS 17#endif /* __ASM_CPUCAPS_H */ arch/arm64/kernel/cpu_errata.c +16
-0@@ -123,6 +123,22 @@ static int cpu_enable_trap_ctr_access(void *__unused) MIDR_RANGE(MIDR_THUNDERX_81XX, 0x00, 0x00), },#endif +#ifdef CONFIG_CAVIUM_ERRATUM_28168 + { +/* Cavium ThunderX, T88 pass 1.x - 2.1 */ + .desc = "Cavium erratum 28168", + .capability =ARM64_WORKAROUND_CAVIUM_28168 , +MIDR_RANGE (MIDR_THUNDERX ,0x00 , + (1 << MIDR_VARIANT_SHIFT) | 1), + }, + { +/* Cavium ThunderX, T81 pass 1.0 */ + .desc ="Cavium erratum 28168" , + .capability = ARM64_WORKAROUND_CAVIUM_28168, + MIDR_RANGE(MIDR_THUNDERX_81XX, 0x00, 0x00), + }, +#endif + { .desc ="Mismatched cache line size" , .capability = ARM64_MISMATCHED_CACHE_LINE_SIZE,drivers/iommu/arm-smmu.c +12
-0@@ -49,6 +49,8 @@#include <linux/spinlock.h> #include <linux/amba/bus.h> +#include <linux/arm-smmu.h> +#include "io-pgtable.h" @@ -577,6 +579,26 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu) } } +#ifdef CONFIG_CAVIUM_ERRATUM_28168 +/* +* Cavium ThunderX erratum 28168 +* +* Due to erratum #28168 PCI-inbound MSI-X store to the interrupt +* controller are delivered to the interrupt controller before older +* PCI-inbound memory stores are committed. Doing a sync on SMMU +* will make sure all prior data transfers are completed before +* invoking ISR. +*
drivers/iommu/arm-smmu.c static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu) · line 601
Quoted 10 lines
drivers/iommu/arm-smmu.c +10
-0+*/ +void dev_smmu_tlb_sync (struct device *dev) +{ +struct iommu_fwspec *fwspec = dev->iommu_fwspec; +struct arm_smmu_device *smmu =fwspec_smmu (fwspec); + +if (smmu) +__arm_smmu_tlb_sync (smmu); +} +#endif
I'll let Robin and Will comment on this, but it strikes be as rather odd that nothing will happen if the SMMU is in bypass or simply compiled out. So this workaround is at best incomplete.
Quoted 25 lines
drivers/iommu/arm-smmu.c +0
-0static void arm_smmu_tlb_sync (void *cookie) {struct arm_smmu_domain *smmu_domain = cookie;drivers/irqchip/irq-gic-v3-its-pci-msi.c +2
-1@@ -20,6 +20,7 @@#include <linux/of.h> #include <linux/of_irq.h> #include <linux/of_pci.h> +#include <linux/arm-smmu.h> static void its_mask_msi_irq (struct irq_data *d) {@@ -86,11 +87,53 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,/* ITS specific DeviceID, as the core ITS ignores dev. */ info->scratchpad[0 ].ul =pci_msi_domain_get_msi_rid (domain, pdev); - + info->scratchpad[1 ].ptr = &pdev->dev;return msi_info->ops->msi_prepare (domain->parent, dev, dev_alias.count, info); }
drivers/irqchip/irq-gic-v3-its-pci-msi.c static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev, · line 104
Quoted 10 lines
drivers/irqchip/irq-gic-v3-its-pci-msi.c +10
-0+#ifdef CONFIG_CAVIUM_ERRATUM_28168 +#define THUNDER_PME_DEVICE_ID 0xA100 + +static void cavium_irq_preflow_handler (struct irq_data *data) +{ +struct pci_dev *pdev; + + pdev =msi_desc_to_pci_dev (irq_data_get_msi_desc (data)); +dev_smmu_tlb_sync (&pdev->dev); +}
Missing space between the two functions.
Quoted 9 lines
drivers/irqchip/irq-gic-v3-its-pci-msi.c +9
-0+static void cavium_its_pci_msi_finish (msi_alloc_info_t *info,int ret) +{ +struct device *dev = info->scratchpad[1 ].ptr; +struct pci_dev *pdev =to_pci_dev (dev); +struct device *parent_dev; +struct msi_desc *desc; + +if (ret) +return ;
drivers/irqchip/irq-gic-v3-its-pci-msi.c static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev, · line 123
Quoted 10 lines
drivers/irqchip/irq-gic-v3-its-pci-msi.c +10
-0+ +if (pci_pcie_type (pdev) !=PCI_EXP_TYPE_ROOT_PORT ) { +for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) { + pdev =to_pci_dev (parent_dev); +if (pci_pcie_type (pdev) !=PCI_EXP_TYPE_ROOT_PORT ) +continue ; + +if (pdev->device ==THUNDER_PME_DEVICE_ID ) +for_each_pci_msi_entry (desc,to_pci_dev (dev)) +__irq_set_preflow_handler (desc->irq, cavium_irq_preflow_handler);
So only the PME device is affected? Why can't you simply use INTx for it (there is already some provision for that in the PME driver ("pcie_pme=nomsi").
If your PME device doesn't implement legacy interrupt, can't you do something in the PME interrupt handler instead? Like reading from the end-point or something similar?
drivers/irqchip/irq-gic-v3-its-pci-msi.c static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev, · line 131
Quoted 8 lines
drivers/irqchip/irq-gic-v3-its-pci-msi.c +8
-0+ break; + } + } +} +#else +static void cavium_irq_preflow_handler (struct irq_data *data) +{ +}
Why is this defined? It is just going to generate a warning if CONFIG_CAVIUM_ERRATUM_28168 is not defined...
drivers/irqchip/irq-gic-v3-its-pci-msi.c static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev, · line 125
Quoted 3 lines
drivers/irqchip/irq-gic-v3-its-pci-msi.c +3
-0+static void cavium_its_pci_msi_finish (msi_alloc_info_t *info,int ret) +{ +}
You might as well have
#define cavium_its_pci_msi_finish NULL
Quoted 22 lines
drivers/irqchip/irq-gic-v3-its-pci-msi.c +5
-0+#endif +static struct msi_domain_ops its_pci_msi_ops = { .msi_prepare = its_pci_msi_prepare, };@@ -118,6 +161,9 @@ static int __init its_pci_msi_init_one(struct fwnode_handle *handle,return -ENXIO ; } +if (cpus_have_cap (ARM64_WORKAROUND_CAVIUM_28168 )) + its_pci_msi_ops.msi_finish = cavium_its_pci_msi_finish; +if (!pci_msi_create_irq_domain (handle, &its_pci_msi_domain_info, parent)) {pr_err ("%s: Unable to create PCI domain\n" , name);include/linux/arm-smmu.h new file +0
-0
include/linux/arm-smmu.h line 8
Quoted 8 lines
include/linux/arm-smmu.h new file +8
-0@@ -0,0 +1,8 @@ +#ifndef _ARM_SMMU_H +#define _ARM_SMMU_H + +#include <linux/pci.h> + +extern void dev_smmu_tlb_sync (struct device *dev); + +#endif /* _ARM_SMMU_H */
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH v3 1/1 arm64: SMMU-v2: Workaround for Cavium ThunderX erratum 28168
Quoted 18 lines · Marc Zyngier
Quoted 17 lines · Geetha sowjanya
From: Tirumalesh Chalamarla <[email protected]>
drivers/iommu/arm-smmu.c +16
-0+#ifdef CONFIG_CAVIUM_ERRATUM_28168 +/* +* Cavium ThunderX erratum 28168 +* +* Due to erratum #28168 PCI-inbound MSI-X store to the interrupt +* controller are delivered to the interrupt controller before older +* PCI-inbound memory stores are committed. Doing a sync on SMMU +* will make sure all prior data transfers are completed before +* invoking ISR. +* +*/ +void dev_smmu_tlb_sync (struct device *dev) +{ +struct iommu_fwspec *fwspec = dev->iommu_fwspec; +struct arm_smmu_device *smmu =fwspec_smmu (fwspec); +
Quoted 8 lines
drivers/iommu/arm-smmu.c static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu) · line 601
Quoted 4 lines
drivers/iommu/arm-smmu.c +4
-0+if (smmu) + __arm_smmu_tlb_sync(smmu); +} +#endif I'll let Robin and Will comment on this, but it strikes be as rather odd that nothing will happen if the SMMU is in bypass or simply compiled out. So this workaround is at best incomplete.
Agreed. Unless the SMMU is the cause of the issue, relying on it to implement the workaround is fragile and unnecessarily introduces a linkage between SMMU driver internals and the interrupt handling code.
Not a fan.
Will
PATCH v3 1/1 arm64: SMMU-v2: Workaround for Cavium ThunderX erratum 28168
Quoted 28 lines · Will Deacon
Quoted 18 lines · Marc Zyngier
Quoted 17 lines · Geetha sowjanya
From: Tirumalesh Chalamarla <[email protected]>
drivers/iommu/arm-smmu.c +16
-0+#ifdef CONFIG_CAVIUM_ERRATUM_28168 +/* +* Cavium ThunderX erratum 28168 +* +* Due to erratum #28168 PCI-inbound MSI-X store to the interrupt +* controller are delivered to the interrupt controller before older +* PCI-inbound memory stores are committed. Doing a sync on SMMU +* will make sure all prior data transfers are completed before +* invoking ISR. +* +*/ +void dev_smmu_tlb_sync (struct device *dev) +{ +struct iommu_fwspec *fwspec = dev->iommu_fwspec; +struct arm_smmu_device *smmu =fwspec_smmu (fwspec); +Quoted 8 lines
drivers/iommu/arm-smmu.c static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu) · line 601
Quoted 4 lines
drivers/iommu/arm-smmu.c +4
-0+if (smmu) + __arm_smmu_tlb_sync(smmu); +} +#endif I'll let Robin and Will comment on this, but it strikes be as rather odd that nothing will happen if the SMMU is in bypass or simply compiled out. So this workaround is at best incomplete.
Quoted 3 lines
Agreed. Unless the SMMU is the cause of the issue, relying on it to implement the workaround is fragile and unnecessarily introduces a linkage between SMMU driver internals and the interrupt handling code.
The SMMU is not the cause of the problem, it is the solution, and as such is required.
Perhaps we should have a Kconfig "select" for the SMMU driver if CAVIUM_ERRATUM_28168 is selected.
Quoted 2 lines
Not a fan.
In general, I don't think anybody is a big fan of errata and their workarounds.
Quoted 8 lines
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
A message that is not in the archive (sent off-list or not yet received): d8aed795-f4ce-609e-ccd2-0daf628b0376-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org
Quoted 28 lines · Will Deacon
Quoted 27 lines · Marc Zyngier
Quoted 25 lines · Geetha sowjanya
Quoted 11 lines
From: Tirumalesh Chalamarla <[email protected]>
drivers/iommu/arm-smmu.c +10
-0+#ifdef CONFIG_CAVIUM_ERRATUM_28168 +/* +* Cavium ThunderX erratum 28168 +* +* Due to erratum #28168 PCI-inbound MSI-X store to the interrupt +* controller are delivered to the interrupt controller before older +* PCI-inbound memory stores are committed. Doing a sync on SMMU +* will make sure all prior data transfers are completed before +* invoking ISR. +* drivers/iommu/arm-smmu.c static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu) · line 601
Quoted 10 lines
drivers/iommu/arm-smmu.c +10
-0+*/ +void dev_smmu_tlb_sync (struct device *dev) +{ +struct iommu_fwspec *fwspec = dev->iommu_fwspec; +struct arm_smmu_device *smmu =fwspec_smmu (fwspec); + +if (smmu) +__arm_smmu_tlb_sync (smmu); +} +#endif I'll let Robin and Will comment on this, but it strikes be as rather odd that nothing will happen if the SMMU is in bypass or simply compiled out. So this workaround is at best incomplete.
Quoted 3 lines
Quoted 3 lines
Agreed. Unless the SMMU is the cause of the issue, relying on it to implement the workaround is fragile and unnecessarily introduces a linkage between SMMU driver internals and the interrupt handling code.
We checked all possible ways. This is the only workaround we found.
Quoted 8 lines
The SMMU is not the cause of the problem, it is the solution, and as such is required.
Perhaps we should have a
Kconfig"select" for the SMMU driver ifCAVIUM_ERRATUM_28168is selected.
Quoted 4 lines
Not a fan.
In general, I don't think anybody is a big fan of errata and their workarounds.
Thank you,
Geetha.
Quoted 8 lines
Will
_______________________________________________
linux-arm-kernel mailing list linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel