[kvm-unit-tests PATCH v1 0/5] s390x: smp: avoid hardcoded CPU addresses

[kvm-unit-tests PATCH v1 0/5] s390x: smp: avoid hardcoded CPU addresses

[Claudio Imbrenda]

On s390x there are no guarantees about the CPU addresses, except that
they shall be unique. This means that in some environments, it's
possible that there is no match between the CPU address and its
position (index) in the list of available CPUs returned by the system.

This series fixes a small bug in the SMP initialization code, adds a
guarantee that the boot CPU will always have index 0, and introduces
some functions to allow tests to use CPU indexes instead of using
hardcoded CPU addresses. This will allow the tests to run successfully
in more environments (e.g. z/VM, LPAR).

Some existing tests are adapted to take advantage of the new
functionalities.

Claudio Imbrenda (5):
  lib: s390x: smp: add functions to work with CPU indexes
  lib: s390x: smp: guarantee that boot CPU has index 0
  s390x: smp: avoid hardcoded CPU addresses
  s390x: firq: avoid hardcoded CPU addresses
  s390x: skrf: avoid hardcoded CPU addresses

 lib/s390x/smp.h |  2 ++
 lib/s390x/smp.c | 28 ++++++++++++-----
 s390x/firq.c    | 17 +++++-----
 s390x/skrf.c    |  8 +++--
 s390x/smp.c     | 83 ++++++++++++++++++++++++++-----------------------
 5 files changed, 79 insertions(+), 59 deletions(-)

-- 
2.34.1

[kvm-unit-tests PATCH v1 1/5] lib: s390x: smp: add functions to work with CPU indexes

[Claudio Imbrenda]

Knowing the number of active CPUs is not enough to know which ones are
active. This patch adds 2 new functions:

* smp_cpu_addr_from_idx to get the CPU address from the index
* smp_cpu_from_idx allows to retrieve the struct cpu from the index

This makes it possible for tests to avoid hardcoding the CPU addresses.
It is useful in cases where the address and the index might not match.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/s390x/smp.h |  2 ++
 lib/s390x/smp.c | 12 ++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
index a2609f11..69aa4003 100644
--- a/lib/s390x/smp.h
+++ b/lib/s390x/smp.h
@@ -37,6 +37,7 @@ struct cpu_status {
 
 int smp_query_num_cpus(void);
 struct cpu *smp_cpu_from_addr(uint16_t addr);
+struct cpu *smp_cpu_from_idx(uint16_t addr);
 bool smp_cpu_stopped(uint16_t addr);
 bool smp_sense_running_status(uint16_t addr);
 int smp_cpu_restart(uint16_t addr);
@@ -47,5 +48,6 @@ int smp_cpu_destroy(uint16_t addr);
 int smp_cpu_setup(uint16_t addr, struct psw psw);
 void smp_teardown(void);
 void smp_setup(void);
+uint16_t smp_cpu_addr_from_idx(uint16_t idx);
 
 #endif
diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index b753eab5..64c647ec 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -46,6 +46,18 @@ struct cpu *smp_cpu_from_addr(uint16_t addr)
 	return NULL;
 }
 
+struct cpu *smp_cpu_from_idx(uint16_t idx)
+{
+	assert(idx < smp_query_num_cpus());
+	return &cpus[idx];
+}
+
+uint16_t smp_cpu_addr_from_idx(uint16_t idx)
+{
+	assert(idx < smp_query_num_cpus());
+	return cpus[idx].addr;
+}
+
 bool smp_cpu_stopped(uint16_t addr)
 {
 	uint32_t status;
-- 
2.34.1

[kvm-unit-tests PATCH v1 2/5] lib: s390x: smp: guarantee that boot CPU has index 0

[Claudio Imbrenda]

Guarantee that the boot CPU has index 0. This simplifies the
implementation of tests that require multiple CPUs.

Also fix a small bug in the allocation of the cpus array.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Fixes: f77c0515 ("s390x: Add initial smp code")
Fixes: 52076a63 ("s390x: Consolidate sclp read info")
---
 lib/s390x/smp.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index 64c647ec..01f513f0 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -25,7 +25,6 @@
 #include "sclp.h"
 
 static struct cpu *cpus;
-static struct cpu *cpu0;
 static struct spinlock lock;
 
 extern void smp_cpu_setup_state(void);
@@ -81,7 +80,7 @@ static int smp_cpu_stop_nolock(uint16_t addr, bool store)
 	uint8_t order = store ? SIGP_STOP_AND_STORE_STATUS : SIGP_STOP;
 
 	cpu = smp_cpu_from_addr(addr);
-	if (!cpu || cpu == cpu0)
+	if (!cpu || addr == cpus[0].addr)
 		return -1;
 
 	if (sigp_retry(addr, order, 0, NULL))
@@ -205,7 +204,7 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
 	sigp_retry(cpu->addr, SIGP_SET_PREFIX, (unsigned long )lc, NULL);
 
 	/* Copy all exception psws. */
-	memcpy(lc, cpu0->lowcore, 512);
+	memcpy(lc, cpus[0].lowcore, 512);
 
 	/* Setup stack */
 	cpu->stack = (uint64_t *)alloc_pages(2);
@@ -263,15 +262,16 @@ void smp_setup(void)
 	if (num > 1)
 		printf("SMP: Initializing, found %d cpus\n", num);
 
-	cpus = calloc(num, sizeof(cpus));
+	cpus = calloc(num, sizeof(*cpus));
 	for (i = 0; i < num; i++) {
 		cpus[i].addr = entry[i].address;
 		cpus[i].active = false;
 		if (entry[i].address == cpu0_addr) {
-			cpu0 = &cpus[i];
-			cpu0->stack = stackptr;
-			cpu0->lowcore = (void *)0;
-			cpu0->active = true;
+			cpus[i].addr = cpus[0].addr;
+			cpus[0].addr = cpu0_addr;
+			cpus[0].stack = stackptr;
+			cpus[0].lowcore = (void *)0;
+			cpus[0].active = true;
 		}
 	}
 	spin_unlock(&lock);
-- 
2.34.1

[kvm-unit-tests PATCH v1 3/5] s390x: smp: avoid hardcoded CPU addresses

[Claudio Imbrenda]

Use the recently introduced functions to work with CPU indexes, instead of
using hardcoded values. This makes the test more portable.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 s390x/smp.c | 83 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 44 insertions(+), 39 deletions(-)

diff --git a/s390x/smp.c b/s390x/smp.c
index 1bbe4c31..da668ca6 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -18,6 +18,7 @@
 #include <smp.h>
 #include <alloc_page.h>
 
+static uint16_t cpu0, cpu1;
 static int testflag = 0;
 
 static void wait_for_flag(void)
@@ -45,7 +46,7 @@ static void test_start(void)
 	psw.addr = (unsigned long)test_func;
 
 	set_flag(0);
-	smp_cpu_start(1, psw);
+	smp_cpu_start(cpu1, psw);
 	wait_for_flag();
 	report_pass("start");
 }
