[PATCH v2] arm64: SMMU-v2: Workaround for Cavium ThunderX erratum 28168

Discussion:

Geetha sowjanya

2016-11-15 07:00:20 UTC

From: Tirumalesh Chalamarla <***@cavium.com>

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.

Signed-off-by: Tirumalesh Chalamarla <***@cavium.com>
Signed-off-by: Geetha sowjanya <***@caviumnetworks.com>
---
arch/arm64/Kconfig | 11 +++++++++++
arch/arm64/Kconfig.platforms | 1 +
arch/arm64/include/asm/cpufeature.h | 3 ++-
arch/arm64/kernel/cpu_errata.c | 16 ++++++++++++++++
drivers/iommu/arm-smmu.c | 24 ++++++++++++++++++++++++
drivers/irqchip/irq-gic-common.h | 1 +
drivers/irqchip/irq-gic-v3.c | 19 +++++++++++++++++++
7 files changed, 74 insertions(+), 1 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 30398db..751972c 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -474,6 +474,17 @@ 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 ARCH_THUNDER && ARM64
+ default y
+ 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

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index cfbdf02..2ac4ac6 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -185,6 +185,7 @@ config ARCH_SPRD

config ARCH_THUNDER
bool "Cavium Inc. Thunder SoC Family"
+ select IRQ_PREFLOW_FASTEOI
help
This enables support for Cavium's Thunder Family of SoCs.

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 758d74f..821fc3c 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -40,8 +40,9 @@
#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

#ifndef __ASSEMBLY__

diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 0150394..0841a12 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -122,6 +122,22 @@ static void 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,
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index c841eb7..1b4555c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -570,6 +570,30 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
}
}

+/*
+ * 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 cavium_arm_smmu_tlb_sync(struct device *dev)
+{
+ struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+ struct arm_smmu_device *smmu;
+
+ smmu = fwspec_smmu(fwspec);
+ if (!smmu)
+ return;
+ __arm_smmu_tlb_sync(smmu);
+
+}
+EXPORT_SYMBOL(cavium_arm_smmu_tlb_sync);
+
+
static void arm_smmu_tlb_sync(void *cookie)
{
struct arm_smmu_domain *smmu_domain = cookie;
diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
index 205e5fd..4e88f55 100644
--- a/drivers/irqchip/irq-gic-common.h
+++ b/drivers/irqchip/irq-gic-common.h
@@ -38,4 +38,5 @@ void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,

void gic_set_kvm_info(const struct gic_kvm_info *info);

+void cavium_arm_smmu_tlb_sync(struct device *dev);
#endif /* _IRQ_GIC_COMMON_H */
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 19d642e..723cebe 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -28,6 +28,8 @@
#include <linux/of_irq.h>
#include <linux/percpu.h>
#include <linux/slab.h>
+#include <linux/msi.h>
+#include <linux/pci.h>

#include <linux/irqchip.h>
#include <linux/irqchip/arm-gic-common.h>
@@ -736,6 +738,20 @@ static inline void gic_cpu_pm_init(void) { }

#define GIC_ID_NR (1U << gic_data.rdists.id_bits)

+/*
+ * Due to #28168 erratum in ThunderX,
+ * we need to make sure DMA data transfer is done before MSIX.
+ */
+static void cavium_irq_perflow_handler(struct irq_data *data)
+{
+ struct pci_dev *pdev;
+
+ pdev = msi_desc_to_pci_dev(irq_data_get_msi_desc(data));
+ if ((pdev->vendor != 0x177d) &&
+ ((pdev->device & 0xA000) != 0xA000))
+ cavium_arm_smmu_tlb_sync(&pdev->dev);
+}
+
static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
irq_hw_number_t hw)
{
@@ -773,6 +789,9 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
return -EPERM;
irq_domain_set_info(d, irq, hw, chip, d->host_data,
handle_fasteoi_irq, NULL, NULL);
+ if (cpus_have_cap(ARM64_WORKAROUND_CAVIUM_28168))
+ __irq_set_preflow_handler(irq,
+ cavium_irq_perflow_handler);
}

