[RFC PATCH] powerpc: add tests for XICS

[RFC PATCH] powerpc: add tests for XICS

[Laurent Vivier]

Check if we can set the xive server and priority, and
check we get values that have been set.
Check we disable/enable interrupts.

This patch also increases NR_CPUS from 8 to 16
(maximum for KVM on POWER9, POWER8 allows 96)

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
Note: I send this as an RFC, because even if this test works well with
TCG and POWER8 KVM hosts, it detects some problems with POWER9 KVM hosts

 lib/powerpc/asm/setup.h |   2 +-
 powerpc/rtas.c          | 146 ++++++++++++++++++++++++++++++++++++++++++++++++
 powerpc/unittests.cfg   |   6 ++
 3 files changed, 153 insertions(+), 1 deletion(-)

diff --git a/lib/powerpc/asm/setup.h b/lib/powerpc/asm/setup.h
index 23b4156..f667aac 100644
--- a/lib/powerpc/asm/setup.h
+++ b/lib/powerpc/asm/setup.h
@@ -7,7 +7,7 @@
  */
 #include <libcflat.h>
 
-#define NR_CPUS			8	/* arbitrarily set for now */
+#define NR_CPUS			16	/* Maximum supported by KVM on P9 */
 extern u32 cpus[NR_CPUS];
 extern int nr_cpus;
 
diff --git a/powerpc/rtas.c b/powerpc/rtas.c
index 5d43f33..4580873 100644
--- a/powerpc/rtas.c
+++ b/powerpc/rtas.c
@@ -5,11 +5,19 @@
 #include <libcflat.h>
 #include <util.h>
 #include <asm/rtas.h>
+#include <asm/setup.h>
+#include <devicetree.h>
 
 #define DAYS(y,m,d) (365UL * (y) + ((y) / 4) - ((y) / 100) + ((y) / 400) + \
 		     367UL * (m) / 12  + \
 		     (d))
 
+/* from qemu/include/hw/ppc/xics.h */
+
+#define XICS_BUID       0x1
+#define XICS_IRQ_BASE   (XICS_BUID << 12)
+#define XICS_IRQS_SPAPR	1024
+
 static unsigned long mktime(int year, int month, int day,
 			    int hour, int minute, int second)
 {
@@ -110,6 +118,140 @@ static void check_set_time_of_day(void)
 	report("running", t1 + DELAY <= t2);
 }
 
