Re: [kvm-unit-tests PATCH 4/6] s390x: Add initial smp code

[Thomas Huth <thuth@redhat.com>]

On 29/08/2019 14.14, Janosch Frank wrote:
> Let's add a rudimentary SMP library, which will scan for cpus and has
> helper functions that manage the cpu state.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_def.h |   8 ++
>  lib/s390x/asm/sigp.h     |  29 ++++-
>  lib/s390x/io.c           |   5 +-
>  lib/s390x/sclp.h         |   1 +
>  lib/s390x/smp.c          | 272 +++++++++++++++++++++++++++++++++++++++
>  lib/s390x/smp.h          |  51 ++++++++
>  s390x/Makefile           |   1 +
>  s390x/cstart64.S         |   7 +
>  8 files changed, 368 insertions(+), 6 deletions(-)
>  create mode 100644 lib/s390x/smp.c
>  create mode 100644 lib/s390x/smp.h
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 5f8f45e..d5a7f51 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -157,6 +157,14 @@ struct cpuid {
>  	uint64_t reserved : 15;
>  };
>  
> +static inline unsigned short stap(void)
> +{
> +	unsigned short cpu_address;
> +
> +	asm volatile("stap %0" : "=Q" (cpu_address));
> +	return cpu_address;
> +}
> +
>  static inline int tprot(unsigned long addr)
>  {
>  	int cc;
> diff --git a/lib/s390x/asm/sigp.h b/lib/s390x/asm/sigp.h
> index fbd94fc..ce85eb7 100644
> --- a/lib/s390x/asm/sigp.h
> +++ b/lib/s390x/asm/sigp.h
> @@ -46,14 +46,33 @@
>  
>  #ifndef __ASSEMBLER__
>  
> -static inline void sigp_stop(void)
> +
> +static inline int sigp(uint16_t addr, uint8_t order, unsigned long parm,
> +		       uint32_t *status)
>  {
> -	register unsigned long status asm ("1") = 0;
> -	register unsigned long cpu asm ("2") = 0;
> +	register unsigned long reg1 asm ("1") = parm;
> +	int cc;
>  
>  	asm volatile(
> -		"	sigp %0,%1,0(%2)\n"
> -		: "+d" (status)  : "d" (cpu), "d" (SIGP_STOP) : "cc");
> +		"	sigp	%1,%2,0(%3)\n"
> +		"	ipm	%0\n"
> +		"	srl	%0,28\n"
> +		: "=d" (cc), "+d" (reg1) : "d" (addr), "a" (order) : "cc");
> +	if (status)
> +		*status = reg1;
> +	return cc;
> +}
> +
> +static inline int sigp_retry(uint16_t addr, uint8_t order, unsigned long parm,
> +			     uint32_t *status)
> +{
> +	int cc;
> +
> +retry:
> +	cc = sigp(addr, order, parm, status);
> +	if (cc == 2)
> +		goto retry;

Please change to:

	do {
		cc = sigp(addr, order, parm, status);
	} while (cc == 2);

> +	return cc;
>  }
>  
>  #endif /* __ASSEMBLER__ */
[...]
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> new file mode 100644
> index 0000000..b1b636a
> --- /dev/null
> +++ b/lib/s390x/smp.c
[...]
> +int smp_cpu_restart(uint16_t addr)
> +{
> +	int rc = 0;
> +	struct cpu *cpu;
> +
> +	spin_lock(&lock);
> +	cpu = smp_cpu_from_addr(addr);
> +	if (!cpu) {
> +		rc = -ENOENT;
> +		goto out;
> +	}
> +
> +	rc = sigp(cpu->addr, SIGP_RESTART, 0, NULL);

I think you could use "addr" instead of "cpu->addr" here.

> +	cpu->active = true;
> +out:
> +	spin_unlock(&lock);
> +	return rc;
> +}
> +
> +int smp_cpu_start(uint16_t addr, struct psw psw)
> +{
> +	int rc = 0;
> +	struct cpu *cpu;
> +	struct lowcore *lc;
> +
> +	spin_lock(&lock);
> +	cpu = smp_cpu_from_addr(addr);
> +	if (!cpu) {
> +		rc = -ENOENT;
> +		goto out;
> +	}
> +
> +	lc = cpu->lowcore;
> +	lc->restart_new_psw.mask = psw.mask;
> +	lc->restart_new_psw.addr = psw.addr;
> +	rc = sigp(cpu->addr, SIGP_RESTART, 0, NULL);

dito

> +out:
> +	spin_unlock(&lock);
> +	return rc;
> +}
> +
> +int smp_cpu_destroy(uint16_t addr)
> +{
> +	struct cpu *cpu;
> +	int rc = 0;
> +
> +	spin_lock(&lock);
> +	rc = smp_cpu_stop_nolock(addr, false);
> +	if (rc)
> +		goto out;
> +
> +	cpu = smp_cpu_from_addr(addr);
> +	free_pages(cpu->lowcore, 2 * PAGE_SIZE);
> +	free_pages(cpu->stack, 4 * PAGE_SIZE);

Maybe do this afterwards to make sure that nobody uses a dangling pointer:

	cpu->lowcore = cpu->stack = -1UL;

?

> +out:
> +	spin_unlock(&lock);
> +	return rc;
> +}
> +
> +int smp_cpu_setup(uint16_t addr, struct psw psw)
> +{
> +	struct lowcore *lc;
> +	struct cpu *cpu;
> +	int rc = 0;
> +
> +	spin_lock(&lock);
> +
> +	if (!cpus) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
> +	cpu = smp_cpu_from_addr(addr);
> +
> +	if (!cpu) {
> +		rc = -ENOENT;
> +		goto out;
> +	}
> +
> +	if (cpu->active) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
> +	sigp_retry(cpu->addr, SIGP_INITIAL_CPU_RESET, 0, NULL);
> +
> +	lc = alloc_pages(1);
> +	cpu->lowcore = lc;
> +	memset(lc, 0, PAGE_SIZE * 2);
> +	sigp_retry(cpu->addr, SIGP_SET_PREFIX, (unsigned long )lc, NULL);
> +
> +	/* Copy all exception psws. */
> +	memcpy(lc, cpu0->lowcore, 512);
> +
> +	/* Setup stack */
> +	cpu->stack = (uint64_t *)alloc_pages(2);
> +
> +	/* Start without DAT and any other mask bits. */
> +	cpu->lowcore->sw_int_grs[14] = psw.addr;
> +	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4) / sizeof(cpu->stack);

