[kvm-unit-tests PATCH v2 0/6] s390x: smp: use CPU indexes instead of addresses

[kvm-unit-tests PATCH v2 0/6] s390x: smp: use CPU indexes instead of 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 is
theoretically possible that there is no match between the CPU address
and its position (index) in the list of available CPUs returned by the
system. Moreover, there are no guarantees about the ordering of the
list, or even that it is consistent each time it is returned.

This series fixes a small bug in the SMP initialization code, adds a
guarantee that the boot CPU will always have index 0, changes the
existing smp_* functions to take indexes instead of addresses, 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.

v1->v2
* refactored the smp_* functions to accept indexes instead of addresses
* also fixed uv-host test

Claudio Imbrenda (6):
  lib: s390x: smp: guarantee that boot CPU has index 0
  lib: s390x: smp: refactor smp functions to accept indexes
  s390x: smp: use CPU indexes instead of addresses
  s390x: firq: use CPU indexes instead of addresses
  s390x: skrf: use CPU indexes instead of addresses
  s390x: uv-host: use CPU indexes instead of addresses

 lib/s390x/smp.h |  20 +++---
 lib/s390x/smp.c | 173 +++++++++++++++++++++++++++++-------------------
 s390x/firq.c    |  26 ++------
 s390x/skrf.c    |   2 +-
 s390x/smp.c     |  22 +++---
 s390x/uv-host.c |   4 +-
 6 files changed, 137 insertions(+), 110 deletions(-)

-- 
2.34.1

[kvm-unit-tests PATCH v2 1/6] 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 | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index b753eab5..eae742d2 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);
@@ -69,7 +68,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))
@@ -193,7 +192,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);
@@ -251,15 +250,27 @@ 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;
+		/*
+		 * Fill in the boot CPU. If the boot CPU is not at index 0,
+		 * swap it with the one at index 0. This guarantees that the
+		 * boot CPU will always have index 0. If the boot CPU was
+		 * already at index 0, a few extra useless assignments are
+		 * performed, but everything will work ok.
+		 * Notice that there is no guarantee that the list of CPUs
+		 * returned by the Read SCP Info command is in any
+		 * particular order, or that its order will stay consistent
+		 * across multiple invocations.
+		 */
 		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 v2 2/6] lib: s390x: smp: refactor smp functions to accept indexes

[Claudio Imbrenda]

Refactor all the smp_* functions to accept CPU indexes instead of CPU
addresses.

Add SIGP wrappers to use indexes instead of addresses. Raw SIGP calls
using addresses are still possible.

Add a few other useful functions to deal with CPU indexes.

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

diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
index a2609f11..1e69a7de 100644
--- a/lib/s390x/smp.h
+++ b/lib/s390x/smp.h
@@ -37,15 +37,19 @@ struct cpu_status {
 
 int smp_query_num_cpus(void);
 struct cpu *smp_cpu_from_addr(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);
-int smp_cpu_start(uint16_t addr, struct psw psw);
-int smp_cpu_stop(uint16_t addr);
-int smp_cpu_stop_store_status(uint16_t addr);
-int smp_cpu_destroy(uint16_t addr);
-int smp_cpu_setup(uint16_t addr, struct psw psw);
+struct cpu *smp_cpu_from_idx(uint16_t idx);
+uint16_t smp_cpu_addr(uint16_t idx);
+bool smp_cpu_stopped(uint16_t idx);
+bool smp_sense_running_status(uint16_t idx);
+int smp_cpu_restart(uint16_t idx);
+int smp_cpu_start(uint16_t idx, struct psw psw);
+int smp_cpu_stop(uint16_t idx);
+int smp_cpu_stop_store_status(uint16_t idx);
+int smp_cpu_destroy(uint16_t idx);
+int smp_cpu_setup(uint16_t idx, struct psw psw);
 void smp_teardown(void);
 void smp_setup(void);
+int smp_sigp(uint16_t idx, uint8_t order, unsigned long parm, uint32_t *status);
+int smp_sigp_retry(uint16_t idx, uint8_t order, unsigned long parm, uint32_t *status);
 
 #endif
diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index eae742d2..dde79274 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -29,11 +29,28 @@ static struct spinlock lock;
 
 extern void smp_cpu_setup_state(void);
 
+static void check_idx(uint16_t idx)
+{
+	assert(idx < smp_query_num_cpus());
+}
+
 int smp_query_num_cpus(void)
 {
 	return sclp_get_cpu_num();
 }
 
+int smp_sigp(uint16_t idx, uint8_t order, unsigned long parm, uint32_t *status)
+{
+	check_idx(idx);
+	return sigp(cpus[idx].addr, order, parm, status);
+}
+
+int smp_sigp_retry(uint16_t idx, uint8_t order, unsigned long parm, uint32_t *status)
+{
+	check_idx(idx);
+	return sigp_retry(cpus[idx].addr, order, parm, status);
+}
+
 struct cpu *smp_cpu_from_addr(uint16_t addr)
 {
 	int i, num = smp_query_num_cpus();
@@ -45,174 +62,183 @@ struct cpu *smp_cpu_from_addr(uint16_t addr)
 	return NULL;
 }
 
-bool smp_cpu_stopped(uint16_t addr)
+struct cpu *smp_cpu_from_idx(uint16_t idx)
+{
+	check_idx(idx);
+	return &cpus[idx];
+}
+
+uint16_t smp_cpu_addr(uint16_t idx)
+{
+	check_idx(idx);
+	return cpus[idx].addr;
+}
+
+bool smp_cpu_stopped(uint16_t idx)
 {
 	uint32_t status;
 
-	if (sigp(addr, SIGP_SENSE, 0, &status) != SIGP_CC_STATUS_STORED)
+	if (smp_sigp(idx, SIGP_SENSE, 0, &status) != SIGP_CC_STATUS_STORED)
 		return false;
 	return !!(status & (SIGP_STATUS_CHECK_STOP|SIGP_STATUS_STOPPED));
 }
 
-bool smp_sense_running_status(uint16_t addr)
+bool smp_sense_running_status(uint16_t idx)
 {
-	if (sigp(addr, SIGP_SENSE_RUNNING, 0, NULL) != SIGP_CC_STATUS_STORED)
+	if (smp_sigp(idx, SIGP_SENSE_RUNNING, 0, NULL) != SIGP_CC_STATUS_STORED)
 		return true;
 	/* Status stored condition code is equivalent to cpu not running. */
 	return false;
 }
 
-static int smp_cpu_stop_nolock(uint16_t addr, bool store)
+static int smp_cpu_stop_nolock(uint16_t idx, bool store)
 {
-	struct cpu *cpu;
 	uint8_t order = store ? SIGP_STOP_AND_STORE_STATUS : SIGP_STOP;
 
-	cpu = smp_cpu_from_addr(addr);
-	if (!cpu || addr == cpus[0].addr)
+	/* refuse to work on the boot CPU */
+	if (idx == 0)
 		return -1;
 
-	if (sigp_retry(addr, order, 0, NULL))
+	if (smp_sigp_retry(idx, order, 0, NULL))
 		return -1;
 
-	while (!smp_cpu_stopped(addr))
+	while (!smp_cpu_stopped(idx))
 		mb();
-	cpu->active = false;
+	/* idx has been already checked by the smp_* functions called above */
+	cpus[idx].active = false;
 	return 0;
 }
 
-int smp_cpu_stop(uint16_t addr)
+int smp_cpu_stop(uint16_t idx)
 {
 	int rc;
 
 	spin_lock(&lock);
-	rc = smp_cpu_stop_nolock(addr, false);
+	rc = smp_cpu_stop_nolock(idx, false);
 	spin_unlock(&lock);
 	return rc;
 }
 
-int smp_cpu_stop_store_status(uint16_t addr)
+int smp_cpu_stop_store_status(uint16_t idx)
 {
 	int rc;
 
 	spin_lock(&lock);
-	rc = smp_cpu_stop_nolock(addr, true);
+	rc = smp_cpu_stop_nolock(idx, true);
 	spin_unlock(&lock);
 	return rc;
 }
 
-static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw)
+static int smp_cpu_restart_nolock(uint16_t idx, struct psw *psw)
 {
 	int rc;
-	struct cpu *cpu = smp_cpu_from_addr(addr);
 
-	if (!cpu)
-		return -1;
+	check_idx(idx);
 	if (psw) {
-		cpu->lowcore->restart_new_psw.mask = psw->mask;
-		cpu->lowcore->restart_new_psw.addr = psw->addr;
+		cpus[idx].lowcore->restart_new_psw.mask = psw->mask;
+		cpus[idx].lowcore->restart_new_psw.addr = psw->addr;
 	}
 	/*
 	 * Stop the cpu, so we don't have a race between a running cpu
 	 * and the restart in the test that checks if the cpu is
 	 * running after the restart.
 	 */
-	smp_cpu_stop_nolock(addr, false);
-	rc = sigp(addr, SIGP_RESTART, 0, NULL);
+	smp_cpu_stop_nolock(idx, false);
+	rc = sigp(cpus[idx].addr, SIGP_RESTART, 0, NULL);
 	if (rc)
 		return rc;
 	/*
 	 * The order has been accepted, but the actual restart may not
 	 * have been performed yet, so wait until the cpu is running.
 	 */
-	while (smp_cpu_stopped(addr))
+	while (smp_cpu_stopped(idx))
 		mb();
-	cpu->active = true;
+	cpus[idx].active = true;
 	return 0;
 }
 
-int smp_cpu_restart(uint16_t addr)
+int smp_cpu_restart(uint16_t idx)
 {
 	int rc;
 
 	spin_lock(&lock);
-	rc = smp_cpu_restart_nolock(addr, NULL);
+	rc = smp_cpu_restart_nolock(idx, NULL);
 	spin_unlock(&lock);
 	return rc;
 }
 
-int smp_cpu_start(uint16_t addr, struct psw psw)
+int smp_cpu_start(uint16_t idx, struct psw psw)
 {
 	int rc;
 
 	spin_lock(&lock);
-	rc = smp_cpu_restart_nolock(addr, &psw);
+	rc = smp_cpu_restart_nolock(idx, &psw);
 	spin_unlock(&lock);
 	return rc;
 }
 
