s390x: smp: Improve setup of additional cpus

PATCH In review Janosch Frank 2 participants 12 messages Dec 11, 2019 – Dec 12, 2019

Subsystems (2)

KVM s390

lore

Share & tools

lore

Janosch Frank

author

2019-12-11 11:59 UTC #0 lore raw

cover s390x: smp: Improve setup of additional cpus

Let's not use constants for the PSW mask and CRs, but set them from input data or cpu 0.

Later on we could add functions that take a ptr for setting up CRs of additional cpus, but for now using the CRs 0,1,7,13 from cpu 0 will at least result in a working DAT environment.

Janosch Frank (2):
s390x: smp: Use full PSW to bringup new cpu
s390x: smp: Setup CRs from cpu 0

 lib/s390x/smp.c  | 7 ++++++-
 s390x/cstart64.S | 4 ++--
 2 files changed, 8 insertions(+), 3 deletions(-)

Signature · 1 line

2.20.1

Janosch Frank

author

2019-12-11 11:59 UTC #1 lore raw

PATCH 1/2 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 <[email protected]>

---
 lib/s390x/smp.c  | 2 ++
 s390x/cstart64.S | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

lib/s390x/smp.c +2 -0

index f57f420..e17751a 100644
@@ -185,6 +185,8 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
 	cpu->stack = (uint64_t *)alloc_pages(2);
 
 	/* Start without DAT and any other mask bits. */
+	cpu->lowcore->sw_int_psw.mask = psw.mask;
+	cpu->lowcore->sw_int_psw.addr = psw.addr;
 	cpu->lowcore->sw_int_grs[14] = psw.addr;
 	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
 	lc->restart_new_psw.mask = 0x0000000180000000UL;

s390x/cstart64.S +1 -1

index 86dd4c4..e6a6bdb 100644
@@ -159,7 +159,7 @@ smp_cpu_setup_state:
 	xgr	%r1, %r1
 	lmg     %r0, %r15, GEN_LC_SW_INT_GRS
 	lctlg   %c0, %c0, GEN_LC_SW_INT_CRS
-	br	%r14
+	lpswe	GEN_LC_SW_INT_PSW
 
 pgm_int:
 	SAVE_REGS

Signature · 1 line

2.20.1

David Hildenbrand

2019-12-11 12:31 UTC #2 lore raw

Re: [kvm-unit-tests PATCH 1/2] s390x: smp: Use full PSW to bringup new cpu

Quoted 15 lines · Janosch Frank

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 <[email protected]>
---
 lib/s390x/smp.c  | 2 ++
 s390x/cstart64.S | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)
lib/s390x/smp.c +0 -0
index f57f420..e17751a 100644

lib/s390x/smp.c int smp_cpu_setup(uint16_t addr, struct psw psw) · line 190

Quoted 6 lines

lib/s390x/smp.c +2 -0

@@ -185,6 +185,8 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
 	cpu->stack = (uint64_t *)alloc_pages(2);
 
 	/* Start without DAT and any other mask bits. */
+	cpu->lowcore->sw_int_psw.mask = psw.mask;
+	cpu->lowcore->sw_int_psw.addr = psw.addr;
 	cpu->lowcore->sw_int_grs[14] = psw.addr;

Not looking at the code (sorry :D ), do we still need this then? (you drop the br bewlo)

Quoted 16 lines

lib/s390x/smp.c +0 -0

 	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
 	lc->restart_new_psw.mask = 0x0000000180000000UL;

s390x/cstart64.S +1 -1

index 86dd4c4..e6a6bdb 100644
@@ -159,7 +159,7 @@ smp_cpu_setup_state:
 	xgr	%r1, %r1
 	lmg     %r0, %r15, GEN_LC_SW_INT_GRS
 	lctlg   %c0, %c0, GEN_LC_SW_INT_CRS
-	br	%r14
+	lpswe	GEN_LC_SW_INT_PSW
 
 pgm_int:
 	SAVE_REGS

Signature · 3 lines

Thanks,

David / dhildenb

Janosch Frank

author

2019-12-11 12:34 UTC #3 lore raw

Re: [kvm-unit-tests PATCH 1/2] s390x: smp: Use full PSW to bringup new cpu

Quoted 18 lines · David Hildenbrand

Quoted 17 lines · Janosch Frank
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 <[email protected]>
---
 lib/s390x/smp.c  | 2 ++
 s390x/cstart64.S | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)
lib/s390x/smp.c +0 -0
index f57f420..e17751a 100644
@@ -185,6 +185,8 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
 	cpu->stack = (uint64_t *)alloc_pages(2);
 

Quoted 7 lines

lib/s390x/smp.c int smp_cpu_setup(uint16_t addr, struct psw psw) · line 190
Quoted 4 lines
lib/s390x/smp.c +2 -0
 	/* Start without DAT and any other mask bits. */
