[PATCH 0/4] powerpc/xive: fixes and debug extensions

[PATCH 0/4] powerpc/xive: fixes and debug extensions

[Cédric Le Goater]

Hello,

First two patches are fixes for non-critical issues. I checked that
they applied on stable.

Last two are debug extensions, one for xmon and the other to dump XIVE
internal state under debugfs, which is easier than xmon.

Cheers,

C.

Cédric Le Goater (4):
  powerpc/xive: Use XIVE_BAD_IRQ instead of zero to catch non configured
    IPIs
  powerpc/xive: Fix xmon support on the PowerNV platform
  powerpc/xmon: Add source flags to output of XIVE interrupts
  powerpc/xive: Add a debugfs file to dump internal XIVE state

 arch/powerpc/sysdev/xive/xive-internal.h |   9 ++
 arch/powerpc/sysdev/xive/common.c        | 126 +++++++++++++++++++++--
 arch/powerpc/sysdev/xive/native.c        |   7 +-
 arch/powerpc/sysdev/xive/spapr.c         |  23 ++++-
 4 files changed, 151 insertions(+), 14 deletions(-)

-- 
2.21.1

[PATCH 1/4] powerpc/xive: Use XIVE_BAD_IRQ instead of zero to catch non configured IPIs

[Cédric Le Goater]

When a CPU is brought up, an IPI number is allocated and recorded
under the XIVE CPU structure. Invalid IPI numbers are tracked with
interrupt number 0x0.

On the PowerNV platform, the interrupt number space starts at 0x10 and
this works fine. However, on the sPAPR platform, it is possible to
allocate the interrupt number 0x0 and this raises an issue when CPU 0
is unplugged. The XIVE spapr driver tracks allocated interrupt numbers
in a bitmask and it is not correctly updated when interrupt number 0x0
is freed. It stays allocated and it is then impossible to reallocate.

Fix by using the XIVE_BAD_IRQ value instead of zero on both platforms.

Reported-by: David Gibson <david@gibson.dropbear.id.au>
Fixes: eac1e731b59e ("powerpc/xive: guest exploitation of the XIVE interrupt controller")
Cc: stable@vger.kernel.org # v4.14+
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 arch/powerpc/sysdev/xive/xive-internal.h |  7 +++++++
 arch/powerpc/sysdev/xive/common.c        | 12 +++---------
 arch/powerpc/sysdev/xive/native.c        |  4 ++--
 arch/powerpc/sysdev/xive/spapr.c         |  4 ++--
 4 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/sysdev/xive/xive-internal.h b/arch/powerpc/sysdev/xive/xive-internal.h
index 59cd366e7933..382980f4de2d 100644
--- a/arch/powerpc/sysdev/xive/xive-internal.h
+++ b/arch/powerpc/sysdev/xive/xive-internal.h
@@ -5,6 +5,13 @@
 #ifndef __XIVE_INTERNAL_H
 #define __XIVE_INTERNAL_H
 
+/*
+ * A "disabled" interrupt should never fire, to catch problems
+ * we set its logical number to this
+ */
+#define XIVE_BAD_IRQ		0x7fffffff
+#define XIVE_MAX_IRQ		(XIVE_BAD_IRQ - 1)
+
 /* Each CPU carry one of these with various per-CPU state */
 struct xive_cpu {
 #ifdef CONFIG_SMP
diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
index fa49193206b6..550baba98ec9 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -68,13 +68,6 @@ static u32 xive_ipi_irq;
 /* Xive state for each CPU */
 static DEFINE_PER_CPU(struct xive_cpu *, xive_cpu);
 
-/*
- * A "disabled" interrupt should never fire, to catch problems
- * we set its logical number to this
- */
-#define XIVE_BAD_IRQ		0x7fffffff
-#define XIVE_MAX_IRQ		(XIVE_BAD_IRQ - 1)
-
 /* An invalid CPU target */
 #define XIVE_INVALID_TARGET	(-1)
 
@@ -1153,7 +1146,7 @@ static int xive_setup_cpu_ipi(unsigned int cpu)
 	xc = per_cpu(xive_cpu, cpu);
 
 	/* Check if we are already setup */
-	if (xc->hw_ipi != 0)
+	if (xc->hw_ipi != XIVE_BAD_IRQ)
 		return 0;
 
 	/* Grab an IPI from the backend, this will populate xc->hw_ipi */
@@ -1190,7 +1183,7 @@ static void xive_cleanup_cpu_ipi(unsigned int cpu, struct xive_cpu *xc)
 	/* Disable the IPI and free the IRQ data */
 
 	/* Already cleaned up ? */
-	if (xc->hw_ipi == 0)
+	if (xc->hw_ipi == XIVE_BAD_IRQ)
 		return;
 
 	/* Mask the IPI */
@@ -1346,6 +1339,7 @@ static int xive_prepare_cpu(unsigned int cpu)
 		if (np)
 			xc->chip_id = of_get_ibm_chip_id(np);
 		of_node_put(np);
+		xc->hw_ipi = XIVE_BAD_IRQ;
 
 		per_cpu(xive_cpu, cpu) = xc;
 	}
diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
index 0ff6b739052c..50e1a8e02497 100644
--- a/arch/powerpc/sysdev/xive/native.c
+++ b/arch/powerpc/sysdev/xive/native.c
@@ -312,7 +312,7 @@ static void xive_native_put_ipi(unsigned int cpu, struct xive_cpu *xc)
 	s64 rc;
 
 	/* Free the IPI */
