| Message ID | 20200827093152.3026-4-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pc-bios: s390x: Cleanup part 2 | expand |
On 27/08/2020 11.31, Janosch Frank wrote: > Currently we always overwrite the mentioned exception new PSWs before > loading the enabled wait PSW. Let's save the PSW before overwriting > and restore it right before starting the loaded kernel. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > > Maybe we should rather statically allocate a lowcore so we don't dirty > 0x0 at all. > > --- > pc-bios/s390-ccw/jump2ipl.c | 3 ++ > pc-bios/s390-ccw/start.S | 62 +++++++++++++++++++++++++++---------- > 2 files changed, 48 insertions(+), 17 deletions(-) > > diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c > index 143d027bf7..a44f3ab5b3 100644 > --- a/pc-bios/s390-ccw/jump2ipl.c > +++ b/pc-bios/s390-ccw/jump2ipl.c > @@ -13,12 +13,15 @@ > #define KERN_IMAGE_START 0x010000UL > #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) > > +extern uint64_t *psw_save_io, *psw_save_ext; I think that should be extern uint64_t psw_save_io[], psw_save_ext[]; instead ... otherwise you'll end up with some funny bugs here, won't you? > uint64_t *reset_psw = 0, save_psw, ipl_continue; > > static void jump_to_IPL_2(void) > { > /* Restore reset PSW and io and external new PSWs */ Ok, now the comment makes sense :-) > *reset_psw = save_psw; > + memcpy((void *)0x1f0, psw_save_io, 16); > + memcpy((void *)0x1b0, psw_save_ext, 16); Could you use &lowcore->external_new_psw and &lowcore->io_new_psw instead of the magic numbers? Thomas
On 8/27/20 2:52 PM, Thomas Huth wrote: > On 27/08/2020 11.31, Janosch Frank wrote: >> Currently we always overwrite the mentioned exception new PSWs before >> loading the enabled wait PSW. Let's save the PSW before overwriting >> and restore it right before starting the loaded kernel. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> >> Maybe we should rather statically allocate a lowcore so we don't dirty >> 0x0 at all. >> >> --- >> pc-bios/s390-ccw/jump2ipl.c | 3 ++ >> pc-bios/s390-ccw/start.S | 62 +++++++++++++++++++++++++++---------- >> 2 files changed, 48 insertions(+), 17 deletions(-) >> >> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c >> index 143d027bf7..a44f3ab5b3 100644 >> --- a/pc-bios/s390-ccw/jump2ipl.c >> +++ b/pc-bios/s390-ccw/jump2ipl.c >> @@ -13,12 +13,15 @@ >> #define KERN_IMAGE_START 0x010000UL >> #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) >> >> +extern uint64_t *psw_save_io, *psw_save_ext; > > I think that should be > > extern uint64_t psw_save_io[], psw_save_ext[]; > > instead ... otherwise you'll end up with some funny bugs here, won't you? What kind of bugs are you expecting? > >> uint64_t *reset_psw = 0, save_psw, ipl_continue; >> >> static void jump_to_IPL_2(void) >> { >> /* Restore reset PSW and io and external new PSWs */ > > Ok, now the comment makes sense :-) >> *reset_psw = save_psw; >> + memcpy((void *)0x1f0, psw_save_io, 16); >> + memcpy((void *)0x1b0, psw_save_ext, 16); > > Could you use &lowcore->external_new_psw and &lowcore->io_new_psw > instead of the magic numbers? I can, but that means that I need to declare lowcore in netmain.c as well as including s390-arch.h > > Thomas > > >
On 27/08/2020 16.30, Janosch Frank wrote: > On 8/27/20 2:52 PM, Thomas Huth wrote: >> On 27/08/2020 11.31, Janosch Frank wrote: >>> Currently we always overwrite the mentioned exception new PSWs before >>> loading the enabled wait PSW. Let's save the PSW before overwriting >>> and restore it right before starting the loaded kernel. >>> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> --- >>> >>> Maybe we should rather statically allocate a lowcore so we don't dirty >>> 0x0 at all. >>> >>> --- >>> pc-bios/s390-ccw/jump2ipl.c | 3 ++ >>> pc-bios/s390-ccw/start.S | 62 +++++++++++++++++++++++++++---------- >>> 2 files changed, 48 insertions(+), 17 deletions(-) >>> >>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c >>> index 143d027bf7..a44f3ab5b3 100644 >>> --- a/pc-bios/s390-ccw/jump2ipl.c >>> +++ b/pc-bios/s390-ccw/jump2ipl.c >>> @@ -13,12 +13,15 @@ >>> #define KERN_IMAGE_START 0x010000UL >>> #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) >>> >>> +extern uint64_t *psw_save_io, *psw_save_ext; >> >> I think that should be >> >> extern uint64_t psw_save_io[], psw_save_ext[]; >> >> instead ... otherwise you'll end up with some funny bugs here, won't you? > > What kind of bugs are you expecting? Well, "extern uint64_t var[];" and "extern uint64_t *var;" are two different kind of things. One is an array, one is a pointer variable. Looking at your assembler code, you obviously tried to declare an array there, not a pointer variable. Have a try with this test program: #include <string.h> extern unsigned long *var; void main(void) { asm volatile (" nop ; nop ; nop "); /* marker */ memcpy((void *)0x1f0, var, 16); asm volatile (" nop ; nop ; nop "); /* marker */ } After compiling that with -O2, and disassembling the corresponding .o file, I get this code between the nops: c: c4 18 00 00 00 00 lgrl %r1,c <main+0xc> e: R_390_PC32DBL var+0x2 12: e7 00 10 00 00 06 vl %v0,0(%r1) 18: e7 00 01 f0 00 0e vst %v0,496 The "lgrl %r1,var" is likely not what you wanted here. If you now replace the "*var" with "var[]", you get this instead: c: c0 10 00 00 00 00 larl %r1,c <main+0xc> e: R_390_PC32DBL var+0x2 12: e7 00 10 00 00 06 vl %v0,0(%r1) 18: e7 00 01 f0 00 0e vst %v0,496 "larl" looks better now, doesn't it? >> >>> uint64_t *reset_psw = 0, save_psw, ipl_continue; >>> >>> static void jump_to_IPL_2(void) >>> { >>> /* Restore reset PSW and io and external new PSWs */ >> >> Ok, now the comment makes sense :-) >>> *reset_psw = save_psw; >>> + memcpy((void *)0x1f0, psw_save_io, 16); >>> + memcpy((void *)0x1b0, psw_save_ext, 16); >> >> Could you use &lowcore->external_new_psw and &lowcore->io_new_psw >> instead of the magic numbers? > > I can, but that means that I need to declare lowcore in netmain.c as > well as including s390-arch.h If that does not cause any other big hurdles, I think I'd prefer that instead of using magic numbers. Thanks, Thomas
diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c index 143d027bf7..a44f3ab5b3 100644 --- a/pc-bios/s390-ccw/jump2ipl.c +++ b/pc-bios/s390-ccw/jump2ipl.c @@ -13,12 +13,15 @@ #define KERN_IMAGE_START 0x010000UL #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) +extern uint64_t *psw_save_io, *psw_save_ext; uint64_t *reset_psw = 0, save_psw, ipl_continue; static void jump_to_IPL_2(void) { /* Restore reset PSW and io and external new PSWs */ *reset_psw = save_psw; + memcpy((void *)0x1f0, psw_save_io, 16); + memcpy((void *)0x1b0, psw_save_ext, 16); /* No reset PSW, let's jump instead. */ if (ipl_continue) { diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S index ce519300a1..939aac3a7c 100644 --- a/pc-bios/s390-ccw/start.S +++ b/pc-bios/s390-ccw/start.S @@ -34,7 +34,17 @@ remainder: larl %r2,memsetxc ex %r3,0(%r2) done: - j main /* And call C */ + /* prepare i/o call handler */ + larl %r1, io_new_code + larl %r2, io_new_psw + stg %r1, 8(%r2) + mvc 0x1f0(16),0(%r2) + /* prepare external call handler */ + larl %r1, external_new_code + larl %r2, external_new_psw + stg %r1, 8(%r2) + mvc 0x1b0(16),0(%r2) + j main /* And call C */ memsetxc: xc 0(1,%r1),0(%r1) @@ -64,13 +74,16 @@ consume_sclp_int: oi 6(%r15),0x2 lctlg %c0,%c0,0(%r15) /* prepare external call handler */ - larl %r1, external_new_code - stg %r1, 0x1b8 - larl %r1, external_new_mask - mvc 0x1b0(8),0(%r1) - /* load enabled wait PSW */ - larl %r1, enabled_wait_psw - lpswe 0(%r1) + larl %r1, external_new_psw + lghi %r2, 0x1b0 + /* Is the BIOS' external new PSW already set? */ + clc 0(16, %r1), 0(%r2) + je load_ewait + /* No, save old PSW and write BIOS PSW */ + larl %r3, psw_save_ext + mvc 0(16, %r3), 0x1b0 + mvc 0x1b0(16),0(%r1) + j load_ewait /* * void consume_io_int(void) @@ -84,11 +97,20 @@ consume_io_int: oi 4(%r15), 0xff lctlg %c6,%c6,0(%r15) /* prepare i/o call handler */ - larl %r1, io_new_code - stg %r1, 0x1f8 - larl %r1, io_new_mask - mvc 0x1f0(8),0(%r1) - /* load enabled wait PSW */ + larl %r1, io_new_psw + lghi %r2, 0x1f0 + /* Is the BIOS' PSW already set? */ + larl %r3, load_ewait + clc 0(16, %r1), 0(%r2) + bcr 8, %r3 + /* No, save old PSW and write BIOS PSW */ + larl %r3, psw_save_io + mvc 0(16, %r3), 0x1f0 + mvc 0x1f0(16),0(%r1) + j load_ewait + +load_ewait: + /* PSW is the correct one, time to load the enabled wait PSW */ larl %r1, enabled_wait_psw lpswe 0(%r1) @@ -107,11 +129,17 @@ io_new_code: br %r14 .align 8 + .globl psw_save_io + .globl psw_save_ext disabled_wait_psw: .quad 0x0002000180000000,0x0000000000000000 enabled_wait_psw: .quad 0x0302000180000000,0x0000000000000000 -external_new_mask: - .quad 0x0000000180000000 -io_new_mask: - .quad 0x0000000180000000 +external_new_psw: + .quad 0x0000000180000000,0 +io_new_psw: + .quad 0x0000000180000000,0 +psw_save_io: + .quad 0,0 +psw_save_ext: + .quad 0,0
Currently we always overwrite the mentioned exception new PSWs before loading the enabled wait PSW. Let's save the PSW before overwriting and restore it right before starting the loaded kernel. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- Maybe we should rather statically allocate a lowcore so we don't dirty 0x0 at all. --- pc-bios/s390-ccw/jump2ipl.c | 3 ++ pc-bios/s390-ccw/start.S | 62 +++++++++++++++++++++++++++---------- 2 files changed, 48 insertions(+), 17 deletions(-)