@@ -56,16 +57,16 @@ static void test_start(void)
  */
 static void test_restart(void)
 {
-	struct cpu *cpu = smp_cpu_from_addr(1);
+	struct cpu *cpu = smp_cpu_from_idx(1);
 	struct lowcore *lc = cpu->lowcore;
 
 	lc->restart_new_psw.mask = extract_psw_mask();
 	lc->restart_new_psw.addr = (unsigned long)test_func;
 
 	/* Make sure cpu is running */
-	smp_cpu_stop(0);
+	smp_cpu_stop(cpu0);
 	set_flag(0);
-	smp_cpu_restart(1);
+	smp_cpu_restart(cpu1);
 	wait_for_flag();
 
 	/*
@@ -73,44 +74,44 @@ static void test_restart(void)
 	 * restart function.
 	 */
 	set_flag(0);
-	smp_cpu_restart(1);
+	smp_cpu_restart(cpu1);
 	wait_for_flag();
 	report_pass("restart while running");
 }
 
 static void test_stop(void)
 {
-	smp_cpu_stop(1);
+	smp_cpu_stop(cpu1);
 	/*
 	 * 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)) {}
+	while (!smp_cpu_stopped(cpu1)) {}
 	report_pass("stop");
 }
 
 static void test_stop_store_status(void)
 {
-	struct cpu *cpu = smp_cpu_from_addr(1);
+	struct cpu *cpu = smp_cpu_from_idx(1);
 	struct lowcore *lc = (void *)0x0;
 
 	report_prefix_push("stop store status");
 	report_prefix_push("running");
-	smp_cpu_restart(1);
+	smp_cpu_restart(cpu1);
 	lc->prefix_sa = 0;
 	lc->grs_sa[15] = 0;
-	smp_cpu_stop_store_status(1);
+	smp_cpu_stop_store_status(cpu1);
 	mb();
 	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
 	report(lc->grs_sa[15], "stack");
-	report(smp_cpu_stopped(1), "cpu stopped");
+	report(smp_cpu_stopped(cpu1), "cpu stopped");
 	report_prefix_pop();
 
 	report_prefix_push("stopped");
 	lc->prefix_sa = 0;
 	lc->grs_sa[15] = 0;
-	smp_cpu_stop_store_status(1);
+	smp_cpu_stop_store_status(cpu1);
 	mb();
 	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
 	report(lc->grs_sa[15], "stack");
@@ -128,8 +129,8 @@ static void test_store_status(void)
 	memset(status, 0, PAGE_SIZE * 2);
 
 	report_prefix_push("running");
-	smp_cpu_restart(1);
-	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, &r);
+	smp_cpu_restart(cpu1);
+	sigp(cpu1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, &r);
 	report(r == SIGP_STATUS_INCORRECT_STATE, "incorrect state");
 	report(!memcmp(status, (void *)status + PAGE_SIZE, PAGE_SIZE),
 	       "status not written");
@@ -137,13 +138,13 @@ static void test_store_status(void)
 
 	memset(status, 0, PAGE_SIZE);
 	report_prefix_push("stopped");
-	smp_cpu_stop(1);
-	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
+	smp_cpu_stop(cpu1);
+	sigp(cpu1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
 	while (!status->prefix) { mb(); }
 	report_pass("status written");
 	free_pages(status);
 	report_prefix_pop();
-	smp_cpu_stop(1);
+	smp_cpu_stop(cpu1);
 
 	report_prefix_pop();
 }
@@ -173,12 +174,12 @@ static void test_ecall(void)
 	report_prefix_push("ecall");
 	set_flag(0);
 
-	smp_cpu_start(1, psw);
+	smp_cpu_start(cpu1, psw);
 	wait_for_flag();
 	set_flag(0);
-	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
+	sigp(cpu1, SIGP_EXTERNAL_CALL, 0, NULL);
 	wait_for_flag();
-	smp_cpu_stop(1);
+	smp_cpu_stop(cpu1);
 	report_prefix_pop();
 }
 
@@ -207,12 +208,12 @@ static void test_emcall(void)
 	report_prefix_push("emcall");
 	set_flag(0);
 
-	smp_cpu_start(1, psw);
+	smp_cpu_start(cpu1, psw);
 	wait_for_flag();
 	set_flag(0);
-	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
+	sigp(cpu1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
 	wait_for_flag();
-	smp_cpu_stop(1);
+	smp_cpu_stop(cpu1);
 	report_prefix_pop();
 }
 
@@ -220,11 +221,11 @@ static void test_sense_running(void)
 {
 	report_prefix_push("sense_running");
 	/* we (CPU0) are running */
-	report(smp_sense_running_status(0), "CPU0 sense claims running");
+	report(smp_sense_running_status(cpu0), "CPU0 sense claims running");
 	/* stop the target CPU (CPU1) to speed up the not running case */
-	smp_cpu_stop(1);
+	smp_cpu_stop(cpu1);
 	/* Make sure to have at least one time with a not running indication */
-	while(smp_sense_running_status(1));
+	while(smp_sense_running_status(cpu1));
 	report_pass("CPU1 sense claims not running");
 	report_prefix_pop();
 }
