[kvm-unit-tests,2/2] s390x: smp: Setup CRs from cpu 0
diff mbox series

Message ID 20191211115923.9191-3-frankja@linux.ibm.com
State New
Headers show
Series
  • s390x: smp: Improve setup of additional cpus
Related show

Commit Message

Janosch Frank Dec. 11, 2019, 11:59 a.m. UTC
Grab the CRs (currently only 0, 1, 7, 13) from cpu 0, so we can
bringup the new cpu in DAT mode or set other control options.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/smp.c  | 5 ++++-
 s390x/cstart64.S | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

Comments

David Hildenbrand Dec. 11, 2019, 12:32 p.m. UTC | #1
On 11.12.19 12:59, Janosch Frank wrote:
> Grab the CRs (currently only 0, 1, 7, 13) from cpu 0, so we can
> bringup the new cpu in DAT mode or set other control options.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/smp.c  | 5 ++++-
>  s390x/cstart64.S | 2 +-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index e17751a..4dfe7c6 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -191,7 +191,10 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
>  	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
>  	lc->restart_new_psw.mask = 0x0000000180000000UL;
>  	lc->restart_new_psw.addr = (uint64_t)smp_cpu_setup_state;
> -	lc->sw_int_crs[0] = 0x0000000000040000UL;
> +	lc->sw_int_crs[0] = stctg(0);
> +	lc->sw_int_crs[1] = stctg(1);
> +	lc->sw_int_crs[7] = stctg(7);
> +	lc->sw_int_crs[13] = stctg(13);

Wouldn't it be better to also be able to specify the CRs explicitly here?
Janosch Frank Dec. 11, 2019, 12:37 p.m. UTC | #2
On 12/11/19 1:32 PM, David Hildenbrand wrote:
> On 11.12.19 12:59, Janosch Frank wrote:
>> Grab the CRs (currently only 0, 1, 7, 13) from cpu 0, so we can
>> bringup the new cpu in DAT mode or set other control options.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  lib/s390x/smp.c  | 5 ++++-
>>  s390x/cstart64.S | 2 +-
>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
>> index e17751a..4dfe7c6 100644
>> --- a/lib/s390x/smp.c
>> +++ b/lib/s390x/smp.c
>> @@ -191,7 +191,10 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
>>  	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
>>  	lc->restart_new_psw.mask = 0x0000000180000000UL;
>>  	lc->restart_new_psw.addr = (uint64_t)smp_cpu_setup_state;
>> -	lc->sw_int_crs[0] = 0x0000000000040000UL;
>> +	lc->sw_int_crs[0] = stctg(0);
>> +	lc->sw_int_crs[1] = stctg(1);
>> +	lc->sw_int_crs[7] = stctg(7);
>> +	lc->sw_int_crs[13] = stctg(13);
> 
> Wouldn't it be better to also be able to specify the CRs explicitly here?
> 

Yes, but currently there are no users for something like that and it
would mean that we might need to add more code to support it.

As I said in the cover letter, this is a good first step to allow DAT on
additional cpus without any real setup needed in a test. Later we could
add a function to specify the CRs explicitly.
David Hildenbrand Dec. 11, 2019, 12:54 p.m. UTC | #3
On 11.12.19 13:37, Janosch Frank wrote:
> On 12/11/19 1:32 PM, David Hildenbrand wrote:
>> On 11.12.19 12:59, Janosch Frank wrote:
>>> Grab the CRs (currently only 0, 1, 7, 13) from cpu 0, so we can
>>> bringup the new cpu in DAT mode or set other control options.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>  lib/s390x/smp.c  | 5 ++++-
>>>  s390x/cstart64.S | 2 +-
>>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
>>> index e17751a..4dfe7c6 100644
>>> --- a/lib/s390x/smp.c
>>> +++ b/lib/s390x/smp.c
>>> @@ -191,7 +191,10 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
>>>  	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
>>>  	lc->restart_new_psw.mask = 0x0000000180000000UL;
>>>  	lc->restart_new_psw.addr = (uint64_t)smp_cpu_setup_state;
>>> -	lc->sw_int_crs[0] = 0x0000000000040000UL;
>>> +	lc->sw_int_crs[0] = stctg(0);
>>> +	lc->sw_int_crs[1] = stctg(1);
>>> +	lc->sw_int_crs[7] = stctg(7);
>>> +	lc->sw_int_crs[13] = stctg(13);
>>
>> Wouldn't it be better to also be able to specify the CRs explicitly here?
>>
> 
> Yes, but currently there are no users for something like that and it
> would mean that we might need to add more code to support it.
> 
> As I said in the cover letter, this is a good first step to allow DAT on
> additional cpus without any real setup needed in a test. Later we could
> add a function to specify the CRs explicitly.
> 

