diff mbox series

[kvm-unit-tests,6/6] s390x: SMP test

Message ID 20190829121459.1708-7-frankja@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390x: Add multiboot and smp | expand

Commit Message

Janosch Frank Aug. 29, 2019, 12:14 p.m. UTC 
Testing SIGP emulation for the following order codes:
* start
* stop
* restart
* set prefix
* store status
* stop and store status
* reset
* initial reset
* external call
* emegergency call

restart and set prefix are part of the library and needed to start
other cpus.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/Makefile |   1 +
 s390x/smp.c    | 242 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 243 insertions(+)
 create mode 100644 s390x/smp.c

Comments

Thomas Huth Sept. 2, 2019, 3:40 p.m. UTC | #1 
On 29/08/2019 14.14, Janosch Frank wrote:
> Testing SIGP emulation for the following order codes:
> * start
> * stop
> * restart
> * set prefix
> * store status
> * stop and store status
> * reset
> * initial reset
> * external call
> * emegergency call
> 
> restart and set prefix are part of the library and needed to start
> other cpus.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/Makefile |   1 +
>  s390x/smp.c    | 242 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 243 insertions(+)
>  create mode 100644 s390x/smp.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index d83dd0b..3744372 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -15,6 +15,7 @@ tests += $(TEST_DIR)/cpumodel.elf
>  tests += $(TEST_DIR)/diag288.elf
>  tests += $(TEST_DIR)/stsi.elf
>  tests += $(TEST_DIR)/skrf.elf
> +tests += $(TEST_DIR)/smp.elf
>  tests_binary = $(patsubst %.elf,%.bin,$(tests))
>  
>  all: directories test_cases test_cases_binary
> diff --git a/s390x/smp.c b/s390x/smp.c
> new file mode 100644
> index 0000000..9363cd2
> --- /dev/null
> +++ b/s390x/smp.c
> @@ -0,0 +1,242 @@
> +/*
> + * Tests sigp emulation
> + *
> + * Copyright 2019 IBM Corp.
> + *
> + * Authors:
> + *    Janosch Frank <frankja@linux.ibm.com>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2.
> + */
> +#include <libcflat.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/interrupt.h>
> +#include <asm/page.h>
> +#include <asm/facility.h>
> +#include <asm-generic/barrier.h>
> +#include <asm/sigp.h>
> +
> +#include <smp.h>
> +#include <alloc_page.h>
> +
> +static int t = 0;

Single letter variables that are used accross functions are a little bit
ugly. I'd maybe give this a better name, like "testflag" or something
similar?

> +static void cpu_loop(void)
> +{
> +	for (;;) {}
> +}
> +
> +static void test_func(void)
> +{
> +	t = 1;

I think I'd rather place a mb() here, just to be sure...?

> +	cpu_loop();
> +}
> +
> +static void test_start(void)
> +{
> +	struct psw psw;
> +	psw.mask =  extract_psw_mask();
> +	psw.addr = (unsigned long)test_func;
> +
> +	smp_cpu_setup(1, psw);
> +	while (!t) {
> +		mb();
> +	}
> +	report("start", 1);
> +}
> +
> +static void test_stop(void)
> +{
> +	int i = 0;
> +
> +	smp_cpu_stop(1);
> +	/*
> +	 * The smp library waits for the CPU to shut down, but let's
> +	 * also do it here, so we don't rely on the library
> +	 * implementation
> +	 */
> +	while (!smp_cpu_stopped(1)) {}
> +	t = 0;
> +	/* Let's leave some time for cpu #2 to change t */

CPU #2 ? Where? Why?

> +	for (; i < 0x100000; i++) {}

I'm pretty sure the compiler optimizes empty loops away.

> +	report("stop", !t);
> +}
> +
> +static void test_stop_store_status(void)
> +{
> +	struct cpu *cpu = smp_cpu_from_addr(1);
> +	struct lowcore *lc = (void *)0x0;

Do you want to erase the values in the save area before calling the
"store_status"? ... just to be sure that we don't see old values there?

> +	smp_cpu_stop_store_status(1);
> +	mb();
> +	report("stop store status",
> +	       lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore);

That confused me. Why does the prefix_sa of the lowcore of CPU 0 match
the prefix of CPU 1 ? I'd rather expect cpu->lowcore->prefix_sa to
contain this value?

Maybe you could also check that at least the stack pointer GPR is != 0
in the save area?

> +}
> +
> +static void test_store_status(void)
> +{
> +	struct cpu_status *status = alloc_pages(0);
> +	uint32_t r;
> +
> +	report_prefix_push("status");
> +	memset(status, 0, PAGE_SIZE);
> +
> +	smp_cpu_restart(1);
> +	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, &r);
> +	report("not stopped", r == SIGP_STATUS_INCORRECT_STATE);