-	if (!xc->hw_ipi)
+	if (xc->hw_ipi == XIVE_BAD_IRQ)
 		return;
 	for (;;) {
 		rc = opal_xive_free_irq(xc->hw_ipi);
@@ -320,7 +320,7 @@ static void xive_native_put_ipi(unsigned int cpu, struct xive_cpu *xc)
 			msleep(OPAL_BUSY_DELAY_MS);
 			continue;
 		}
-		xc->hw_ipi = 0;
+		xc->hw_ipi = XIVE_BAD_IRQ;
 		break;
 	}
 }
diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c
index 55dc61cb4867..3f15615712b5 100644
--- a/arch/powerpc/sysdev/xive/spapr.c
+++ b/arch/powerpc/sysdev/xive/spapr.c
@@ -560,11 +560,11 @@ static int xive_spapr_get_ipi(unsigned int cpu, struct xive_cpu *xc)
 
 static void xive_spapr_put_ipi(unsigned int cpu, struct xive_cpu *xc)
 {
-	if (!xc->hw_ipi)
+	if (xc->hw_ipi == XIVE_BAD_IRQ)
 		return;
 
 	xive_irq_bitmap_free(xc->hw_ipi);
-	xc->hw_ipi = 0;
+	xc->hw_ipi = XIVE_BAD_IRQ;
 }
 #endif /* CONFIG_SMP */
 
-- 
2.21.1

[PATCH 2/4] powerpc/xive: Fix xmon support on the PowerNV platform

[Cédric Le Goater]

The PowerNV platform has multiple IRQ chips and the xmon command
dumping the state of the XIVE interrupt should only operate on the
XIVE IRQ chip.

