[PATCH v4 0/5] irqchip, gicv3: Updates and Cavium ThunderX errata workarounds

[PATCH v4 0/5] irqchip, gicv3: Updates and Cavium ThunderX errata workarounds

[Robert Richter]

From: Robert Richter <rrichter@cavium.com>

This patch series adds gicv3 updates and workarounds for HW errata in
Cavium's ThunderX GICV3.

The first one is an unchanged resubmission of a patch from a gicv3
series I sent a while ago.

The next patches implement the workarounds for ThunderX's gicv3. Patch
#2 implements the cpu workaround for gicv3 on ThunderX. Patch #3 is a
prerequisit for patch #5. Patch #4 adds generic code to parse the hw
revision provided by an IIDR. This patch is used for the implementa-
tion of the actual gicv3-its workaround in #5.

All current review comments addressed so far with v4.

v4:
 * simplify code to only use cpus_have_cap() in gicv3_enable_quirks()
 * only enable hw detection for its in its_enable_quirks()
 * removed gicv3_check_capabilities()
 * drop special cpu capability for zero

v3:
 * use arm64 errata framework for midr check
 * fix mixup of errata to be dependend from midr/iidr

v2:
 * Workaround for 23154:
   * implement code in a single asm() to keep instruction sequence
   * added comment to the code that explains the erratum
   * apply workaround also if running as guest, thus check MIDR
 * adding MIDR check

Robert Richter (5):
  irqchip, gicv3-its: Add range check for number of allocated pages
  irqchip, gicv3: Workaround for Cavium ThunderX erratum 23154
  irqchip, gicv3-its: Read typer register outside the loop
  irqchip, gicv3-its: Add HW revision detection and configuration
  irqchip, gicv3-its: Workaround for Cavium ThunderX errata 22375, 24313

 arch/arm64/Kconfig                  | 11 +++++++
 arch/arm64/include/asm/cpufeature.h |  3 +-
 arch/arm64/include/asm/cputype.h    | 18 ++++++-----
 arch/arm64/kernel/cpu_errata.c      |  9 ++++++
 drivers/irqchip/irq-gic-common.c    | 11 +++++++
 drivers/irqchip/irq-gic-common.h    |  9 ++++++
 drivers/irqchip/irq-gic-v3-its.c    | 62 +++++++++++++++++++++++++++++++++----
 drivers/irqchip/irq-gic-v3.c        | 42 ++++++++++++++++++++++++-
 include/linux/irqchip/arm-gic-v3.h  |  1 +
 9 files changed, 151 insertions(+), 15 deletions(-)

-- 
2.1.1

[PATCH v4 1/5] irqchip, gicv3-its: Add range check for number of allocated pages

[Robert Richter]

From: Robert Richter <rrichter@cavium.com>

The number of pages for the its table may exceed the maximum of 256.
Adding a range check and limitting the number to its maximum.

Based on a patch from Tirumalesh Chalamarla <tchalamarla@cavium.com>.

Signed-off-by: Tirumalesh Chalamarla <tchalamarla@cavium.com>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 drivers/irqchip/irq-gic-v3-its.c   | 11 ++++++++++-
 include/linux/irqchip/arm-gic-v3.h |  1 +
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 1b7e155869f6..466edf8a7477 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -810,6 +810,7 @@ static int its_alloc_tables(struct its_node *its)
 		u64 entry_size = GITS_BASER_ENTRY_SIZE(val);
 		int order = get_order(psz);
 		int alloc_size;
+		int alloc_pages;
 		u64 tmp;
 		void *base;
 
@@ -844,6 +845,14 @@ static int its_alloc_tables(struct its_node *its)
 		}
 
 		alloc_size = (1 << order) * PAGE_SIZE;
+		alloc_pages = (alloc_size / psz);
+		if (alloc_pages > GITS_BASER_PAGES_MAX) {
+			alloc_pages = GITS_BASER_PAGES_MAX;
+			order = get_order(GITS_BASER_PAGES_MAX * psz);
+			pr_warn("%s: Device Table too large, reduce its page order to %u (%u pages)\n",
+				its->msi_chip.of_node->full_name, order, alloc_pages);
+		}
+
 		base = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, order);
 		if (!base) {
 			err = -ENOMEM;
@@ -872,7 +881,7 @@ static int its_alloc_tables(struct its_node *its)
 			break;
 		}
 
-		val |= (alloc_size / psz) - 1;
+		val |= alloc_pages - 1;
 
 		writeq_relaxed(val, its->base + GITS_BASER + i * 8);
 		tmp = readq_relaxed(its->base + GITS_BASER + i * 8);
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index ffbc034c8810..f28da189c4aa 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -229,6 +229,7 @@
 #define GITS_BASER_PAGE_SIZE_16K	(1UL << GITS_BASER_PAGE_SIZE_SHIFT)
 #define GITS_BASER_PAGE_SIZE_64K	(2UL << GITS_BASER_PAGE_SIZE_SHIFT)
 #define GITS_BASER_PAGE_SIZE_MASK	(3UL << GITS_BASER_PAGE_SIZE_SHIFT)
+#define GITS_BASER_PAGES_MAX		256
 
 #define GITS_BASER_TYPE_NONE		0
 #define GITS_BASER_TYPE_DEVICE		1
-- 
2.1.1

[PATCH v4 2/5] irqchip, gicv3: Workaround for Cavium ThunderX erratum 23154

[Robert Richter]

From: Robert Richter <rrichter@cavium.com>

This patch implements Cavium ThunderX erratum 23154.

The gicv3 of ThunderX requires a modified version for reading the IAR
status to ensure data synchronization. Since this is in the fast-path
and called with each interrupt, runtime patching is used using jump
label patching for smallest overhead (no-op). This is the same
technique as used for tracepoints.

v4:
 * simplify code to only use cpus_have_cap() in gicv3_enable_quirks()

v3:
 * fix erratum to be dependend from midr
 * use arm64 errata framework

v2:
 * implement code in a single asm() to keep instruction sequence
 * added comment to the code that explains the erratum
 * apply workaround also if running as guest, thus check MIDR

Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 arch/arm64/Kconfig                  | 11 ++++++++++
 arch/arm64/include/asm/cpufeature.h |  3 ++-
 arch/arm64/include/asm/cputype.h    | 18 +++++++++-------
 arch/arm64/kernel/cpu_errata.c      |  9 ++++++++
 drivers/irqchip/irq-gic-v3.c        | 42 ++++++++++++++++++++++++++++++++++++-
 5 files changed, 74 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 0f6edb14b7e4..4f866a4c6536 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -417,6 +417,17 @@ config ARM64_ERRATUM_845719
 
 	  If unsure, say Y.
 
+config CAVIUM_ERRATUM_23154
+	bool "Cavium erratum 23154: Access to ICC_IAR1_EL1 is not sync'ed"
+	depends on ARCH_THUNDER
+	default y
+	help
+	  The gicv3 of ThunderX requires a modified version for
+	  reading the IAR status to ensure data synchronization
+	  (access to icc_iar1_el1 is not sync'ed before and after).
+
+	  If unsure, say Y.
+
 endmenu
 
 
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index c1044218a63a..2a5e4c163ee5 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -25,8 +25,9 @@
 #define ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE	1
 #define ARM64_WORKAROUND_845719			2
 #define ARM64_HAS_SYSREG_GIC_CPUIF		3
+#define ARM64_WORKAROUND_CAVIUM_23154		4
 
-#define ARM64_NCAPS				4
+#define ARM64_NCAPS				5
 
 #ifndef __ASSEMBLY__
 
diff --git a/arch/arm64/include/asm/cputype.h b/arch/arm64/include/asm/cputype.h
index a84ec605bed8..3f0c7683f252 100644
--- a/arch/arm64/include/asm/cputype.h
+++ b/arch/arm64/include/asm/cputype.h
@@ -62,15 +62,19 @@
 	(0xf			<< MIDR_ARCHITECTURE_SHIFT) | \
 	((partnum)		<< MIDR_PARTNUM_SHIFT))
 
