Re: [kvm-unit-tests PATCH v3 4/9] s390x: smp: Rework cpu start and active tracking

[David Hildenbrand <david@redhat.com>]

On 17.01.20 11:46, Janosch Frank wrote:
> The architecture specifies that processing sigp orders may be
> asynchronous, and this is indeed the case on some hypervisors, so we
> need to wait until the cpu runs before we return from the setup/start
> function.
> 
> As there was a lot of duplicate code, a common function for cpu
> restarts has been introduced.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  lib/s390x/smp.c | 50 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index f57f420..84e681d 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -104,35 +104,46 @@ int smp_cpu_stop_store_status(uint16_t addr)
>  	return rc;
>  }
>  
> +static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw)
> +{
> +	int rc;
> +	struct cpu *cpu = smp_cpu_from_addr(addr);

I'd exchange these two (reverse christmas tree)

> +
> +	if (!cpu)
> +		return -1;

-EINVAL?

> +	if (psw) {
> +		cpu->lowcore->restart_new_psw.mask = psw->mask;
> +		cpu->lowcore->restart_new_psw.addr = psw->addr;
> +	}

Does this make sense to have optional? (the other CPU will execute
random crap if not set, won't it?)

> +	rc = sigp(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_running(addr))
> +		mb();

Should you make sure to stop the CPU before issuing the restart?
Otherwise you will get false positives if it is still running (but
hasn't processed the RESTART yet)



-- 
Thanks,

David / dhildenb