Re: [PATCH v2 08/10] s390x: smp: Wait for sigp completion

[David Hildenbrand <david@redhat.com>]

On 23.04.20 11:10, Janosch Frank wrote:
> Sigp orders are not necessarily finished when the processor finished
> the sigp instruction. We need to poll if the order has been finished
> before we continue.
> 
> For (re)start and stop we already use sigp sense running and sigp
> sense loops. But we still lack completion checks for stop and store
> status, as well as the cpu resets.
> 
> Let's add them.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  lib/s390x/smp.c | 8 ++++++++
>  lib/s390x/smp.h | 1 +
>  s390x/smp.c     | 4 ++++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index 6ef0335..2555bf4 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -154,6 +154,14 @@ int smp_cpu_start(uint16_t addr, struct psw psw)
>  	return rc;
>  }
>  
> +void smp_cpu_wait_for_completion(uint16_t addr)
> +{
> +	uint32_t status;
> +
> +	/* Loops when cc == 2, i.e. when the cpu is busy with a sigp order */
> +	sigp_retry(1, SIGP_SENSE, 0, &status);
> +}
> +
>  int smp_cpu_destroy(uint16_t addr)
>  {
>  	struct cpu *cpu;
> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
> index ce63a89..a8b98c0 100644
> --- a/lib/s390x/smp.h
> +++ b/lib/s390x/smp.h
> @@ -45,6 +45,7 @@ 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);
> +void smp_cpu_wait_for_completion(uint16_t addr);
>  int smp_cpu_destroy(uint16_t addr);
>  int smp_cpu_setup(uint16_t addr, struct psw psw);
>  void smp_teardown(void);
> diff --git a/s390x/smp.c b/s390x/smp.c
> index 7462211..48321f4 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -75,6 +75,7 @@ static void test_stop_store_status(void)
>  	lc->prefix_sa = 0;
>  	lc->grs_sa[15] = 0;
>  	smp_cpu_stop_store_status(1);
> +	smp_cpu_wait_for_completion(1);
>  	mb();
>  	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
>  	report(lc->grs_sa[15], "stack");
> @@ -85,6 +86,7 @@ static void test_stop_store_status(void)
>  	lc->prefix_sa = 0;
>  	lc->grs_sa[15] = 0;
>  	smp_cpu_stop_store_status(1);

Just curious: Would it make sense to add that inside
smp_cpu_stop_store_status() instead?

-- 
Thanks,

David / dhildenb