return 0;

--
1.7.1

Marc Zyngier

2016-11-15 09:26:27 UTC

Permalink

Post by Geetha sowjanya
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.
---
arch/arm64/Kconfig | 11 +++++++++++
arch/arm64/Kconfig.platforms | 1 +
arch/arm64/include/asm/cpufeature.h | 3 ++-
arch/arm64/kernel/cpu_errata.c | 16 ++++++++++++++++
drivers/iommu/arm-smmu.c | 24 ++++++++++++++++++++++++
drivers/irqchip/irq-gic-common.h | 1 +
drivers/irqchip/irq-gic-v3.c | 19 +++++++++++++++++++
7 files changed, 74 insertions(+), 1 deletions(-)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 30398db..751972c 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -474,6 +474,17 @@ 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 ARCH_THUNDER && ARM64
+ default y
+ 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.

Where is the entry in Documentation/arm64/silicon-errata.txt?

Post by Geetha sowjanya
endmenu
diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index cfbdf02..2ac4ac6 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -185,6 +185,7 @@ config ARCH_SPRD
config ARCH_THUNDER
bool "Cavium Inc. Thunder SoC Family"
+ select IRQ_PREFLOW_FASTEOI
help
This enables support for Cavium's Thunder Family of SoCs.
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 758d74f..821fc3c 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -40,8 +40,9 @@
#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
#ifndef __ASSEMBLY__
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 0150394..0841a12 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -122,6 +122,22 @@ static void 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

How is that a CPU bug? Shouldn't that be keyed on the SMMU version or
the ITs version?

Post by Geetha sowjanya
+
{
.desc = "Mismatched cache line size",
.capability = ARM64_MISMATCHED_CACHE_LINE_SIZE,
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index c841eb7..1b4555c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -570,6 +570,30 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
}
}
+/*
+ * 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 cavium_arm_smmu_tlb_sync(struct device *dev)
+{
+ struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+ struct arm_smmu_device *smmu;
+
+ smmu = fwspec_smmu(fwspec);
+ if (!smmu)
+ return;
+ __arm_smmu_tlb_sync(smmu);
+
+}
+EXPORT_SYMBOL(cavium_arm_smmu_tlb_sync);

Why does this need to be exported? The only user can only be built-in.

Post by Geetha sowjanya
+
+
static void arm_smmu_tlb_sync(void *cookie)
{
struct arm_smmu_domain *smmu_domain = cookie;
diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
index 205e5fd..4e88f55 100644
--- a/drivers/irqchip/irq-gic-common.h
+++ b/drivers/irqchip/irq-gic-common.h
@@ -38,4 +38,5 @@ void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
void gic_set_kvm_info(const struct gic_kvm_info *info);
+void cavium_arm_smmu_tlb_sync(struct device *dev);

Why should this be visible to GICv2 as well? I have the ugly feeling
this should stay private to the SMMU code and that a more standard
mechanism should be used... Robin, is there anything else we could
piggy-back on?

Post by Geetha sowjanya
#endif /* _IRQ_GIC_COMMON_H */
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 19d642e..723cebe 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -28,6 +28,8 @@
#include <linux/of_irq.h>
#include <linux/percpu.h>
#include <linux/slab.h>
+#include <linux/msi.h>
+#include <linux/pci.h>
#include <linux/irqchip.h>
#include <linux/irqchip/arm-gic-common.h>
@@ -736,6 +738,20 @@ static inline void gic_cpu_pm_init(void) { }
#define GIC_ID_NR (1U << gic_data.rdists.id_bits)
+/*
+ * Due to #28168 erratum in ThunderX,
+ * we need to make sure DMA data transfer is done before MSIX.
+ */
+static void cavium_irq_perflow_handler(struct irq_data *data)
+{
+ struct pci_dev *pdev;
+
+ pdev = msi_desc_to_pci_dev(irq_data_get_msi_desc(data));

What happens if this is not a PCI device?

Post by Geetha sowjanya
+ if ((pdev->vendor != 0x177d) &&
+ ((pdev->device & 0xA000) != 0xA000))
+ cavium_arm_smmu_tlb_sync(&pdev->dev);

I've asked that before. What makes Cavium devices so special that they
are not sensitive to this bug?

Post by Geetha sowjanya
+}
+
static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
irq_hw_number_t hw)
{
@@ -773,6 +789,9 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
return -EPERM;
irq_domain_set_info(d, irq, hw, chip, d->host_data,
handle_fasteoi_irq, NULL, NULL);
+ if (cpus_have_cap(ARM64_WORKAROUND_CAVIUM_28168))
+ __irq_set_preflow_handler(irq,
+ cavium_irq_perflow_handler);

What happens if SMMUv2 is not compiled in? Also, since this only affects
LPI signaling, why is this in the core GICv3 code and not in the ITS.
And more specifically, in the PCI part of the ITS, since you seem to
exclusively consider PCI?

Post by Geetha sowjanya
}
return 0;

