[kvm-unit-tests,v4,6/9] s390x: smp: Loop if secondary cpu returns into cpu setup again
diff mbox series

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

Commit Message

Janosch Frank Jan. 21, 2020, 1:42 p.m. UTC
Up to now a secondary cpu could have returned from the function it was
executing and ending up somewhere in cstart64.S. This was mostly
circumvented by an endless loop in the function that it executed.

Let's add a loop to the end of the cpu setup, so we don't have to rely
on added loops in the tests.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/cstart64.S | 2 ++
 1 file changed, 2 insertions(+)

Comments

Cornelia Huck Jan. 21, 2020, 2:28 p.m. UTC | #1
On Tue, 21 Jan 2020 08:42:51 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Up to now a secondary cpu could have returned from the function it was
> executing and ending up somewhere in cstart64.S. This was mostly
> circumvented by an endless loop in the function that it executed.
> 
> Let's add a loop to the end of the cpu setup, so we don't have to rely
> on added loops in the tests.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/cstart64.S | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 9af6bb3..5fd8d2f 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -162,6 +162,8 @@ smp_cpu_setup_state:
>  	/* We should only go once through cpu setup and not for every restart */
>  	stg	%r14, GEN_LC_RESTART_NEW_PSW + 8
>  	br	%r14
> +	/* If the function returns, just loop here */
> +0:	j	0
>  
>  pgm_int:
>  	SAVE_REGS

Acked-by: Cornelia Huck <cohuck@redhat.com>
David Hildenbrand Jan. 21, 2020, 5:36 p.m. UTC | #2
On 21.01.20 14:42, Janosch Frank wrote:
> Up to now a secondary cpu could have returned from the function it was
> executing and ending up somewhere in cstart64.S. This was mostly
> circumvented by an endless loop in the function that it executed.
> 
> Let's add a loop to the end of the cpu setup, so we don't have to rely
> on added loops in the tests.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/cstart64.S | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 9af6bb3..5fd8d2f 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -162,6 +162,8 @@ smp_cpu_setup_state:
>  	/* We should only go once through cpu setup and not for every restart */
>  	stg	%r14, GEN_LC_RESTART_NEW_PSW + 8
>  	br	%r14
> +	/* If the function returns, just loop here */
> +0:	j	0
>  
>  pgm_int:
>  	SAVE_REGS
> 

Acked-by: David Hildenbrand <david@redhat.com>
David Hildenbrand Jan. 23, 2020, 1:32 p.m. UTC | #3
On 21.01.20 14:42, Janosch Frank wrote:
> Up to now a secondary cpu could have returned from the function it was
> executing and ending up somewhere in cstart64.S. This was mostly
> circumvented by an endless loop in the function that it executed.
> 
> Let's add a loop to the end of the cpu setup, so we don't have to rely
> on added loops in the tests.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/cstart64.S | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 9af6bb3..5fd8d2f 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -162,6 +162,8 @@ smp_cpu_setup_state:
>  	/* We should only go once through cpu setup and not for every restart */
>  	stg	%r14, GEN_LC_RESTART_NEW_PSW + 8
>  	br	%r14
> +	/* If the function returns, just loop here */
> +0:	j	0
>  
>  pgm_int:
>  	SAVE_REGS
> 

This patch collides with a patch I have still queued

Author: Janosch Frank <frankja@linux.ibm.com>
Date:   Wed Dec 11 06:59:22 2019 -0500

    s390x: smp: Use full PSW to bringup new cpu
    
    Up to now we ignored the psw mask and only used the psw address when
    bringing up a new cpu. For DAT we need to also load the mask, so let's
    do that.
    
    Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
    Reviewed-by: David Hildenbrand <david@redhat.com>
    Message-Id: <20191211115923.9191-2-frankja@linux.ibm.com>
    Signed-off-by: David Hildenbrand <david@redhat.com>


In that patch we use a lpswe to jump to the target code, not a br. So the
return address will no longer be stored in %14 and this code here would stop working
AFAIKS.

Shall I drop that patch for now?
Janosch Frank Jan. 23, 2020, 1:47 p.m. UTC | #4
On 1/23/20 2:32 PM, David Hildenbrand wrote:
> On 21.01.20 14:42, Janosch Frank wrote:
>> Up to now a secondary cpu could have returned from the function it was
>> executing and ending up somewhere in cstart64.S. This was mostly
>> circumvented by an endless loop in the function that it executed.
>>
>> Let's add a loop to the end of the cpu setup, so we don't have to rely
>> on added loops in the tests.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  s390x/cstart64.S | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
>> index 9af6bb3..5fd8d2f 100644
>> --- a/s390x/cstart64.S
>> +++ b/s390x/cstart64.S
>> @@ -162,6 +162,8 @@ smp_cpu_setup_state:
>>  	/* We should only go once through cpu setup and not for every restart */
>>  	stg	%r14, GEN_LC_RESTART_NEW_PSW + 8
>>  	br	%r14
>> +	/* If the function returns, just loop here */
>> +0:	j	0
>>  
>>  pgm_int:
>>  	SAVE_REGS
>>
> 
> This patch collides with a patch I have still queued
> 
> Author: Janosch Frank <frankja@linux.ibm.com>
> Date:   Wed Dec 11 06:59:22 2019 -0500
> 
>     s390x: smp: Use full PSW to bringup new cpu
>     
>     Up to now we ignored the psw mask and only used the psw address when
>     bringing up a new cpu. For DAT we need to also load the mask, so let's
>     do that.
>     
>     Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>     Reviewed-by: David Hildenbrand <david@redhat.com>
>     Message-Id: <20191211115923.9191-2-frankja@linux.ibm.com>
>     Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> 
> In that patch we use a lpswe to jump to the target code, not a br. So the
> return address will no longer be stored in %14 and this code here would stop working
> AFAIKS.
> 
> Shall I drop that patch for now?

Please drop "s390x: smp: Use full PSW to bringup new cpu"
I will send out a fixed version of that patch soonish. It will load a
label for lpswe into r14 before doing the lpswe.

Patch
diff mbox series

diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index 9af6bb3..5fd8d2f 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -162,6 +162,8 @@  smp_cpu_setup_state:
 	/* We should only go once through cpu setup and not for every restart */
 	stg	%r14, GEN_LC_RESTART_NEW_PSW + 8
 	br	%r14
+	/* If the function returns, just loop here */
+0:	j	0
 
 pgm_int:
 	SAVE_REGS