+	cpu->lowcore->sw_int_psw.mask = psw.mask;
+	cpu->lowcore->sw_int_psw.addr = psw.addr;
 	cpu->lowcore->sw_int_grs[14] = psw.addr;
Not looking at the code (sorry :D ), do we still need this then? (you drop the br bewlo)

r14 is the return address, saving/initialising it doesn't sound like a bad idea to me. If we ever have stack traces, it might show up, or won't it?

Quoted 19 lines

Quoted 17 lines

lib/s390x/smp.c +0 -0

 	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
 	lc->restart_new_psw.mask = 0x0000000180000000UL;

s390x/cstart64.S +1 -1

index 86dd4c4..e6a6bdb 100644
@@ -159,7 +159,7 @@ smp_cpu_setup_state:
 	xgr	%r1, %r1
 	lmg     %r0, %r15, GEN_LC_SW_INT_GRS
 	lctlg   %c0, %c0, GEN_LC_SW_INT_CRS
-	br	%r14
+	lpswe	GEN_LC_SW_INT_PSW
 
 pgm_int:
 	SAVE_REGS

David Hildenbrand

Reviewed-by

2019-12-11 12:54 UTC #4 lore raw

Re: [kvm-unit-tests PATCH 1/2] s390x: smp: Use full PSW to bringup new cpu

Quoted 31 lines · Janosch Frank

Quoted 18 lines · David Hildenbrand
Quoted 17 lines · Janosch Frank
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 <[email protected]>
---
 lib/s390x/smp.c  | 2 ++
 s390x/cstart64.S | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)
lib/s390x/smp.c +0 -0
index f57f420..e17751a 100644
@@ -185,6 +185,8 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
 	cpu->stack = (uint64_t *)alloc_pages(2);
 
Quoted 7 lines
lib/s390x/smp.c int smp_cpu_setup(uint16_t addr, struct psw psw) · line 190
Quoted 4 lines
lib/s390x/smp.c +2 -0
 	/* Start without DAT and any other mask bits. */
+	cpu->lowcore->sw_int_psw.mask = psw.mask;
+	cpu->lowcore->sw_int_psw.addr = psw.addr;
 	cpu->lowcore->sw_int_grs[14] = psw.addr;
Not looking at the code (sorry :D ), do we still need this then? (you drop the br bewlo)
r14 is the return address, saving/initialising it doesn't sound like a bad idea to me. If we ever have stack traces, it might show up, or won't it?
Quoted 1 line

Quoted 13 lines

Quoted 5 lines
Quoted 5 lines
lib/s390x/smp.c +0 -0
 	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
 	lc->restart_new_psw.mask = 0x0000000180000000UL;
s390x/cstart64.S +0 -1
index 86dd4c4..e6a6bdb 100644
--- a/s390x/cstart64.S
Quoted 8 lines
s390x/cstart64.S smp_cpu_setup_state: · line 163
Quoted 8 lines
+++ b/s390x/cstart64.S
s390x/cstart64.S +1 -1
@@ -159,7 +159,7 @@ smp_cpu_setup_state:
 	xgr	%r1, %r1
 	lmg     %r0, %r15, GEN_LC_SW_INT_GRS
 	lctlg   %c0, %c0, GEN_LC_SW_INT_CRS
-	br	%r14
+	lpswe	GEN_LC_SW_INT_PSW
 

Makes sense

Reviewed-by David Hildenbrand <[email protected]>

Signature · 3 lines

Thanks,

David / dhildenb

David Hildenbrand

2019-12-12 14:16 UTC #5 lore raw

Re: [kvm-unit-tests PATCH 1/2] s390x: smp: Use full PSW to bringup new cpu

In reply to Janosch Frank

Quoted 29 lines · Janosch Frank

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 <[email protected]>

---
 lib/s390x/smp.c  | 2 ++
 s390x/cstart64.S | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

lib/s390x/smp.c +2 -0

index f57f420..e17751a 100644
@@ -185,6 +185,8 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
 	cpu->stack = (uint64_t *)alloc_pages(2);
 
 	/* Start without DAT and any other mask bits. */
+	cpu->lowcore->sw_int_psw.mask = psw.mask;
+	cpu->lowcore->sw_int_psw.addr = psw.addr;
 	cpu->lowcore->sw_int_grs[14] = psw.addr;
 	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
 	lc->restart_new_psw.mask = 0x0000000180000000UL;

s390x/cstart64.S +0 -0

index 86dd4c4..e6a6bdb 100644

s390x/cstart64.S smp_cpu_setup_state: · line 165

Quoted 8 lines

s390x/cstart64.S +1 -1

@@ -159,7 +159,7 @@ smp_cpu_setup_state:
 	xgr	%r1, %r1
 	lmg     %r0, %r15, GEN_LC_SW_INT_GRS
 	lctlg   %c0, %c0, GEN_LC_SW_INT_CRS