-int smp_cpu_destroy(uint16_t addr)
+int smp_cpu_destroy(uint16_t idx)
 {
-	struct cpu *cpu;
 	int rc;
 
 	spin_lock(&lock);
-	rc = smp_cpu_stop_nolock(addr, false);
+	rc = smp_cpu_stop_nolock(idx, false);
 	if (!rc) {
-		cpu = smp_cpu_from_addr(addr);
-		free_pages(cpu->lowcore);
-		free_pages(cpu->stack);
-		cpu->lowcore = (void *)-1UL;
-		cpu->stack = (void *)-1UL;
+		free_pages(cpus[idx].lowcore);
+		free_pages(cpus[idx].stack);
+		cpus[idx].lowcore = (void *)-1UL;
+		cpus[idx].stack = (void *)-1UL;
 	}
 	spin_unlock(&lock);
 	return rc;
 }
 
-int smp_cpu_setup(uint16_t addr, struct psw psw)
+static int smp_cpu_setup_nolock(uint16_t idx, struct psw psw)
 {
 	struct lowcore *lc;
-	struct cpu *cpu;
-	int rc = -1;
-
-	spin_lock(&lock);
-
-	if (!cpus)
-		goto out;
 
-	cpu = smp_cpu_from_addr(addr);
-
-	if (!cpu || cpu->active)
-		goto out;
+	if (cpus[idx].active)
+		return -1;
 
-	sigp_retry(cpu->addr, SIGP_INITIAL_CPU_RESET, 0, NULL);
+	sigp_retry(cpus[idx].addr, SIGP_INITIAL_CPU_RESET, 0, NULL);
 
 	lc = alloc_pages_flags(1, AREA_DMA31);
-	cpu->lowcore = lc;
-	memset(lc, 0, PAGE_SIZE * 2);
-	sigp_retry(cpu->addr, SIGP_SET_PREFIX, (unsigned long )lc, NULL);
+	cpus[idx].lowcore = lc;
+	sigp_retry(cpus[idx].addr, SIGP_SET_PREFIX, (unsigned long )lc, NULL);
 
 	/* Copy all exception psws. */
 	memcpy(lc, cpus[0].lowcore, 512);
 
 	/* Setup stack */
-	cpu->stack = (uint64_t *)alloc_pages(2);
+	cpus[idx].stack = (uint64_t *)alloc_pages(2);
 
 	/* Start without DAT and any other mask bits. */
-	cpu->lowcore->sw_int_psw.mask = psw.mask;
-	cpu->lowcore->sw_int_psw.addr = psw.addr;
-	cpu->lowcore->sw_int_grs[14] = psw.addr;
-	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
+	lc->sw_int_psw.mask = psw.mask;
+	lc->sw_int_psw.addr = psw.addr;
+	lc->sw_int_grs[14] = psw.addr;
+	lc->sw_int_grs[15] = (uint64_t)cpus[idx].stack + (PAGE_SIZE * 4);
 	lc->restart_new_psw.mask = PSW_MASK_64;
 	lc->restart_new_psw.addr = (uint64_t)smp_cpu_setup_state;
 	lc->sw_int_crs[0] = BIT_ULL(CTL0_AFP);
 
 	/* Start processing */
-	smp_cpu_restart_nolock(addr, NULL);
+	smp_cpu_restart_nolock(idx, NULL);
 	/* Wait until the cpu has finished setup and started the provided psw */
 	while (lc->restart_new_psw.addr != psw.addr)
 		mb();
-	rc = 0;
-out:
+
+	return 0;
+}
+
+int smp_cpu_setup(uint16_t idx, struct psw psw)
+{
+	int rc = -1;
+
+	spin_lock(&lock);
+	if (cpus) {
+		check_idx(idx);
+		rc = smp_cpu_setup_nolock(idx, psw);
+	}
 	spin_unlock(&lock);
 	return rc;
 }
-- 
2.34.1

[kvm-unit-tests PATCH v2 3/6] s390x: smp: use CPU indexes instead of addresses

[Claudio Imbrenda]

Adapt the test to the new semantics of the smp_* functions, and use CPU
indexes instead of addresses.

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

diff --git a/s390x/smp.c b/s390x/smp.c
index 1bbe4c31..068ac74d 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -56,7 +56,7 @@ 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();
@@ -92,7 +92,7 @@ static void test_stop(void)
 
 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");
@@ -129,7 +129,7 @@ static void test_store_status(void)
 
 	report_prefix_push("running");
 	smp_cpu_restart(1);
-	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, &r);
+	smp_sigp(1, 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");
@@ -138,7 +138,7 @@ 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_sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
 	while (!status->prefix) { mb(); }
 	report_pass("status written");
 	free_pages(status);
@@ -176,7 +176,7 @@ static void test_ecall(void)
 	smp_cpu_start(1, psw);
 	wait_for_flag();
 	set_flag(0);
-	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
+	smp_sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
 	wait_for_flag();
 	smp_cpu_stop(1);
 	report_prefix_pop();
@@ -210,7 +210,7 @@ static void test_emcall(void)
 	smp_cpu_start(1, psw);
 	wait_for_flag();
 	set_flag(0);
-	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
+	smp_sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
 	wait_for_flag();
 	smp_cpu_stop(1);
 	report_prefix_pop();
@@ -253,8 +253,8 @@ static void test_reset_initial(void)
 	smp_cpu_start(1, psw);
 	wait_for_flag();
 
-	sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
-	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
+	smp_sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
+	smp_sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
 
 	report_prefix_push("clear");
 	report(!status->psw.mask && !status->psw.addr, "psw");
@@ -299,11 +299,11 @@ 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_sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
+	smp_sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
 	smp_cpu_start(1, psw);
 
-	sigp_retry(1, SIGP_CPU_RESET, 0, NULL);
+	smp_sigp_retry(1, SIGP_CPU_RESET, 0, NULL);
 	report(smp_cpu_stopped(1), "cpu stopped");
 
 	set_flag(0);
-- 
2.34.1

[kvm-unit-tests PATCH v2 4/6] s390x: firq: use CPU indexes instead of addresses

[Claudio Imbrenda]

Adapt the test to the new semantics of the smp_* functions, and use CPU
indexes instead of addresses.

replace the checks with asserts, the 3 CPUs are guaranteed to be there.

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

diff --git a/s390x/firq.c b/s390x/firq.c
index fb9a2906..b4b3542e 100644
--- a/s390x/firq.c
+++ b/s390x/firq.c
@@ -44,24 +44,13 @@ static void test_wait_state_delivery(void)
 		goto out;
 	}
 