Maybe also check that the save are is still 0?

> +	memset(status, 0, PAGE_SIZE);
> +	smp_cpu_stop(1);
> +	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
> +	while (!status->prefix) {}

status->prefix is not marked as volatile, so please put a "mb()" into
the curly braces here.

> +	report("store status", 1);
> +	free_pages(status, PAGE_SIZE);
> +	report_prefix_pop();
> +}
> +
> +static void ecall(void)
> +{
> +	unsigned long mask;
> +	struct lowcore *lc = (void *)0x0;
> +
> +	ctl_set_bit(0, 13);
> +	mask = extract_psw_mask();
> +	mask |= PSW_MASK_EXT;
> +	load_psw_mask(mask);
> +	expect_ext_int();

I think you should move the expect_ext_int() before the enablement of
the interrupt, to avoid races?

> +	t = 1;
> +	while (lc->ext_int_code != 0x1202) {mb();}

Spaces around the "mb();", please.

> +	report("ecall", 1);
> +	t = 1;
> +}
> +
> +static void test_ecall(void)
> +{
> +	struct psw psw;
> +	psw.mask =  extract_psw_mask();
> +	psw.addr = (unsigned long)ecall;
> +
> +	report_prefix_push("ecall");
> +	t = 0;
> +	smp_cpu_destroy(1);
> +
> +	mb();

Why this mb() here?

> +	smp_cpu_setup(1, psw);
> +	while (!t) {
> +		mb();
> +	}
> +	t = 0;
> +	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
> +	while(!t) {mb();}

Spaces, please.

> +	smp_cpu_stop(1);
> +	report_prefix_pop();
> +}
> +
> +static void emcall(void)
> +{
> +	unsigned long mask;
> +	struct lowcore *lc = (void *)0x0;
> +
> +	ctl_set_bit(0, 14);
> +	mask = extract_psw_mask();
> +	mask |= PSW_MASK_EXT;
> +	load_psw_mask(mask);
> +	expect_ext_int();

I think you should move the expect_ext_int() before the enablement of
the interrupt, to avoid races?

> +	t = 1;
> +	while (lc->ext_int_code != 0x1201) {mb();}

Spaces.

> +	report("ecall", 1);
> +	t = 1;
> +}
> +
> +static void test_emcall(void)
> +{
> +	struct psw psw;
> +	psw.mask =  extract_psw_mask();
> +	psw.addr = (unsigned long)emcall;
> +
> +	report_prefix_push("emcall");
> +	t = 0;
> +	smp_cpu_destroy(1);
> +
> +	mb();
> +	smp_cpu_setup(1, psw);
> +	while (!t) {
> +		mb();
> +	}
> +	t = 0;
> +	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
> +	while(!t) {mb();}

Spaces.

> +	smp_cpu_stop(1);
> +	report_prefix_pop();
> +}
> +
> +static void test_reset_initial(void)
> +{
> +	struct cpu_status *status = alloc_pages(0);
> +	struct psw psw;
> +
> +	psw.mask =  extract_psw_mask();
> +	psw.addr = (unsigned long)test_func;
> +
> +	report_prefix_push("reset initial");
> +	smp_cpu_setup(1, psw);
> +
> +	sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
> +	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
> +
> +	report_prefix_push("clear");
> +	report("psw", !status->psw.mask && !status->psw.addr);
> +	report("prefix", !status->prefix);
> +	report("fpc", !status->fpc);
> +	report("cpu timer", !status->cputm);
> +	report("todpr", !status->todpr);
> +	report_prefix_pop();
> +
> +	report_prefix_push("initialized");
> +	report("cr0 == 0xE0", status->crs[0] == 0xE0UL);
> +	report("cr14 == 0xC2000000", status->crs[14] == 0xC2000000UL);
> +	report_prefix_pop();
> +
> +	report("cpu stopped", smp_cpu_stopped(1));
> +	free_pages(status, PAGE_SIZE);
> +	report_prefix_pop();
> +}
> +
> +static void test_reset(void)
> +{
> +	struct psw psw;
> +
> +	psw.mask =  extract_psw_mask();
> +	psw.addr = (unsigned long)test_func;
> +
> +	report_prefix_push("cpu reset");
> +	smp_cpu_setup(1, psw);
> +
> +	sigp_retry(1, SIGP_CPU_RESET, 0, NULL);
> +	report("cpu stopped", smp_cpu_stopped(1));
> +	report_prefix_pop();
> +}
> +
> +int main(void)
> +{
> +	report_prefix_push("smp");
> +
> +	if (smp_query_num_cpus() == 1) {
> +		report_abort("need at least 2 cpus for this test");
> +		goto done;
> +	}
> +
> +	test_start();
> +	test_stop();
> +	test_stop_store_status();
> +	test_store_status();
> +	test_ecall();
> +	test_emcall();
> +	test_reset();
> +	test_reset_initial();
> +
> +done:
> +	report_prefix_pop();
> +	return report_summary();
> +}
> 

 Thomas