-#define ARM_CPU_IMP_ARM		0x41
-#define ARM_CPU_IMP_APM		0x50
+#define ARM_CPU_IMP_ARM			0x41
+#define ARM_CPU_IMP_APM			0x50
+#define ARM_CPU_IMP_CAVIUM		0x43
 
-#define ARM_CPU_PART_AEM_V8	0xD0F
-#define ARM_CPU_PART_FOUNDATION	0xD00
-#define ARM_CPU_PART_CORTEX_A57	0xD07
-#define ARM_CPU_PART_CORTEX_A53	0xD03
+#define ARM_CPU_PART_AEM_V8		0xD0F
+#define ARM_CPU_PART_FOUNDATION		0xD00
+#define ARM_CPU_PART_CORTEX_A57		0xD07
+#define ARM_CPU_PART_CORTEX_A53		0xD03
+
+#define APM_CPU_PART_POTENZA		0x000
+
+#define CAVIUM_CPU_PART_THUNDERX	0x0A1
 
-#define APM_CPU_PART_POTENZA	0x000
 
 #define ID_AA64MMFR0_BIGENDEL0_SHIFT	16
 #define ID_AA64MMFR0_BIGENDEL0_MASK	(0xf << ID_AA64MMFR0_BIGENDEL0_SHIFT)
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 6ffd91438560..574450c257a4 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -23,6 +23,7 @@
 
 #define MIDR_CORTEX_A53 MIDR_CPU_PART(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A53)
 #define MIDR_CORTEX_A57 MIDR_CPU_PART(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A57)
+#define MIDR_THUNDERX	MIDR_CPU_PART(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX)
 
 #define CPU_MODEL_MASK (MIDR_IMPLEMENTOR_MASK | MIDR_PARTNUM_MASK | \
 			MIDR_ARCHITECTURE_MASK)
@@ -82,6 +83,14 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
 		MIDR_RANGE(MIDR_CORTEX_A53, 0x00, 0x04),
 	},
 #endif
+#ifdef CONFIG_CAVIUM_ERRATUM_23154
+	{
+	/* Cavium ThunderX, pass 1.x */
+		.desc = "Cavium erratum 23154",
+		.capability = ARM64_WORKAROUND_CAVIUM_23154,
+		MIDR_RANGE(MIDR_THUNDERX, 0x00, 0x01),
+	},
+#endif
 	{
 	}
 };
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index c52f7ba205b4..4211c39b8744 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -107,7 +107,7 @@ static void gic_redist_wait_for_rwp(void)
 }
 
 /* Low level accessors */
-static u64 __maybe_unused gic_read_iar(void)
+static u64 gic_read_iar_common(void)
 {
 	u64 irqstat;
 
@@ -115,6 +115,38 @@ static u64 __maybe_unused gic_read_iar(void)
 	return irqstat;
 }
 
+/*
+ * Cavium ThunderX erratum 23154
+ *
+ * The gicv3 of ThunderX requires a modified version for reading the
+ * IAR status to ensure data synchronization (access to icc_iar1_el1
+ * is not sync'ed before and after).
+ */
+static u64 gic_read_iar_cavium_thunderx(void)
+{
+	u64 irqstat;
+
+	asm volatile(
+		"nop;nop;nop;nop\n\t"
+		"nop;nop;nop;nop\n\t"
+		"mrs_s %0, " __stringify(ICC_IAR1_EL1) "\n\t"
+		"nop;nop;nop;nop"
+		: "=r" (irqstat));
+	mb();
+
+	return irqstat;
+}
+
+struct static_key is_cavium_thunderx = STATIC_KEY_INIT_FALSE;
+
+static u64 __maybe_unused gic_read_iar(void)
+{
+	if (static_key_false(&is_cavium_thunderx))
+		return gic_read_iar_common();
+	else
+		return gic_read_iar_cavium_thunderx();
+}
+
 static void __maybe_unused gic_write_pmr(u64 val)
 {
 	asm volatile("msr_s " __stringify(ICC_PMR_EL1) ", %0" : : "r" (val));
@@ -766,6 +798,12 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
 	.free = gic_irq_domain_free,
 };
 
+static void gicv3_enable_quirks(void)
+{
+	if (cpus_have_cap(ARM64_WORKAROUND_CAVIUM_23154))
+		static_key_slow_inc(&is_cavium_thunderx);
+}
+
 static int __init gic_of_init(struct device_node *node, struct device_node *parent)
 {
 	void __iomem *dist_base;
@@ -825,6 +863,8 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
 	gic_data.nr_redist_regions = nr_redist_regions;
 	gic_data.redist_stride = redist_stride;
 
+	gicv3_enable_quirks();
+
 	/*
 	 * Find out how many interrupts are supported.
 	 * The GIC only supports up to 1020 interrupt sources (SGI+PPI+SPI)
-- 
2.1.1

[PATCH v4 3/5] irqchip, gicv3-its: Read typer register outside the loop

[Robert Richter]

From: Robert Richter <rrichter@cavium.com>

No need to read the typer register in the loop. Values do not change.

This patch is basically a prerequisite for a follow-on patch that adds
errata code for Cavium ThunderX. It moves the calculation of the
number of id entries to the beginning of the function close to other
setup values that are needed to allocate the its table. Now we have a
central location to modify the setup parameters and the errata code
can be implemented in a single block.

Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 466edf8a7477..06131db7a198 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -803,6 +803,8 @@ static int its_alloc_tables(struct its_node *its)
 	int psz = SZ_64K;
 	u64 shr = GITS_BASER_InnerShareable;
 	u64 cache = GITS_BASER_WaWb;
+	u64 typer = readq_relaxed(its->base + GITS_TYPER);
+	u32 ids = GITS_TYPER_DEVBITS(typer);
 
 	for (i = 0; i < GITS_BASER_NR_REGS; i++) {
 		u64 val = readq_relaxed(its->base + GITS_BASER + i * 8);
@@ -826,9 +828,6 @@ static int its_alloc_tables(struct its_node *its)
 		 * For other tables, only allocate a single page.
 		 */
 		if (type == GITS_BASER_TYPE_DEVICE) {
-			u64 typer = readq_relaxed(its->base + GITS_TYPER);
-			u32 ids = GITS_TYPER_DEVBITS(typer);
-
 			/*
 			 * 'order' was initialized earlier to the default page
 			 * granule of the the ITS.  We can't have an allocation
-- 
2.1.1

[PATCH v4 4/5] irqchip, gicv3-its: Add HW revision detection and configuration

[Robert Richter]

From: Robert Richter <rrichter@cavium.com>

Some GIC revisions require an individual configuration to esp. add
workarounds for HW bugs. This patch implements generic code to parse
the hw revision provided by an IIDR register value and runs specific
code if hw matches. There are functions that read the IIDR registers
for GICV3 and ITS (GICD_IIDR/GITS_IIDR) and then go through a list of
init functions to be called for specific versions.

A MIDR register value may also be used, this is especially useful for
hw detection from a guest.

The patch is needed to implement workarounds for HW errata in Cavium's
ThunderX GICV3.

v4:
 * only enable hw detection for its in its_enable_quirks()
 * removed gicv3_check_capabilities()

v3:
 * use arm64 errata framework for midr check

v2:
 * adding MIDR check

Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 drivers/irqchip/irq-gic-common.c | 11 +++++++++++
 drivers/irqchip/irq-gic-common.h |  9 +++++++++
 drivers/irqchip/irq-gic-v3-its.c | 15 +++++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index 9448e391cb71..ee789b07f2d1 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -21,6 +21,17 @@
 
 #include "irq-gic-common.h"
 
+void gic_check_capabilities(u32 iidr, const struct gic_capabilities *cap,
+			void *data)
+{
+	for (; cap->desc; cap++) {
+		if (cap->iidr != (cap->mask & iidr))
+			continue;
+		cap->init(data);
+		pr_info("%s\n", cap->desc);
+	}
+}
+
 int gic_configure_irq(unsigned int irq, unsigned int type,
 		       void __iomem *base, void (*sync_access)(void))
 {
diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
index 35a9884778bd..ca12635bbe3c 100644
--- a/drivers/irqchip/irq-gic-common.h
+++ b/drivers/irqchip/irq-gic-common.h
@@ -20,10 +20,19 @@
 #include <linux/of.h>
 #include <linux/irqdomain.h>
 
+struct gic_capabilities {
+	const char *desc;
+	void (*init)(void *data);
+	u32 iidr;
+	u32 mask;
+};
+
 int gic_configure_irq(unsigned int irq, unsigned int type,
                        void __iomem *base, void (*sync_access)(void));
 void gic_dist_config(void __iomem *base, int gic_irqs,
 		     void (*sync_access)(void));
 void gic_cpu_config(void __iomem *base, void (*sync_access)(void));
+void gic_check_capabilities(u32 iidr, const struct gic_capabilities *cap,
+			void *data);
 
 #endif /* _IRQ_GIC_COMMON_H */
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 06131db7a198..697421e834ee 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -36,6 +36,7 @@
 #include <asm/cputype.h>
 #include <asm/exception.h>
 
+#include "irq-gic-common.h"
 #include "irqchip.h"
 
 #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING		(1 << 0)
@@ -1390,6 +1391,18 @@ static int its_force_quiescent(void __iomem *base)
 	}
 }
 