-	if (stap()) {
-		report_skip("need to start on CPU #0");
-		goto out;
-	}
-
-	/*
-	 * 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.
-	 */
+	/* Stop CPU #2. It must succeed because we have at least 3 CPUs */
 	ret = smp_cpu_stop(2);
-	if (ret) {
-		report_skip("CPU #2 not found");
-		goto out;
-	}
+	assert(!ret);
 
 	/*
-	 * We're going to perform an SCLP service call but expect
-	 * the interrupt on CPU #1 while it is in the wait state.
+	 * We're going to perform an SCLP service call but expect the
+	 * interrupt on CPU #1 while it is in the wait state.
 	 */
 	sclp_mark_busy();
 
@@ -69,11 +58,8 @@ static void test_wait_state_delivery(void)
 	psw.mask = extract_psw_mask();
 	psw.addr = (unsigned long)wait_for_sclp_int;
 	ret = smp_cpu_setup(1, psw);
-	if (ret) {
-		sclp_clear_busy();
-		report_skip("cpu #1 not found");
-		goto out;
-	}
+	/* This must not fail because we have at least 3 CPUs */
+	assert(!ret);
 
 	/*
 	 * We'd have to jump trough some hoops to sense e.g., via SIGP
-- 
2.34.1

[kvm-unit-tests PATCH v2 5/6] s390x: skrf: use CPU indexes instead of addresses

[Claudio Imbrenda]

Adapt the test to the new semantics of the smp_* functions, and use CPU
indexes instead of addresses.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 s390x/skrf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/s390x/skrf.c b/s390x/skrf.c
index ca4efbf1..b9a2e902 100644
--- a/s390x/skrf.c
+++ b/s390x/skrf.c
@@ -176,7 +176,7 @@ static void test_exception_ext_new(void)
 	wait_for_flag();
 	set_flag(0);
 
-	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
+	smp_sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
 	wait_for_flag();
 	smp_cpu_stop(1);
 	report_prefix_pop();
-- 
2.34.1

[kvm-unit-tests PATCH v2 6/6] s390x: uv-host: use CPU indexes instead of addresses

[Claudio Imbrenda]

Adapt the test to the new semantics of the smp_* functions, and use CPU
indexes instead of addresses.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 s390x/uv-host.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 92a41069..a3d45d63 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -267,12 +267,12 @@ static void test_config_create(void)
 	uvcb_cgc.conf_base_stor_origin = tmp;
 
 	if (smp_query_num_cpus() == 1) {
-		sigp_retry(1, SIGP_SET_PREFIX,
+		smp_sigp_retry(1, SIGP_SET_PREFIX,
 			   uvcb_cgc.conf_var_stor_origin + PAGE_SIZE, NULL);
 		rc = uv_call(0, (uint64_t)&uvcb_cgc);
 		report(uvcb_cgc.header.rc == 0x10e && rc == 1 &&
 		       !uvcb_cgc.guest_handle, "variable storage area contains lowcore");
-		sigp_retry(1, SIGP_SET_PREFIX, 0x0, NULL);
+		smp_sigp_retry(1, SIGP_SET_PREFIX, 0x0, NULL);
 	}
 
 	tmp = uvcb_cgc.guest_sca;
-- 
2.34.1

Re: [kvm-unit-tests PATCH v2 1/6] lib: s390x: smp: guarantee that boot CPU has index 0

[Nico Boehr]

On Fri, 2022-02-04 at 14:08 +0100, 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>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH v2 2/6] lib: s390x: smp: refactor smp functions to accept indexes

[Nico Boehr]

On Fri, 2022-02-04 at 14:08 +0100, Claudio Imbrenda wrote:
> Refactor all the smp_* functions to accept CPU indexes instead of CPU
> addresses.
> 
> Add SIGP wrappers to use indexes instead of addresses. Raw SIGP calls
> using addresses are still possible.
> 
> Add a few other useful functions to deal with CPU indexes.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH v2 3/6] s390x: smp: use CPU indexes instead of addresses

[Nico Boehr]

On Fri, 2022-02-04 at 14:08 +0100, Claudio Imbrenda wrote:
> Adapt the test to the new semantics of the smp_* functions, and use
> CPU
> indexes instead of addresses.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm..com>

Re: [kvm-unit-tests PATCH v2 4/6] s390x: firq: use CPU indexes instead of addresses

[Nico Boehr]

On Fri, 2022-02-04 at 14:08 +0100, Claudio Imbrenda wrote:
> Adapt the test to the new semantics of the smp_* functions, and use
> CPU
> indexes instead of addresses.
> 
> replace the checks with asserts, the 3 CPUs are guaranteed to be
> there.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH v2 5/6] s390x: skrf: use CPU indexes instead of addresses

[Nico Boehr]

On Fri, 2022-02-04 at 14:08 +0100, Claudio Imbrenda wrote:
> Adapt the test to the new semantics of the smp_* functions, and use
> CPU
> indexes instead of addresses.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH v2 6/6] s390x: uv-host: use CPU indexes instead of addresses

[Nico Boehr]

On Fri, 2022-02-04 at 14:08 +0100, Claudio Imbrenda wrote:
> Adapt the test to the new semantics of the smp_* functions, and use
> CPU
> indexes instead of addresses.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  s390x/uv-host.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index 92a41069..a3d45d63 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -267,12 +267,12 @@ static void test_config_create(void)
>         uvcb_cgc.conf_base_stor_origin = tmp;
>  
>         if (smp_query_num_cpus() == 1) {
> -               sigp_retry(1, SIGP_SET_PREFIX,
> +               smp_sigp_retry(1, SIGP_SET_PREFIX,

As smp_query_num_cpus() == 1, smp_sigp* will always run into an
assert() here.

Re: [kvm-unit-tests PATCH v2 1/6] lib: s390x: smp: guarantee that boot CPU has index 0

[Steffen Eiden]

On 2/4/22 14:08, 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>

Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>

Re: [kvm-unit-tests PATCH v2 2/6] lib: s390x: smp: refactor smp functions to accept indexes

[Steffen Eiden]

On 2/4/22 14:08, Claudio Imbrenda wrote:
> Refactor all the smp_* functions to accept CPU indexes instead of CPU
> addresses.
> 
> Add SIGP wrappers to use indexes instead of addresses. Raw SIGP calls
> using addresses are still possible.
> 
> Add a few other useful functions to deal with CPU indexes.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>   lib/s390x/smp.h |  20 ++++---
>   lib/s390x/smp.c | 148 ++++++++++++++++++++++++++++--------------------
>   2 files changed, 99 insertions(+), 69 deletions(-)
> 
> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
> index a2609f11..1e69a7de 100644
> --- a/lib/s390x/smp.h
> +++ b/lib/s390x/smp.h
> @@ -37,15 +37,19 @@ struct cpu_status {
>   
>   int smp_query_num_cpus(void);
>   struct cpu *smp_cpu_from_addr(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);
> -int smp_cpu_start(uint16_t addr, struct psw psw);
> -int smp_cpu_stop(uint16_t addr);
> -int smp_cpu_stop_store_status(uint16_t addr);
> -int smp_cpu_destroy(uint16_t addr);
> -int smp_cpu_setup(uint16_t addr, struct psw psw);
> +struct cpu *smp_cpu_from_idx(uint16_t idx);
> +uint16_t smp_cpu_addr(uint16_t idx);
> +bool smp_cpu_stopped(uint16_t idx);
> +bool smp_sense_running_status(uint16_t idx);
> +int smp_cpu_restart(uint16_t idx);
> +int smp_cpu_start(uint16_t idx, struct psw psw);
> +int smp_cpu_stop(uint16_t idx);
> +int smp_cpu_stop_store_status(uint16_t idx);
> +int smp_cpu_destroy(uint16_t idx);
> +int smp_cpu_setup(uint16_t idx, struct psw psw);
>   void smp_teardown(void);
>   void smp_setup(void);
> +int smp_sigp(uint16_t idx, uint8_t order, unsigned long parm, uint32_t *status);
> +int smp_sigp_retry(uint16_t idx, uint8_t order, unsigned long parm, uint32_t *status);
>   
>   #endif
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index eae742d2..dde79274 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -29,11 +29,28 @@ static struct spinlock lock;
>   
>   extern void smp_cpu_setup_state(void);
>   
> +static void check_idx(uint16_t idx)
> +{
> +	assert(idx < smp_query_num_cpus());
> +}
> +
>   int smp_query_num_cpus(void)
>   {
>   	return sclp_get_cpu_num();
>   }
>   
> +int smp_sigp(uint16_t idx, uint8_t order, unsigned long parm, uint32_t *status)
> +{
> +	check_idx(idx);
> +	return sigp(cpus[idx].addr, order, parm, status);
> +}
> +
> +int smp_sigp_retry(uint16_t idx, uint8_t order, unsigned long parm, uint32_t *status)
> +{
> +	check_idx(idx);
> +	return sigp_retry(cpus[idx].addr, order, parm, status);
> +}
> +
>   struct cpu *smp_cpu_from_addr(uint16_t addr)
>   {
>   	int i, num = smp_query_num_cpus();
> @@ -45,174 +62,183 @@ struct cpu *smp_cpu_from_addr(uint16_t addr)
>   	return NULL;
>   } >
> -bool smp_cpu_stopped(uint16_t addr)
> +struct cpu *smp_cpu_from_idx(uint16_t idx)
> +{
> +	check_idx(idx);
> +	return &cpus[idx];
> +}
> +
> +uint16_t smp_cpu_addr(uint16_t idx)
> +{
> +	check_idx(idx);
> +	return cpus[idx].addr;
> +}
> +
> +bool smp_cpu_stopped(uint16_t idx)
>   {
>   	uint32_t status;
>   
> -	if (sigp(addr, SIGP_SENSE, 0, &status) != SIGP_CC_STATUS_STORED)
> +	if (smp_sigp(idx, SIGP_SENSE, 0, &status) != SIGP_CC_STATUS_STORED)
>   		return false;
>   	return !!(status & (SIGP_STATUS_CHECK_STOP|SIGP_STATUS_STOPPED));
>   }
>   
> -bool smp_sense_running_status(uint16_t addr)
> +bool smp_sense_running_status(uint16_t idx)
>   {
> -	if (sigp(addr, SIGP_SENSE_RUNNING, 0, NULL) != SIGP_CC_STATUS_STORED)
> +	if (smp_sigp(idx, SIGP_SENSE_RUNNING, 0, NULL) != SIGP_CC_STATUS_STORED)
>   		return true;
>   	/* Status stored condition code is equivalent to cpu not running. */
>   	return false;
>   }
>   
> -static int smp_cpu_stop_nolock(uint16_t addr, bool store)
> +static int smp_cpu_stop_nolock(uint16_t idx, bool store)
>   {
> -	struct cpu *cpu;
>   	uint8_t order = store ? SIGP_STOP_AND_STORE_STATUS : SIGP_STOP;
>   
> -	cpu = smp_cpu_from_addr(addr);
> -	if (!cpu || addr == cpus[0].addr)
> +	/* refuse to work on the boot CPU */
> +	if (idx == 0)
>   		return -1;
>   
> -	if (sigp_retry(addr, order, 0, NULL))
> +	if (smp_sigp_retry(idx, order, 0, NULL))
>   		return -1;
>   
> -	while (!smp_cpu_stopped(addr))
> +	while (!smp_cpu_stopped(idx))
>   		mb();
> -	cpu->active = false;
> +	/* idx has been already checked by the smp_* functions called above */
> +	cpus[idx].active = false;
>   	return 0;
>   }
>   
> -int smp_cpu_stop(uint16_t addr)
> +int smp_cpu_stop(uint16_t idx)
>   {
>   	int rc;
>   
>   	spin_lock(&lock);
> -	rc = smp_cpu_stop_nolock(addr, false);
> +	rc = smp_cpu_stop_nolock(idx, false);
>   	spin_unlock(&lock);
>   	return rc;
>   }
>   
> -int smp_cpu_stop_store_status(uint16_t addr)
> +int smp_cpu_stop_store_status(uint16_t idx)
>   {
>   	int rc;
>   
>   	spin_lock(&lock);
> -	rc = smp_cpu_stop_nolock(addr, true);
> +	rc = smp_cpu_stop_nolock(idx, true);
>   	spin_unlock(&lock);
>   	return rc;
>   }
>   
> -static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw)
> +static int smp_cpu_restart_nolock(uint16_t idx, struct psw *psw)
>   {
>   	int rc;
> -	struct cpu *cpu = smp_cpu_from_addr(addr);
>   
> -	if (!cpu)
> -		return -1;
> +	check_idx(idx);
>   	if (psw) {
> -		cpu->lowcore->restart_new_psw.mask = psw->mask;
> -		cpu->lowcore->restart_new_psw.addr = psw->addr;
> +		cpus[idx].lowcore->restart_new_psw.mask = psw->mask;
> +		cpus[idx].lowcore->restart_new_psw.addr = psw->addr;
>   	}
>   	/*
>   	 * Stop the cpu, so we don't have a race between a running cpu
>   	 * and the restart in the test that checks if the cpu is
>   	 * running after the restart.
>   	 */
> -	smp_cpu_stop_nolock(addr, false);
> -	rc = sigp(addr, SIGP_RESTART, 0, NULL);
> +	smp_cpu_stop_nolock(idx, false);
> +	rc = sigp(cpus[idx].addr, SIGP_RESTART, 0, NULL);

What about using the smp wrapper 'smp_sigp(idx, SIGP_RESTART, 0, NULL)' 
here as well?


>   	if (rc)
>   		return rc;
>   	/*
>   	 * The order has been accepted, but the actual restart may not
>   	 * have been performed yet, so wait until the cpu is running.
>   	 */
> -	while (smp_cpu_stopped(addr))
> +	while (smp_cpu_stopped(idx))
>   		mb();
> -	cpu->active = true;
> +	cpus[idx].active = true;
>   	return 0;
>   }
>   
> -int smp_cpu_restart(uint16_t addr)
> +int smp_cpu_restart(uint16_t idx)
>   {
>   	int rc;
>   
>   	spin_lock(&lock);
> -	rc = smp_cpu_restart_nolock(addr, NULL);
> +	rc = smp_cpu_restart_nolock(idx, NULL);
>   	spin_unlock(&lock);
>   	return rc;
>   }
>   
> -int smp_cpu_start(uint16_t addr, struct psw psw)
> +int smp_cpu_start(uint16_t idx, struct psw psw)
>   {
>   	int rc;
>   
>   	spin_lock(&lock);
> -	rc = smp_cpu_restart_nolock(addr, &psw);
> +	rc = smp_cpu_restart_nolock(idx, &psw);
>   	spin_unlock(&lock);
>   	return rc;
>   }
>   
> -int smp_cpu_destroy(uint16_t addr)
> +int smp_cpu_destroy(uint16_t idx)
>   {
> -	struct cpu *cpu;
>   	int rc;
>   
>   	spin_lock(&lock);
> -	rc = smp_cpu_stop_nolock(addr, false);
> +	rc = smp_cpu_stop_nolock(idx, false);
>   	if (!rc) {
> -		cpu = smp_cpu_from_addr(addr);
> -		free_pages(cpu->lowcore);
> -		free_pages(cpu->stack);
> -		cpu->lowcore = (void *)-1UL;
> -		cpu->stack = (void *)-1UL;
> +		free_pages(cpus[idx].lowcore);
> +		free_pages(cpus[idx].stack);
> +		cpus[idx].lowcore = (void *)-1UL;
> +		cpus[idx].stack = (void *)-1UL;
>   	}
>   	spin_unlock(&lock);
>   	return rc;
>   }
>   
> -int smp_cpu_setup(uint16_t addr, struct psw psw)
> +static int smp_cpu_setup_nolock(uint16_t idx, struct psw psw)
>   {
>   	struct lowcore *lc;
> -	struct cpu *cpu;
> -	int rc = -1;
> -
> -	spin_lock(&lock);
> -
> -	if (!cpus)
> -		goto out;
>   
> -	cpu = smp_cpu_from_addr(addr);
> -
> -	if (!cpu || cpu->active)
> -		goto out;
> +	if (cpus[idx].active)
> +		return -1;
>   
> -	sigp_retry(cpu->addr, SIGP_INITIAL_CPU_RESET, 0, NULL);
> +	sigp_retry(cpus[idx].addr, SIGP_INITIAL_CPU_RESET, 0, NULL);

You may want to use the smp wrapper 'smp_sigp_retry' here.

>   
>   	lc = alloc_pages_flags(1, AREA_DMA31);
> -	cpu->lowcore = lc;
> -	memset(lc, 0, PAGE_SIZE * 2);
> -	sigp_retry(cpu->addr, SIGP_SET_PREFIX, (unsigned long )lc, NULL);
> +	cpus[idx].lowcore = lc;
> +	sigp_retry(cpus[idx].addr, SIGP_SET_PREFIX, (unsigned long )lc, NULL);
smp_sigp_retry

>   
>   	/* Copy all exception psws. */
>   	memcpy(lc, cpus[0].lowcore, 512);
>   
>   	/* Setup stack */
> -	cpu->stack = (uint64_t *)alloc_pages(2);
> +	cpus[idx].stack = (uint64_t *)alloc_pages(2);
>   
>   	/* Start without DAT and any other mask bits. */
> -	cpu->lowcore->sw_int_psw.mask = psw.mask;
> -	cpu->lowcore->sw_int_psw.addr = psw.addr;
> -	cpu->lowcore->sw_int_grs[14] = psw.addr;
> -	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
> +	lc->sw_int_psw.mask = psw.mask;
> +	lc->sw_int_psw.addr = psw.addr;
> +	lc->sw_int_grs[14] = psw.addr;
> +	lc->sw_int_grs[15] = (uint64_t)cpus[idx].stack + (PAGE_SIZE * 4);
>   	lc->restart_new_psw.mask = PSW_MASK_64;
>   	lc->restart_new_psw.addr = (uint64_t)smp_cpu_setup_state;
>   	lc->sw_int_crs[0] = BIT_ULL(CTL0_AFP);
>   
>   	/* Start processing */
> -	smp_cpu_restart_nolock(addr, NULL);
> +	smp_cpu_restart_nolock(idx, NULL);
>   	/* Wait until the cpu has finished setup and started the provided psw */
>   	while (lc->restart_new_psw.addr != psw.addr)
>   		mb();
> -	rc = 0;
> -out:
> +
> +	return 0;
> +}
> +
> +int smp_cpu_setup(uint16_t idx, struct psw psw)
> +{
> +	int rc = -1;
> +
> +	spin_lock(&lock);
> +	if (cpus) {
> +		check_idx(idx);
> +		rc = smp_cpu_setup_nolock(idx, psw);
> +	}
>   	spin_unlock(&lock);
>   	return rc;
>   }
> 