Janosch Frank Sept. 3, 2019, 8:44 a.m. UTC | #2 
On 9/2/19 5:40 PM, Thomas Huth wrote:
> On 29/08/2019 14.14, Janosch Frank wrote:
>> Testing SIGP emulation for the following order codes:
>> * start
>> * stop
>> * restart
>> * set prefix
>> * store status
>> * stop and store status
>> * reset
>> * initial reset
>> * external call
>> * emegergency call
>>
>> restart and set prefix are part of the library and needed to start
>> other cpus.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
[...]
>> +static void test_stop(void)
>> +{
>> +	int i = 0;
>> +
>> +	smp_cpu_stop(1);
>> +	/*
>> +	 * The smp library waits for the CPU to shut down, but let's
>> +	 * also do it here, so we don't rely on the library
>> +	 * implementation
>> +	 */
>> +	while (!smp_cpu_stopped(1)) {}
>> +	t = 0;
>> +	/* Let's leave some time for cpu #2 to change t */
> 
> CPU #2 ? Where? Why?
> 
>> +	for (; i < 0x100000; i++) {}
> 
> I'm pretty sure the compiler optimizes empty loops away.

Yeah, I removed all of that...

> 
>> +	report("stop", !t);
>> +}
>> +
>> +static void test_stop_store_status(void)
>> +{
>> +	struct cpu *cpu = smp_cpu_from_addr(1);
>> +	struct lowcore *lc = (void *)0x0;
> 
> Do you want to erase the values in the save area before calling the
> "store_status"? ... just to be sure that we don't see old values there?

Well at least resetting the prefix and gr15 to 0

> 
>> +	smp_cpu_stop_store_status(1);
>> +	mb();
>> +	report("stop store status",
>> +	       lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore);
> 
> That confused me. Why does the prefix_sa of the lowcore of CPU 0 match
> the prefix of CPU 1 ? I'd rather expect cpu->lowcore->prefix_sa to
> contain this value?

Store status saves at absolute 0, i.e. we get the status in cpu0's lowcore.

> 
> Maybe you could also check that at least the stack pointer GPR is != 0
> in the save area?

Sure, I also fixed everything below
Thomas Huth Sept. 3, 2019, 8:56 a.m. UTC | #3 
On 03/09/2019 10.44, Janosch Frank wrote:
> On 9/2/19 5:40 PM, Thomas Huth wrote:
>> On 29/08/2019 14.14, Janosch Frank wrote:
[...]
>>> +	smp_cpu_stop_store_status(1);
>>> +	mb();
>>> +	report("stop store status",
>>> +	       lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore);
>>
>> That confused me. Why does the prefix_sa of the lowcore of CPU 0 match
>> the prefix of CPU 1 ? I'd rather expect cpu->lowcore->prefix_sa to
>> contain this value?
> 
> Store status saves at absolute 0, i.e. we get the status in cpu0's lowcore.

Ah, now that you mention it, the PoP indeed talks about "absolut"
locations here. TIL, thanks for the explanation!

 Thomas
diff mbox series

Patch

diff --git a/s390x/Makefile b/s390x/Makefile
index d83dd0b..3744372 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -15,6 +15,7 @@  tests += $(TEST_DIR)/cpumodel.elf
 tests += $(TEST_DIR)/diag288.elf
 tests += $(TEST_DIR)/stsi.elf
 tests += $(TEST_DIR)/skrf.elf
+tests += $(TEST_DIR)/smp.elf
 tests_binary = $(patsubst %.elf,%.bin,$(tests))
 
 all: directories test_cases test_cases_binary
diff --git a/s390x/smp.c b/s390x/smp.c
new file mode 100644
index 0000000..9363cd2
--- /dev/null
+++ b/s390x/smp.c
@@ -0,0 +1,242 @@ 
+/*
+ * Tests sigp emulation
+ *
+ * Copyright 2019 IBM Corp.
+ *
+ * Authors:
+ *    Janosch Frank <frankja@linux.ibm.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2.
+ */
+#include <libcflat.h>
+#include <asm/asm-offsets.h>
+#include <asm/interrupt.h>
+#include <asm/page.h>
+#include <asm/facility.h>
+#include <asm-generic/barrier.h>
+#include <asm/sigp.h>
+
+#include <smp.h>
+#include <alloc_page.h>
+
+static int t = 0;
+
+static void cpu_loop(void)
+{
+	for (;;) {}
+}
+
+static void test_func(void)
+{
+	t = 1;
+	cpu_loop();
+}
+
+static void test_start(void)
+{
+	struct psw psw;
+	psw.mask =  extract_psw_mask();
+	psw.addr = (unsigned long)test_func;
+
+	smp_cpu_setup(1, psw);
+	while (!t) {
+		mb();
+	}
+	report("start", 1);
+}
+
+static void test_stop(void)
+{
+	int i = 0;
+
+	smp_cpu_stop(1);
+	/*
+	 * The smp library waits for the CPU to shut down, but let's
+	 * also do it here, so we don't rely on the library
+	 * implementation
+	 */
+	while (!smp_cpu_stopped(1)) {}
+	t = 0;
+	/* Let's leave some time for cpu #2 to change t */
+	for (; i < 0x100000; i++) {}
+	report("stop", !t);
+}
+
+static void test_stop_store_status(void)
+{
+	struct cpu *cpu = smp_cpu_from_addr(1);
+	struct lowcore *lc = (void *)0x0;
+
+	smp_cpu_stop_store_status(1);
+	mb();
+	report("stop store status",
+	       lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore);
+}
+
+static void test_store_status(void)
+{
+	struct cpu_status *status = alloc_pages(0);
+	uint32_t r;
+
+	report_prefix_push("status");
+	memset(status, 0, PAGE_SIZE);
+
+	smp_cpu_restart(1);
+	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, &r);
+	report("not stopped", r == SIGP_STATUS_INCORRECT_STATE);
+
+	memset(status, 0, PAGE_SIZE);
+	smp_cpu_stop(1);
+	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
+	while (!status->prefix) {}
+	report("store status", 1);
+	free_pages(status, PAGE_SIZE);
+	report_prefix_pop();
+}
+
+static void ecall(void)
+{
+	unsigned long mask;
+	struct lowcore *lc = (void *)0x0;
+
+	ctl_set_bit(0, 13);
+	mask = extract_psw_mask();
+	mask |= PSW_MASK_EXT;
+	load_psw_mask(mask);
+	expect_ext_int();
+	t = 1;
+	while (lc->ext_int_code != 0x1202) {mb();}
+	report("ecall", 1);
+	t = 1;
+}
+
+static void test_ecall(void)
+{
+	struct psw psw;
+	psw.mask =  extract_psw_mask();
+	psw.addr = (unsigned long)ecall;
+
+	report_prefix_push("ecall");
+	t = 0;
+	smp_cpu_destroy(1);
+
+	mb();
+	smp_cpu_setup(1, psw);
+	while (!t) {
+		mb();
+	}
+	t = 0;
+	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
+	while(!t) {mb();}
+	smp_cpu_stop(1);
+	report_prefix_pop();
+}
+
+static void emcall(void)
+{
+	unsigned long mask;
+	struct lowcore *lc = (void *)0x0;
+
+	ctl_set_bit(0, 14);
+	mask = extract_psw_mask();
+	mask |= PSW_MASK_EXT;
+	load_psw_mask(mask);
+	expect_ext_int();
+	t = 1;
+	while (lc->ext_int_code != 0x1201) {mb();}
+	report("ecall", 1);
+	t = 1;
+}
+
+static void test_emcall(void)
+{
+	struct psw psw;
+	psw.mask =  extract_psw_mask();
+	psw.addr = (unsigned long)emcall;
+
+	report_prefix_push("emcall");
+	t = 0;
+	smp_cpu_destroy(1);
+
+	mb();
+	smp_cpu_setup(1, psw);
+	while (!t) {
+		mb();
+	}
+	t = 0;
+	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
+	while(!t) {mb();}
+	smp_cpu_stop(1);
+	report_prefix_pop();
+}
+
+static void test_reset_initial(void)
+{
+	struct cpu_status *status = alloc_pages(0);
+	struct psw psw;
+
+	psw.mask =  extract_psw_mask();
+	psw.addr = (unsigned long)test_func;
+
+	report_prefix_push("reset initial");
+	smp_cpu_setup(1, psw);
+
+	sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
+	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
+
+	report_prefix_push("clear");
+	report("psw", !status->psw.mask && !status->psw.addr);
+	report("prefix", !status->prefix);
+	report("fpc", !status->fpc);
+	report("cpu timer", !status->cputm);
+	report("todpr", !status->todpr);
+	report_prefix_pop();
+
+	report_prefix_push("initialized");
+	report("cr0 == 0xE0", status->crs[0] == 0xE0UL);
+	report("cr14 == 0xC2000000", status->crs[14] == 0xC2000000UL);
+	report_prefix_pop();
+
+	report("cpu stopped", smp_cpu_stopped(1));
+	free_pages(status, PAGE_SIZE);
+	report_prefix_pop();
+}
+
+static void test_reset(void)
+{
+	struct psw psw;
+
+	psw.mask =  extract_psw_mask();
+	psw.addr = (unsigned long)test_func;
+
+	report_prefix_push("cpu reset");
+	smp_cpu_setup(1, psw);
+
+	sigp_retry(1, SIGP_CPU_RESET, 0, NULL);
+	report("cpu stopped", smp_cpu_stopped(1));
+	report_prefix_pop();
+}
+
+int main(void)
+{
+	report_prefix_push("smp");
+
+	if (smp_query_num_cpus() == 1) {
+		report_abort("need at least 2 cpus for this test");
+		goto done;
+	}
+
+	test_start();
+	test_stop();
+	test_stop_store_status();
+	test_store_status();
+	test_ecall();
+	test_emcall();
+	test_reset();
+	test_reset_initial();
+
+done:
+	report_prefix_pop();
+	return report_summary();
+}