On 9/2/19 3:21 PM, Thomas Huth wrote:
> 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@xxxxxxxxxxxxx>
>> ---
>> 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);
Seems like I've been writing too much assembly lately to write proper
loops :)
>
>> + 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.
Yes, if it bothers you that much :)
[...]
>> +
>> +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;
>
> ?
Great idea
>
>> +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);
That one
>
> ?
>
>> + 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)"?
Sure
>
>> + 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 ?
Yes
>
>> +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 ?
calloc does a 0 memset
But to mirror the boot cpu case, I added it.
>
>> + }
>> + }
>> + spin_unlock(&lock);
>> +}
>
> Thomas
>
Attachment:
signature.asc
Description: OpenPGP digital signature