+static const struct gic_capabilities its_errata[] = {
+	{
+	}
+};
+
+static void its_enable_quirks(struct its_node *its)
+{
+	u32 iidr = readl_relaxed(its->base + GITS_IIDR);
+
+	gic_check_capabilities(iidr, its_errata, its);
+}
+
 static int its_probe(struct device_node *node, struct irq_domain *parent)
 {
 	struct resource res;
@@ -1448,6 +1461,8 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
 	}
 	its->cmd_write = its->cmd_base;
 
+	its_enable_quirks(its);
+
 	err = its_alloc_tables(its);
 	if (err)
 		goto out_free_cmd;
-- 
2.1.1

[PATCH v4 5/5] irqchip, gicv3-its: Workaround for Cavium ThunderX errata 22375, 24313

[Robert Richter]

From: Robert Richter <rrichter@cavium.com>

This implements two gicv3-its errata workarounds for ThunderX. Both
with small impact affecting only ITS table allocation.

 erratum 22375: only alloc 8MB table size
 erratum 24313: ignore memory access type

The fixes are in ITS initialization and basically ignore memory access
type and table size provided by the TYPER and BASER registers.

v3:
 * fix erratum to be dependend from iidr

Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 697421e834ee..30459df2ee2c 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -39,7 +39,8 @@
 #include "irq-gic-common.h"
 #include "irqchip.h"
 
-#define ITS_FLAGS_CMDQ_NEEDS_FLUSHING		(1 << 0)
+#define ITS_FLAGS_CMDQ_NEEDS_FLUSHING		(1ULL << 0)
+#define ITS_FLAGS_CAVIUM_THUNDERX		(1ULL << 1)
 
 #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
 
@@ -803,9 +804,22 @@ static int its_alloc_tables(struct its_node *its)
 	int i;
 	int psz = SZ_64K;
 	u64 shr = GITS_BASER_InnerShareable;
-	u64 cache = GITS_BASER_WaWb;
-	u64 typer = readq_relaxed(its->base + GITS_TYPER);
-	u32 ids = GITS_TYPER_DEVBITS(typer);
+	u64 cache;
+	u64 typer;
+	u32 ids;
+
+	if (its->flags & ITS_FLAGS_CAVIUM_THUNDERX) {
+		/*
+		 * erratum 22375: only alloc 8MB table size
+		 * erratum 24313: ignore memory access type
+		 */
+		cache	= 0;
+		ids	= 0x13;			/* 20 bits, 8MB */
+	} else {
+		cache	= GITS_BASER_WaWb;
+		typer	= readq_relaxed(its->base + GITS_TYPER);
+		ids	= GITS_TYPER_DEVBITS(typer);
+	}
 
 	for (i = 0; i < GITS_BASER_NR_REGS; i++) {
 		u64 val = readq_relaxed(its->base + GITS_BASER + i * 8);
@@ -1391,8 +1405,21 @@ static int its_force_quiescent(void __iomem *base)
 	}
 }
 
+static void its_enable_cavium_thunderx(void *data)
+{
+	struct its_node *its = data;
+
+	its->flags |= ITS_FLAGS_CAVIUM_THUNDERX;
+}
+
 static const struct gic_capabilities its_errata[] = {
 	{
+		.desc	= "ITS: Cavium errata 22375, 24313",
+		.iidr	= 0xa100034c,	/* ThunderX pass 1.x */
+		.mask	= 0xffff0fff,
+		.init	= its_enable_cavium_thunderx,
+	},
+	{
 	}
 };
 
-- 
2.1.1

Re: [PATCH v4 2/5] irqchip, gicv3: Workaround for Cavium ThunderX erratum 23154

[Catalin Marinas]

On Fri, Aug 14, 2015 at 08:28:02PM +0200, Robert Richter wrote:
> +struct static_key is_cavium_thunderx = STATIC_KEY_INIT_FALSE;

This could also be "static struct ...". BTW, the use of static_key
directly is deprecated, so just do:

static DEFINE_STATIC_KEY_FALSE(is_cavium_thunderx);

-- 
Catalin

Re: [PATCH v4 2/5] irqchip, gicv3: Workaround for Cavium ThunderX erratum 23154

[David Daney]

On 08/14/2015 11:28 AM, Robert Richter wrote:
> From: Robert Richter <rrichter@cavium.com>
>
> This patch implements Cavium ThunderX erratum 23154.
>
> The gicv3 of ThunderX requires a modified version for reading the IAR
> status to ensure data synchronization. Since this is in the fast-path
> and called with each interrupt, runtime patching is used using jump
> label patching for smallest overhead (no-op). This is the same
> technique as used for tracepoints.
>
> v4:
>   * simplify code to only use cpus_have_cap() in gicv3_enable_quirks()
>
> v3:
>   * fix erratum to be dependend from midr
>   * use arm64 errata framework
>
> v2:
>   * implement code in a single asm() to keep instruction sequence
>   * added comment to the code that explains the erratum
>   * apply workaround also if running as guest, thus check MIDR
>
> Signed-off-by: Robert Richter <rrichter@cavium.com>
> ---
>   arch/arm64/Kconfig                  | 11 ++++++++++
>   arch/arm64/include/asm/cpufeature.h |  3 ++-
>   arch/arm64/include/asm/cputype.h    | 18 +++++++++-------
>   arch/arm64/kernel/cpu_errata.c      |  9 ++++++++
>   drivers/irqchip/irq-gic-v3.c        | 42 ++++++++++++++++++++++++++++++++++++-
>   5 files changed, 74 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 0f6edb14b7e4..4f866a4c6536 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -417,6 +417,17 @@ config ARM64_ERRATUM_845719
>
>   	  If unsure, say Y.
>
> +config CAVIUM_ERRATUM_23154
> +	bool "Cavium erratum 23154: Access to ICC_IAR1_EL1 is not sync'ed"
> +	depends on ARCH_THUNDER