With my nits fixed:

Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>

Re: [kvm-unit-tests PATCH v2 3/6] s390x: smp: use CPU indexes instead of addresses

[Steffen Eiden]

On 2/4/22 14:08, Claudio Imbrenda wrote:
> Adapt the test to the new semantics of the smp_* functions, and use CPU
> indexes instead of addresses.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>

Re: [kvm-unit-tests PATCH v2 4/6] s390x: firq: use CPU indexes instead of addresses

[Steffen Eiden]

On 2/4/22 14:08, Claudio Imbrenda wrote:
> Adapt the test to the new semantics of the smp_* functions, and use CPU
> indexes instead of addresses.
> 
> replace the checks with asserts, the 3 CPUs are guaranteed to be there.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>

Re: [kvm-unit-tests PATCH v2 2/6] lib: s390x: smp: refactor smp functions to accept indexes

[Claudio Imbrenda]

On Tue, 15 Feb 2022 12:09:53 +0100
Steffen Eiden <seiden@linux.ibm.com> wrote:

[...]

> What about using the smp wrapper 'smp_sigp(idx, SIGP_RESTART, 0, NULL)' 
> here as well?

[...]

> With my nits fixed:

maybe I should add a comment explaining why I did not use the smp_
variants.

the reason is that the smp_ variants check the validity of the CPU
index. but in those places, we have already checked (directly or
indirectly) that the index is valid, so I save one useless check.