-	br	%r14
+	lpswe	GEN_LC_SW_INT_PSW
 
 pgm_int:
 	SAVE_REGS

Queued to

https://github.com/davidhildenbrand/kvm-unit-tests.git s390x-next

Thanks!

Signature · 3 lines

Thanks,

David / dhildenb

Janosch Frank

author

2019-12-11 11:59 UTC #6 lore raw

PATCH 2/2 s390x: smp: Setup CRs from cpu 0

In reply to Janosch Frank

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 <[email protected]>

---
 lib/s390x/smp.c  | 5 ++++-
 s390x/cstart64.S | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

lib/s390x/smp.c +4 -1

index e17751a..4dfe7c6 100644
@@ -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);

s390x/cstart64.S +1 -1

index e6a6bdb..399ae9b 100644
@@ -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:

Signature · 1 line

2.20.1

David Hildenbrand

2019-12-11 12:33 UTC #7 lore raw

Re: [kvm-unit-tests PATCH 2/2] s390x: smp: Setup CRs from cpu 0

Quoted 14 lines · Janosch Frank

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 <[email protected]>
---
 lib/s390x/smp.c  | 5 ++++-
 s390x/cstart64.S | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)
lib/s390x/smp.c +0 -0
index e17751a..4dfe7c6 100644

lib/s390x/smp.c int smp_cpu_setup(uint16_t addr, struct psw psw) · line 197

Quoted 8 lines

lib/s390x/smp.c +4 -1

@@ -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?

Signature · 3 lines

Thanks,

David / dhildenb

Janosch Frank

author

2019-12-11 12:37 UTC #8 lore raw

Re: [kvm-unit-tests PATCH 2/2] s390x: smp: Setup CRs from cpu 0

Quoted 10 lines · David Hildenbrand

Quoted 9 lines · Janosch Frank
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 <[email protected]>
---
 lib/s390x/smp.c  | 5 ++++-
 s390x/cstart64.S | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

Quoted 16 lines

lib/s390x/smp.c int smp_cpu_setup(uint16_t addr, struct psw psw) · line 197

Quoted 13 lines

lib/s390x/smp.c +4 -1

index e17751a..4dfe7c6 100644
@@ -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

2019-12-11 12:54 UTC #9 lore raw

Re: [kvm-unit-tests PATCH 2/2] s390x: smp: Setup CRs from cpu 0

Quoted 28 lines · Janosch Frank

Quoted 10 lines · David Hildenbrand
Quoted 9 lines · Janosch Frank
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 <[email protected]>
---
 lib/s390x/smp.c  | 5 ++++-
 s390x/cstart64.S | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)
Quoted 16 lines
lib/s390x/smp.c int smp_cpu_setup(uint16_t addr, struct psw psw) · line 197
Quoted 13 lines
lib/s390x/smp.c +4 -1
index e17751a..4dfe7c6 100644
@@ -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?

Quoted 7 lines

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).

Signature · 3 lines

Thanks,

David / dhildenb

Janosch Frank

author

2019-12-11 13:09 UTC #10 lore raw

Re: [kvm-unit-tests PATCH 2/2] s390x: smp: Setup CRs from cpu 0

Quoted 32 lines · David Hildenbrand

Quoted 31 lines · Janosch Frank
Quoted 10 lines · David Hildenbrand
Quoted 9 lines · Janosch Frank
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 <[email protected]>
---
 lib/s390x/smp.c  | 5 ++++-
 s390x/cstart64.S | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)
Quoted 16 lines
lib/s390x/smp.c int smp_cpu_setup(uint16_t addr, struct psw psw) · line 197
Quoted 13 lines
lib/s390x/smp.c +4 -1
index e17751a..4dfe7c6 100644
@@ -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.

Quoted 9 lines

Quoted 4 lines
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

2019-12-11 13:14 UTC #11 lore raw

Re: [kvm-unit-tests PATCH 2/2] s390x: smp: Setup CRs from cpu 0

Quoted 43 lines · Janosch Frank

Quoted 32 lines · David Hildenbrand
Quoted 31 lines · Janosch Frank
Quoted 10 lines · David Hildenbrand
Quoted 9 lines · Janosch Frank
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 <[email protected]>
---
 lib/s390x/smp.c  | 5 ++++-
 s390x/cstart64.S | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)
Quoted 16 lines
lib/s390x/smp.c int smp_cpu_setup(uint16_t addr, struct psw psw) · line 197
Quoted 13 lines
lib/s390x/smp.c +4 -1
index e17751a..4dfe7c6 100644
@@ -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.
Quoted 9 lines
Quoted 4 lines
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).

Quoted 4 lines

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).

Signature · 3 lines

Thanks,

David / dhildenb