None of the other errata depend on a specific ARCH_*.  I think we should 
remove this 'depends on', so that a generic kernel can be configured to 
work on Thunder without having to first select ARCH_THUNDER.

David Daney


> +	default y
> +	help
> +	  The gicv3 of ThunderX requires a modified version for
> +	  reading the IAR status to ensure data synchronization
> +	  (access to icc_iar1_el1 is not sync'ed before and after).
> +
> +	  If unsure, say Y.
> +
>   endmenu
>
>
>

Re: [PATCH v4 2/5] irqchip, gicv3: Workaround for Cavium ThunderX erratum 23154

[Robert Richter]

On 17.08.15 17:40:03, Catalin Marinas wrote:
> On Fri, Aug 14, 2015 at 08:28:02PM +0200, Robert Richter wrote:
> > +struct static_key is_cavium_thunderx = STATIC_KEY_INIT_FALSE;

Will add the static ...

> This could also be "static struct ...". BTW, the use of static_key
> directly is deprecated, so just do:
> 
> static DEFINE_STATIC_KEY_FALSE(is_cavium_thunderx);

... and for simplicity a patch with this after the jump laber bits are
merged upstream.

-Robert

Re: [PATCH v4 2/5] irqchip, gicv3: Workaround for Cavium ThunderX erratum 23154

[Robert Richter]

On 17.08.15 10:00:53, David Daney wrote:
> On 08/14/2015 11:28 AM, Robert Richter wrote:
> >+config CAVIUM_ERRATUM_23154
> >+	bool "Cavium erratum 23154: Access to ICC_IAR1_EL1 is not sync'ed"
> >+	depends on ARCH_THUNDER
> 
> None of the other errata depend on a specific ARCH_*.  I think we should
> remove this 'depends on', so that a generic kernel can be configured to work
> on Thunder without having to first select ARCH_THUNDER.

Right, will remove the dependency. Same as for the other errata then.

Thanks,

-Robert

> >+	default y
> >+	help
> >+	  The gicv3 of ThunderX requires a modified version for
> >+	  reading the IAR status to ensure data synchronization
> >+	  (access to icc_iar1_el1 is not sync'ed before and after).
> >+
> >+	  If unsure, say Y.

Re: [PATCH v4 4/5] irqchip, gicv3-its: Add HW revision detection and configuration

[Marc Zyngier]

Hi Robert,

On 14/08/15 19:28, Robert Richter wrote:
> From: Robert Richter <rrichter@cavium.com>
> 
> Some GIC revisions require an individual configuration to esp. add
> workarounds for HW bugs. This patch implements generic code to parse
> the hw revision provided by an IIDR register value and runs specific
> code if hw matches. There are functions that read the IIDR registers
> for GICV3 and ITS (GICD_IIDR/GITS_IIDR) and then go through a list of
> init functions to be called for specific versions.
> 
> A MIDR register value may also be used, this is especially useful for
> hw detection from a guest.

I don't think this sentence is relevant anymore.

> 
> The patch is needed to implement workarounds for HW errata in Cavium's
> ThunderX GICV3.
> 
> v4:
>  * only enable hw detection for its in its_enable_quirks()
>  * removed gicv3_check_capabilities()
> 
> v3:
>  * use arm64 errata framework for midr check
> 
> v2:
>  * adding MIDR check
> 
> Signed-off-by: Robert Richter <rrichter@cavium.com>
> ---
>  drivers/irqchip/irq-gic-common.c | 11 +++++++++++
>  drivers/irqchip/irq-gic-common.h |  9 +++++++++
>  drivers/irqchip/irq-gic-v3-its.c | 15 +++++++++++++++
>  3 files changed, 35 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
> index 9448e391cb71..ee789b07f2d1 100644
> --- a/drivers/irqchip/irq-gic-common.c
> +++ b/drivers/irqchip/irq-gic-common.c
> @@ -21,6 +21,17 @@
>  
>  #include "irq-gic-common.h"
>  
> +void gic_check_capabilities(u32 iidr, const struct gic_capabilities *cap,
> +			void *data)

Let's call a duck a duck, and replace all occurrences of
capabilit{y,ies} with "quirk".

> +{
> +	for (; cap->desc; cap++) {
> +		if (cap->iidr != (cap->mask & iidr))
> +			continue;
> +		cap->init(data);
> +		pr_info("%s\n", cap->desc);
> +	}
> +}
> +
>  int gic_configure_irq(unsigned int irq, unsigned int type,
>  		       void __iomem *base, void (*sync_access)(void))
>  {
> diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
> index 35a9884778bd..ca12635bbe3c 100644
> --- a/drivers/irqchip/irq-gic-common.h
> +++ b/drivers/irqchip/irq-gic-common.h
> @@ -20,10 +20,19 @@
>  #include <linux/of.h>
>  #include <linux/irqdomain.h>
>  
> +struct gic_capabilities {
> +	const char *desc;
> +	void (*init)(void *data);
> +	u32 iidr;
> +	u32 mask;
> +};
> +
>  int gic_configure_irq(unsigned int irq, unsigned int type,
>                         void __iomem *base, void (*sync_access)(void));
>  void gic_dist_config(void __iomem *base, int gic_irqs,
>  		     void (*sync_access)(void));
>  void gic_cpu_config(void __iomem *base, void (*sync_access)(void));
> +void gic_check_capabilities(u32 iidr, const struct gic_capabilities *cap,
> +			void *data);
>  
>  #endif /* _IRQ_GIC_COMMON_H */
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 06131db7a198..697421e834ee 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -36,6 +36,7 @@
>  #include <asm/cputype.h>
>  #include <asm/exception.h>
>  
> +#include "irq-gic-common.h"
>  #include "irqchip.h"
>  
>  #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING		(1 << 0)
> @@ -1390,6 +1391,18 @@ static int its_force_quiescent(void __iomem *base)
>  	}
>  }
>  
> +static const struct gic_capabilities its_errata[] = {
> +	{
> +	}
> +};
> +
> +static void its_enable_quirks(struct its_node *its)
> +{
> +	u32 iidr = readl_relaxed(its->base + GITS_IIDR);
> +
> +	gic_check_capabilities(iidr, its_errata, its);
> +}
> +
>  static int its_probe(struct device_node *node, struct irq_domain *parent)
>  {
>  	struct resource res;
> @@ -1448,6 +1461,8 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
>  	}
>  	its->cmd_write = its->cmd_base;
>  
> +	its_enable_quirks(its);
> +
>  	err = its_alloc_tables(its);
>  	if (err)
>  		goto out_free_cmd;
> 

Otherwise looks good to me.

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

Re: [PATCH v4 5/5] irqchip, gicv3-its: Workaround for Cavium ThunderX errata 22375, 24313

[Marc Zyngier]

On 14/08/15 19:28, Robert Richter wrote:
> From: Robert Richter <rrichter@cavium.com>
> 
> This implements two gicv3-its errata workarounds for ThunderX. Both
> with small impact affecting only ITS table allocation.
> 
>  erratum 22375: only alloc 8MB table size
>  erratum 24313: ignore memory access type
> 
> The fixes are in ITS initialization and basically ignore memory access
> type and table size provided by the TYPER and BASER registers.
> 
> v3:
>  * fix erratum to be dependend from iidr
> 
> Signed-off-by: Robert Richter <rrichter@cavium.com>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 35 +++++++++++++++++++++++++++++++----
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 697421e834ee..30459df2ee2c 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -39,7 +39,8 @@
>  #include "irq-gic-common.h"
>  #include "irqchip.h"
>  
> -#define ITS_FLAGS_CMDQ_NEEDS_FLUSHING		(1 << 0)
> +#define ITS_FLAGS_CMDQ_NEEDS_FLUSHING		(1ULL << 0)
> +#define ITS_FLAGS_CAVIUM_THUNDERX		(1ULL << 1)

I think you might need something slightly more explicit, as I'd expect
some ulterior revision of ThunderX to be eventually fixed...
ITS_FLAGS_THUNDERX_BOGUS_TYPER? Or something based on the errata numbers?

>  
>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
>  
> @@ -803,9 +804,22 @@ static int its_alloc_tables(struct its_node *its)
>  	int i;
>  	int psz = SZ_64K;
>  	u64 shr = GITS_BASER_InnerShareable;
> -	u64 cache = GITS_BASER_WaWb;
> -	u64 typer = readq_relaxed(its->base + GITS_TYPER);
> -	u32 ids = GITS_TYPER_DEVBITS(typer);
> +	u64 cache;
> +	u64 typer;
> +	u32 ids;
> +
> +	if (its->flags & ITS_FLAGS_CAVIUM_THUNDERX) {
> +		/*
> +		 * erratum 22375: only alloc 8MB table size
> +		 * erratum 24313: ignore memory access type
> +		 */
> +		cache	= 0;
> +		ids	= 0x13;			/* 20 bits, 8MB */
> +	} else {

You can move the typer definition here, as it is only used here.

> +		cache	= GITS_BASER_WaWb;
> +		typer	= readq_relaxed(its->base + GITS_TYPER);
> +		ids	= GITS_TYPER_DEVBITS(typer);
> +	}
>  
>  	for (i = 0; i < GITS_BASER_NR_REGS; i++) {
>  		u64 val = readq_relaxed(its->base + GITS_BASER + i * 8);
> @@ -1391,8 +1405,21 @@ static int its_force_quiescent(void __iomem *base)
>  	}
>  }
>  
> +static void its_enable_cavium_thunderx(void *data)
> +{
> +	struct its_node *its = data;
> +
> +	its->flags |= ITS_FLAGS_CAVIUM_THUNDERX;
> +}
> +
>  static const struct gic_capabilities its_errata[] = {
>  	{
> +		.desc	= "ITS: Cavium errata 22375, 24313",
> +		.iidr	= 0xa100034c,	/* ThunderX pass 1.x */
> +		.mask	= 0xffff0fff,
> +		.init	= its_enable_cavium_thunderx,
> +	},
> +	{
>  	}
>  };
>  
> 