on the other hand, I don't know if it makes sense to optimize for that,
since it's not a hot path, and one extra check will not kill the
performance.

> 
> Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>

Re: [kvm-unit-tests PATCH v2 5/6] s390x: skrf: use CPU indexes instead of addresses

[Steffen Eiden]

On 2/4/22 14:08, Claudio Imbrenda wrote:
> Adapt the test to the new semantics of the smp_* functions, and use CPU
> indexes instead of addresses.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>

Re: [kvm-unit-tests PATCH v2 6/6] s390x: uv-host: use CPU indexes instead of addresses

[Steffen Eiden]

On 2/4/22 14:08, Claudio Imbrenda wrote:
> Adapt the test to the new semantics of the smp_* functions, and use CPU
> indexes instead of addresses.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>

Re: [kvm-unit-tests PATCH v2 2/6] lib: s390x: smp: refactor smp functions to accept indexes

[Steffen Eiden]

On 2/15/22 12:23, Claudio Imbrenda wrote:
> On Tue, 15 Feb 2022 12:09:53 +0100
> Steffen Eiden <seiden@linux.ibm.com> wrote:
> 
> [...]
> 
>> What about using the smp wrapper 'smp_sigp(idx, SIGP_RESTART, 0, NULL)'
>> here as well?
> 
> [...]
> 
>> With my nits fixed:
> 
> maybe I should add a comment explaining why I did not use the smp_
> variants.
> 
> the reason is that the smp_ variants check the validity of the CPU
> index. but in those places, we have already checked (directly or
> indirectly) that the index is valid, so I save one useless check.
> > on the other hand, I don't know if it makes sense to optimize for that,
> since it's not a hot path, and one extra check will not kill the
> performance.
>
I would prefer the use of the smp_ variant. The extra assert won't 
clutter the output and the code is more consistent.
However, a short comment is also fine for me if you prefer that.


