diff mbox series

[kvm-unit-tests,v3,2/3] s390x: smp: use an array for sigp calls

Message ID 20220725155420.2009109-3-nrb@linux.ibm.com (mailing list archive)
State Superseded, archived
Headers show
Series s390x: add tests for SIGP call orders in enabled wait | expand

Commit Message

Nico Boehr July 25, 2022, 3:54 p.m. UTC 
Tests for the SIGP calls are quite similar, so we have a lot of code
duplication right now. Since upcoming changes will add more cases,
refactor the code to iterate over an array, similarily as we already do
for test_invalid().

The receiving CPU is disabled for IO interrupts. This makes sure the
conditional emergency signal is accepted and doesn't hurt the other
orders.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/smp.c | 124 ++++++++++++++++++++--------------------------------
 1 file changed, 48 insertions(+), 76 deletions(-)

Comments

Claudio Imbrenda July 28, 2022, 3:47 p.m. UTC | #1 
On Mon, 25 Jul 2022 17:54:19 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> Tests for the SIGP calls are quite similar, so we have a lot of code
> duplication right now. Since upcoming changes will add more cases,
> refactor the code to iterate over an array, similarily as we already do
> for test_invalid().
> 
> The receiving CPU is disabled for IO interrupts. This makes sure the
> conditional emergency signal is accepted and doesn't hurt the other
> orders.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/smp.c | 124 ++++++++++++++++++++--------------------------------
>  1 file changed, 48 insertions(+), 76 deletions(-)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index 34ae91c3fe12..12c40cadaed2 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -43,6 +43,20 @@ static const struct sigp_invalid_cases cases_valid_cpu_addr[] = {
>  
>  static uint32_t cpu1_prefix;
>  
> +struct sigp_call_cases {
> +	char name[20];
> +	int call;
> +	uint16_t ext_int_expected_type;
> +	uint32_t cr0_bit;

does it need to be 32 bits? the range of valid values is 0 ~ 63
bonus, if you use an uint8_t, the whole struct will shrink by 8 bytes

with that fixed:

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

(unless there is a good enough reason to use uint32_t)

> +	bool supports_pv;
> +};
> +static const struct sigp_call_cases cases_sigp_call[] = {
> +	{ "emcall",      SIGP_EMERGENCY_SIGNAL,      0x1201, CTL0_EMERGENCY_SIGNAL, true },
> +	{ "cond emcall", SIGP_COND_EMERGENCY_SIGNAL, 0x1201, CTL0_EMERGENCY_SIGNAL, false },
> +	{ "ecall",       SIGP_EXTERNAL_CALL,         0x1202, CTL0_EXTERNAL_CALL,    true },
> +};
> +static const struct sigp_call_cases *current_sigp_call_case;
> +
>  static void test_invalid(void)
>  {
>  	const struct sigp_invalid_cases *c;
> @@ -289,97 +303,57 @@ static void test_set_prefix(void)
>  
>  }
>  
> -static void ecall(void)
> +static void call_received(void)
>  {
>  	expect_ext_int();
> -	ctl_set_bit(0, CTL0_EXTERNAL_CALL);
> -	psw_mask_set_bits(PSW_MASK_EXT);
> -	set_flag(1);
> -	while (lowcore.ext_int_code != 0x1202) { mb(); }
> -	report_pass("received");
> -	set_flag(1);
> -}
> +	ctl_set_bit(0, current_sigp_call_case->cr0_bit);
> +	/* make sure conditional emergency is accepted by disabling IO interrupts */
> +	psw_mask_clear_and_set_bits(PSW_MASK_IO, PSW_MASK_EXT);
>  
> -static void test_ecall(void)
> -{
> -	struct psw psw;
> -	psw.mask = extract_psw_mask();
> -	psw.addr = (unsigned long)ecall;
> +	/* Indicate that we're ready to receive the call */
> +	set_flag(1);
>  
> -	report_prefix_push("ecall");
> -	set_flag(0);
> +	while (lowcore.ext_int_code != current_sigp_call_case->ext_int_expected_type)
> +		mb();
> +	report_pass("received");
>  
> -	smp_cpu_start(1, psw);
> -	wait_for_flag();
> -	set_flag(0);
> -	smp_sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
> -	wait_for_flag();
> -	smp_cpu_stop(1);
> -	report_prefix_pop();
> -}
> +	ctl_clear_bit(0, current_sigp_call_case->cr0_bit);
>  
> -static void emcall(void)
> -{
> -	expect_ext_int();
> -	ctl_set_bit(0, CTL0_EMERGENCY_SIGNAL);
> -	psw_mask_set_bits(PSW_MASK_EXT);
> -	set_flag(1);
> -	while (lowcore.ext_int_code != 0x1201) { mb(); }
> -	report_pass("received");
> +	/* Indicate that we're done */
>  	set_flag(1);
>  }
>  
> -static void test_emcall(void)
> +static void test_calls(void)
>  {
> +	int i;
>  	struct psw psw;
> -	psw.mask = extract_psw_mask();
> -	psw.addr = (unsigned long)emcall;
>  
> -	report_prefix_push("emcall");
> -	set_flag(0);
> +	for (i = 0; i < ARRAY_SIZE(cases_sigp_call); i++) {
> +		current_sigp_call_case = &cases_sigp_call[i];
>  
> -	smp_cpu_start(1, psw);
> -	wait_for_flag();
> -	set_flag(0);
> -	smp_sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
> -	wait_for_flag();
> -	smp_cpu_stop(1);
> +		report_prefix_push(current_sigp_call_case->name);
> +		if (!current_sigp_call_case->supports_pv && uv_os_is_guest()) {
> +			report_skip("Not supported under PV");
> +			report_prefix_pop();
> +			continue;
> +		}
>  
> -	report_prefix_pop();
> -}
> +		set_flag(0);
> +		psw.mask = extract_psw_mask();
> +		psw.addr = (unsigned long)call_received;
> +		smp_cpu_start(1, psw);
>  
> -static void test_cond_emcall(void)
> -{
> -	uint32_t status = 0;
> -	struct psw psw;
> -	int cc;
> -	psw.mask = extract_psw_mask() & ~PSW_MASK_IO;
> -	psw.addr = (unsigned long)emcall;
> +		/* Wait until the receiver has finished setup */
> +		wait_for_flag();
> +		set_flag(0);
>  
> -	report_prefix_push("conditional emergency call");
> +		smp_sigp(1, current_sigp_call_case->call, 0, NULL);
>  
> -	if (uv_os_is_guest()) {
> -		report_skip("unsupported under PV");
> -		goto out;
> +		/* Wait until the receiver has handled the call */
> +		wait_for_flag();
> +		smp_cpu_stop(1);
> +		report_prefix_pop();
>  	}
> -
> -	report_prefix_push("success");
> -	set_flag(0);
> -
> -	smp_cpu_start(1, psw);
> -	wait_for_flag();
> -	set_flag(0);
> -	cc = smp_sigp(1, SIGP_COND_EMERGENCY_SIGNAL, 0, &status);
> -	report(!cc, "CC = 0");
> -
> -	wait_for_flag();
> -	smp_cpu_stop(1);
> -
> -	report_prefix_pop();
> -
> -out:
> -	report_prefix_pop();
> -
>  }
>  
>  static void test_sense_running(void)
> @@ -499,9 +473,7 @@ int main(void)
>  	test_stop_store_status();
>  	test_store_status();
>  	test_set_prefix();
> -	test_ecall();
> -	test_emcall();
> -	test_cond_emcall();
> +	test_calls();
>  	test_sense_running();
>  	test_reset();
>  	test_reset_initial();
Nico Boehr Aug. 2, 2022, 12:28 p.m. UTC | #2 
Quoting Claudio Imbrenda (2022-07-28 17:47:35)
[...]
> > diff --git a/s390x/smp.c b/s390x/smp.c
> > index 34ae91c3fe12..12c40cadaed2 100644
> > --- a/s390x/smp.c
> > +++ b/s390x/smp.c
> > @@ -43,6 +43,20 @@ static const struct sigp_invalid_cases cases_valid_cpu_addr[] = {
> >  
> >  static uint32_t cpu1_prefix;
> >  
> > +struct sigp_call_cases {
> > +     char name[20];
> > +     int call;
> > +     uint16_t ext_int_expected_type;
> > +     uint32_t cr0_bit;
> 
> does it need to be 32 bits? the range of valid values is 0 ~ 63
> bonus, if you use an uint8_t, the whole struct will shrink by 8 bytes

uint32_t is clearly inappropriate here. Since I see little reason to optimize for size here, I would argue for int, but I am also ok with uint8_t.
diff mbox series

Patch

diff --git a/s390x/smp.c b/s390x/smp.c
index 34ae91c3fe12..12c40cadaed2 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -43,6 +43,20 @@  static const struct sigp_invalid_cases cases_valid_cpu_addr[] = {
 
 static uint32_t cpu1_prefix;
 
+struct sigp_call_cases {
+	char name[20];
+	int call;
+	uint16_t ext_int_expected_type;
+	uint32_t cr0_bit;
+	bool supports_pv;
+};
+static const struct sigp_call_cases cases_sigp_call[] = {
+	{ "emcall",      SIGP_EMERGENCY_SIGNAL,      0x1201, CTL0_EMERGENCY_SIGNAL, true },
+	{ "cond emcall", SIGP_COND_EMERGENCY_SIGNAL, 0x1201, CTL0_EMERGENCY_SIGNAL, false },
+	{ "ecall",       SIGP_EXTERNAL_CALL,         0x1202, CTL0_EXTERNAL_CALL,    true },
+};
+static const struct sigp_call_cases *current_sigp_call_case;
+
 static void test_invalid(void)
 {
 	const struct sigp_invalid_cases *c;
@@ -289,97 +303,57 @@  static void test_set_prefix(void)
 
 }
 
-static void ecall(void)
+static void call_received(void)
 {
 	expect_ext_int();
-	ctl_set_bit(0, CTL0_EXTERNAL_CALL);
-	psw_mask_set_bits(PSW_MASK_EXT);
-	set_flag(1);
-	while (lowcore.ext_int_code != 0x1202) { mb(); }
-	report_pass("received");
-	set_flag(1);
-}
+	ctl_set_bit(0, current_sigp_call_case->cr0_bit);
+	/* make sure conditional emergency is accepted by disabling IO interrupts */
+	psw_mask_clear_and_set_bits(PSW_MASK_IO, PSW_MASK_EXT);
 
-static void test_ecall(void)
-{
-	struct psw psw;
-	psw.mask = extract_psw_mask();
-	psw.addr = (unsigned long)ecall;
+	/* Indicate that we're ready to receive the call */
+	set_flag(1);
 
-	report_prefix_push("ecall");
-	set_flag(0);
+	while (lowcore.ext_int_code != current_sigp_call_case->ext_int_expected_type)
+		mb();
+	report_pass("received");
 
-	smp_cpu_start(1, psw);
-	wait_for_flag();
-	set_flag(0);
-	smp_sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
-	wait_for_flag();
-	smp_cpu_stop(1);
-	report_prefix_pop();
-}
+	ctl_clear_bit(0, current_sigp_call_case->cr0_bit);
 
-static void emcall(void)
-{
-	expect_ext_int();
-	ctl_set_bit(0, CTL0_EMERGENCY_SIGNAL);
-	psw_mask_set_bits(PSW_MASK_EXT);
-	set_flag(1);
-	while (lowcore.ext_int_code != 0x1201) { mb(); }
-	report_pass("received");
+	/* Indicate that we're done */
 	set_flag(1);
 }
 
-static void test_emcall(void)
+static void test_calls(void)
 {
+	int i;
 	struct psw psw;
-	psw.mask = extract_psw_mask();
-	psw.addr = (unsigned long)emcall;
 
-	report_prefix_push("emcall");
-	set_flag(0);
+	for (i = 0; i < ARRAY_SIZE(cases_sigp_call); i++) {
+		current_sigp_call_case = &cases_sigp_call[i];
 
-	smp_cpu_start(1, psw);
-	wait_for_flag();
-	set_flag(0);
-	smp_sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
-	wait_for_flag();
-	smp_cpu_stop(1);
+		report_prefix_push(current_sigp_call_case->name);
+		if (!current_sigp_call_case->supports_pv && uv_os_is_guest()) {
+			report_skip("Not supported under PV");
+			report_prefix_pop();
+			continue;
+		}
 
-	report_prefix_pop();
-}
+		set_flag(0);
+		psw.mask = extract_psw_mask();
+		psw.addr = (unsigned long)call_received;
+		smp_cpu_start(1, psw);
 
-static void test_cond_emcall(void)
-{
-	uint32_t status = 0;
-	struct psw psw;
-	int cc;
-	psw.mask = extract_psw_mask() & ~PSW_MASK_IO;
-	psw.addr = (unsigned long)emcall;
+		/* Wait until the receiver has finished setup */
+		wait_for_flag();
+		set_flag(0);
 
-	report_prefix_push("conditional emergency call");
+		smp_sigp(1, current_sigp_call_case->call, 0, NULL);
 
-	if (uv_os_is_guest()) {
-		report_skip("unsupported under PV");
-		goto out;
+		/* Wait until the receiver has handled the call */
+		wait_for_flag();
+		smp_cpu_stop(1);
+		report_prefix_pop();
 	}
-
-	report_prefix_push("success");
-	set_flag(0);
-
-	smp_cpu_start(1, psw);
-	wait_for_flag();
-	set_flag(0);
-	cc = smp_sigp(1, SIGP_COND_EMERGENCY_SIGNAL, 0, &status);
-	report(!cc, "CC = 0");
-
-	wait_for_flag();
-	smp_cpu_stop(1);
-
-	report_prefix_pop();
-
-out:
-	report_prefix_pop();
-
 }
 
 static void test_sense_running(void)
@@ -499,9 +473,7 @@  int main(void)
 	test_stop_store_status();
 	test_store_status();
 	test_set_prefix();
-	test_ecall();
-	test_emcall();
-	test_cond_emcall();
+	test_calls();
 	test_sense_running();
 	test_reset();
 	test_reset_initial();