@@ -250,11 +251,11 @@ static void test_reset_initial(void)
 
 	report_prefix_push("reset initial");
 	set_flag(0);
-	smp_cpu_start(1, psw);
+	smp_cpu_start(cpu1, psw);
 	wait_for_flag();
 
-	sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
-	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
+	sigp_retry(cpu1, SIGP_INITIAL_CPU_RESET, 0, NULL);
+	sigp(cpu1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
 
 	report_prefix_push("clear");
 	report(!status->psw.mask && !status->psw.addr, "psw");
@@ -273,7 +274,7 @@ static void test_reset_initial(void)
 	report(status->crs[14] == 0xC2000000UL, "cr14 == 0xC2000000");
 	report_prefix_pop();
 
-	report(smp_cpu_stopped(1), "cpu stopped");
+	report(smp_cpu_stopped(cpu1), "cpu stopped");
 	free_pages(status);
 	report_prefix_pop();
 }
@@ -299,16 +300,16 @@ static void test_reset(void)
 	psw.addr = (unsigned long)test_func;
 
 	report_prefix_push("cpu reset");
-	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
-	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
-	smp_cpu_start(1, psw);
+	sigp(cpu1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
+	sigp(cpu1, SIGP_EXTERNAL_CALL, 0, NULL);
+	smp_cpu_start(cpu1, psw);
 
-	sigp_retry(1, SIGP_CPU_RESET, 0, NULL);
-	report(smp_cpu_stopped(1), "cpu stopped");
+	sigp_retry(cpu1, SIGP_CPU_RESET, 0, NULL);
+	report(smp_cpu_stopped(cpu1), "cpu stopped");
 
 	set_flag(0);
 	psw.addr = (unsigned long)test_local_ints;
-	smp_cpu_start(1, psw);
+	smp_cpu_start(cpu1, psw);
 	wait_for_flag();
 	report_pass("local interrupts cleared");
 	report_prefix_pop();
@@ -324,11 +325,15 @@ int main(void)
 		goto done;
 	}
 
+	/* the boot CPU is guaranteed to have index 0 */
+	cpu0 = stap();
+	cpu1 = smp_cpu_addr_from_idx(1);
+
 	/* Setting up the cpu to give it a stack and lowcore */
 	psw.mask = extract_psw_mask();
 	psw.addr = (unsigned long)test_func;
-	smp_cpu_setup(1, psw);
-	smp_cpu_stop(1);
+	smp_cpu_setup(cpu1, psw);
+	smp_cpu_stop(cpu1);
 
 	test_start();
 	test_restart();
@@ -340,7 +345,7 @@ int main(void)
 	test_sense_running();
 	test_reset();
 	test_reset_initial();
-	smp_cpu_destroy(1);
+	smp_cpu_destroy(cpu1);
 
 done:
 	report_prefix_pop();
-- 
2.34.1

[kvm-unit-tests PATCH v1 4/5] s390x: firq: avoid hardcoded CPU addresses

[Claudio Imbrenda]

Use the recently introduced smp_cpu_addr_from_idx to discover the
addresses of the CPUs to use in the test, instead of using hardcoded
values. This makes the test more portable.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 s390x/firq.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/s390x/firq.c b/s390x/firq.c
index fb9a2906..14d0d102 100644
--- a/s390x/firq.c
+++ b/s390x/firq.c
@@ -33,6 +33,7 @@ static void wait_for_sclp_int(void)
  */
 static void test_wait_state_delivery(void)
 {
+	uint16_t cpu1, cpu2;
 	struct psw psw;
 	SCCBHeader *h;
 	int ret;
@@ -44,16 +45,14 @@ static void test_wait_state_delivery(void)
 		goto out;
 	}
 
-	if (stap()) {
-		report_skip("need to start on CPU #0");
-		goto out;
-	}
+	cpu1 = smp_cpu_addr_from_idx(1);
+	cpu2 = smp_cpu_addr_from_idx(2);
 
 	/*
 	 * We want CPU #2 to be stopped. This should be the case at this
 	 * point, however, we want to sense if it even exists as well.
 	 */