Otherwise looks OK to me.

Thanks,

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

Re: [PATCH v4 0/5] irqchip, gicv3: Updates and Cavium ThunderX errata workarounds

[Marc Zyngier]

Hi Robert,

On 14/08/15 19:28, Robert Richter wrote:
> From: Robert Richter <rrichter@cavium.com>
> 
> This patch series adds gicv3 updates and workarounds for HW errata in
> Cavium's ThunderX GICV3.
> 
> The first one is an unchanged resubmission of a patch from a gicv3
> series I sent a while ago.
> 
> The next patches implement the workarounds for ThunderX's gicv3. Patch
> #2 implements the cpu workaround for gicv3 on ThunderX. Patch #3 is a
> prerequisit for patch #5. Patch #4 adds generic code to parse the hw
> revision provided by an IIDR. This patch is used for the implementa-
> tion of the actual gicv3-its workaround in #5.
> 
> All current review comments addressed so far with v4.

There has been a small number of comments on this series. Would you mind
respining it so that it could make it a a 4.3-rc?

Thanks,

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

Re: [PATCH v4 2/5] irqchip, gicv3: Workaround for Cavium ThunderX erratum 23154

[Suzuki K. Poulose]

On 14/08/15 19:28, Robert Richter wrote:
> From: Robert Richter <rrichter@cavium.com>
>
> This patch implements Cavium ThunderX erratum 23154.
>
> The gicv3 of ThunderX requires a modified version for reading the IAR
> status to ensure data synchronization. Since this is in the fast-path
> and called with each interrupt, runtime patching is used using jump
> label patching for smallest overhead (no-op). This is the same
> technique as used for tracepoints.
>
> v4:
>   * simplify code to only use cpus_have_cap() in gicv3_enable_quirks()
>
> v3:
>   * fix erratum to be dependend from midr
>   * use arm64 errata framework
>
> v2:
>   * implement code in a single asm() to keep instruction sequence
>   * added comment to the code that explains the erratum
>   * apply workaround also if running as guest, thus check MIDR
>
> Signed-off-by: Robert Richter <rrichter@cavium.com>
> ---
>   arch/arm64/Kconfig                  | 11 ++++++++++
>   arch/arm64/include/asm/cpufeature.h |  3 ++-
>   arch/arm64/include/asm/cputype.h    | 18 +++++++++-------
>   arch/arm64/kernel/cpu_errata.c      |  9 ++++++++
>   drivers/irqchip/irq-gic-v3.c        | 42 ++++++++++++++++++++++++++++++++++++-
>   5 files changed, 74 insertions(+), 9 deletions(-)
>

...

>   };
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index c52f7ba205b4..4211c39b8744 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -107,7 +107,7 @@ static void gic_redist_wait_for_rwp(void)

...

