[kvm-unit-tests,v2,4/7] s390x: smp: Rework cpu start and active tracking
diff mbox series

Message ID 20200116120513.2244-5-frankja@linux.ibm.com
State New
Headers show
Series
  • s390x: smp: Improve smp code and reset checks
Related show

Commit Message

Janosch Frank Jan. 16, 2020, 12:05 p.m. UTC
sigp is not synchronous on all hypervisors, so we need to wait until
the cpu runs until we return from the setup/start function.

As there was a lot of duplicate code a common function for cpu
restarts has been intropduced.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/smp.c | 45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

Comments

Cornelia Huck Jan. 16, 2020, 2:14 p.m. UTC | #1
On Thu, 16 Jan 2020 07:05:10 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> sigp is not synchronous on all hypervisors, so we need to wait until

"The architecture specifies that processing sigp orders may be
asynchronous, and this is indeed the case on some hypervisors, so..."

? (Or is that overkill?)

> the cpu runs until we return from the setup/start function.

s/until we return/before we return/

> 
> As there was a lot of duplicate code a common function for cpu

s/code/code,/

> restarts has been intropduced.

s/intropduced/introduced/

> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/smp.c | 45 ++++++++++++++++++++++++---------------------
>  1 file changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index f57f420..f984a34 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -104,35 +104,41 @@ 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);
> +
> +	if (!cpu)
> +		return -1;
> +	if (psw) {
> +		cpu->lowcore->restart_new_psw.mask = psw->mask;
> +		cpu->lowcore->restart_new_psw.addr = psw->addr;
> +	}
> +	rc = sigp(addr, SIGP_RESTART, 0, NULL);
> +	if (rc)
> +		return rc;
> +	while (!smp_cpu_running(addr)) { mb(); }

Maybe split this statement? Also, maybe add a comment

/*
 * The order has been accepted, but the actual restart may not
 * have been performed yet, so wait until the cpu is running.
 */

?

> +	cpu->active = true;
> +	return 0;
> +}

The changes look good to me AFAICS.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Janosch Frank Jan. 16, 2020, 2:44 p.m. UTC | #2
On 1/16/20 3:14 PM, Cornelia Huck wrote:
> On Thu, 16 Jan 2020 07:05:10 -0500
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> sigp is not synchronous on all hypervisors, so we need to wait until
> 
> "The architecture specifies that processing sigp orders may be
> asynchronous, and this is indeed the case on some hypervisors, so..."
> 
> ? (Or is that overkill?)
> 
>> the cpu runs until we return from the setup/start function.
> 
> s/until we return/before we return/
> 
>>
>> As there was a lot of duplicate code a common function for cpu
> 
> s/code/code,/
> 
>> restarts has been intropduced.
> 
> s/intropduced/introduced/
> 
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  lib/s390x/smp.c | 45 ++++++++++++++++++++++++---------------------
>>  1 file changed, 24 insertions(+), 21 deletions(-)
>>
>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
>> index f57f420..f984a34 100644
>> --- a/lib/s390x/smp.c
>> +++ b/lib/s390x/smp.c
>> @@ -104,35 +104,41 @@ 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);
>> +
>> +	if (!cpu)
>> +		return -1;
>> +	if (psw) {
>> +		cpu->lowcore->restart_new_psw.mask = psw->mask;
>> +		cpu->lowcore->restart_new_psw.addr = psw->addr;
>> +	}
>> +	rc = sigp(addr, SIGP_RESTART, 0, NULL);
>> +	if (rc)
>> +		return rc;
>> +	while (!smp_cpu_running(addr)) { mb(); }
> 
> Maybe split this statement? Also, maybe add a comment

/* Wait until the target cpu is running */
?

This is not QEMU with two line ifs taking up 3 lines :)