Fixes: 5896163f7f91 ("powerpc/xmon: Improve output of XIVE interrupts")
Cc: stable@vger.kernel.org # v5.4+
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 arch/powerpc/sysdev/xive/common.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
index 550baba98ec9..8155adc2225a 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -261,11 +261,15 @@ notrace void xmon_xive_do_dump(int cpu)
 
 int xmon_xive_get_irq_config(u32 hw_irq, struct irq_data *d)
 {
+	struct irq_chip *chip = irq_data_get_irq_chip(d);
 	int rc;
 	u32 target;
 	u8 prio;
 	u32 lirq;
 
+	if (!is_xive_irq(chip))
+		return -EINVAL;
+
 	rc = xive_ops->get_irq_config(hw_irq, &target, &prio, &lirq);
 	if (rc) {
 		xmon_printf("IRQ 0x%08x : no config rc=%d\n", hw_irq, rc);
-- 
2.21.1

[PATCH 3/4] powerpc/xmon: Add source flags to output of XIVE interrupts

[Cédric Le Goater]

Some firmwares or hypervisors can advertise different source
characteristics. Track their value under XMON. What we are mostly
interested in is the StoreEOI flag.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 arch/powerpc/sysdev/xive/common.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
index 8155adc2225a..c865ae554605 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -283,7 +283,10 @@ int xmon_xive_get_irq_config(u32 hw_irq, struct irq_data *d)
 		struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
 		u64 val = xive_esb_read(xd, XIVE_ESB_GET);
 
-		xmon_printf("PQ=%c%c",
+		xmon_printf("flags=%c%c%c PQ=%c%c",
+			    xd->flags & XIVE_IRQ_FLAG_STORE_EOI ? 'S' : ' ',
+			    xd->flags & XIVE_IRQ_FLAG_LSI ? 'L' : ' ',
+			    xd->flags & XIVE_IRQ_FLAG_H_INT_ESB ? 'H' : ' ',
 			    val & XIVE_ESB_VAL_P ? 'P' : '-',
 			    val & XIVE_ESB_VAL_Q ? 'Q' : '-');
 	}
-- 
2.21.1

[PATCH 4/4] powerpc/xive: Add a debugfs file to dump internal XIVE state

[Cédric Le Goater]

As does XMON, the debugfs file /sys/kernel/debug/powerpc/xive exposes
the XIVE internal state of the machine CPUs and interrupts. Available
on the PowerNV and sPAPR platforms.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 arch/powerpc/sysdev/xive/xive-internal.h |   2 +
 arch/powerpc/sysdev/xive/common.c        | 105 +++++++++++++++++++++++
 arch/powerpc/sysdev/xive/native.c        |   3 +
 arch/powerpc/sysdev/xive/spapr.c         |  19 ++++
 4 files changed, 129 insertions(+)

diff --git a/arch/powerpc/sysdev/xive/xive-internal.h b/arch/powerpc/sysdev/xive/xive-internal.h
index 382980f4de2d..b7b901da2168 100644
--- a/arch/powerpc/sysdev/xive/xive-internal.h
+++ b/arch/powerpc/sysdev/xive/xive-internal.h
@@ -57,12 +57,14 @@ struct xive_ops {
 	int	(*get_ipi)(unsigned int cpu, struct xive_cpu *xc);
 	void	(*put_ipi)(unsigned int cpu, struct xive_cpu *xc);
 #endif
+	int	(*debug_show)(struct seq_file *m, void *private);
 	const char *name;
 };
 
 bool xive_core_init(const struct xive_ops *ops, void __iomem *area, u32 offset,
 		    u8 max_prio);
 __be32 *xive_queue_page_alloc(unsigned int cpu, u32 queue_shift);
+int xive_core_debug_init(void);
 
 static inline u32 xive_alloc_order(u32 queue_shift)
 {
diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
index c865ae554605..e192077481a1 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -20,6 +20,7 @@
 #include <linux/spinlock.h>
 #include <linux/msi.h>
 
+#include <asm/debugfs.h>
 #include <asm/prom.h>
 #include <asm/io.h>
 #include <asm/smp.h>
@@ -1558,3 +1559,107 @@ static int __init xive_off(char *arg)
 	return 0;
 }
 __setup("xive=off", xive_off);
+
+void xive_debug_show_cpu(struct seq_file *m, int cpu)
+{
+	struct xive_cpu *xc = per_cpu(xive_cpu, cpu);
+
+	seq_printf(m, "CPU %d:", cpu);
+	if (xc) {
+		seq_printf(m, "pp=%02x CPPR=%02x ", xc->pending_prio, xc->cppr);
+
+#ifdef CONFIG_SMP
+		{
+			u64 val = xive_esb_read(&xc->ipi_data, XIVE_ESB_GET);
+
+			seq_printf(m, "IPI=0x%08x PQ=%c%c ", xc->hw_ipi,
+				   val & XIVE_ESB_VAL_P ? 'P' : '-',
+				   val & XIVE_ESB_VAL_Q ? 'Q' : '-');
+		}
+#endif
+		{
+			struct xive_q *q = &xc->queue[xive_irq_priority];
+			u32 i0, i1, idx;
+
+			if (q->qpage) {
+				idx = q->idx;
+				i0 = be32_to_cpup(q->qpage + idx);
+				idx = (idx + 1) & q->msk;
+				i1 = be32_to_cpup(q->qpage + idx);
+				seq_printf(m, "EQ idx=%d T=%d %08x %08x ...",
+					   q->idx, q->toggle, i0, i1);
+			}
+		}
+	}
+	seq_puts(m, "\n");
+}
+
+void xive_debug_show_irq(struct seq_file *m, u32 hw_irq, struct irq_data *d)
+{
+	struct irq_chip *chip = irq_data_get_irq_chip(d);
+	int rc;
+	u32 target;
+	u8 prio;
+	u32 lirq;
+
+	if (!is_xive_irq(chip))
+		return;
+
+	rc = xive_ops->get_irq_config(hw_irq, &target, &prio, &lirq);
+	if (rc) {
+		seq_printf(m, "IRQ 0x%08x : no config rc=%d\n", hw_irq, rc);
+		return;
+	}
+
+	seq_printf(m, "IRQ 0x%08x : target=0x%x prio=%02x lirq=0x%x ",
+		   hw_irq, target, prio, lirq);
+
+	if (d) {
+		struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
+		u64 val = xive_esb_read(xd, XIVE_ESB_GET);
+
+		seq_printf(m, "flags=%c%c%c PQ=%c%c",
+			   xd->flags & XIVE_IRQ_FLAG_STORE_EOI ? 'S' : ' ',
+			   xd->flags & XIVE_IRQ_FLAG_LSI ? 'L' : ' ',
+			   xd->flags & XIVE_IRQ_FLAG_H_INT_ESB ? 'H' : ' ',
+			   val & XIVE_ESB_VAL_P ? 'P' : '-',
+			   val & XIVE_ESB_VAL_Q ? 'Q' : '-');
+	}
+	seq_puts(m, "\n");
+}
+
+static int xive_core_debug_show(struct seq_file *m, void *private)
+{
+	unsigned int i;
+	struct irq_desc *desc;
+	int cpu;
+
+	if (xive_ops->debug_show)
+		xive_ops->debug_show(m, private);
+
+	for_each_possible_cpu(cpu)
+		xive_debug_show_cpu(m, cpu);
+
+	for_each_irq_desc(i, desc) {
+		struct irq_data *d = irq_desc_get_irq_data(desc);
+		unsigned int hw_irq;
+
+		if (!d)
+			continue;
+
+		hw_irq = (unsigned int)irqd_to_hwirq(d);
+
+		/* IPIs are special (HW number 0) */
+		if (hw_irq)
+			xive_debug_show_irq(m, hw_irq, d);
+	}
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(xive_core_debug);
+
+int xive_core_debug_init(void)
+{
+	debugfs_create_file("xive", 0444, powerpc_debugfs_root,
+			    NULL, &xive_core_debug_fops);
+	return 0;
+}
diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
index 50e1a8e02497..5218fdc4b29a 100644
--- a/arch/powerpc/sysdev/xive/native.c
+++ b/arch/powerpc/sysdev/xive/native.c
@@ -19,6 +19,7 @@
 #include <linux/cpumask.h>
 #include <linux/mm.h>
 
+#include <asm/machdep.h>
 #include <asm/prom.h>
 #include <asm/io.h>
 #include <asm/smp.h>
@@ -850,3 +851,5 @@ int xive_native_get_vp_state(u32 vp_id, u64 *out_state)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(xive_native_get_vp_state);
+
+machine_arch_initcall(powernv, xive_core_debug_init);
diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c
index 3f15615712b5..7ab5c6780997 100644
--- a/arch/powerpc/sysdev/xive/spapr.c
+++ b/arch/powerpc/sysdev/xive/spapr.c
@@ -18,6 +18,7 @@
 #include <linux/delay.h>
 #include <linux/libfdt.h>
 
+#include <asm/machdep.h>
 #include <asm/prom.h>
 #include <asm/io.h>
 #include <asm/smp.h>
@@ -645,6 +646,21 @@ static void xive_spapr_sync_source(u32 hw_irq)
 	plpar_int_sync(0, hw_irq);
 }
 
+static int xive_spapr_debug_show(struct seq_file *m, void *private)
+{
+	struct xive_irq_bitmap *xibm;
+	char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+
+	list_for_each_entry(xibm, &xive_irq_bitmaps, list) {
+		memset(buf, 0, PAGE_SIZE);
+		bitmap_print_to_pagebuf(true, buf, xibm->bitmap, xibm->count);
+		seq_printf(m, "bitmap #%d: %s", xibm->count, buf);
+	}
+	kfree(buf);
+
+	return 0;
+}
+
 static const struct xive_ops xive_spapr_ops = {
 	.populate_irq_data	= xive_spapr_populate_irq_data,
 	.configure_irq		= xive_spapr_configure_irq,
@@ -662,6 +678,7 @@ static const struct xive_ops xive_spapr_ops = {
 #ifdef CONFIG_SMP
 	.get_ipi		= xive_spapr_get_ipi,
 	.put_ipi		= xive_spapr_put_ipi,
+	.debug_show		= xive_spapr_debug_show,
 #endif /* CONFIG_SMP */
 	.name			= "spapr",
 };
@@ -839,3 +856,5 @@ bool __init xive_spapr_init(void)
 	pr_info("Using %dkB queues\n", 1 << (xive_queue_shift - 10));
 	return true;
 }
+
+machine_arch_initcall(pseries, xive_core_debug_init);
-- 
2.21.1

Re: [PATCH 1/4] powerpc/xive: Use XIVE_BAD_IRQ instead of zero to catch non configured IPIs

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5040 bytes --]

On Fri, Mar 06, 2020 at 04:01:40PM +0100, Cédric Le Goater wrote:
> When a CPU is brought up, an IPI number is allocated and recorded
> under the XIVE CPU structure. Invalid IPI numbers are tracked with
> interrupt number 0x0.
> 
> On the PowerNV platform, the interrupt number space starts at 0x10 and
> this works fine. However, on the sPAPR platform, it is possible to
> allocate the interrupt number 0x0 and this raises an issue when CPU 0
> is unplugged. The XIVE spapr driver tracks allocated interrupt numbers
> in a bitmask and it is not correctly updated when interrupt number 0x0
> is freed. It stays allocated and it is then impossible to reallocate.
> 
> Fix by using the XIVE_BAD_IRQ value instead of zero on both platforms.
> 
> Reported-by: David Gibson <david@gibson.dropbear.id.au>
> Fixes: eac1e731b59e ("powerpc/xive: guest exploitation of the XIVE interrupt controller")
> Cc: stable@vger.kernel.org # v4.14+
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Tested-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  arch/powerpc/sysdev/xive/xive-internal.h |  7 +++++++
>  arch/powerpc/sysdev/xive/common.c        | 12 +++---------
>  arch/powerpc/sysdev/xive/native.c        |  4 ++--
>  arch/powerpc/sysdev/xive/spapr.c         |  4 ++--
>  4 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/powerpc/sysdev/xive/xive-internal.h b/arch/powerpc/sysdev/xive/xive-internal.h
> index 59cd366e7933..382980f4de2d 100644
> --- a/arch/powerpc/sysdev/xive/xive-internal.h
> +++ b/arch/powerpc/sysdev/xive/xive-internal.h
> @@ -5,6 +5,13 @@
>  #ifndef __XIVE_INTERNAL_H
>  #define __XIVE_INTERNAL_H
>  
> +/*
> + * A "disabled" interrupt should never fire, to catch problems
> + * we set its logical number to this
> + */
> +#define XIVE_BAD_IRQ		0x7fffffff
> +#define XIVE_MAX_IRQ		(XIVE_BAD_IRQ - 1)
> +
>  /* Each CPU carry one of these with various per-CPU state */
>  struct xive_cpu {
>  #ifdef CONFIG_SMP
> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
> index fa49193206b6..550baba98ec9 100644
> --- a/arch/powerpc/sysdev/xive/common.c
> +++ b/arch/powerpc/sysdev/xive/common.c
> @@ -68,13 +68,6 @@ static u32 xive_ipi_irq;
>  /* Xive state for each CPU */
>  static DEFINE_PER_CPU(struct xive_cpu *, xive_cpu);
>  
> -/*
> - * A "disabled" interrupt should never fire, to catch problems
> - * we set its logical number to this
> - */
> -#define XIVE_BAD_IRQ		0x7fffffff
> -#define XIVE_MAX_IRQ		(XIVE_BAD_IRQ - 1)
> -
>  /* An invalid CPU target */
>  #define XIVE_INVALID_TARGET	(-1)
>  
> @@ -1153,7 +1146,7 @@ static int xive_setup_cpu_ipi(unsigned int cpu)
>  	xc = per_cpu(xive_cpu, cpu);
>  
>  	/* Check if we are already setup */
> -	if (xc->hw_ipi != 0)
> +	if (xc->hw_ipi != XIVE_BAD_IRQ)
>  		return 0;
>  
>  	/* Grab an IPI from the backend, this will populate xc->hw_ipi */
> @@ -1190,7 +1183,7 @@ static void xive_cleanup_cpu_ipi(unsigned int cpu, struct xive_cpu *xc)
>  	/* Disable the IPI and free the IRQ data */
>  
>  	/* Already cleaned up ? */
> -	if (xc->hw_ipi == 0)
> +	if (xc->hw_ipi == XIVE_BAD_IRQ)
>  		return;
>  
>  	/* Mask the IPI */
> @@ -1346,6 +1339,7 @@ static int xive_prepare_cpu(unsigned int cpu)
>  		if (np)
>  			xc->chip_id = of_get_ibm_chip_id(np);
>  		of_node_put(np);
> +		xc->hw_ipi = XIVE_BAD_IRQ;
>  
>  		per_cpu(xive_cpu, cpu) = xc;
>  	}
> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
> index 0ff6b739052c..50e1a8e02497 100644
> --- a/arch/powerpc/sysdev/xive/native.c
> +++ b/arch/powerpc/sysdev/xive/native.c
> @@ -312,7 +312,7 @@ static void xive_native_put_ipi(unsigned int cpu, struct xive_cpu *xc)
>  	s64 rc;
>  
>  	/* Free the IPI */
> -	if (!xc->hw_ipi)
> +	if (xc->hw_ipi == XIVE_BAD_IRQ)
>  		return;
>  	for (;;) {
>  		rc = opal_xive_free_irq(xc->hw_ipi);
> @@ -320,7 +320,7 @@ static void xive_native_put_ipi(unsigned int cpu, struct xive_cpu *xc)
>  			msleep(OPAL_BUSY_DELAY_MS);
>  			continue;
>  		}
> -		xc->hw_ipi = 0;
> +		xc->hw_ipi = XIVE_BAD_IRQ;
>  		break;
>  	}
>  }
> diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c
> index 55dc61cb4867..3f15615712b5 100644
> --- a/arch/powerpc/sysdev/xive/spapr.c
> +++ b/arch/powerpc/sysdev/xive/spapr.c
> @@ -560,11 +560,11 @@ static int xive_spapr_get_ipi(unsigned int cpu, struct xive_cpu *xc)
>  
>  static void xive_spapr_put_ipi(unsigned int cpu, struct xive_cpu *xc)
>  {
> -	if (!xc->hw_ipi)
> +	if (xc->hw_ipi == XIVE_BAD_IRQ)
>  		return;
>  
>  	xive_irq_bitmap_free(xc->hw_ipi);
> -	xc->hw_ipi = 0;
> +	xc->hw_ipi = XIVE_BAD_IRQ;
>  }
>  #endif /* CONFIG_SMP */
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/4] powerpc/xive: Use XIVE_BAD_IRQ instead of zero to catch non configured IPIs