>>
>> Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
> 

Re: [kvm-unit-tests PATCH v2 2/6] lib: s390x: smp: refactor smp functions to accept indexes

[Claudio Imbrenda]

On Tue, 15 Feb 2022 12:43:15 +0100
Steffen Eiden <seiden@linux.ibm.com> wrote:

> On 2/15/22 12:23, Claudio Imbrenda wrote:
> > On Tue, 15 Feb 2022 12:09:53 +0100
> > Steffen Eiden <seiden@linux.ibm.com> wrote:
> > 
> > [...]
> >   
> >> What about using the smp wrapper 'smp_sigp(idx, SIGP_RESTART, 0, NULL)'
> >> here as well?  
> > 
> > [...]
> >   
> >> With my nits fixed:  
> > 
> > maybe I should add a comment explaining why I did not use the smp_
> > variants.
> > 
> > the reason is that the smp_ variants check the validity of the CPU
> > index. but in those places, we have already checked (directly or
> > indirectly) that the index is valid, so I save one useless check.  
> > > on the other hand, I don't know if it makes sense to optimize for that,  
> > since it's not a hot path, and one extra check will not kill the
> > performance.
> >  
> I would prefer the use of the smp_ variant. The extra assert won't 
> clutter the output and the code is more consistent.
> However, a short comment is also fine for me if you prefer that.

