Skip to contentSkip to conversation

arm64: SMMU-v2: Workaround for Cavium ThunderX erratum 28168

PATCH New Geetha sowjanya 5 participants 5 messages Dec 20, 2016 – Dec 21, 2016 1 message not in the archive

Subsystems (5)
lore
Share & tools
lore
#0 lore raw

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
index 405da11..1311f90 100644
@@ -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
index 969ef88..cb647f4 100644
@@ -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
index 87b4465..6975a01 100644
@@ -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
index b75e917..fac6d74 100644
@@ -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
index 06167da..451b393 100644
@@ -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
new file mode 100644
index 0000000..d3f5dff
@@ -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
#1 lore raw

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 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.

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.h
Documentation/arm64/silicon-errata.txt +1 -0
index 405da11..1311f90 100644
@@ -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
index 969ef88..cb647f4 100644
@@ -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
index 87b4465..6975a01 100644
@@ -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
index b75e917..fac6d74 100644
@@ -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
index 06167da..451b393 100644
@@ -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 -0
 static 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
index aee1c60..36ce696 100644
@@ -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
new file mode 100644
index 0000000..d3f5dff

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...
#2 lore raw

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

#3 lore raw

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

#5 lore raw
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 if CAVIUM_ERRATUM_28168 is 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