+static int current_cpu;
+static int xics_server[NR_CPUS];
+
+static void cpu_get_server(int cpu_node, u64 regval, void *info __unused)
+{
+	const struct fdt_property *prop;
+	int len;
+	u32 *data;
+
+	prop = fdt_get_property(dt_fdt(), cpu_node,
+				"ibm,ppc-interrupt-server#s", &len);
+
+	data = (u32 *)prop->data;
+	xics_server[current_cpu++] = fdt32_to_cpu(*data);
+}
+
+static int xics_get_server(int cpu)
+{
+	return xics_server[cpu];
+}
+
+static void check_xics(void)
+{
+	int ret;
+	uint32_t set_xive_token, get_xive_token;
+	uint32_t int_off_token, int_on_token;
+	int state[3];
+	int irq;
+
+	ret = rtas_token("ibm,get-xive", &get_xive_token);
+	report("get-xive token available", ret == 0);
+	if (ret)
+		return;
+
+	ret = rtas_token("ibm,set-xive", &set_xive_token);
+	report("set-xive token available", ret == 0);
+	if (ret)
+		return;
+
+	ret = rtas_token("ibm,int-off", &int_off_token);
+	report("int-off token available", ret == 0);
+	if (ret)
+		return;
+
+	ret = rtas_token("ibm,int-on", &int_on_token);
+	report("int-on token available", ret == 0);
+	if (ret)
+		return;
+
+	report("%d cpus detected", nr_cpus > 1, nr_cpus);
+
+	/* retrieve XICS server id / cpu */
+	ret = dt_for_each_cpu_node(cpu_get_server, NULL);
+	assert(ret == 0);
+
+	for (irq = XICS_IRQ_BASE; irq < XICS_IRQ_BASE + XICS_IRQS_SPAPR;
+	     irq++) {
+		ret = rtas_call(set_xive_token, 3, 1, state, irq,
+				xics_get_server(irq % nr_cpus), irq % 256);
+		if (ret) {
+			report("set-xive: irq #%d, cpu %d prio %d, ret = %d",
+			       false, irq, irq % nr_cpus, irq % 256, ret);
+			return;
+		}
+	}
+	report("set-xive", true);
+
+	for (irq = XICS_IRQ_BASE; irq < XICS_IRQ_BASE + XICS_IRQS_SPAPR;
+	     irq++) {
+		state[0] = -1;
+		state[1] = -1;
+		ret = rtas_call(get_xive_token, 1, 3, state, irq);
+		if (ret || state[0] != xics_get_server(irq % nr_cpus) ||
+                    state[1] != irq % 256) {
+			report("get-xive: irq #%d, expected cpu %d prio %d,"
+			       " had cpu %d prio %d, ret = %d", false,
+			       irq, irq % nr_cpus, irq % 256, state[0], state[1],
+			       ret);
+			return;
+		}
+	}
+	report("get-xive", true);
+
+	for (irq = XICS_IRQ_BASE; irq < XICS_IRQ_BASE + XICS_IRQS_SPAPR;
+	     irq++) {
+		ret = rtas_call(int_off_token, 1, 1, state, irq);
+		if (ret) {
+			report("int-off: irq #%d, ret = %d", false, irq, ret);
+			return;
+		}
+	}
+
+	for (irq = XICS_IRQ_BASE; irq < XICS_IRQ_BASE + XICS_IRQS_SPAPR;
+	     irq++) {
+		state[0] = -1;
+		state[1] = 0;
+		ret = rtas_call(get_xive_token, 1, 3, state, irq);
+		if (ret || state[0] != xics_get_server(irq % nr_cpus) ||
+                    state[1] != 0xff) {
+			report("int-off: irq #%d, expected cpu %d prio %d,"
+			       " had cpu %d prio %d, ret = %d", false,
+			       irq, irq % nr_cpus, 0xff, state[0], state[1],
+			       ret);
+			return;
+		}
+	}
+	report("int-off", true);
+
+	for (irq = XICS_IRQ_BASE; irq < XICS_IRQ_BASE + XICS_IRQS_SPAPR;
+	     irq++) {
+		ret = rtas_call(int_on_token, 1, 1, state, irq);
+		if (ret) {
+			report("int-on: irq #%d, ret = %d", false, irq, ret);
+			return;
+		}
+	}
+
+	for (irq = XICS_IRQ_BASE; irq < XICS_IRQ_BASE + XICS_IRQS_SPAPR;
+	     irq++) {
+		state[0] = -1;
+		state[1] = -1;
+		ret = rtas_call(get_xive_token, 1, 3, state, irq);
+		if (ret || state[0] != xics_get_server(irq % nr_cpus) ||
+                    state[1] != irq % 256) {
+			report("int-on: irq #%d, expected cpu %d prio %d,"
+			       " had cpu %d prio %d, ret = %d", false,
+			       irq, irq % nr_cpus, irq % 256, state[0], state[1],
+                               ret);
+			return;
+		}
+	}
+	report("int-on", true);
+}
+
 int main(int argc, char **argv)
 {
 	int len;
@@ -137,6 +279,10 @@ int main(int argc, char **argv)
 
 		check_set_time_of_day();
 
+	} else if (strcmp(argv[1], "xics") == 0) {
+
+		check_xics();
+
 	} else {
 		printf("Unknown subtest\n");
 		abort();
diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
index 4eda258..76ccb62 100644
--- a/powerpc/unittests.cfg
+++ b/powerpc/unittests.cfg
@@ -57,6 +57,12 @@ extra_params = -append "set-time-of-day"
 timeout = 5
 groups = rtas
 
+[xics]
+file = rtas.elf
+extra_params = -smp 16 -append "xics"
+timeout = 5
+groups = rtas
+
 [emulator]
 file = emulator.elf
 
-- 
2.13.6

[RFC PATCH] powerpc: add tests for XICS

[Laurent Vivier]

Check if we can set the xive server and priority, and
check we get values that have been set.
Check we disable/enable interrupts.

This patch also increases NR_CPUS from 8 to 16
(maximum for KVM on POWER9, POWER8 allows 96)

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
Note: I send this as an RFC, because even if this test works well with
TCG and POWER8 KVM hosts, it detects some problems with POWER9 KVM hosts

 lib/powerpc/asm/setup.h |   2 +-
 powerpc/rtas.c          | 146 ++++++++++++++++++++++++++++++++++++++++++++++++
 powerpc/unittests.cfg   |   6 ++
 3 files changed, 153 insertions(+), 1 deletion(-)

diff --git a/lib/powerpc/asm/setup.h b/lib/powerpc/asm/setup.h
index 23b4156..f667aac 100644
--- a/lib/powerpc/asm/setup.h
+++ b/lib/powerpc/asm/setup.h
@@ -7,7 +7,7 @@
  */
 #include <libcflat.h>
 
-#define NR_CPUS			8	/* arbitrarily set for now */
+#define NR_CPUS			16	/* Maximum supported by KVM on P9 */
 extern u32 cpus[NR_CPUS];
 extern int nr_cpus;
 
diff --git a/powerpc/rtas.c b/powerpc/rtas.c
index 5d43f33..4580873 100644
--- a/powerpc/rtas.c
+++ b/powerpc/rtas.c
@@ -5,11 +5,19 @@
 #include <libcflat.h>
 #include <util.h>
 #include <asm/rtas.h>
+#include <asm/setup.h>
+#include <devicetree.h>
 
 #define DAYS(y,m,d) (365UL * (y) + ((y) / 4) - ((y) / 100) + ((y) / 400) + \
 		     367UL * (m) / 12  + \
 		     (d))
 