Thanks,

M.

--
Jazz is not dead. It just smells funny...

Robin Murphy

2016-11-15 12:36:12 UTC

Permalink

Post by Marc Zyngier

Where is the entry in Documentation/arm64/silicon-errata.txt?

How is that a CPU bug? Shouldn't that be keyed on the SMMU version or
the ITs version?

Seconded. I assume the actual component at fault is the PCI RC, SMMU, or
interconnect in between - are there no ID registers in those parts that
will indicate a fixed version (or ECO) in future?

Post by Marc Zyngier

Further to my questions on the last posting, is TLBGSYNC alone
guaranteed to always do something, even if there are no outstanding
invalidation requests? Will this workaround still apply if
__arm_smmu_tlb_sync() were to use CBn_TLBSYNC instead?

Post by Marc Zyngier

Post by Geetha sowjanya
+
+}
+EXPORT_SYMBOL(cavium_arm_smmu_tlb_sync);

Why does this need to be exported? The only user can only be built-in.

I'm not sure it's actually any less horrible, but technically, one
*could* externally force a sync like so:

struct iommu_domain *dom = iommu_get_domain_for_dev(dev);
iommu_map(dom, <some reserved IOVA>, <some safe PA>, PAGE_SIZE, IOMMU_READ;
iommu_unmap(dom, <IOVA>, PAGE_SIZE);

(needs also to be error-safe and race-safe, obviously)

although there's rather a lot of extra overhead involved, and it also
relies on the driver not doing any Intel-style lazy unmapping.

The other 'generic' way which comes to mind would be some magic domain
attribute which has a sync side-effect on read (or write), but that's
utterly vile. And I'm not even going to entertain the thought of having
the SMMU driver implement a fake irqchip stacked on top of the ITS for
the sake of keeping all the code in one place...

Post by Marc Zyngier

perflow?

Post by Marc Zyngier

Post by Geetha sowjanya
+{
+ struct pci_dev *pdev;
+
+ pdev = msi_desc_to_pci_dev(irq_data_get_msi_desc(data));

What happens if this is not a PCI device?

Post by Geetha sowjanya
+ if ((pdev->vendor != 0x177d) &&
+ ((pdev->device & 0xA000) != 0xA000))
+ cavium_arm_smmu_tlb_sync(&pdev->dev);

I've asked that before. What makes Cavium devices so special that they
are not sensitive to this bug?

What happens if SMMUv2 is not compiled in?

drivers/built-in.o: In function `cavium_irq_perflow_handler':
/work/src/linux/drivers/irqchip/irq-gic-v3.c:752: undefined reference to
`cavium_arm_smmu_tlb_sync'
make: *** [vmlinux] Error 1

Robin

Post by Marc Zyngier
Also, since this only affects
LPI signaling, why is this in the core GICv3 code and not in the ITS.
And more specifically, in the PCI part of the ITS, since you seem to
exclusively consider PCI?

Post by Geetha sowjanya
}
return 0;

Thanks,
M.

David Daney

2016-11-15 18:24:22 UTC

Permalink

Post by Marc Zyngier

[...]

Post by Marc Zyngier

Post by Geetha sowjanya
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -28,6 +28,8 @@
#include <linux/of_irq.h>
#include <linux/percpu.h>
#include <linux/slab.h>
+#include <linux/msi.h>
+#include <linux/pci.h>
#include <linux/irqchip.h>
#include <linux/irqchip/arm-gic-common.h>
@@ -736,6 +738,20 @@ static inline void gic_cpu_pm_init(void) { }
#define GIC_ID_NR (1U << gic_data.rdists.id_bits)
+/*
+ * Due to #28168 erratum in ThunderX,
+ * we need to make sure DMA data transfer is done before MSIX.
+ */
+static void cavium_irq_perflow_handler(struct irq_data *data)
+{
+ struct pci_dev *pdev;
+
+ pdev = msi_desc_to_pci_dev(irq_data_get_msi_desc(data));

What happens if this is not a PCI device?

Post by Geetha sowjanya
+ if ((pdev->vendor != 0x177d) &&
+ ((pdev->device & 0xA000) != 0xA000))
+ cavium_arm_smmu_tlb_sync(&pdev->dev);

I've asked that before. What makes Cavium devices so special that they
are not sensitive to this bug?

This is a heuristic for devices connected to external PCIe buses as
opposed to on-SoC devices (which don't suffer from the erratum).

In any event what would happen if we got rid of this check and ...

Post by Marc Zyngier

... move the registration of the preflow_handler into a
msi_domain_ops.msi_finish() handler in irq-git-v3-its-pic-msi.c?

There we will know that it is a pci device, and can walk up the bus
hierarchy to see if there is a Cavium PCIe root port present. If such a
port is found, we know we are on an external Cavium PCIe bus, and can
register the preflow_handler without having to check the device identifiers.

Post by Marc Zyngier
What happens if SMMUv2 is not compiled in? Also, since this only affects
LPI signaling, why is this in the core GICv3 code and not in the ITS.
And more specifically, in the PCI part of the ITS, since you seem to
exclusively consider PCI?

Post by Geetha sowjanya
}
return 0;

Thanks,
M.

Marc Zyngier

2016-11-16 09:54:35 UTC

Permalink

Post by David Daney

Post by Marc Zyngier

[...]

Post by Marc Zyngier

What happens if this is not a PCI device?

Post by Geetha sowjanya
+ if ((pdev->vendor != 0x177d) &&
+ ((pdev->device & 0xA000) != 0xA000))
+ cavium_arm_smmu_tlb_sync(&pdev->dev);

I've asked that before. What makes Cavium devices so special that they
are not sensitive to this bug?

This is a heuristic for devices connected to external PCIe buses as
opposed to on-SoC devices (which don't suffer from the erratum).
In any event what would happen if we got rid of this check and ...

Post by Marc Zyngier

... move the registration of the preflow_handler into a
msi_domain_ops.msi_finish() handler in irq-git-v3-its-pic-msi.c?

That's the kind of thing I was angling for. You'll have to store the
device pointer into the scratchpad (we still have plenty of space there)
so that msi_finish() can have a peek.

Post by David Daney
There we will know that it is a pci device, and can walk up the bus
hierarchy to see if there is a Cavium PCIe root port present. If such a
port is found, we know we are on an external Cavium PCIe bus, and can
register the preflow_handler without having to check the device identifiers.

Something like that (though I'm unclear why other devices wouldn't see a
root port, but that's probably me lacking some PCIe foo).

Thanks,

M.

--
Jazz is not dead. It just smells funny...