> 
> /*
>  * The order has been accepted, but the actual restart may not
>  * have been performed yet, so wait until the cpu is running.
>  */
> 
> ?
> 
>> +	cpu->active = true;
>> +	return 0;
>> +}
> 
> The changes look good to me AFAICS.
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Thanks!
Cornelia Huck Jan. 16, 2020, 3:05 p.m. UTC | #3
On Thu, 16 Jan 2020 15:44:24 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 1/16/20 3:14 PM, Cornelia Huck wrote:
> > On Thu, 16 Jan 2020 07:05:10 -0500
> > Janosch Frank <frankja@linux.ibm.com> wrote:

> >> +static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw)
> >> +{
> >> +	int rc;
> >> +	struct cpu *cpu = smp_cpu_from_addr(addr);
> >> +
> >> +	if (!cpu)
> >> +		return -1;
> >> +	if (psw) {
> >> +		cpu->lowcore->restart_new_psw.mask = psw->mask;
> >> +		cpu->lowcore->restart_new_psw.addr = psw->addr;
> >> +	}
> >> +	rc = sigp(addr, SIGP_RESTART, 0, NULL);
> >> +	if (rc)
> >> +		return rc;
> >> +	while (!smp_cpu_running(addr)) { mb(); }  
> > 
> > Maybe split this statement? Also, maybe add a comment  
> 
> /* Wait until the target cpu is running */
> ?

Fine with me as well :)

> 
> This is not QEMU with two line ifs taking up 3 lines :)

Heh, it's just the style I'm used to :)

> 
> > 
> > /*
> >  * The order has been accepted, but the actual restart may not
> >  * have been performed yet, so wait until the cpu is running.
> >  */
> > 
> > ?
> >   
> >> +	cpu->active = true;
> >> +	return 0;
> >> +}  
> > 
> > The changes look good to me AFAICS.
> > 
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>  
> 
> Thanks!
> 
>

Patch
diff mbox series

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index f57f420..f984a34 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -104,35 +104,41 @@  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);
+
+	if (!cpu)
+		return -1;
+	if (psw) {
+		cpu->lowcore->restart_new_psw.mask = psw->mask;
+		cpu->lowcore->restart_new_psw.addr = psw->addr;
+	}
+	rc = sigp(addr, SIGP_RESTART, 0, NULL);
+	if (rc)
+		return rc;
+	while (!smp_cpu_running(addr)) { mb(); }
+	cpu->active = true;
+	return 0;
+}
+
 int smp_cpu_restart(uint16_t addr)
 {
-	int rc = -1;
-	struct cpu *cpu;
+	int rc;
 
 	spin_lock(&lock);
-	cpu = smp_cpu_from_addr(addr);
-	if (cpu) {
-		rc = sigp(addr, SIGP_RESTART, 0, NULL);
-		cpu->active = true;
-	}
+	rc = smp_cpu_restart_nolock(addr, NULL);
 	spin_unlock(&lock);
 	return rc;
 }
 
 int smp_cpu_start(uint16_t addr, struct psw psw)
 {
-	int rc = -1;
-	struct cpu *cpu;
-	struct lowcore *lc;
+	int rc;
 
 	spin_lock(&lock);
-	cpu = smp_cpu_from_addr(addr);
-	if (cpu) {
-		lc = cpu->lowcore;
-		lc->restart_new_psw.mask = psw.mask;
-		lc->restart_new_psw.addr = psw.addr;
-		rc = sigp(addr, SIGP_RESTART, 0, NULL);
-	}
+	rc = smp_cpu_restart_nolock(addr, &psw);
 	spin_unlock(&lock);
 	return rc;
 }
@@ -192,10 +198,7 @@  int smp_cpu_setup(uint16_t addr, struct psw psw)
 	lc->sw_int_crs[0] = 0x0000000000040000UL;
 
 	/* Start processing */
-	rc = sigp_retry(cpu->addr, SIGP_RESTART, 0, NULL);
-	if (!rc)
-		cpu->active = true;
-
+	smp_cpu_restart_nolock(addr, NULL);
 out:
 	spin_unlock(&lock);
 	return rc;