> +}
> +
>   static void __maybe_unused gic_write_pmr(u64 val)
>   {
>   	asm volatile("msr_s " __stringify(ICC_PMR_EL1) ", %0" : : "r" (val));
> @@ -766,6 +798,12 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
>   	.free = gic_irq_domain_free,
>   };
>
> +static void gicv3_enable_quirks(void)
> +{
> +	if (cpus_have_cap(ARM64_WORKAROUND_CAVIUM_23154))
> +		static_key_slow_inc(&is_cavium_thunderx);

May be you could use the enable() method added to struct arm64_cpu_capability
here to perform the above operation, added by James :

commit 1c0763037f1e1caef739e36e09c6d41ed7b61b2d
Author: James Morse <james.morse@arm.com>
Date:   Tue Jul 21 13:23:28 2015 +0100

     arm64: kernel: Add cpufeature 'enable' callback


> +}
> +
>   static int __init gic_of_init(struct device_node *node, struct device_node *parent)
>   {
>   	void __iomem *dist_base;
> @@ -825,6 +863,8 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
>   	gic_data.nr_redist_regions = nr_redist_regions;
>   	gic_data.redist_stride = redist_stride;
>
> +	gicv3_enable_quirks();
> +

than adding a hook here ?

Cheers
Suzuki

Re: [PATCH v4 2/5] irqchip, gicv3: Workaround for Cavium ThunderX erratum 23154

[Marc Zyngier]

On 07/09/15 17:54, Suzuki K. Poulose wrote:
> On 14/08/15 19:28, Robert Richter wrote:
>> From: Robert Richter <rrichter@cavium.com>
>>
>> This patch implements Cavium ThunderX erratum 23154.
>>
>> The gicv3 of ThunderX requires a modified version for reading the IAR
>> status to ensure data synchronization. Since this is in the fast-path
>> and called with each interrupt, runtime patching is used using jump
>> label patching for smallest overhead (no-op). This is the same
>> technique as used for tracepoints.
>>
>> v4:
>>   * simplify code to only use cpus_have_cap() in gicv3_enable_quirks()
>>
>> v3:
>>   * fix erratum to be dependend from midr
>>   * use arm64 errata framework
>>
>> v2:
>>   * implement code in a single asm() to keep instruction sequence
>>   * added comment to the code that explains the erratum
>>   * apply workaround also if running as guest, thus check MIDR
>>
>> Signed-off-by: Robert Richter <rrichter@cavium.com>
>> ---
>>   arch/arm64/Kconfig                  | 11 ++++++++++
>>   arch/arm64/include/asm/cpufeature.h |  3 ++-
>>   arch/arm64/include/asm/cputype.h    | 18 +++++++++-------
>>   arch/arm64/kernel/cpu_errata.c      |  9 ++++++++
>>   drivers/irqchip/irq-gic-v3.c        | 42 ++++++++++++++++++++++++++++++++++++-
>>   5 files changed, 74 insertions(+), 9 deletions(-)
>>
> 
> ...
> 
>>   };
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index c52f7ba205b4..4211c39b8744 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -107,7 +107,7 @@ static void gic_redist_wait_for_rwp(void)
> 
> ...
> 
>> +}
>> +
>>   static void __maybe_unused gic_write_pmr(u64 val)
>>   {
>>   	asm volatile("msr_s " __stringify(ICC_PMR_EL1) ", %0" : : "r" (val));
>> @@ -766,6 +798,12 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
>>   	.free = gic_irq_domain_free,
>>   };
>>
>> +static void gicv3_enable_quirks(void)
>> +{
>> +	if (cpus_have_cap(ARM64_WORKAROUND_CAVIUM_23154))
>> +		static_key_slow_inc(&is_cavium_thunderx);
> 
> May be you could use the enable() method added to struct arm64_cpu_capability
> here to perform the above operation, added by James :
> 
> commit 1c0763037f1e1caef739e36e09c6d41ed7b61b2d
> Author: James Morse <james.morse@arm.com>
> Date:   Tue Jul 21 13:23:28 2015 +0100
> 
>      arm64: kernel: Add cpufeature 'enable' callback
> 
> 
>> +}
>> +
>>   static int __init gic_of_init(struct device_node *node, struct device_node *parent)
>>   {
>>   	void __iomem *dist_base;
>> @@ -825,6 +863,8 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
>>   	gic_data.nr_redist_regions = nr_redist_regions;
>>   	gic_data.redist_stride = redist_stride;
>>
>> +	gicv3_enable_quirks();
>> +
> 
> than adding a hook here ?

It feels like a good idea, except that it creates a weird dependency
between the core arch code and the GIC driver. Can you think of an
elegant way to deal with this?

Thanks,

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

Re: [PATCH v4 2/5] irqchip, gicv3: Workaround for Cavium ThunderX erratum 23154

[Catalin Marinas]

On Mon, Sep 07, 2015 at 05:54:06PM +0100, Suzuki K. Poulose wrote:
> On 14/08/15 19:28, Robert Richter wrote:
> >diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> >index c52f7ba205b4..4211c39b8744 100644
> >--- a/drivers/irqchip/irq-gic-v3.c
> >+++ b/drivers/irqchip/irq-gic-v3.c
> >@@ -107,7 +107,7 @@ static void gic_redist_wait_for_rwp(void)
> 
> ...
> 
> >+}
> >+
> >  static void __maybe_unused gic_write_pmr(u64 val)
> >  {
> >  	asm volatile("msr_s " __stringify(ICC_PMR_EL1) ", %0" : : "r" (val));
> >@@ -766,6 +798,12 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
> >  	.free = gic_irq_domain_free,
> >  };
> >
> >+static void gicv3_enable_quirks(void)
> >+{
> >+	if (cpus_have_cap(ARM64_WORKAROUND_CAVIUM_23154))
> >+		static_key_slow_inc(&is_cavium_thunderx);
> 
> May be you could use the enable() method added to struct arm64_cpu_capability
> here to perform the above operation, added by James :
> 
> commit 1c0763037f1e1caef739e36e09c6d41ed7b61b2d
> Author: James Morse <james.morse@arm.com>
> Date:   Tue Jul 21 13:23:28 2015 +0100
> 
>     arm64: kernel: Add cpufeature 'enable' callback

I thought about this as well when looking at the patch but decided it's
better as it is. The "enable" method is meant to enable per-CPU features
(or workarounds) but here it is about GICv3, so we don't want to enable
for every CPU.

-- 
Catalin

Re: [PATCH v4 2/5] irqchip, gicv3: Workaround for Cavium ThunderX erratum 23154

[Robert Richter]

On 07.09.15 18:09:48, Marc Zyngier wrote:
> On 07/09/15 17:54, Suzuki K. Poulose wrote:
> > On 14/08/15 19:28, Robert Richter wrote:
> >> From: Robert Richter <rrichter@cavium.com>

> >> +static void gicv3_enable_quirks(void)
> >> +{
> >> +	if (cpus_have_cap(ARM64_WORKAROUND_CAVIUM_23154))
> >> +		static_key_slow_inc(&is_cavium_thunderx);
> > 
> > May be you could use the enable() method added to struct arm64_cpu_capability
> > here to perform the above operation, added by James :
> > 
> > commit 1c0763037f1e1caef739e36e09c6d41ed7b61b2d
> > Author: James Morse <james.morse@arm.com>
> > Date:   Tue Jul 21 13:23:28 2015 +0100
> > 
> >      arm64: kernel: Add cpufeature 'enable' callback
> > 
> > 
> >> +}
> >> +
> >>   static int __init gic_of_init(struct device_node *node, struct device_node *parent)
> >>   {
> >>   	void __iomem *dist_base;
> >> @@ -825,6 +863,8 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
> >>   	gic_data.nr_redist_regions = nr_redist_regions;
> >>   	gic_data.redist_stride = redist_stride;
> >>
> >> +	gicv3_enable_quirks();
> >> +
> > 
> > than adding a hook here ?
> 
> It feels like a good idea, except that it creates a weird dependency
> between the core arch code and the GIC driver. Can you think of an
> elegant way to deal with this?

The only chance I see is to move it all to the driver and adding
enable() calls to the caps in gicv3_errata:

static void gicv3_enable_quirks(void)
{
        check_cpu_capabilities(gicv3_errata, "enabling workaround for");
}

Here the code is kept in the driver and called during driver init.

But current solution looks more elegant and simpler to me. So I would
not change it.

-Robert

Re: [PATCH v4 2/5] irqchip, gicv3: Workaround for Cavium ThunderX erratum 23154

[Suzuki K. Poulose]

On 07/09/15 18:15, Catalin Marinas wrote:
> On Mon, Sep 07, 2015 at 05:54:06PM +0100, Suzuki K. Poulose wrote:
>> On 14/08/15 19:28, Robert Richter wrote:
>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>> index c52f7ba205b4..4211c39b8744 100644
>>> --- a/drivers/irqchip/irq-gic-v3.c
>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>> @@ -107,7 +107,7 @@ static void gic_redist_wait_for_rwp(void)
>>
>> ...
>>
>>> +}
>>> +
>>>   static void __maybe_unused gic_write_pmr(u64 val)
>>>   {
>>>   	asm volatile("msr_s " __stringify(ICC_PMR_EL1) ", %0" : : "r" (val));
>>> @@ -766,6 +798,12 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
>>>   	.free = gic_irq_domain_free,
>>>   };
>>>
>>> +static void gicv3_enable_quirks(void)
>>> +{
>>> +	if (cpus_have_cap(ARM64_WORKAROUND_CAVIUM_23154))
>>> +		static_key_slow_inc(&is_cavium_thunderx);
>>
>> May be you could use the enable() method added to struct arm64_cpu_capability
>> here to perform the above operation, added by James :
>>
>> commit 1c0763037f1e1caef739e36e09c6d41ed7b61b2d
>> Author: James Morse <james.morse@arm.com>
>> Date:   Tue Jul 21 13:23:28 2015 +0100
>>
>>      arm64: kernel: Add cpufeature 'enable' callback
>
> I thought about this as well when looking at the patch but decided it's
> better as it is. The "enable" method is meant to enable per-CPU features
> (or workarounds) but here it is about GICv3, so we don't want to enable
> for every CPU.

Right. I have been playing with a series where the checks are delayed until
all CPUs are brought up. But yes, I understand this usecase is slightly different
and may not match what I was thinking about.

May be, gic can have its own private list of _cpu_capability which it can run
over check_cpu_capabilities(), which it can run over and that will fall back to
what we have at the moment. So, may be what we have here is as good as we can
get.

Cheers
Suzuki

Re: [PATCH v4 2/5] irqchip, gicv3: Workaround for Cavium ThunderX erratum 23154

[Catalin Marinas]

On Mon, Sep 07, 2015 at 06:41:50PM +0100, Suzuki K. Poulose wrote:
> On 07/09/15 18:15, Catalin Marinas wrote:
> >On Mon, Sep 07, 2015 at 05:54:06PM +0100, Suzuki K. Poulose wrote:
> >>On 14/08/15 19:28, Robert Richter wrote:
> >>>+static void gicv3_enable_quirks(void)
> >>>+{
> >>>+	if (cpus_have_cap(ARM64_WORKAROUND_CAVIUM_23154))
> >>>+		static_key_slow_inc(&is_cavium_thunderx);
> >>
> >>May be you could use the enable() method added to struct arm64_cpu_capability
> >>here to perform the above operation, added by James :
> >>
> >>commit 1c0763037f1e1caef739e36e09c6d41ed7b61b2d
> >>Author: James Morse <james.morse@arm.com>
> >>Date:   Tue Jul 21 13:23:28 2015 +0100
> >>
> >>     arm64: kernel: Add cpufeature 'enable' callback
> >
> >I thought about this as well when looking at the patch but decided it's
> >better as it is. The "enable" method is meant to enable per-CPU features
> >(or workarounds) but here it is about GICv3, so we don't want to enable
> >for every CPU.
> 
> Right. I have been playing with a series where the checks are delayed until
> all CPUs are brought up.

Unrelated to the GIC workaround, delaying the enable feature until the
CPUs are brought up is not always be feasible. At some point we may
implement support to defer the CPU on to user space (I already have a
patch that does this when no DT enable-method is specified, but I won't
publish it before Qualcomm fixes its firmware ;)). But we may have other
reasons to start with CPUs hot-unplugged by default and turn them on
later.

-- 
Catalin

Re: [PATCH v4 2/5] irqchip, gicv3: Workaround for Cavium ThunderX erratum 23154

[Suzuki K. Poulose]

On 08/09/15 10:00, Catalin Marinas wrote:
> On Mon, Sep 07, 2015 at 06:41:50PM +0100, Suzuki K. Poulose wrote:
>> On 07/09/15 18:15, Catalin Marinas wrote:
>>> On Mon, Sep 07, 2015 at 05:54:06PM +0100, Suzuki K. Poulose wrote:
>>>> On 14/08/15 19:28, Robert Richter wrote:
>>>>> +static void gicv3_enable_quirks(void)
>>>>> +{
>>>>> +	if (cpus_have_cap(ARM64_WORKAROUND_CAVIUM_23154))
>>>>> +		static_key_slow_inc(&is_cavium_thunderx);
>>>>
>>>> May be you could use the enable() method added to struct arm64_cpu_capability
>>>> here to perform the above operation, added by James :
>>>>
>>>> commit 1c0763037f1e1caef739e36e09c6d41ed7b61b2d
>>>> Author: James Morse <james.morse@arm.com>
>>>> Date:   Tue Jul 21 13:23:28 2015 +0100
>>>>
>>>>      arm64: kernel: Add cpufeature 'enable' callback
>>>
>>> I thought about this as well when looking at the patch but decided it's
>>> better as it is. The "enable" method is meant to enable per-CPU features
>>> (or workarounds) but here it is about GICv3, so we don't want to enable
>>> for every CPU.
>>
>> Right. I have been playing with a series where the checks are delayed until
>> all CPUs are brought up.
>
> Unrelated to the GIC workaround, delaying the enable feature until the
> CPUs are brought up is not always be feasible.

Right. But then, enabling a feature(and applying the alternatives) based on
a single CPU may not be safe, always, like PAN. If one of the boot time CPU
doesn't have it, then we are in trouble (even though we WARN about it from
SANITY check)

> At some point we may
> implement support to defer the CPU on to user space (I already have a
> patch that does this when no DT enable-method is specified, but I won't
> publish it before Qualcomm fixes its firmware ;)). But we may have other
> reasons to start with CPUs hot-unplugged by default and turn them on
> later.

We have SANITY check infrastructure that WARNs in such cases, if the features
don't match.  But still, wouldn't it be better to enable a feature
only if all the boot-time enabled CPUs have it ? (Errata is an exception though,
which only depends on whether one of the CPU needs it).

Thanks
Suzuki

>

Re: [PATCH v4 2/5] irqchip, gicv3: Workaround for Cavium ThunderX erratum 23154

[Catalin Marinas]

On Tue, Sep 08, 2015 at 10:09:30AM +0100, Suzuki K. Poulose wrote:
> On 08/09/15 10:00, Catalin Marinas wrote:
> >On Mon, Sep 07, 2015 at 06:41:50PM +0100, Suzuki K. Poulose wrote:
> >>On 07/09/15 18:15, Catalin Marinas wrote:
> >>>On Mon, Sep 07, 2015 at 05:54:06PM +0100, Suzuki K. Poulose wrote:
> >>>>On 14/08/15 19:28, Robert Richter wrote:
> >>>>>+static void gicv3_enable_quirks(void)
> >>>>>+{
> >>>>>+	if (cpus_have_cap(ARM64_WORKAROUND_CAVIUM_23154))
> >>>>>+		static_key_slow_inc(&is_cavium_thunderx);
> >>>>
> >>>>May be you could use the enable() method added to struct arm64_cpu_capability
> >>>>here to perform the above operation, added by James :
> >>>>
> >>>>commit 1c0763037f1e1caef739e36e09c6d41ed7b61b2d
> >>>>Author: James Morse <james.morse@arm.com>
> >>>>Date:   Tue Jul 21 13:23:28 2015 +0100
> >>>>
> >>>>     arm64: kernel: Add cpufeature 'enable' callback
> >>>
> >>>I thought about this as well when looking at the patch but decided it's
> >>>better as it is. The "enable" method is meant to enable per-CPU features
> >>>(or workarounds) but here it is about GICv3, so we don't want to enable
> >>>for every CPU.
> >>
> >>Right. I have been playing with a series where the checks are delayed until
> >>all CPUs are brought up.
> >
> >Unrelated to the GIC workaround, delaying the enable feature until the
> >CPUs are brought up is not always be feasible.
> 
> Right. But then, enabling a feature(and applying the alternatives) based on
> a single CPU may not be safe, always, like PAN. If one of the boot time CPU
> doesn't have it, then we are in trouble (even though we WARN about it from
> SANITY check)

I see your point but there's a trade-off. For some features it's not be
feasible to postpone until user space (e.g. errata workarounds). But if
a CPU coming up late doesn't have compatible features, just keep it in a
loop (or park it back if possible or even refuse to boot any further). I
don't think we should cater for insane hardware configurations (e.g. mix
of PAN/no-PAN as we already do the code patching). Do you plan to defer
code patching as well?