I guess I'll use the smp_ variant and add a few lines in the patch
description to explain that we're doing some extra checks, but the code
is more readable

> 
> >>
> >> Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>  
> >   

Re: [kvm-unit-tests PATCH v2 2/6] lib: s390x: smp: refactor smp functions to accept indexes

[Janosch Frank]

On 2/15/22 12:54, Claudio Imbrenda wrote:
> On Tue, 15 Feb 2022 12:43:15 +0100
> Steffen Eiden <seiden@linux.ibm.com> wrote:
> 
>> On 2/15/22 12:23, Claudio Imbrenda wrote:
>>> On Tue, 15 Feb 2022 12:09:53 +0100
>>> Steffen Eiden <seiden@linux.ibm.com> wrote:
>>>
>>> [...]
>>>    
>>>> What about using the smp wrapper 'smp_sigp(idx, SIGP_RESTART, 0, NULL)'
>>>> here as well?
>>>
>>> [...]
>>>    
>>>> With my nits fixed:
>>>
>>> maybe I should add a comment explaining why I did not use the smp_
>>> variants.
>>>
>>> the reason is that the smp_ variants check the validity of the CPU
>>> index. but in those places, we have already checked (directly or
>>> indirectly) that the index is valid, so I save one useless check.
>>>> on the other hand, I don't know if it makes sense to optimize for that,
>>> since it's not a hot path, and one extra check will not kill the
>>> performance.
>>>   
>> I would prefer the use of the smp_ variant. The extra assert won't
>> clutter the output and the code is more consistent.
>> However, a short comment is also fine for me if you prefer that.
> 
> I guess I'll use the smp_ variant and add a few lines in the patch
> description to explain that we're doing some extra checks, but the code
> is more readable
> 
>>
>>>>
>>>> Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
>>>    
> 

Doesn't make a difference to me as you use cpu.addr in the sigp_ which 
tells me it's a cpu address and not an idx.