-	ret = smp_cpu_stop(2);
+	ret = smp_cpu_stop(cpu2);
 	if (ret) {
 		report_skip("CPU #2 not found");
 		goto out;
@@ -68,10 +67,10 @@ static void test_wait_state_delivery(void)
 	/* Start CPU #1 and let it wait for the interrupt. */
 	psw.mask = extract_psw_mask();
 	psw.addr = (unsigned long)wait_for_sclp_int;
-	ret = smp_cpu_setup(1, psw);
+	ret = smp_cpu_setup(cpu1, psw);
 	if (ret) {
 		sclp_clear_busy();
-		report_skip("cpu #1 not found");
+		report_skip("CPU #1 not found");
 		goto out;
 	}
 
@@ -85,7 +84,7 @@ static void test_wait_state_delivery(void)
 	 * will take some time as well and smp_cpu_setup() returns when we're
 	 * either already in wait_for_sclp_int() or just about to execute it.
 	 */
-	while(smp_sense_running_status(1));
+	while(smp_sense_running_status(cpu1));
 
 	h = alloc_pages_flags(0, AREA_DMA31);
 	h->length = 4096;
@@ -106,7 +105,7 @@ static void test_wait_state_delivery(void)
 
 out_destroy:
 	free_page(h);
-	smp_cpu_destroy(1);
+	smp_cpu_destroy(cpu1);
 out:
 	report_prefix_pop();
 }
-- 
2.34.1

[kvm-unit-tests PATCH v1 5/5] s390x: skrf: avoid hardcoded CPU addresses

[Claudio Imbrenda]

Use the recently introduced smp_cpu_addr_from_idx to discover the
addresses of the CPUs to use in the test, instead of using hardcoded
values. This makes the test more portable.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 s390x/skrf.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/s390x/skrf.c b/s390x/skrf.c
index ca4efbf1..bd0abba0 100644
--- a/s390x/skrf.c
+++ b/s390x/skrf.c
@@ -164,6 +164,7 @@ static void test_exception_ext_new(void)
 		.mask = extract_psw_mask(),
 		.addr = (unsigned long)ecall_setup
 	};
+	uint16_t cpu1;
 
 	report_prefix_push("exception external new");
 	if (smp_query_num_cpus() < 2) {
@@ -171,14 +172,15 @@ static void test_exception_ext_new(void)
 		report_prefix_pop();
 		return;
 	}
+	cpu1 = smp_cpu_addr_from_idx(1);
 
-	smp_cpu_setup(1, psw);
+	smp_cpu_setup(cpu1, psw);
 	wait_for_flag();
 	set_flag(0);
 
-	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
+	sigp(cpu1, SIGP_EXTERNAL_CALL, 0, NULL);
 	wait_for_flag();
-	smp_cpu_stop(1);
+	smp_cpu_stop(cpu1);
 	report_prefix_pop();
 }
 
-- 
2.34.1

Re: [kvm-unit-tests PATCH v1 1/5] lib: s390x: smp: add functions to work with CPU indexes

[David Hildenbrand]

On 28.01.22 19:54, Claudio Imbrenda wrote:
> Knowing the number of active CPUs is not enough to know which ones are
> active. This patch adds 2 new functions:
> 
> * smp_cpu_addr_from_idx to get the CPU address from the index
> * smp_cpu_from_idx allows to retrieve the struct cpu from the index
> 
> This makes it possible for tests to avoid hardcoding the CPU addresses.
> It is useful in cases where the address and the index might not match.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  lib/s390x/smp.h |  2 ++
>  lib/s390x/smp.c | 12 ++++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
> index a2609f11..69aa4003 100644
> --- a/lib/s390x/smp.h
> +++ b/lib/s390x/smp.h
> @@ -37,6 +37,7 @@ struct cpu_status {
>  
>  int smp_query_num_cpus(void);
>  struct cpu *smp_cpu_from_addr(uint16_t addr);
> +struct cpu *smp_cpu_from_idx(uint16_t addr);

s/addr/idx/


-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v1 2/5] lib: s390x: smp: guarantee that boot CPU has index 0

[David Hildenbrand]