Note that we may have to use the .enable function for errata workarounds
as well, not just features like PAN (we currently only do code patching
but we may have to do other things like issuing SMC calls, you never
know what's going to hit us).

> >At some point we may
> >implement support to defer the CPU on to user space (I already have a
> >patch that does this when no DT enable-method is specified, but I won't
> >publish it before Qualcomm fixes its firmware ;)). But we may have other
> >reasons to start with CPUs hot-unplugged by default and turn them on
> >later.
> 
> We have SANITY check infrastructure that WARNs in such cases, if the features
> don't match.  But still, wouldn't it be better to enable a feature
> only if all the boot-time enabled CPUs have it ? (Errata is an exception though,
> which only depends on whether one of the CPU needs it).

If we ever need this, I think we should implement a separate late_enable
function as just deferring all features enabling is not generic enough.
But in the meantime, I don't think we should worry about this case,
let's wait and see whether we ever get such configurations (panicking
the kernel on incompatible features is a good starting point -
FPSIMD/no-FPSIMD, PAN/no-PAN etc.)

-- 
Catalin

Re: [PATCH v4 2/5] irqchip, gicv3: Workaround for Cavium ThunderX erratum 23154

[Suzuki K. Poulose]

On 08/09/15 10:37, Catalin Marinas wrote:
> On Tue, Sep 08, 2015 at 10:09:30AM +0100, Suzuki K. Poulose wrote:
>> On 08/09/15 10:00, Catalin Marinas wrote:
>>> On Mon, Sep 07, 2015 at 06:41:50PM +0100, Suzuki K. Poulose wrote:
>>>> On 07/09/15 18:15, Catalin Marinas wrote:
>>>>> On Mon, Sep 07, 2015 at 05:54:06PM +0100, Suzuki K. Poulose wrote:
>>>>>> On 14/08/15 19:28, Robert Richter wrote:
>>>>>>> +static void gicv3_enable_quirks(void)
>>>>>>> +{
>>>>>>> +	if (cpus_have_cap(ARM64_WORKAROUND_CAVIUM_23154))
>>>>>>> +		static_key_slow_inc(&is_cavium_thunderx);
>>>>>>
>>>>>> May be you could use the enable() method added to struct arm64_cpu_capability
>>>>>> here to perform the above operation, added by James :
>>>>>>
>>>>>> commit 1c0763037f1e1caef739e36e09c6d41ed7b61b2d
>>>>>> Author: James Morse <james.morse@arm.com>
>>>>>> Date:   Tue Jul 21 13:23:28 2015 +0100
>>>>>>
>>>>>>      arm64: kernel: Add cpufeature 'enable' callback
>>>>>
>>>>> I thought about this as well when looking at the patch but decided it's
>>>>> better as it is. The "enable" method is meant to enable per-CPU features
>>>>> (or workarounds) but here it is about GICv3, so we don't want to enable
>>>>> for every CPU.
>>>>
>>>> Right. I have been playing with a series where the checks are delayed until
>>>> all CPUs are brought up.
>>>
>>> Unrelated to the GIC workaround, delaying the enable feature until the
>>> CPUs are brought up is not always be feasible.
>>
>> Right. But then, enabling a feature(and applying the alternatives) based on
>> a single CPU may not be safe, always, like PAN. If one of the boot time CPU
>> doesn't have it, then we are in trouble (even though we WARN about it from
>> SANITY check)
>
> I see your point but there's a trade-off. For some features it's not be
> feasible to postpone until user space (e.g. errata workarounds). But if