Can you clarify why we need this patch now (e.g., DAT)? This patch
sounds like it would make sense in the future only (it is easier to
review with future changes IMHO).
Janosch Frank Dec. 11, 2019, 1:08 p.m. UTC | #4
On 12/11/19 1:54 PM, David Hildenbrand wrote:
> On 11.12.19 13:37, Janosch Frank wrote:
>> On 12/11/19 1:32 PM, David Hildenbrand wrote:
>>> On 11.12.19 12:59, Janosch Frank wrote:
>>>> Grab the CRs (currently only 0, 1, 7, 13) from cpu 0, so we can
>>>> bringup the new cpu in DAT mode or set other control options.
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>>  lib/s390x/smp.c  | 5 ++++-
>>>>  s390x/cstart64.S | 2 +-
>>>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
>>>> index e17751a..4dfe7c6 100644
>>>> --- a/lib/s390x/smp.c
>>>> +++ b/lib/s390x/smp.c
>>>> @@ -191,7 +191,10 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
>>>>  	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
>>>>  	lc->restart_new_psw.mask = 0x0000000180000000UL;
>>>>  	lc->restart_new_psw.addr = (uint64_t)smp_cpu_setup_state;
>>>> -	lc->sw_int_crs[0] = 0x0000000000040000UL;
>>>> +	lc->sw_int_crs[0] = stctg(0);
>>>> +	lc->sw_int_crs[1] = stctg(1);
>>>> +	lc->sw_int_crs[7] = stctg(7);
>>>> +	lc->sw_int_crs[13] = stctg(13);
>>>
>>> Wouldn't it be better to also be able to specify the CRs explicitly here?
>>>
>>
>> Yes, but currently there are no users for something like that and it
>> would mean that we might need to add more code to support it.
>>
>> As I said in the cover letter, this is a good first step to allow DAT on
>> additional cpus without any real setup needed in a test. Later we could
>> add a function to specify the CRs explicitly.
>>
> 
> Can you clarify why we need this patch now (e.g., DAT)? This patch
> sounds like it would make sense in the future only (it is easier to
> review with future changes IMHO).
> 

Some G1 UV calls need the home space and therefore I added this patch to
my concurrency tests which are still in my queue. I thought these fixes
might make sense anyway and could be flushed from my queue before the UV
patches are ready.
David Hildenbrand Dec. 11, 2019, 1:14 p.m. UTC | #5
On 11.12.19 14:08, Janosch Frank wrote:
> On 12/11/19 1:54 PM, David Hildenbrand wrote:
>> On 11.12.19 13:37, Janosch Frank wrote:
>>> On 12/11/19 1:32 PM, David Hildenbrand wrote:
>>>> On 11.12.19 12:59, Janosch Frank wrote:
>>>>> Grab the CRs (currently only 0, 1, 7, 13) from cpu 0, so we can
>>>>> bringup the new cpu in DAT mode or set other control options.
>>>>>
>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>> ---
>>>>>  lib/s390x/smp.c  | 5 ++++-
>>>>>  s390x/cstart64.S | 2 +-
>>>>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
>>>>> index e17751a..4dfe7c6 100644
>>>>> --- a/lib/s390x/smp.c
>>>>> +++ b/lib/s390x/smp.c
>>>>> @@ -191,7 +191,10 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
>>>>>  	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
>>>>>  	lc->restart_new_psw.mask = 0x0000000180000000UL;
>>>>>  	lc->restart_new_psw.addr = (uint64_t)smp_cpu_setup_state;
>>>>> -	lc->sw_int_crs[0] = 0x0000000000040000UL;
>>>>> +	lc->sw_int_crs[0] = stctg(0);
>>>>> +	lc->sw_int_crs[1] = stctg(1);
>>>>> +	lc->sw_int_crs[7] = stctg(7);
>>>>> +	lc->sw_int_crs[13] = stctg(13);
>>>>
>>>> Wouldn't it be better to also be able to specify the CRs explicitly here?
>>>>
>>>
>>> Yes, but currently there are no users for something like that and it
>>> would mean that we might need to add more code to support it.
>>>
>>> As I said in the cover letter, this is a good first step to allow DAT on
>>> additional cpus without any real setup needed in a test. Later we could
>>> add a function to specify the CRs explicitly.
>>>
>>
>> Can you clarify why we need this patch now (e.g., DAT)? This patch
>> sounds like it would make sense in the future only (it is easier to
>> review with future changes IMHO).
>>
> 
> Some G1 UV calls need the home space and therefore I added this patch to
> my concurrency tests which are still in my queue. I thought these fixes
> might make sense anyway and could be flushed from my queue before the UV
> patches are ready.

Okay, so it needs e.g., CR13, but we only populate CR0 and CR1 right now
if I'm not wrong. IOW, let's send this with the other patches (or only
populate cr0/cr1 in this patch and add the other stuff on top later on).

Patch
diff mbox series

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index e17751a..4dfe7c6 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -191,7 +191,10 @@  int smp_cpu_setup(uint16_t addr, struct psw psw)
 	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
 	lc->restart_new_psw.mask = 0x0000000180000000UL;
 	lc->restart_new_psw.addr = (uint64_t)smp_cpu_setup_state;
-	lc->sw_int_crs[0] = 0x0000000000040000UL;
+	lc->sw_int_crs[0] = stctg(0);
+	lc->sw_int_crs[1] = stctg(1);
+	lc->sw_int_crs[7] = stctg(7);
+	lc->sw_int_crs[13] = stctg(13);
 
 	/* Start processing */
 	rc = sigp_retry(cpu->addr, SIGP_RESTART, 0, NULL);
diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index e6a6bdb..399ae9b 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -158,7 +158,7 @@  diag308_load_reset:
 smp_cpu_setup_state:
 	xgr	%r1, %r1
 	lmg     %r0, %r15, GEN_LC_SW_INT_GRS
-	lctlg   %c0, %c0, GEN_LC_SW_INT_CRS
+	lctlg   %c0, %c15, GEN_LC_SW_INT_CRS
 	lpswe	GEN_LC_SW_INT_PSW
 
 pgm_int: