| Message ID | 20171012080704.31037-1-lvivier@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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.
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
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(-)