[Greg Kurz]

On Fri,  6 Mar 2020 16:01:40 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> When a CPU is brought up, an IPI number is allocated and recorded
> under the XIVE CPU structure. Invalid IPI numbers are tracked with
> interrupt number 0x0.
> 
> On the PowerNV platform, the interrupt number space starts at 0x10 and
> this works fine. However, on the sPAPR platform, it is possible to
> allocate the interrupt number 0x0 and this raises an issue when CPU 0
> is unplugged. The XIVE spapr driver tracks allocated interrupt numbers
> in a bitmask and it is not correctly updated when interrupt number 0x0
> is freed. It stays allocated and it is then impossible to reallocate.
> 
> Fix by using the XIVE_BAD_IRQ value instead of zero on both platforms.
> 
> Reported-by: David Gibson <david@gibson.dropbear.id.au>
> Fixes: eac1e731b59e ("powerpc/xive: guest exploitation of the XIVE interrupt controller")
> Cc: stable@vger.kernel.org # v4.14+
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

This looks mostly good. I'm juste wondering about potential overlooks:

$ git grep 'if.*hw_i' arch/powerpc/ | egrep -v 'xics|XIVE_BAD_IRQ'
arch/powerpc/kvm/book3s_xive.h:         if (out_hw_irq)
arch/powerpc/kvm/book3s_xive.h:         if (out_hw_irq)
arch/powerpc/kvm/book3s_xive_template.c:        else if (hw_irq && xd->flags & XIVE_IRQ_FLAG_EOI_FW)
arch/powerpc/sysdev/xive/common.c:      else if (hw_irq && xd->flags & XIVE_IRQ_FLAG_EOI_FW) {

This hw_irq check in xive_do_source_eoi() for example is related to:

	/*
	 * Note: We pass "0" to the hw_irq argument in order to
	 * avoid calling into the backend EOI code which we don't
	 * want to do in the case of a re-trigger. Backends typically
	 * only do EOI for LSIs anyway.
	 */
	xive_do_source_eoi(0, xd);

but it can get hw_irq from:

	xive_do_source_eoi(xc->hw_ipi, &xc->ipi_data);

It seems that these should use XIVE_BAD_IRQ as well or I'm missing
something ?

arch/powerpc/sysdev/xive/common.c:      if (hw_irq)
arch/powerpc/sysdev/xive/common.c:              if (d->domain != xive_irq_domain || hw_irq == 0)



>  arch/powerpc/sysdev/xive/xive-internal.h |  7 +++++++
>  arch/powerpc/sysdev/xive/common.c        | 12 +++---------
>  arch/powerpc/sysdev/xive/native.c        |  4 ++--
>  arch/powerpc/sysdev/xive/spapr.c         |  4 ++--
>  4 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/powerpc/sysdev/xive/xive-internal.h b/arch/powerpc/sysdev/xive/xive-internal.h
> index 59cd366e7933..382980f4de2d 100644
> --- a/arch/powerpc/sysdev/xive/xive-internal.h
> +++ b/arch/powerpc/sysdev/xive/xive-internal.h
> @@ -5,6 +5,13 @@
>  #ifndef __XIVE_INTERNAL_H
>  #define __XIVE_INTERNAL_H
>  
> +/*
> + * A "disabled" interrupt should never fire, to catch problems
> + * we set its logical number to this
> + */
> +#define XIVE_BAD_IRQ		0x7fffffff
> +#define XIVE_MAX_IRQ		(XIVE_BAD_IRQ - 1)
> +
>  /* Each CPU carry one of these with various per-CPU state */
>  struct xive_cpu {
>  #ifdef CONFIG_SMP
> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
> index fa49193206b6..550baba98ec9 100644
> --- a/arch/powerpc/sysdev/xive/common.c
> +++ b/arch/powerpc/sysdev/xive/common.c
> @@ -68,13 +68,6 @@ static u32 xive_ipi_irq;
>  /* Xive state for each CPU */
>  static DEFINE_PER_CPU(struct xive_cpu *, xive_cpu);
>  
> -/*
> - * A "disabled" interrupt should never fire, to catch problems
> - * we set its logical number to this
> - */
> -#define XIVE_BAD_IRQ		0x7fffffff
> -#define XIVE_MAX_IRQ		(XIVE_BAD_IRQ - 1)
> -
>  /* An invalid CPU target */
>  #define XIVE_INVALID_TARGET	(-1)
>  
> @@ -1153,7 +1146,7 @@ static int xive_setup_cpu_ipi(unsigned int cpu)
>  	xc = per_cpu(xive_cpu, cpu);
>  
>  	/* Check if we are already setup */
> -	if (xc->hw_ipi != 0)
> +	if (xc->hw_ipi != XIVE_BAD_IRQ)
>  		return 0;
>  
>  	/* Grab an IPI from the backend, this will populate xc->hw_ipi */
> @@ -1190,7 +1183,7 @@ static void xive_cleanup_cpu_ipi(unsigned int cpu, struct xive_cpu *xc)
>  	/* Disable the IPI and free the IRQ data */
>  
>  	/* Already cleaned up ? */
> -	if (xc->hw_ipi == 0)
> +	if (xc->hw_ipi == XIVE_BAD_IRQ)
>  		return;
>  
>  	/* Mask the IPI */
> @@ -1346,6 +1339,7 @@ static int xive_prepare_cpu(unsigned int cpu)
>  		if (np)
>  			xc->chip_id = of_get_ibm_chip_id(np);
>  		of_node_put(np);
> +		xc->hw_ipi = XIVE_BAD_IRQ;
>  
>  		per_cpu(xive_cpu, cpu) = xc;
>  	}
> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
> index 0ff6b739052c..50e1a8e02497 100644
> --- a/arch/powerpc/sysdev/xive/native.c
> +++ b/arch/powerpc/sysdev/xive/native.c
> @@ -312,7 +312,7 @@ static void xive_native_put_ipi(unsigned int cpu, struct xive_cpu *xc)
>  	s64 rc;
>  
>  	/* Free the IPI */
> -	if (!xc->hw_ipi)
> +	if (xc->hw_ipi == XIVE_BAD_IRQ)
>  		return;
>  	for (;;) {
>  		rc = opal_xive_free_irq(xc->hw_ipi);
> @@ -320,7 +320,7 @@ static void xive_native_put_ipi(unsigned int cpu, struct xive_cpu *xc)
>  			msleep(OPAL_BUSY_DELAY_MS);
>  			continue;
>  		}
> -		xc->hw_ipi = 0;
> +		xc->hw_ipi = XIVE_BAD_IRQ;
>  		break;
>  	}
>  }
> diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c
> index 55dc61cb4867..3f15615712b5 100644
> --- a/arch/powerpc/sysdev/xive/spapr.c
> +++ b/arch/powerpc/sysdev/xive/spapr.c
> @@ -560,11 +560,11 @@ static int xive_spapr_get_ipi(unsigned int cpu, struct xive_cpu *xc)
>  
>  static void xive_spapr_put_ipi(unsigned int cpu, struct xive_cpu *xc)
>  {
> -	if (!xc->hw_ipi)
> +	if (xc->hw_ipi == XIVE_BAD_IRQ)
>  		return;
>  
>  	xive_irq_bitmap_free(xc->hw_ipi);
> -	xc->hw_ipi = 0;
> +	xc->hw_ipi = XIVE_BAD_IRQ;
>  }
>  #endif /* CONFIG_SMP */
>  