Right, I agree. I should have been more descriptive. Here is my plan :

Classify the capabilities / workarounds as two different types.

1) Errata workaround capability checks are triggered for each booting
    CPU.
2) CPU Feature capabilities are checked until all boot-time enabled CPUs are
active, in smp_cpus_done() and before apply_alternatives_all().

(We could even classify some of the capabilities as CPU_LOCAL and check it
  per-CPU).

Delay the feature/capability detection to smp_cpus_done() and before
apply_alternatives_all().

i.e, :

  void __init smp_cpus_done(unsigned int max_cpus)
  {
         pr_info("SMP: Total of %d processors activated.\n", num_online_cpus());
+       setup_cpu_features();
         hyp_mode_check();
         apply_alternatives_all();
  }

Where setup_cpu_features() will do all the CPU feature related processing
based on the system wide safe value(will be available from the new infrastructure) :

1) cpu capability based on feature registers (e.g, GIC SYSREG, PAN, ATOMICS )
2) ELF_HWCAP


> a CPU coming up late doesn't have compatible features, just keep it in a
> loop (or park it back if possible or even refuse to boot any further). I
> don't think we should cater for insane hardware configurations (e.g. mix


Any other new CPU, which is missing an available system capability, could be
made to loop, as you mentioned.

> of PAN/no-PAN as we already do the code patching). Do you plan to defer
> code patching as well?

As shown above, the apply_alternatives_all() is already done from smp_cpus_done(),
which will stay there.

>
> Note that we may have to use the .enable function for errata workarounds
> as well, not just features like PAN (we currently only do code patching
> but we may have to do other things like issuing SMC calls, you never
> know what's going to hit us).

Given that ERRATAs are checked for each CPU and are not delayed, we need not
worry about. But yes, we could have flags to indicate how/when the enable methods
should be invoked ? e.g, per CPU (like PAN), or per SYSTEM (once for the entire system)

>>> At some point we may
>>> implement support to defer the CPU on to user space (I already have a
>>> patch that does this when no DT enable-method is specified, but I won't
>>> publish it before Qualcomm fixes its firmware ;)). But we may have other
>>> reasons to start with CPUs hot-unplugged by default and turn them on
>>> later.
>>
>> We have SANITY check infrastructure that WARNs in such cases, if the features
>> don't match.  But still, wouldn't it be better to enable a feature
>> only if all the boot-time enabled CPUs have it ? (Errata is an exception though,
>> which only depends on whether one of the CPU needs it).
>
> If we ever need this, I think we should implement a separate late_enable
> function as just deferring all features enabling is not generic enough.
> But in the meantime, I don't think we should worry about this case,
> let's wait and see whether we ever get such configurations (panicking
> the kernel on incompatible features is a good starting point -
> FPSIMD/no-FPSIMD, PAN/no-PAN etc.)

OK. I will post the series after the merge window. We can discuss further
then.

Cheers
Suzuki

Re: [PATCH v4 5/5] irqchip, gicv3-its: Workaround for Cavium ThunderX errata 22375, 24313

[Robert Richter]

On 07.09.15 17:32:25, Marc Zyngier wrote:
> On 14/08/15 19:28, Robert Richter wrote:

> > +	if (its->flags & ITS_FLAGS_CAVIUM_THUNDERX) {
> > +		/*
> > +		 * erratum 22375: only alloc 8MB table size
> > +		 * erratum 24313: ignore memory access type
> > +		 */
> > +		cache	= 0;
> > +		ids	= 0x13;			/* 20 bits, 8MB */
> > +	} else {
> 
> You can move the typer definition here, as it is only used here.
> 
> > +		cache	= GITS_BASER_WaWb;
> > +		typer	= readq_relaxed(its->base + GITS_TYPER);
> > +		ids	= GITS_TYPER_DEVBITS(typer);
> > +	}

I am dropping this change as this would raise merge conflicts with
others, e.g. KVM patches.

-Robert