The end-of-stack calculation looks wrong to me. I think you either meant:

 ... = (uint64_t)(cpu->stack + (PAGE_SIZE * 4) / sizeof(*cpu->stack));

or:

 ... = (uint64_t)cpu->stack + (PAGE_SIZE * 4);

?

> +	lc->restart_new_psw.mask = 0x0000000180000000UL;
> +	lc->restart_new_psw.addr = (unsigned long)smp_cpu_setup_state;

Maybe use "(uint64_t)" instead of "(unsigned long)"?

> +	lc->sw_int_cr0 = 0x0000000000040000UL;
> +
> +	/* Start processing */
> +	cpu->active = true;
> +	rc = sigp_retry(cpu->addr, SIGP_RESTART, 0, NULL);

Should cpu->active only be set to true if rc == 0 ?

> +out:
> +	spin_unlock(&lock);
> +	return rc;
> +}
> +
> +/*
> + * Disregarding state, stop all cpus that once were online except for
> + * calling cpu.
> + */
> +void smp_teardown(void)
> +{
> +	int i = 0;
> +	uint16_t this_cpu = stap();
> +	struct ReadCpuInfo *info = (void *)cpu_info_buffer;
> +
> +	spin_lock(&lock);
> +	for (; i < info->nr_configured; i++) {
> +		if (cpus[i].active &&
> +		    cpus[i].addr != this_cpu) {
> +			sigp_retry(cpus[i].addr, SIGP_STOP, 0, NULL);

Maybe set cpus[i].active = false afterwards ?

> +		}
> +	}
> +	spin_unlock(&lock);
> +}

 Thomas