On 28.01.22 19:54, Claudio Imbrenda wrote:
> Guarantee that the boot CPU has index 0. This simplifies the
> implementation of tests that require multiple CPUs.
> 
> Also fix a small bug in the allocation of the cpus array.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Fixes: f77c0515 ("s390x: Add initial smp code")
> Fixes: 52076a63 ("s390x: Consolidate sclp read info")
> ---
>  lib/s390x/smp.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index 64c647ec..01f513f0 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -25,7 +25,6 @@
>  #include "sclp.h"
>  
>  static struct cpu *cpus;
> -static struct cpu *cpu0;
>  static struct spinlock lock;
>  
>  extern void smp_cpu_setup_state(void);
> @@ -81,7 +80,7 @@ static int smp_cpu_stop_nolock(uint16_t addr, bool store)
>  	uint8_t order = store ? SIGP_STOP_AND_STORE_STATUS : SIGP_STOP;
>  
>  	cpu = smp_cpu_from_addr(addr);
> -	if (!cpu || cpu == cpu0)
> +	if (!cpu || addr == cpus[0].addr)
>  		return -1;
>  
>  	if (sigp_retry(addr, order, 0, NULL))
> @@ -205,7 +204,7 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
>  	sigp_retry(cpu->addr, SIGP_SET_PREFIX, (unsigned long )lc, NULL);
>  
>  	/* Copy all exception psws. */
> -	memcpy(lc, cpu0->lowcore, 512);
> +	memcpy(lc, cpus[0].lowcore, 512);
>  
>  	/* Setup stack */
>  	cpu->stack = (uint64_t *)alloc_pages(2);
> @@ -263,15 +262,16 @@ void smp_setup(void)
>  	if (num > 1)
>  		printf("SMP: Initializing, found %d cpus\n", num);
>  
> -	cpus = calloc(num, sizeof(cpus));
> +	cpus = calloc(num, sizeof(*cpus));
>  	for (i = 0; i < num; i++) {
>  		cpus[i].addr = entry[i].address;
>  		cpus[i].active = false;
>  		if (entry[i].address == cpu0_addr) {
> -			cpu0 = &cpus[i];
> -			cpu0->stack = stackptr;
> -			cpu0->lowcore = (void *)0;
> -			cpu0->active = true;
> +			cpus[i].addr = cpus[0].addr;

Might deserve a comment that we'll move the the boot CPU to index 0.

What's the expected behavior if i == 0?

-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH v1 1/5] lib: s390x: smp: add functions to work with CPU indexes

[Steffen Eiden]

On 1/31/22 14:50, David Hildenbrand wrote:
> On 28.01.22 19:54, Claudio Imbrenda wrote:
>> Knowing the number of active CPUs is not enough to know which ones are
>> active. This patch adds 2 new functions:
>>
>> * smp_cpu_addr_from_idx to get the CPU address from the index
>> * smp_cpu_from_idx allows to retrieve the struct cpu from the index
>>
>> This makes it possible for tests to avoid hardcoding the CPU addresses.
>> It is useful in cases where the address and the index might not match.
>>
>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
With Davids nit
Reviewed-by Steffen Eiden <seiden@linux.ibm.com>
>> ---
>>   lib/s390x/smp.h |  2 ++
>>   lib/s390x/smp.c | 12 ++++++++++++
>>   2 files changed, 14 insertions(+)
>>
>> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
>> index a2609f11..69aa4003 100644
>> --- a/lib/s390x/smp.h
>> +++ b/lib/s390x/smp.h
>> @@ -37,6 +37,7 @@ struct cpu_status {
>>   
>>   int smp_query_num_cpus(void);
>>   struct cpu *smp_cpu_from_addr(uint16_t addr);
>> +struct cpu *smp_cpu_from_idx(uint16_t addr);
> 
> s/addr/idx/
> 
> 

Re: [kvm-unit-tests PATCH v1 0/5] s390x: smp: avoid hardcoded CPU addresses

[Janosch Frank]

On 1/28/22 19:54, Claudio Imbrenda wrote:
> On s390x there are no guarantees about the CPU addresses, except that
> they shall be unique. This means that in some environments, it's
> possible that there is no match between the CPU address and its
> position (index) in the list of available CPUs returned by the system.

While I support this patch set I've yet to find an environment where 
this gave me headaches.

> 
> This series fixes a small bug in the SMP initialization code, adds a
> guarantee that the boot CPU will always have index 0, and introduces
> some functions to allow tests to use CPU indexes instead of using
> hardcoded CPU addresses. This will allow the tests to run successfully
> in more environments (e.g. z/VM, LPAR).

I'm wondering if we should do it the other way round and make the smp_* 
functions take a idx instead of a cpu addr. The only instance where this 
gets a bit ugly is the sigp calls which we would also need to convert.

> Some existing tests are adapted to take advantage of the new
> functionalities.
> 
> Claudio Imbrenda (5):
>    lib: s390x: smp: add functions to work with CPU indexes
>    lib: s390x: smp: guarantee that boot CPU has index 0
>    s390x: smp: avoid hardcoded CPU addresses
>    s390x: firq: avoid hardcoded CPU addresses
>    s390x: skrf: avoid hardcoded CPU addresses
> 
>   lib/s390x/smp.h |  2 ++
>   lib/s390x/smp.c | 28 ++++++++++++-----
>   s390x/firq.c    | 17 +++++-----
>   s390x/skrf.c    |  8 +++--
>   s390x/smp.c     | 83 ++++++++++++++++++++++++++-----------------------
>   5 files changed, 79 insertions(+), 59 deletions(-)
> 

Re: [kvm-unit-tests PATCH v1 2/5] lib: s390x: smp: guarantee that boot CPU has index 0

[Claudio Imbrenda]

On Mon, 31 Jan 2022 14:55:09 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 28.01.22 19:54, Claudio Imbrenda wrote:
> > Guarantee that the boot CPU has index 0. This simplifies the
> > implementation of tests that require multiple CPUs.
> > 
> > Also fix a small bug in the allocation of the cpus array.
> > 
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > Fixes: f77c0515 ("s390x: Add initial smp code")
> > Fixes: 52076a63 ("s390x: Consolidate sclp read info")
> > ---
> >  lib/s390x/smp.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> > index 64c647ec..01f513f0 100644
> > --- a/lib/s390x/smp.c
> > +++ b/lib/s390x/smp.c
> > @@ -25,7 +25,6 @@
> >  #include "sclp.h"
> >  
> >  static struct cpu *cpus;
> > -static struct cpu *cpu0;
> >  static struct spinlock lock;
> >  
> >  extern void smp_cpu_setup_state(void);
> > @@ -81,7 +80,7 @@ static int smp_cpu_stop_nolock(uint16_t addr, bool store)
> >  	uint8_t order = store ? SIGP_STOP_AND_STORE_STATUS : SIGP_STOP;
> >  
> >  	cpu = smp_cpu_from_addr(addr);
> > -	if (!cpu || cpu == cpu0)
> > +	if (!cpu || addr == cpus[0].addr)
> >  		return -1;
> >  
> >  	if (sigp_retry(addr, order, 0, NULL))
> > @@ -205,7 +204,7 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
> >  	sigp_retry(cpu->addr, SIGP_SET_PREFIX, (unsigned long )lc, NULL);
> >  
> >  	/* Copy all exception psws. */
> > -	memcpy(lc, cpu0->lowcore, 512);
> > +	memcpy(lc, cpus[0].lowcore, 512);
> >  
> >  	/* Setup stack */
> >  	cpu->stack = (uint64_t *)alloc_pages(2);
> > @@ -263,15 +262,16 @@ void smp_setup(void)
> >  	if (num > 1)
> >  		printf("SMP: Initializing, found %d cpus\n", num);
> >  
> > -	cpus = calloc(num, sizeof(cpus));
> > +	cpus = calloc(num, sizeof(*cpus));
> >  	for (i = 0; i < num; i++) {
> >  		cpus[i].addr = entry[i].address;
> >  		cpus[i].active = false;
> >  		if (entry[i].address == cpu0_addr) {
> > -			cpu0 = &cpus[i];
> > -			cpu0->stack = stackptr;
> > -			cpu0->lowcore = (void *)0;
> > -			cpu0->active = true;
> > +			cpus[i].addr = cpus[0].addr;  
> 
> Might deserve a comment that we'll move the the boot CPU to index 0.

fair enough.

> 
> What's the expected behavior if i == 0?
> 

in that case, the boot CPU was already the one with index 0. The code
will do a few extra useless steps, but in the end everything should Just
Work™

Re: [kvm-unit-tests PATCH v1 1/5] lib: s390x: smp: add functions to work with CPU indexes

[Claudio Imbrenda]

On Mon, 31 Jan 2022 14:50:37 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 28.01.22 19:54, Claudio Imbrenda wrote:
> > Knowing the number of active CPUs is not enough to know which ones are
> > active. This patch adds 2 new functions:
> > 
> > * smp_cpu_addr_from_idx to get the CPU address from the index
> > * smp_cpu_from_idx allows to retrieve the struct cpu from the index
> > 
> > This makes it possible for tests to avoid hardcoding the CPU addresses.
> > It is useful in cases where the address and the index might not match.
> > 
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > ---
> >  lib/s390x/smp.h |  2 ++
> >  lib/s390x/smp.c | 12 ++++++++++++
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
> > index a2609f11..69aa4003 100644
> > --- a/lib/s390x/smp.h
> > +++ b/lib/s390x/smp.h
> > @@ -37,6 +37,7 @@ struct cpu_status {
> >  
> >  int smp_query_num_cpus(void);
> >  struct cpu *smp_cpu_from_addr(uint16_t addr);
> > +struct cpu *smp_cpu_from_idx(uint16_t addr);  
> 
> s/addr/idx/
> 
> 

oops!

will fix

Re: [kvm-unit-tests PATCH v1 0/5] s390x: smp: avoid hardcoded CPU addresses

[Claudio Imbrenda]

On Thu, 3 Feb 2022 09:45:56 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 1/28/22 19:54, Claudio Imbrenda wrote:
> > On s390x there are no guarantees about the CPU addresses, except that
> > they shall be unique. This means that in some environments, it's
> > possible that there is no match between the CPU address and its
> > position (index) in the list of available CPUs returned by the system.  
> 
> While I support this patch set I've yet to find an environment where 
> this gave me headaches.
> 
> > 
> > This series fixes a small bug in the SMP initialization code, adds a
> > guarantee that the boot CPU will always have index 0, and introduces
> > some functions to allow tests to use CPU indexes instead of using
> > hardcoded CPU addresses. This will allow the tests to run successfully
> > in more environments (e.g. z/VM, LPAR).  
> 
> I'm wondering if we should do it the other way round and make the smp_* 
> functions take a idx instead of a cpu addr. The only instance where this 
> gets a bit ugly is the sigp calls which we would also need to convert.

yes, in fact this is something I was already planning to do :)

for sigp, we can either convert, or add a wrapper with idx.

> 
> > Some existing tests are adapted to take advantage of the new
> > functionalities.
> > 
> > Claudio Imbrenda (5):
> >    lib: s390x: smp: add functions to work with CPU indexes
> >    lib: s390x: smp: guarantee that boot CPU has index 0
> >    s390x: smp: avoid hardcoded CPU addresses
> >    s390x: firq: avoid hardcoded CPU addresses
> >    s390x: skrf: avoid hardcoded CPU addresses
> > 
> >   lib/s390x/smp.h |  2 ++
> >   lib/s390x/smp.c | 28 ++++++++++++-----
> >   s390x/firq.c    | 17 +++++-----
> >   s390x/skrf.c    |  8 +++--
> >   s390x/smp.c     | 83 ++++++++++++++++++++++++++-----------------------
> >   5 files changed, 79 insertions(+), 59 deletions(-)
> >   
> 

Re: [kvm-unit-tests PATCH v1 0/5] s390x: smp: avoid hardcoded CPU addresses

[Janosch Frank]

On 2/3/22 14:37, Claudio Imbrenda wrote:
> On Thu, 3 Feb 2022 09:45:56 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 1/28/22 19:54, Claudio Imbrenda wrote:
>>> On s390x there are no guarantees about the CPU addresses, except that
>>> they shall be unique. This means that in some environments, it's
>>> possible that there is no match between the CPU address and its
>>> position (index) in the list of available CPUs returned by the system.
>>
>> While I support this patch set I've yet to find an environment where
>> this gave me headaches.
>>
>>>
>>> This series fixes a small bug in the SMP initialization code, adds a
>>> guarantee that the boot CPU will always have index 0, and introduces
>>> some functions to allow tests to use CPU indexes instead of using
>>> hardcoded CPU addresses. This will allow the tests to run successfully
>>> in more environments (e.g. z/VM, LPAR).
>>
>> I'm wondering if we should do it the other way round and make the smp_*
>> functions take a idx instead of a cpu addr. The only instance where this
>> gets a bit ugly is the sigp calls which we would also need to convert.
> 
> yes, in fact this is something I was already planning to do :)

Do you want to add that in a v2 to reduce the additional churn?

> 
> for sigp, we can either convert, or add a wrapper with idx.

How about adding a wrapper to smp.c?

smp_cpu_sigp()
smp_cpu_sigp_retry()


That would fall in line with the naming of the smp functions and it's 
clear that we refer to a specific cpu from the smp lib.

We can then leave the sigp.h functions as is so Nico can use them for 
the invalid addr tests.

> 
>>
>>> Some existing tests are adapted to take advantage of the new
>>> functionalities.
>>>
>>> Claudio Imbrenda (5):
>>>     lib: s390x: smp: add functions to work with CPU indexes
>>>     lib: s390x: smp: guarantee that boot CPU has index 0
>>>     s390x: smp: avoid hardcoded CPU addresses
>>>     s390x: firq: avoid hardcoded CPU addresses
>>>     s390x: skrf: avoid hardcoded CPU addresses
>>>
>>>    lib/s390x/smp.h |  2 ++
>>>    lib/s390x/smp.c | 28 ++++++++++++-----
>>>    s390x/firq.c    | 17 +++++-----
>>>    s390x/skrf.c    |  8 +++--
>>>    s390x/smp.c     | 83 ++++++++++++++++++++++++++-----------------------
>>>    5 files changed, 79 insertions(+), 59 deletions(-)
>>>    
>>
> 

Re: [kvm-unit-tests PATCH v1 0/5] s390x: smp: avoid hardcoded CPU addresses

[Claudio Imbrenda]

On Thu, 3 Feb 2022 16:23:23 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 2/3/22 14:37, Claudio Imbrenda wrote:
> > On Thu, 3 Feb 2022 09:45:56 +0100
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> On 1/28/22 19:54, Claudio Imbrenda wrote:  
> >>> On s390x there are no guarantees about the CPU addresses, except that
> >>> they shall be unique. This means that in some environments, it's
> >>> possible that there is no match between the CPU address and its
> >>> position (index) in the list of available CPUs returned by the system.  
> >>
> >> While I support this patch set I've yet to find an environment where
> >> this gave me headaches.
> >>  
> >>>
> >>> This series fixes a small bug in the SMP initialization code, adds a
> >>> guarantee that the boot CPU will always have index 0, and introduces
> >>> some functions to allow tests to use CPU indexes instead of using
> >>> hardcoded CPU addresses. This will allow the tests to run successfully
> >>> in more environments (e.g. z/VM, LPAR).  
> >>
> >> I'm wondering if we should do it the other way round and make the smp_*
> >> functions take a idx instead of a cpu addr. The only instance where this
> >> gets a bit ugly is the sigp calls which we would also need to convert.  
> > 
> > yes, in fact this is something I was already planning to do :)  
> 
> Do you want to add that in a v2 to reduce the additional churn?

ok

> 
> > 
> > for sigp, we can either convert, or add a wrapper with idx.  
> 
> How about adding a wrapper to smp.c?
> 
> smp_cpu_sigp()
> smp_cpu_sigp_retry()

I was actually thinking about sigp_idx, but I like your names better

> 
> 
> That would fall in line with the naming of the smp functions and it's 
> clear that we refer to a specific cpu from the smp lib.
> 
> We can then leave the sigp.h functions as is so Nico can use them for 
> the invalid addr tests.

yes, a "raw" sigp function should definitely stay there

> 
> >   
> >>  
> >>> Some existing tests are adapted to take advantage of the new
> >>> functionalities.
> >>>
> >>> Claudio Imbrenda (5):
> >>>     lib: s390x: smp: add functions to work with CPU indexes
> >>>     lib: s390x: smp: guarantee that boot CPU has index 0
> >>>     s390x: smp: avoid hardcoded CPU addresses
> >>>     s390x: firq: avoid hardcoded CPU addresses
> >>>     s390x: skrf: avoid hardcoded CPU addresses
> >>>
> >>>    lib/s390x/smp.h |  2 ++
> >>>    lib/s390x/smp.c | 28 ++++++++++++-----
> >>>    s390x/firq.c    | 17 +++++-----
> >>>    s390x/skrf.c    |  8 +++--
> >>>    s390x/smp.c     | 83 ++++++++++++++++++++++++++-----------------------
> >>>    5 files changed, 79 insertions(+), 59 deletions(-)
> >>>      
> >>  
> >   
> 

Re: [kvm-unit-tests PATCH v1 0/5] s390x: smp: avoid hardcoded CPU addresses

[Janosch Frank]

On 1/28/22 19:54, Claudio Imbrenda wrote:
> On s390x there are no guarantees about the CPU addresses, except that
> they shall be unique. This means that in some environments, it's
> possible that there is no match between the CPU address and its
> position (index) in the list of available CPUs returned by the system.
> 
> This series fixes a small bug in the SMP initialization code, adds a
> guarantee that the boot CPU will always have index 0, and introduces
> some functions to allow tests to use CPU indexes instead of using
> hardcoded CPU addresses. This will allow the tests to run successfully
> in more environments (e.g. z/VM, LPAR).
> 
> Some existing tests are adapted to take advantage of the new
> functionalities.
> 
> Claudio Imbrenda (5):
>    lib: s390x: smp: add functions to work with CPU indexes
>    lib: s390x: smp: guarantee that boot CPU has index 0
>    s390x: smp: avoid hardcoded CPU addresses
>    s390x: firq: avoid hardcoded CPU addresses
>    s390x: skrf: avoid hardcoded CPU addresses
> 
>   lib/s390x/smp.h |  2 ++
>   lib/s390x/smp.c | 28 ++++++++++++-----
>   s390x/firq.c    | 17 +++++-----
>   s390x/skrf.c    |  8 +++--
>   s390x/smp.c     | 83 ++++++++++++++++++++++++++-----------------------

We use smp/sigp in uv-host.c and one of those uses looks a bit strange 
to me anyway.

I think we also need to fix the sigp in cstart.S to only stop itself and 
not the cpu with the addr 0.

Up to now we very much assumed that cpu 0 is always our boot cpu so if 
you start running the test with cpu addr 1 and 2 and leave out 0 you 
might find more problematic code.


>   5 files changed, 79 insertions(+), 59 deletions(-)
> 

Re: [kvm-unit-tests PATCH v1 0/5] s390x: smp: avoid hardcoded CPU addresses

[Claudio Imbrenda]

On Fri, 4 Feb 2022 16:01:54 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 1/28/22 19:54, Claudio Imbrenda wrote:
> > On s390x there are no guarantees about the CPU addresses, except that
> > they shall be unique. This means that in some environments, it's
> > possible that there is no match between the CPU address and its
> > position (index) in the list of available CPUs returned by the system.
> > 
> > This series fixes a small bug in the SMP initialization code, adds a
> > guarantee that the boot CPU will always have index 0, and introduces
> > some functions to allow tests to use CPU indexes instead of using
> > hardcoded CPU addresses. This will allow the tests to run successfully
> > in more environments (e.g. z/VM, LPAR).
> > 
> > Some existing tests are adapted to take advantage of the new
> > functionalities.
> > 
> > Claudio Imbrenda (5):
> >    lib: s390x: smp: add functions to work with CPU indexes
> >    lib: s390x: smp: guarantee that boot CPU has index 0
> >    s390x: smp: avoid hardcoded CPU addresses
> >    s390x: firq: avoid hardcoded CPU addresses
> >    s390x: skrf: avoid hardcoded CPU addresses
> > 
> >   lib/s390x/smp.h |  2 ++
> >   lib/s390x/smp.c | 28 ++++++++++++-----
> >   s390x/firq.c    | 17 +++++-----
> >   s390x/skrf.c    |  8 +++--
> >   s390x/smp.c     | 83 ++++++++++++++++++++++++++-----------------------  
> 
> We use smp/sigp in uv-host.c and one of those uses looks a bit strange 
> to me anyway.

I had noticed that, it's fixed in the v2 (and that test will almost
surely be rewritten anyway)

> 
> I think we also need to fix the sigp in cstart.S to only stop itself and 
> not the cpu with the addr 0.

not sure if that is needed right now. that is only used for snippets,
right?

or did you mean cstart64.S?
but there, sigp is only used for SET_ARCHITECTURE, so it doesn't really
matter I guess?

> 
> Up to now we very much assumed that cpu 0 is always our boot cpu so if 
> you start running the test with cpu addr 1 and 2 and leave out 0 you 
> might find more problematic code.
> 
> 
> >   5 files changed, 79 insertions(+), 59 deletions(-)
> >   
> 

Re: [kvm-unit-tests PATCH v1 0/5] s390x: smp: avoid hardcoded CPU addresses

[Janosch Frank]

On 2/4/22 16:14, Claudio Imbrenda wrote:
> On Fri, 4 Feb 2022 16:01:54 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 1/28/22 19:54, Claudio Imbrenda wrote:
>>> On s390x there are no guarantees about the CPU addresses, except that
>>> they shall be unique. This means that in some environments, it's
>>> possible that there is no match between the CPU address and its
>>> position (index) in the list of available CPUs returned by the system.
>>>
>>> This series fixes a small bug in the SMP initialization code, adds a
>>> guarantee that the boot CPU will always have index 0, and introduces
>>> some functions to allow tests to use CPU indexes instead of using
>>> hardcoded CPU addresses. This will allow the tests to run successfully
>>> in more environments (e.g. z/VM, LPAR).
>>>
>>> Some existing tests are adapted to take advantage of the new
>>> functionalities.
>>>
>>> Claudio Imbrenda (5):
>>>     lib: s390x: smp: add functions to work with CPU indexes
>>>     lib: s390x: smp: guarantee that boot CPU has index 0
>>>     s390x: smp: avoid hardcoded CPU addresses
>>>     s390x: firq: avoid hardcoded CPU addresses
>>>     s390x: skrf: avoid hardcoded CPU addresses
>>>
>>>    lib/s390x/smp.h |  2 ++
>>>    lib/s390x/smp.c | 28 ++++++++++++-----
>>>    s390x/firq.c    | 17 +++++-----
>>>    s390x/skrf.c    |  8 +++--
>>>    s390x/smp.c     | 83 ++++++++++++++++++++++++++-----------------------
>>
>> We use smp/sigp in uv-host.c and one of those uses looks a bit strange
>> to me anyway.
> 
> I had noticed that, it's fixed in the v2 (and that test will almost
> surely be rewritten anyway)
> 
>>
>> I think we also need to fix the sigp in cstart.S to only stop itself and
>> not the cpu with the addr 0.
> 
> not sure if that is needed right now. that is only used for snippets,
> right?

Yes

> 
> or did you mean cstart64.S?
> but there, sigp is only used for SET_ARCHITECTURE, so it doesn't really
> matter I guess?

My bad, seems like it's Friday

> 
>>
>> Up to now we very much assumed that cpu 0 is always our boot cpu so if
>> you start running the test with cpu addr 1 and 2 and leave out 0 you
>> might find more problematic code.
>>
>>
>>>    5 files changed, 79 insertions(+), 59 deletions(-)
>>>    
>>
>