+/* from qemu/include/hw/ppc/xics.h */
+
+#define XICS_BUID       0x1
+#define XICS_IRQ_BASE   (XICS_BUID << 12)
+#define XICS_IRQS_SPAPR	1024
+
 static unsigned long mktime(int year, int month, int day,
 			    int hour, int minute, int second)
 {
@@ -110,6 +118,140 @@ static void check_set_time_of_day(void)
 	report("running", t1 + DELAY <= t2);
 }
 
+static int current_cpu;
+static int xics_server[NR_CPUS];
+
+static void cpu_get_server(int cpu_node, u64 regval, void *info __unused)
+{
+	const struct fdt_property *prop;
+	int len;
+	u32 *data;
+
+	prop = fdt_get_property(dt_fdt(), cpu_node,
+				"ibm,ppc-interrupt-server#s", &len);
+
+	data = (u32 *)prop->data;
+	xics_server[current_cpu++] = fdt32_to_cpu(*data);
+}
+
+static int xics_get_server(int cpu)
+{
+	return xics_server[cpu];
+}
+
+static void check_xics(void)
+{
+	int ret;
+	uint32_t set_xive_token, get_xive_token;
+	uint32_t int_off_token, int_on_token;
+	int state[3];
+	int irq;
+
+	ret = rtas_token("ibm,get-xive", &get_xive_token);
+	report("get-xive token available", ret = 0);
+	if (ret)
+		return;
+
+	ret = rtas_token("ibm,set-xive", &set_xive_token);
+	report("set-xive token available", ret = 0);
+	if (ret)
+		return;
+
+	ret = rtas_token("ibm,int-off", &int_off_token);
+	report("int-off token available", ret = 0);
+	if (ret)
+		return;
+
+	ret = rtas_token("ibm,int-on", &int_on_token);
+	report("int-on token available", ret = 0);
+	if (ret)
+		return;
+
+	report("%d cpus detected", nr_cpus > 1, nr_cpus);
+
+	/* retrieve XICS server id / cpu */
+	ret = dt_for_each_cpu_node(cpu_get_server, NULL);
+	assert(ret = 0);
+
+	for (irq = XICS_IRQ_BASE; irq < XICS_IRQ_BASE + XICS_IRQS_SPAPR;
+	     irq++) {
+		ret = rtas_call(set_xive_token, 3, 1, state, irq,
+				xics_get_server(irq % nr_cpus), irq % 256);
+		if (ret) {
+			report("set-xive: irq #%d, cpu %d prio %d, ret = %d",
+			       false, irq, irq % nr_cpus, irq % 256, ret);
+			return;
+		}
+	}
+	report("set-xive", true);
+
+	for (irq = XICS_IRQ_BASE; irq < XICS_IRQ_BASE + XICS_IRQS_SPAPR;
+	     irq++) {
+		state[0] = -1;
+		state[1] = -1;
+		ret = rtas_call(get_xive_token, 1, 3, state, irq);
+		if (ret || state[0] != xics_get_server(irq % nr_cpus) ||
+                    state[1] != irq % 256) {
+			report("get-xive: irq #%d, expected cpu %d prio %d,"
+			       " had cpu %d prio %d, ret = %d", false,
+			       irq, irq % nr_cpus, irq % 256, state[0], state[1],
+			       ret);
+			return;
+		}
+	}
+	report("get-xive", true);
+
+	for (irq = XICS_IRQ_BASE; irq < XICS_IRQ_BASE + XICS_IRQS_SPAPR;
+	     irq++) {
+		ret = rtas_call(int_off_token, 1, 1, state, irq);
+		if (ret) {
+			report("int-off: irq #%d, ret = %d", false, irq, ret);
+			return;
+		}
+	}
+
+	for (irq = XICS_IRQ_BASE; irq < XICS_IRQ_BASE + XICS_IRQS_SPAPR;
+	     irq++) {
+		state[0] = -1;
+		state[1] = 0;
+		ret = rtas_call(get_xive_token, 1, 3, state, irq);
+		if (ret || state[0] != xics_get_server(irq % nr_cpus) ||
+                    state[1] != 0xff) {
+			report("int-off: irq #%d, expected cpu %d prio %d,"
+			       " had cpu %d prio %d, ret = %d", false,
+			       irq, irq % nr_cpus, 0xff, state[0], state[1],
+			       ret);
+			return;
+		}
+	}
+	report("int-off", true);
+
+	for (irq = XICS_IRQ_BASE; irq < XICS_IRQ_BASE + XICS_IRQS_SPAPR;
+	     irq++) {
+		ret = rtas_call(int_on_token, 1, 1, state, irq);
+		if (ret) {
+			report("int-on: irq #%d, ret = %d", false, irq, ret);
+			return;
+		}
+	}
+
+	for (irq = XICS_IRQ_BASE; irq < XICS_IRQ_BASE + XICS_IRQS_SPAPR;
+	     irq++) {
+		state[0] = -1;
+		state[1] = -1;
+		ret = rtas_call(get_xive_token, 1, 3, state, irq);
+		if (ret || state[0] != xics_get_server(irq % nr_cpus) ||
+                    state[1] != irq % 256) {
+			report("int-on: irq #%d, expected cpu %d prio %d,"
+			       " had cpu %d prio %d, ret = %d", false,
+			       irq, irq % nr_cpus, irq % 256, state[0], state[1],
+                               ret);
+			return;
+		}
+	}
+	report("int-on", true);
+}
+
 int main(int argc, char **argv)
 {
 	int len;
@@ -137,6 +279,10 @@ int main(int argc, char **argv)
 
 		check_set_time_of_day();
 
+	} else if (strcmp(argv[1], "xics") = 0) {
+
+		check_xics();
+
 	} else {
 		printf("Unknown subtest\n");
 		abort();
diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
index 4eda258..76ccb62 100644
--- a/powerpc/unittests.cfg
+++ b/powerpc/unittests.cfg
@@ -57,6 +57,12 @@ extra_params = -append "set-time-of-day"
 timeout = 5
 groups = rtas
 
+[xics]
+file = rtas.elf
+extra_params = -smp 16 -append "xics"
+timeout = 5
+groups = rtas
+
 [emulator]
 file = emulator.elf
 
-- 
2.13.6

Re: [RFC kvm-unit-tests PATCH] powerpc: add tests for XICS

[Thomas Huth]

On 12.10.2017 10:07, Laurent Vivier wrote:
> Check if we can set the xive server and priority, and
> check we get values that have been set.
> Check we disable/enable interrupts.
> 
> This patch also increases NR_CPUS from 8 to 16
> (maximum for KVM on POWER9, POWER8 allows 96)
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> Note: I send this as an RFC, because even if this test works well with
> TCG and POWER8 KVM hosts, it detects some problems with POWER9 KVM hosts

What kind of errors does it detect? Are they expected due to the
different interrupt controllers?

> main(int argc, char **argv)
>  
>  		check_set_time_of_day();
>  
> +	} else if (strcmp(argv[1], "xics") == 0) {
> +

Superfluous empty line?

> +		check_xics();
> +

 Thomas

Re: [RFC kvm-unit-tests PATCH] powerpc: add tests for XICS

[Thomas Huth]

On 12.10.2017 10:07, Laurent Vivier wrote:
> Check if we can set the xive server and priority, and
> check we get values that have been set.
> Check we disable/enable interrupts.
> 
> This patch also increases NR_CPUS from 8 to 16
> (maximum for KVM on POWER9, POWER8 allows 96)
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> Note: I send this as an RFC, because even if this test works well with
> TCG and POWER8 KVM hosts, it detects some problems with POWER9 KVM hosts

What kind of errors does it detect? Are they expected due to the
different interrupt controllers?

> main(int argc, char **argv)
>  
>  		check_set_time_of_day();
>  
> +	} else if (strcmp(argv[1], "xics") = 0) {
> +

Superfluous empty line?

> +		check_xics();
> +

 Thomas

Re: [RFC kvm-unit-tests PATCH] powerpc: add tests for XICS

[Laurent Vivier]

On 13/10/2017 11:45, Thomas Huth wrote:
> On 12.10.2017 10:07, Laurent Vivier wrote:
>> Check if we can set the xive server and priority, and
>> check we get values that have been set.
>> Check we disable/enable interrupts.
>>
>> This patch also increases NR_CPUS from 8 to 16
>> (maximum for KVM on POWER9, POWER8 allows 96)
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>> Note: I send this as an RFC, because even if this test works well with
>> TCG and POWER8 KVM hosts, it detects some problems with POWER9 KVM hosts
> 
> What kind of errors does it detect? Are they expected due to the
> different interrupt controllers?
> 

[I cc' Sam as he already did a fix for this part in the kernel]

I have two kinds of error:

- "xics: get-xive: irq #4351, expected cpu 15 prio 255, had cpu 0 prio
   255",
  Priority 255 is set to disable the IRQs, I don't think it's really a
  problem at this level, it could be only cosmetic,

- "xics: int-on: irq #4351, ret = -3",
  but here it becomes more serious: as the IRQ server has been lost, it
  seems we are not able to re-enable the interrupt.

I have no access to a POWER9 host to debug this at the kernel level for
the moment, so I can't do more.

Thanks,
Laurent

Re: [RFC kvm-unit-tests PATCH] powerpc: add tests for XICS

[Laurent Vivier]

On 13/10/2017 11:45, Thomas Huth wrote:
> On 12.10.2017 10:07, Laurent Vivier wrote:
>> Check if we can set the xive server and priority, and
>> check we get values that have been set.
>> Check we disable/enable interrupts.
>>
>> This patch also increases NR_CPUS from 8 to 16
>> (maximum for KVM on POWER9, POWER8 allows 96)
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>> Note: I send this as an RFC, because even if this test works well with
>> TCG and POWER8 KVM hosts, it detects some problems with POWER9 KVM hosts
> 
> What kind of errors does it detect? Are they expected due to the
> different interrupt controllers?
> 

[I cc' Sam as he already did a fix for this part in the kernel]

I have two kinds of error:

- "xics: get-xive: irq #4351, expected cpu 15 prio 255, had cpu 0 prio
   255",
  Priority 255 is set to disable the IRQs, I don't think it's really a
  problem at this level, it could be only cosmetic,

- "xics: int-on: irq #4351, ret = -3",
  but here it becomes more serious: as the IRQ server has been lost, it
  seems we are not able to re-enable the interrupt.

I have no access to a POWER9 host to debug this at the kernel level for
the moment, so I can't do more.

Thanks,
Laurent

Re: [RFC kvm-unit-tests PATCH] powerpc: add tests for XICS

[Laurent Vivier]

On 13/10/2017 11:45, Thomas Huth wrote:
> On 12.10.2017 10:07, Laurent Vivier wrote:
>> Check if we can set the xive server and priority, and
>> check we get values that have been set.
>> Check we disable/enable interrupts.
>>
>> This patch also increases NR_CPUS from 8 to 16
>> (maximum for KVM on POWER9, POWER8 allows 96)
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>> Note: I send this as an RFC, because even if this test works well with
>> TCG and POWER8 KVM hosts, it detects some problems with POWER9 KVM hosts
> 
> What kind of errors does it detect? Are they expected due to the
> different interrupt controllers?
> 
>> main(int argc, char **argv)
>>  
>>  		check_set_time_of_day();
>>  
>> +	} else if (strcmp(argv[1], "xics") == 0) {
>> +
> 
> Superfluous empty line?
> 
>> +		check_xics();
>> +

This is a standard pattern in kvm-unit-tests:

...
        } else if (strcmp(argv[1], "XXXX") == 0) {

                function(...);

        } else if (strcmp(argv[1], "YYY") == 0) {
...

Laurent

Re: [RFC kvm-unit-tests PATCH] powerpc: add tests for XICS

[Laurent Vivier]

On 13/10/2017 11:45, Thomas Huth wrote:
> On 12.10.2017 10:07, Laurent Vivier wrote:
>> Check if we can set the xive server and priority, and
>> check we get values that have been set.
>> Check we disable/enable interrupts.
>>
>> This patch also increases NR_CPUS from 8 to 16
>> (maximum for KVM on POWER9, POWER8 allows 96)
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>> Note: I send this as an RFC, because even if this test works well with
>> TCG and POWER8 KVM hosts, it detects some problems with POWER9 KVM hosts
> 
> What kind of errors does it detect? Are they expected due to the
> different interrupt controllers?
> 
>> main(int argc, char **argv)
>>  
>>  		check_set_time_of_day();
>>  
>> +	} else if (strcmp(argv[1], "xics") = 0) {
>> +
> 
> Superfluous empty line?
> 
>> +		check_xics();
>> +

This is a standard pattern in kvm-unit-tests:

...
        } else if (strcmp(argv[1], "XXXX") = 0) {

                function(...);

        } else if (strcmp(argv[1], "YYY") = 0) {
...

Laurent

Re: [RFC kvm-unit-tests PATCH] powerpc: add tests for XICS

[Andrew Jones]

On Mon, Oct 16, 2017 at 07:08:03PM +0200, Laurent Vivier wrote:
> On 13/10/2017 11:45, Thomas Huth wrote:
> > On 12.10.2017 10:07, Laurent Vivier wrote:
> >> Check if we can set the xive server and priority, and
> >> check we get values that have been set.
> >> Check we disable/enable interrupts.
> >>
> >> This patch also increases NR_CPUS from 8 to 16
> >> (maximum for KVM on POWER9, POWER8 allows 96)
> >>
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >> ---
> >> Note: I send this as an RFC, because even if this test works well with
> >> TCG and POWER8 KVM hosts, it detects some problems with POWER9 KVM hosts
> > 
> > What kind of errors does it detect? Are they expected due to the
> > different interrupt controllers?
> > 
> >> main(int argc, char **argv)
> >>  
> >>  		check_set_time_of_day();
> >>  
> >> +	} else if (strcmp(argv[1], "xics") == 0) {
> >> +
> > 
> > Superfluous empty line?
> > 
> >> +		check_xics();
> >> +
> 
> This is a standard pattern in kvm-unit-tests:
> 
> ...
>         } else if (strcmp(argv[1], "XXXX") == 0) {
> 
>                 function(...);
> 
>         } else if (strcmp(argv[1], "YYY") == 0) {
> ...
> 

I think that pattern is primarily used in arm/selftest.c and
powerpc/selftest.c, which I'm to blame for it. I found it easier to read
that way, but do agree it's not a typical use of whitespace. Anyway, to
me, it's not a big deal either way. I wouldn't post a patch removing those
blank lines, but I also wouldn't request them to be added to patches that
don't have them...

Thanks,
drew

Re: [RFC kvm-unit-tests PATCH] powerpc: add tests for XICS

[Andrew Jones]

On Mon, Oct 16, 2017 at 07:08:03PM +0200, Laurent Vivier wrote:
> On 13/10/2017 11:45, Thomas Huth wrote:
> > On 12.10.2017 10:07, Laurent Vivier wrote:
> >> Check if we can set the xive server and priority, and
> >> check we get values that have been set.
> >> Check we disable/enable interrupts.
> >>
> >> This patch also increases NR_CPUS from 8 to 16
> >> (maximum for KVM on POWER9, POWER8 allows 96)
> >>
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >> ---
> >> Note: I send this as an RFC, because even if this test works well with
> >> TCG and POWER8 KVM hosts, it detects some problems with POWER9 KVM hosts
> > 
> > What kind of errors does it detect? Are they expected due to the
> > different interrupt controllers?
> > 
> >> main(int argc, char **argv)
> >>  
> >>  		check_set_time_of_day();
> >>  
> >> +	} else if (strcmp(argv[1], "xics") = 0) {
> >> +
> > 
> > Superfluous empty line?
> > 
> >> +		check_xics();
> >> +
> 
> This is a standard pattern in kvm-unit-tests:
> 
> ...
>         } else if (strcmp(argv[1], "XXXX") = 0) {
> 
>                 function(...);
> 
>         } else if (strcmp(argv[1], "YYY") = 0) {
> ...
> 

I think that pattern is primarily used in arm/selftest.c and
powerpc/selftest.c, which I'm to blame for it. I found it easier to read
that way, but do agree it's not a typical use of whitespace. Anyway, to
me, it's not a big deal either way. I wouldn't post a patch removing those
blank lines, but I also wouldn't request them to be added to patches that
don't have them...

Thanks,
drew

Re: [RFC kvm-unit-tests PATCH] powerpc: add tests for XICS

[Laurent Vivier]

On 16/10/2017 14:34, Laurent Vivier wrote:
> On 13/10/2017 11:45, Thomas Huth wrote:
>> On 12.10.2017 10:07, Laurent Vivier wrote:
>>> Check if we can set the xive server and priority, and
>>> check we get values that have been set.
>>> Check we disable/enable interrupts.
>>>
>>> This patch also increases NR_CPUS from 8 to 16
>>> (maximum for KVM on POWER9, POWER8 allows 96)
>>>
>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>> ---
>>> Note: I send this as an RFC, because even if this test works well with
>>> TCG and POWER8 KVM hosts, it detects some problems with POWER9 KVM hosts
>>
>> What kind of errors does it detect? Are they expected due to the
>> different interrupt controllers?
>>
> 
> [I cc' Sam as he already did a fix for this part in the kernel]

I finally found some time to debug this.

> I have two kinds of error:
> 
> - "xics: get-xive: irq #4351, expected cpu 15 prio 255, had cpu 0 prio
>    255",
>   Priority 255 is set to disable the IRQs, I don't think it's really a
>   problem at this level, it could be only cosmetic,

In this case, we have set priority to 255 with set-xive, and in the
emulation, 255 is the "MASKED" value and in this case set-xive doesn't
store the interrupt server, so get-xive always returns an invalid server.

> - "xics: int-on: irq #4351, ret = -3",
>   but here it becomes more serious: as the IRQ server has been lost, it
>   seems we are not able to re-enable the interrupt.

int-on explicitly checks the priority value, and if the value is
"MASKED" (255, the value we set previously with set-xive) it returns an
error.

These errors can be fixed by two different ways:

A- don't allow set-xive to set the priority to 255 (the MASKED value)
   and return an error value, and get-xive return an error value if
   priority is the MASKED value

B- store the server in set-xive even it the value is the MASKED one, so
we can get it with get-xive and use it with int-on (with the saved
priority value)

(A) follows the specs:

Linux on Power Architecture Platform Reference,  v1.1
R1–7.3.10.2–5. For the PowerPC External Interrupt option: The
               ibm,set-xive call must return the Status of -3
               (Argument Error) for an unimplemented Interrupt number.

(B) would behave like XICS with P8 and TCG.

Any ideas?

Thanks,
Laurent

Re: [RFC kvm-unit-tests PATCH] powerpc: add tests for XICS

[Laurent Vivier]

On 16/10/2017 14:34, Laurent Vivier wrote:
> On 13/10/2017 11:45, Thomas Huth wrote:
>> On 12.10.2017 10:07, Laurent Vivier wrote:
>>> Check if we can set the xive server and priority, and
>>> check we get values that have been set.
>>> Check we disable/enable interrupts.
>>>
>>> This patch also increases NR_CPUS from 8 to 16
>>> (maximum for KVM on POWER9, POWER8 allows 96)
>>>
>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>> ---
>>> Note: I send this as an RFC, because even if this test works well with
>>> TCG and POWER8 KVM hosts, it detects some problems with POWER9 KVM hosts
>>
>> What kind of errors does it detect? Are they expected due to the
>> different interrupt controllers?
>>
> 
> [I cc' Sam as he already did a fix for this part in the kernel]

I finally found some time to debug this.

> I have two kinds of error:
> 
> - "xics: get-xive: irq #4351, expected cpu 15 prio 255, had cpu 0 prio
>    255",
>   Priority 255 is set to disable the IRQs, I don't think it's really a
>   problem at this level, it could be only cosmetic,

In this case, we have set priority to 255 with set-xive, and in the
emulation, 255 is the "MASKED" value and in this case set-xive doesn't
store the interrupt server, so get-xive always returns an invalid server.

> - "xics: int-on: irq #4351, ret = -3",
>   but here it becomes more serious: as the IRQ server has been lost, it
>   seems we are not able to re-enable the interrupt.

int-on explicitly checks the priority value, and if the value is
"MASKED" (255, the value we set previously with set-xive) it returns an
error.

These errors can be fixed by two different ways:

A- don't allow set-xive to set the priority to 255 (the MASKED value)
   and return an error value, and get-xive return an error value if
   priority is the MASKED value

B- store the server in set-xive even it the value is the MASKED one, so
we can get it with get-xive and use it with int-on (with the saved
priority value)

(A) follows the specs:

Linux on Power Architecture Platform Reference,  v1.1
R1–7.3.10.2–5. For the PowerPC External Interrupt option: The
               ibm,set-xive call must return the Status of -3
               (Argument Error) for an unimplemented Interrupt number.

(B) would behave like XICS with P8 and TCG.

Any ideas?

Thanks,
Laurent

Re: [RFC kvm-unit-tests PATCH] powerpc: add tests for XICS

[Benjamin Herrenschmidt]

On Wed, 2017-11-22 at 13:39 +0100, Laurent Vivier wrote:
> These errors can be fixed by two different ways:
> 
> A- don't allow set-xive to set the priority to 255 (the MASKED value)
>    and return an error value, and get-xive return an error value if
>    priority is the MASKED value
> 
> B- store the server in set-xive even it the value is the MASKED one, so
> we can get it with get-xive and use it with int-on (with the saved
> priority value)
> 
> (A) follows the specs:
> 
> Linux on Power Architecture Platform Reference,  v1.1
> R1–7.3.10.2–5. For the PowerPC External Interrupt option: The
>                ibm,set-xive call must return the Status of -3
>                (Argument Error) for an unimplemented Interrupt number.
> 
> (B) would behave like XICS with P8 and TCG.
> 
> Any ideas?

I don't see much point in supporting an unbalance where somebody
masks with set_xive and unmasks with int_on...

These PAPR APIs originates from pre-historical times, it's a bit
of a mess.

set_xive should be able to set a priority of 255, that's a
requirement, but we could fix the emulation to store the server
in that case I suppose.


Ben.

Re: [RFC kvm-unit-tests PATCH] powerpc: add tests for XICS

[Benjamin Herrenschmidt]

On Wed, 2017-11-22 at 13:39 +0100, Laurent Vivier wrote:
> These errors can be fixed by two different ways:
> 
> A- don't allow set-xive to set the priority to 255 (the MASKED value)
>    and return an error value, and get-xive return an error value if
>    priority is the MASKED value
> 
> B- store the server in set-xive even it the value is the MASKED one, so
> we can get it with get-xive and use it with int-on (with the saved
> priority value)
> 
> (A) follows the specs:
> 
> Linux on Power Architecture Platform Reference,  v1.1
> R1–7.3.10.2–5. For the PowerPC External Interrupt option: The
>                ibm,set-xive call must return the Status of -3
>                (Argument Error) for an unimplemented Interrupt number.
> 
> (B) would behave like XICS with P8 and TCG.
> 
> Any ideas?

I don't see much point in supporting an unbalance where somebody
masks with set_xive and unmasks with int_on...

These PAPR APIs originates from pre-historical times, it's a bit
of a mess.

set_xive should be able to set a priority of 255, that's a
requirement, but we could fix the emulation to store the server
in that case I suppose.


Ben.