Re: [PATCH 2/4] powerpc/xive: Fix xmon support on the PowerNV platform

[Greg Kurz]

On Fri,  6 Mar 2020 16:01:41 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> The PowerNV platform has multiple IRQ chips and the xmon command
> dumping the state of the XIVE interrupt should only operate on the
> XIVE IRQ chip.
> 
> Fixes: 5896163f7f91 ("powerpc/xmon: Improve output of XIVE interrupts")
> Cc: stable@vger.kernel.org # v5.4+
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  arch/powerpc/sysdev/xive/common.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
> index 550baba98ec9..8155adc2225a 100644
> --- a/arch/powerpc/sysdev/xive/common.c
> +++ b/arch/powerpc/sysdev/xive/common.c
> @@ -261,11 +261,15 @@ notrace void xmon_xive_do_dump(int cpu)
>  
>  int xmon_xive_get_irq_config(u32 hw_irq, struct irq_data *d)
>  {
> +	struct irq_chip *chip = irq_data_get_irq_chip(d);
>  	int rc;
>  	u32 target;
>  	u8 prio;
>  	u32 lirq;
>  
> +	if (!is_xive_irq(chip))
> +		return -EINVAL;
> +
>  	rc = xive_ops->get_irq_config(hw_irq, &target, &prio, &lirq);
>  	if (rc) {
>  		xmon_printf("IRQ 0x%08x : no config rc=%d\n", hw_irq, rc);

Re: [PATCH 3/4] powerpc/xmon: Add source flags to output of XIVE interrupts

[Greg Kurz]

On Fri,  6 Mar 2020 16:01:42 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> Some firmwares or hypervisors can advertise different source
> characteristics. Track their value under XMON. What we are mostly
> interested in is the StoreEOI flag.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  arch/powerpc/sysdev/xive/common.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
> index 8155adc2225a..c865ae554605 100644
> --- a/arch/powerpc/sysdev/xive/common.c
> +++ b/arch/powerpc/sysdev/xive/common.c
> @@ -283,7 +283,10 @@ int xmon_xive_get_irq_config(u32 hw_irq, struct irq_data *d)
>  		struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
>  		u64 val = xive_esb_read(xd, XIVE_ESB_GET);
>  
> -		xmon_printf("PQ=%c%c",
> +		xmon_printf("flags=%c%c%c PQ=%c%c",
> +			    xd->flags & XIVE_IRQ_FLAG_STORE_EOI ? 'S' : ' ',
> +			    xd->flags & XIVE_IRQ_FLAG_LSI ? 'L' : ' ',
> +			    xd->flags & XIVE_IRQ_FLAG_H_INT_ESB ? 'H' : ' ',
>  			    val & XIVE_ESB_VAL_P ? 'P' : '-',
>  			    val & XIVE_ESB_VAL_Q ? 'Q' : '-');
>  	}

Re: [PATCH 1/4] powerpc/xive: Use XIVE_BAD_IRQ instead of zero to catch non configured IPIs

[Cédric Le Goater]

On 3/10/20 4:09 PM, Greg Kurz wrote:
> On Fri,  6 Mar 2020 16:01:40 +0100
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> When a CPU is brought up, an IPI number is allocated and recorded
>> under the XIVE CPU structure. Invalid IPI numbers are tracked with
>> interrupt number 0x0.
>>
>> On the PowerNV platform, the interrupt number space starts at 0x10 and
>> this works fine. However, on the sPAPR platform, it is possible to
>> allocate the interrupt number 0x0 and this raises an issue when CPU 0
>> is unplugged. The XIVE spapr driver tracks allocated interrupt numbers
>> in a bitmask and it is not correctly updated when interrupt number 0x0
>> is freed. It stays allocated and it is then impossible to reallocate.
>>
>> Fix by using the XIVE_BAD_IRQ value instead of zero on both platforms.
>>
>> Reported-by: David Gibson <david@gibson.dropbear.id.au>
>> Fixes: eac1e731b59e ("powerpc/xive: guest exploitation of the XIVE interrupt controller")
>> Cc: stable@vger.kernel.org # v4.14+
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
> 
> This looks mostly good. I'm juste wondering about potential overlooks:

Yes. Thanks for doing so. There are some non-obvious use of interrupt
numbers under the hood. 

One thing we should be adding is a comment on the different interrupt 
number spaces. The HW interrupt numbers, the EISN numbers found on the 
XIVE even queue and the Linux interrupt numbers are different spaces 
and have different limits. XIVE_BAD_IRQ was introduced for the EISN 
numbers and the patch is using this value for HW numbers. This is ok 
because it's more a tag than a limit.

> $ git grep 'if.*hw_i' arch/powerpc/ | egrep -v 'xics|XIVE_BAD_IRQ'
>
>
> arch/powerpc/kvm/book3s_xive.h:         if (out_hw_irq)
> arch/powerpc/kvm/book3s_xive.h:         if (out_hw_irq)
> arch/powerpc/kvm/book3s_xive_template.c:        else if (hw_irq && xd->flags & XIVE_IRQ_FLAG_EOI_FW)
> arch/powerpc/sysdev/xive/common.c:      else if (hw_irq && xd->flags & XIVE_IRQ_FLAG_EOI_FW) {
>
> This hw_irq check in xive_do_source_eoi() for example is related to:
> 
> 	/*
> 	 * Note: We pass "0" to the hw_irq argument in order to
> 	 * avoid calling into the backend EOI code which we don't
> 	 * want to do in the case of a re-trigger. Backends typically
> 	 * only do EOI for LSIs anyway.
> 	 */
> 	xive_do_source_eoi(0, xd);

that's a hack to skip the following part of the code in case of EOI 
being done through FW:

	else if (hw_irq && xd->flags & XIVE_IRQ_FLAG_EOI_FW) {
		/*
		 * The FW told us to call it. This happens for some
		 * interrupt sources that need additional HW whacking
		 * beyond the ESB manipulation. For example LPC interrupts
		 * on P9 DD1.0 needed a latch to be clared in the LPC bridge
		 * itself. The Firmware will take care of it.
		 */
		if (WARN_ON_ONCE(!xive_ops->eoi))
			return;
		xive_ops->eoi(hw_irq);

That was the case for PHB4 LSIs on P9 DD1 only. We could probably drop
the code in Linux unless similar bugs show up on new platforms.

> but it can get hw_irq from:
> 
> 	xive_do_source_eoi(xc->hw_ipi, &xc->ipi_data);

That part is fine. It's an IPI EOI.

> 
> It seems that these should use XIVE_BAD_IRQ as well or I'm missing
> something ?
> 
> arch/powerpc/sysdev/xive/common.c:      if (hw_irq)

This tests the XIVE IPI number which is mapped to 0 for all CPUs.
See xive_request_ipi() and xive_irq_domain_map()

C.

> arch/powerpc/sysdev/xive/common.c:              if (d->domain != xive_irq_domain || hw_irq == 0)
> 
> 
> 
>>  arch/powerpc/sysdev/xive/xive-internal.h |  7 +++++++
>>  arch/powerpc/sysdev/xive/common.c        | 12 +++---------
>>  arch/powerpc/sysdev/xive/native.c        |  4 ++--
>>  arch/powerpc/sysdev/xive/spapr.c         |  4 ++--
>>  4 files changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/powerpc/sysdev/xive/xive-internal.h b/arch/powerpc/sysdev/xive/xive-internal.h
>> index 59cd366e7933..382980f4de2d 100644
>> --- a/arch/powerpc/sysdev/xive/xive-internal.h
>> +++ b/arch/powerpc/sysdev/xive/xive-internal.h
>> @@ -5,6 +5,13 @@
>>  #ifndef __XIVE_INTERNAL_H
>>  #define __XIVE_INTERNAL_H
>>  
>> +/*
>> + * A "disabled" interrupt should never fire, to catch problems
>> + * we set its logical number to this
>> + */
>> +#define XIVE_BAD_IRQ		0x7fffffff
>> +#define XIVE_MAX_IRQ		(XIVE_BAD_IRQ - 1)
>> +
>>  /* Each CPU carry one of these with various per-CPU state */
>>  struct xive_cpu {
>>  #ifdef CONFIG_SMP
>> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
>> index fa49193206b6..550baba98ec9 100644
>> --- a/arch/powerpc/sysdev/xive/common.c
>> +++ b/arch/powerpc/sysdev/xive/common.c
>> @@ -68,13 +68,6 @@ static u32 xive_ipi_irq;
>>  /* Xive state for each CPU */
>>  static DEFINE_PER_CPU(struct xive_cpu *, xive_cpu);
>>  
>> -/*
>> - * A "disabled" interrupt should never fire, to catch problems
>> - * we set its logical number to this
>> - */
>> -#define XIVE_BAD_IRQ		0x7fffffff
>> -#define XIVE_MAX_IRQ		(XIVE_BAD_IRQ - 1)
>> -
>>  /* An invalid CPU target */
>>  #define XIVE_INVALID_TARGET	(-1)
>>  
>> @@ -1153,7 +1146,7 @@ static int xive_setup_cpu_ipi(unsigned int cpu)
>>  	xc = per_cpu(xive_cpu, cpu);
>>  
>>  	/* Check if we are already setup */
>> -	if (xc->hw_ipi != 0)
>> +	if (xc->hw_ipi != XIVE_BAD_IRQ)
>>  		return 0;
>>  
>>  	/* Grab an IPI from the backend, this will populate xc->hw_ipi */
>> @@ -1190,7 +1183,7 @@ static void xive_cleanup_cpu_ipi(unsigned int cpu, struct xive_cpu *xc)
>>  	/* Disable the IPI and free the IRQ data */
>>  
>>  	/* Already cleaned up ? */
>> -	if (xc->hw_ipi == 0)
>> +	if (xc->hw_ipi == XIVE_BAD_IRQ)
>>  		return;
>>  
>>  	/* Mask the IPI */
>> @@ -1346,6 +1339,7 @@ static int xive_prepare_cpu(unsigned int cpu)
>>  		if (np)
>>  			xc->chip_id = of_get_ibm_chip_id(np);
>>  		of_node_put(np);
>> +		xc->hw_ipi = XIVE_BAD_IRQ;
>>  
>>  		per_cpu(xive_cpu, cpu) = xc;
>>  	}
>> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
>> index 0ff6b739052c..50e1a8e02497 100644
>> --- a/arch/powerpc/sysdev/xive/native.c
>> +++ b/arch/powerpc/sysdev/xive/native.c
>> @@ -312,7 +312,7 @@ static void xive_native_put_ipi(unsigned int cpu, struct xive_cpu *xc)
>>  	s64 rc;
>>  
>>  	/* Free the IPI */
>> -	if (!xc->hw_ipi)
>> +	if (xc->hw_ipi == XIVE_BAD_IRQ)
>>  		return;
>>  	for (;;) {
>>  		rc = opal_xive_free_irq(xc->hw_ipi);
>> @@ -320,7 +320,7 @@ static void xive_native_put_ipi(unsigned int cpu, struct xive_cpu *xc)
>>  			msleep(OPAL_BUSY_DELAY_MS);
>>  			continue;
>>  		}
>> -		xc->hw_ipi = 0;
>> +		xc->hw_ipi = XIVE_BAD_IRQ;
>>  		break;
>>  	}
>>  }
>> diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c
>> index 55dc61cb4867..3f15615712b5 100644
>> --- a/arch/powerpc/sysdev/xive/spapr.c
>> +++ b/arch/powerpc/sysdev/xive/spapr.c
>> @@ -560,11 +560,11 @@ static int xive_spapr_get_ipi(unsigned int cpu, struct xive_cpu *xc)
>>  
>>  static void xive_spapr_put_ipi(unsigned int cpu, struct xive_cpu *xc)
>>  {
>> -	if (!xc->hw_ipi)
>> +	if (xc->hw_ipi == XIVE_BAD_IRQ)
>>  		return;
>>  
>>  	xive_irq_bitmap_free(xc->hw_ipi);
>> -	xc->hw_ipi = 0;
>> +	xc->hw_ipi = XIVE_BAD_IRQ;
>>  }
>>  #endif /* CONFIG_SMP */
>>  
> 

Re: [PATCH 1/4] powerpc/xive: Use XIVE_BAD_IRQ instead of zero to catch non configured IPIs

[Michael Ellerman]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1118 bytes --]

On Fri, 2020-03-06 at 15:01:40 UTC, =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= wrote:
> When a CPU is brought up, an IPI number is allocated and recorded
> under the XIVE CPU structure. Invalid IPI numbers are tracked with
> interrupt number 0x0.
> 
> On the PowerNV platform, the interrupt number space starts at 0x10 and
> this works fine. However, on the sPAPR platform, it is possible to
> allocate the interrupt number 0x0 and this raises an issue when CPU 0
> is unplugged. The XIVE spapr driver tracks allocated interrupt numbers
> in a bitmask and it is not correctly updated when interrupt number 0x0
> is freed. It stays allocated and it is then impossible to reallocate.
> 
> Fix by using the XIVE_BAD_IRQ value instead of zero on both platforms.
> 
> Reported-by: David Gibson <david@gibson.dropbear.id.au>
> Fixes: eac1e731b59e ("powerpc/xive: guest exploitation of the XIVE interrupt controller")
> Cc: stable@vger.kernel.org # v4.14+
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/b1a504a6500df50e83b701b7946b34fce27ad8a3

cheers