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

[PATCH v2 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 gicv3 series I sent a
while ago.

The next patches implement the workarounds for ThunderX's gicv3. Patch
#2 adds generic code to parse the hw revision provided by an IIDR or
MIDR register value and runs specific code if hw matches. This is used
to implement the actual errata fixes in patch #3 (gicv3) and #5
(gicv3-its). Patch #4 prerequisit for patch #5.

All current review comments addressed so far with V2.

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):
  arm64: gicv3: its: Add range check for number of allocated pages
  irqchip, gicv3: Add HW revision detection and configuration
  irqchip, gicv3: Workaround for Cavium ThunderX erratum 23154
  irqchip, gicv3-its: Read typer register outside the loop
  irqchip, gicv3-its: Workaround for Cavium ThunderX errata 22375, 24313

 drivers/irqchip/irq-gic-common.c   | 13 ++++++++
 drivers/irqchip/irq-gic-common.h   | 11 +++++++
 drivers/irqchip/irq-gic-v3-its.c   | 66 ++++++++++++++++++++++++++++++++++----
 drivers/irqchip/irq-gic-v3.c       | 59 +++++++++++++++++++++++++++++++++-
 include/linux/irqchip/arm-gic-v3.h |  1 +
 5 files changed, 143 insertions(+), 7 deletions(-)

-- 
2.1.1

[PATCH v2 1/5] arm64: 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 v2 2/5] irqchip, gicv3: 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.

V2:
 * adding MIDR check

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

diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index 9448e391cb71..886c09e645bf 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -21,6 +21,19 @@
 
 #include "irq-gic-common.h"
 
+void gic_check_capabilities(u32 iidr, const struct gic_capabilities *cap,
+			void *data)
+{
+	for (; cap->desc; cap++) {
+		if (cap->midr != (cap->midr_mask & read_cpuid_id()))
+			continue;
+		if (cap->iidr != (cap->iidr_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..e9a3e2800005 100644
--- a/drivers/irqchip/irq-gic-common.h
+++ b/drivers/irqchip/irq-gic-common.h
@@ -20,10 +20,21 @@
 #include <linux/of.h>
 #include <linux/irqdomain.h>
 
+struct gic_capabilities {
+	const char *desc;
+	void (*init)(void *data);
+	u32 iidr;
+	u32 iidr_mask;
+	u32 midr;
+	u32 midr_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 466edf8a7477..105674037618 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)
@@ -1391,6 +1392,18 @@ static int its_force_quiescent(void __iomem *base)
 	}
 }
 
+static const struct gic_capabilities its_errata[] = {
+	{
+	}
+};
+
+static void its_check_capabilities(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;
@@ -1449,6 +1462,8 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
 	}
 	its->cmd_write = its->cmd_base;
 
+	its_check_capabilities(its);
+
 	err = its_alloc_tables(its);
 	if (err)
 		goto out_free_cmd;
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index c52f7ba205b4..1a91902be0b1 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -766,6 +766,18 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
 	.free = gic_irq_domain_free,
 };
 
+static const struct gic_capabilities gicv3_errata[] = {
+	{
+	}
+};
+
+static void gicv3_check_capabilities(void)
+{
+	u32 iidr = readl_relaxed(gic_data.dist_base + GICD_IIDR);
+
+	gic_check_capabilities(iidr, gicv3_errata, NULL);
+}
+
 static int __init gic_of_init(struct device_node *node, struct device_node *parent)
 {
 	void __iomem *dist_base;
@@ -825,6 +837,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_check_capabilities();
+
 	/*
 	 * Find out how many interrupts are supported.
 	 * The GIC only supports up to 1020 interrupt sources (SGI+PPI+SPI)
-- 
2.1.1

[PATCH v2 3/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.

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>
---
 drivers/irqchip/irq-gic-v3.c | 45 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 1a91902be0b1..2f80a11621a0 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,8 +798,19 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
 	.free = gic_irq_domain_free,
 };
 
+static void gicv3_enable_cavium_thunderx(void *data)
+{
+	static_key_slow_inc(&is_cavium_thunderx);
+}
+
 static const struct gic_capabilities gicv3_errata[] = {
 	{
+		.desc		= "GIC: Cavium erratum 23154",
+		.iidr		= 0xa100034c,	/* ThunderX pass 1.x */
+		.iidr_mask	= 0xffff0fff,
+		.init		= gicv3_enable_cavium_thunderx,
+	},
+	{
 	}
 };
 
