Re: [kvm-unit-tests PATCH v2 2/3] s390x: smp: use an array for sigp calls

[Janosch Frank <frankja@linux.ibm.com>]

On 7/22/22 09:20, Nico Boehr 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>

I'd like a few comments to make it more understandable. Other than that:
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>


Thanks for improving this

> ---
>   s390x/smp.c | 127 +++++++++++++++++++---------------------------------
>   1 file changed, 45 insertions(+), 82 deletions(-)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index 34ae91c3fe12..683b0e618a48 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 },
> +};

/* Indicate that we're ready to receive the call */

>   	set_flag(1);
> -	while (lowcore.ext_int_code != 0x1201) { mb(); }
> +	while (lowcore.ext_int_code != current_sigp_call_case->ext_int_expected_type)
> +		mb();
>   	report_pass("received");
> +	ctl_clear_bit(0, current_sigp_call_case->cr0_bit);

/* Indicate that we're done */

>   	set_flag(1);
>   }
[..]

>   
> -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;
>   
> -	report_prefix_push("conditional emergency call");
> -
> -	if (uv_os_is_guest()) {
> -		report_skip("unsupported under PV");
> -		goto out;
> +	for (i = 0; i < ARRAY_SIZE(cases_sigp_call); i++) {
> +		current_sigp_call_case = &cases_sigp_call[i];
> +
> +		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;
> +		}
> +
> +		set_flag(0);
> +		psw.mask = extract_psw_mask();
> +		psw.addr = (unsigned long)call_received;
> +
> +		smp_cpu_start(1, psw);

/* Wait until the receiver has finished setup */

> +		wait_for_flag();
> +		set_flag(0);
> +		smp_sigp(1, current_sigp_call_case->call, 0, NULL);

/* 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 +464,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();