-- 
2.1.1

[PATCH v2 4/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 105674037618..bf0659821683 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -804,6 +804,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);
@@ -827,9 +829,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 v2 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.

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

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index bf0659821683..d629e0e03322 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,25 @@ 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[] = {
 	{
+		/*
+		 * Need to be applied in guests too, thus use midr as
+		 * iidr is emulated with different vendor id.
+		 */
+		.desc		= "ITS: Cavium errata 22375, 24313",
+		.midr		= 0x43000a10,	/* ThunderX pass 1.x */
+		.midr_mask	= 0xfff0fff0,
+		.init		= its_enable_cavium_thunderx,
+	},
+	{
 	}
 };
 
-- 
2.1.1

Re: [PATCH v2 2/5] irqchip, gicv3: Add HW revision detection and configuration

[Marc Zyngier]

On 13/08/15 15:47, 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.
> 
> The patch is needed to implement workarounds for HW errata in Cavium's
> ThunderX GICV3.
> 
> V2:
>  * adding MIDR check
> 
> Signed-off-by: Robert Richter <rrichter@cavium.com>
> ---
>  drivers/irqchip/irq-gic-common.c | 13 +++++++++++++
>  drivers/irqchip/irq-gic-common.h | 11 +++++++++++
>  drivers/irqchip/irq-gic-v3-its.c | 15 +++++++++++++++
>  drivers/irqchip/irq-gic-v3.c     | 14 ++++++++++++++
>  4 files changed, 53 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
> index 9448e391cb71..886c09e645bf 100644
> --- a/drivers/irqchip/irq-gic-common.c
> +++ b/drivers/irqchip/irq-gic-common.c
> @@ -21,6 +21,19 @@
>  
>  #include "irq-gic-common.h"
>  
> +void gic_check_capabilities(u32 iidr, const struct gic_capabilities *cap,
> +			void *data)
> +{
> +	for (; cap->desc; cap++) {
> +		if (cap->midr != (cap->midr_mask & read_cpuid_id()))
> +			continue;
> +		if (cap->iidr != (cap->iidr_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..e9a3e2800005 100644
> --- a/drivers/irqchip/irq-gic-common.h
> +++ b/drivers/irqchip/irq-gic-common.h
> @@ -20,10 +20,21 @@
>  #include <linux/of.h>
>  #include <linux/irqdomain.h>
>  
> +struct gic_capabilities {
> +	const char *desc;
> +	void (*init)(void *data);
> +	u32 iidr;
> +	u32 iidr_mask;
> +	u32 midr;
> +	u32 midr_mask;
> +};

Sorry to ask the obvious, but why should we implement another MIDR check
that is private to the GIC driver, while we already have an
infrastructure that deals with that kind of things?

Also, the GIC CPU interface is very much part of the CPU, not part the
GIC itself. I'd rather see a CPU erratum being handler at the same
location as all the other errata.

Thanks,

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

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

[Marc Zyngier]

On 13/08/15 15:47, 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.
> 
> 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>
> ---
>  drivers/irqchip/irq-gic-v3.c | 45 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 1a91902be0b1..2f80a11621a0 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,8 +798,19 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
>  	.free = gic_irq_domain_free,
>  };
>  
> +static void gicv3_enable_cavium_thunderx(void *data)
> +{
> +	static_key_slow_inc(&is_cavium_thunderx);
> +}
> +
>  static const struct gic_capabilities gicv3_errata[] = {
>  	{
> +		.desc		= "GIC: Cavium erratum 23154",
> +		.iidr		= 0xa100034c,	/* ThunderX pass 1.x */
> +		.iidr_mask	= 0xffff0fff,
> +		.init		= gicv3_enable_cavium_thunderx,
> +	},

I'm even more puzzled. You're working around a CPU bug based on the ITS
ID registers? Or have you swapped the detection methods for the two errata?

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

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

[Robert Richter]

Marc,

thanks for your quick review.

On 13.08.15 16:11:15, Marc Zyngier wrote:
> On 13/08/15 15:47, Robert Richter wrote:
> > From: Robert Richter <rrichter@cavium.com>

> >  static const struct gic_capabilities gicv3_errata[] = {
> >  	{
> > +		.desc		= "GIC: Cavium erratum 23154",
> > +		.iidr		= 0xa100034c,	/* ThunderX pass 1.x */
> > +		.iidr_mask	= 0xffff0fff,
> > +		.init		= gicv3_enable_cavium_thunderx,
> > +	},
> 
> I'm even more puzzled. You're working around a CPU bug based on the ITS
> ID registers? Or have you swapped the detection methods for the two errata?

:/ Right, I mixed this up... Must have starred on this for too long.
Will fix that.

Wrt midr: Originally this was written to support iidr. I wanted to
keep the version check in the driver of the hw, an implementation
outside of drivers/irqchip looked not appropriate here as it would
rely then on arch arm64 only. This is the main reason. Apart from
that, I think an implmentation based on struct arm64_cpu_capabilities,
etc. would require much rework compared to my current easy
implementation, e.g:

 * binding flags to callbacks and actually run them,

 * handing over private driver data (base addr for iidr detection) to
   a capabilty's match function.

Overall this looked bloated. Now, that the MIDR also needs to be
checked, it looked better to me to keep the gic hw detection at a
single location in the driver. This also allows us to check a
combination of midr and iidr values.

I hope this sounds reasonable?

-Robert

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

[Marc Zyngier]

On 13/08/15 17:17, Robert Richter wrote:
> Marc,
> 
> thanks for your quick review.
> 
> On 13.08.15 16:11:15, Marc Zyngier wrote:
>> On 13/08/15 15:47, Robert Richter wrote:
>>> From: Robert Richter <rrichter@cavium.com>
> 
>>>  static const struct gic_capabilities gicv3_errata[] = {
>>>  	{
>>> +		.desc		= "GIC: Cavium erratum 23154",
>>> +		.iidr		= 0xa100034c,	/* ThunderX pass 1.x */
>>> +		.iidr_mask	= 0xffff0fff,
>>> +		.init		= gicv3_enable_cavium_thunderx,
>>> +	},
>>
>> I'm even more puzzled. You're working around a CPU bug based on the ITS
>> ID registers? Or have you swapped the detection methods for the two errata?
> 
> :/ Right, I mixed this up... Must have starred on this for too long.
> Will fix that.
> 
> Wrt midr: Originally this was written to support iidr. I wanted to
> keep the version check in the driver of the hw, an implementation
> outside of drivers/irqchip looked not appropriate here as it would
> rely then on arch arm64 only. This is the main reason. Apart from
> that, I think an implmentation based on struct arm64_cpu_capabilities,
> etc. would require much rework compared to my current easy
> implementation, e.g:
> 
>  * binding flags to callbacks and actually run them,
> 
>  * handing over private driver data (base addr for iidr detection) to
>    a capabilty's match function.
> 
> Overall this looked bloated. Now, that the MIDR also needs to be
> checked, it looked better to me to keep the gic hw detection at a
> single location in the driver. This also allows us to check a
> combination of midr and iidr values.
> 
> I hope this sounds reasonable?

+Will.

The point I was trying to make is that a CPU interface bug is a CPU bug,
and that it feels quite weird weird to have the detection in the GIC.
Will, what do you think?

Also, I don't really buy the combined MIDR/GITS_IIDR detection. These
are two *very* distinct pieces of HW that are not even directly
connected (the redistributors are in between).

I wouldn't mind having something like:

struct gic_capabilities {
	const char *desc;
	void (*init)(void *data);
	u32 iidr;
	u32 iidr_mask;
	int feature;
};

where "feature" is a one of things declared in cpufeature.h, and that
would condition the capability (I love the name!) if that really
happens. I don't think we're there yet.

As for the complexity of implementation, testing a flag in the probe
function and tingling a static key is not really a big deal.

Thanks,

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

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

[Robert Richter]

On 13.08.15 17:54:41, Marc Zyngier wrote:
> On 13/08/15 17:17, Robert Richter wrote:
> > Marc,
> > 
> > thanks for your quick review.
> > 
> > On 13.08.15 16:11:15, Marc Zyngier wrote:
> >> On 13/08/15 15:47, Robert Richter wrote:
> >>> From: Robert Richter <rrichter@cavium.com>
> > 
> >>>  static const struct gic_capabilities gicv3_errata[] = {
> >>>  	{
> >>> +		.desc		= "GIC: Cavium erratum 23154",
> >>> +		.iidr		= 0xa100034c,	/* ThunderX pass 1.x */
> >>> +		.iidr_mask	= 0xffff0fff,
> >>> +		.init		= gicv3_enable_cavium_thunderx,
> >>> +	},
> >>
> >> I'm even more puzzled. You're working around a CPU bug based on the ITS
> >> ID registers? Or have you swapped the detection methods for the two errata?
> > 
> > :/ Right, I mixed this up... Must have starred on this for too long.
> > Will fix that.
> > 
> > Wrt midr: Originally this was written to support iidr. I wanted to
> > keep the version check in the driver of the hw, an implementation
> > outside of drivers/irqchip looked not appropriate here as it would
> > rely then on arch arm64 only. This is the main reason. Apart from
> > that, I think an implmentation based on struct arm64_cpu_capabilities,
> > etc. would require much rework compared to my current easy
> > implementation, e.g:
> > 
> >  * binding flags to callbacks and actually run them,
> > 
> >  * handing over private driver data (base addr for iidr detection) to
> >    a capabilty's match function.
> > 
> > Overall this looked bloated. Now, that the MIDR also needs to be
> > checked, it looked better to me to keep the gic hw detection at a
> > single location in the driver. This also allows us to check a
> > combination of midr and iidr values.
> > 
> > I hope this sounds reasonable?
> 
> +Will.
> 
> The point I was trying to make is that a CPU interface bug is a CPU bug,
> and that it feels quite weird weird to have the detection in the GIC.
> Will, what do you think?
> 
> Also, I don't really buy the combined MIDR/GITS_IIDR detection. These
> are two *very* distinct pieces of HW that are not even directly
> connected (the redistributors are in between).
> 
> I wouldn't mind having something like:
> 
> struct gic_capabilities {
> 	const char *desc;
> 	void (*init)(void *data);
> 	u32 iidr;
> 	u32 iidr_mask;
> 	int feature;
> };

Yes, once we leave this in the driver it is much easier. But why do
the read_cpuid_id() in cpu_errata.c and not in his file? The
value/mask pairs will be then on complete different locations for the
same kind of hw depending on midr/iidr. And the only reason for using
midr is not, that it's a cpu, but just that it needs to be applied to
guests too and this is the only way to find out the real hw, otherwise
we would use iidr.  Apart from the fact that this looks inconsistent
having one errata^Mfeature flag for one errata, but not for the
other. And only because one is useing midr for hw detection and the
other iidr.

> where "feature" is a one of things declared in cpufeature.h, and that
> would condition the capability (I love the name!) if that really
> happens. I don't think we're there yet.

Yeah, some "Newspeak". :)

-Robert

> 
> As for the complexity of implementation, testing a flag in the probe
> function and tingling a static key is not really a big deal.
> 
> Thanks,
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...

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

[Marc Zyngier]

On 13/08/15 18:11, Robert Richter wrote:
> On 13.08.15 17:54:41, Marc Zyngier wrote:
>> On 13/08/15 17:17, Robert Richter wrote:
>>> Marc,
>>>
>>> thanks for your quick review.
>>>
>>> On 13.08.15 16:11:15, Marc Zyngier wrote:
>>>> On 13/08/15 15:47, Robert Richter wrote:
>>>>> From: Robert Richter <rrichter@cavium.com>
>>>
>>>>>  static const struct gic_capabilities gicv3_errata[] = {
>>>>>  	{
>>>>> +		.desc		= "GIC: Cavium erratum 23154",
>>>>> +		.iidr		= 0xa100034c,	/* ThunderX pass 1.x */
>>>>> +		.iidr_mask	= 0xffff0fff,
>>>>> +		.init		= gicv3_enable_cavium_thunderx,
>>>>> +	},
>>>>
>>>> I'm even more puzzled. You're working around a CPU bug based on the ITS
>>>> ID registers? Or have you swapped the detection methods for the two errata?
>>>
>>> :/ Right, I mixed this up... Must have starred on this for too long.
>>> Will fix that.
>>>
>>> Wrt midr: Originally this was written to support iidr. I wanted to
>>> keep the version check in the driver of the hw, an implementation
>>> outside of drivers/irqchip looked not appropriate here as it would
>>> rely then on arch arm64 only. This is the main reason. Apart from
>>> that, I think an implmentation based on struct arm64_cpu_capabilities,
>>> etc. would require much rework compared to my current easy
>>> implementation, e.g:
>>>
>>>  * binding flags to callbacks and actually run them,
>>>
>>>  * handing over private driver data (base addr for iidr detection) to
>>>    a capabilty's match function.
>>>
>>> Overall this looked bloated. Now, that the MIDR also needs to be
>>> checked, it looked better to me to keep the gic hw detection at a
>>> single location in the driver. This also allows us to check a
>>> combination of midr and iidr values.
>>>
>>> I hope this sounds reasonable?
>>
>> +Will.
>>
>> The point I was trying to make is that a CPU interface bug is a CPU bug,
>> and that it feels quite weird weird to have the detection in the GIC.
>> Will, what do you think?
>>
>> Also, I don't really buy the combined MIDR/GITS_IIDR detection. These
>> are two *very* distinct pieces of HW that are not even directly
>> connected (the redistributors are in between).
>>
>> I wouldn't mind having something like:
>>
>> struct gic_capabilities {
>> 	const char *desc;
>> 	void (*init)(void *data);
>> 	u32 iidr;
>> 	u32 iidr_mask;
>> 	int feature;
>> };
> 
> Yes, once we leave this in the driver it is much easier. But why do
> the read_cpuid_id() in cpu_errata.c and not in his file? The
> value/mask pairs will be then on complete different locations for the
> same kind of hw depending on midr/iidr. And the only reason for using
> midr is not, that it's a cpu, but just that it needs to be applied to
> guests too and this is the only way to find out the real hw, otherwise
> we would use iidr. 

But that's the thing! Using GITS_IIDR would be the complete wrong thing
to check for a CPU interface, which is what your ICC_IAR1_EL1 workaround
is about.

> Apart from the fact that this looks inconsistent
> having one errata^Mfeature flag for one errata, but not for the
> other. And only because one is useing midr for hw detection and the
> other iidr.

Because they are architecturally two different things. I strongly
suspect that you could take the ThunderX core, remove Cavium's own
implementation of the GIC and slap another GIC implementation in front
of it, and you would have the exact same CPU interface bug, because
that's where the issue is.

Thanks,

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

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

[Robert Richter]

On 14.08.15 09:28:12, Marc Zyngier wrote:
> On 13/08/15 18:11, Robert Richter wrote:
> > On 13.08.15 17:54:41, Marc Zyngier wrote:
> >> On 13/08/15 17:17, Robert Richter wrote:
> >>> Marc,
> >>>
> >>> thanks for your quick review.
> >>>
> >>> On 13.08.15 16:11:15, Marc Zyngier wrote:
> >>>> On 13/08/15 15:47, Robert Richter wrote:
> >>>>> From: Robert Richter <rrichter@cavium.com>
> >>>
> >>>>>  static const struct gic_capabilities gicv3_errata[] = {
> >>>>>  	{
> >>>>> +		.desc		= "GIC: Cavium erratum 23154",
> >>>>> +		.iidr		= 0xa100034c,	/* ThunderX pass 1.x */
> >>>>> +		.iidr_mask	= 0xffff0fff,
> >>>>> +		.init		= gicv3_enable_cavium_thunderx,
> >>>>> +	},
> >>>>
> >>>> I'm even more puzzled. You're working around a CPU bug based on the ITS
> >>>> ID registers? Or have you swapped the detection methods for the two errata?
> >>>
> >>> :/ Right, I mixed this up... Must have starred on this for too long.
> >>> Will fix that.
> >>>
> >>> Wrt midr: Originally this was written to support iidr. I wanted to
> >>> keep the version check in the driver of the hw, an implementation
> >>> outside of drivers/irqchip looked not appropriate here as it would
> >>> rely then on arch arm64 only. This is the main reason. Apart from
> >>> that, I think an implmentation based on struct arm64_cpu_capabilities,
> >>> etc. would require much rework compared to my current easy
> >>> implementation, e.g:
> >>>
> >>>  * binding flags to callbacks and actually run them,
> >>>
> >>>  * handing over private driver data (base addr for iidr detection) to
> >>>    a capabilty's match function.
> >>>
> >>> Overall this looked bloated. Now, that the MIDR also needs to be
> >>> checked, it looked better to me to keep the gic hw detection at a
> >>> single location in the driver. This also allows us to check a
> >>> combination of midr and iidr values.
> >>>
> >>> I hope this sounds reasonable?
> >>
> >> +Will.
> >>
> >> The point I was trying to make is that a CPU interface bug is a CPU bug,
> >> and that it feels quite weird weird to have the detection in the GIC.
> >> Will, what do you think?
> >>
> >> Also, I don't really buy the combined MIDR/GITS_IIDR detection. These
> >> are two *very* distinct pieces of HW that are not even directly
> >> connected (the redistributors are in between).
> >>
> >> I wouldn't mind having something like:
> >>
> >> struct gic_capabilities {
> >> 	const char *desc;
> >> 	void (*init)(void *data);
> >> 	u32 iidr;
> >> 	u32 iidr_mask;
> >> 	int feature;
> >> };
> > 
> > Yes, once we leave this in the driver it is much easier. But why do
> > the read_cpuid_id() in cpu_errata.c and not in his file? The
> > value/mask pairs will be then on complete different locations for the
> > same kind of hw depending on midr/iidr. And the only reason for using
> > midr is not, that it's a cpu, but just that it needs to be applied to
> > guests too and this is the only way to find out the real hw, otherwise
> > we would use iidr. 
> 
> But that's the thing! Using GITS_IIDR would be the complete wrong thing
> to check for a CPU interface, which is what your ICC_IAR1_EL1 workaround
> is about.
> 
> > Apart from the fact that this looks inconsistent
> > having one errata^Mfeature flag for one errata, but not for the
> > other. And only because one is useing midr for hw detection and the
> > other iidr.
> 
> Because they are architecturally two different things. I strongly
> suspect that you could take the ThunderX core, remove Cavium's own
> implementation of the GIC and slap another GIC implementation in front
> of it, and you would have the exact same CPU interface bug, because
> that's where the issue is.

Ok, I am going to send an update based on your suggestions.

Thanks,

-Robert