Absolutely no need to set them up every time before we enable our
interrupt masks.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
pc-bios/s390-ccw/start.S | 30 +++++++++++++-----------------
1 file changed, 13 insertions(+), 17 deletions(-)
diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index 01c4c21b26..0059a15d21 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -34,6 +34,12 @@ loop:
remainder:
larl %r2,memsetxc
ex %r3,0(%r2)
+ /* Store io new PSW */
+ larl %r1,io_new_psw
+ mvc 0x1f0(16),0(%r1)
+ /* Store ext new PSW */
+ larl %r1,external_new_psw
+ mvc 0x1b0(16),0(%r1)
done:
j main /* And call C */
@@ -64,11 +70,6 @@ consume_sclp_int:
stctg %c0,%c0,0(%r15)
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)
@@ -81,14 +82,9 @@ consume_sclp_int:
.globl consume_io_int
consume_io_int:
/* enable I/O interrupts in cr6 */
- stctg %c6,%c6,0(%r15)
- 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)
+ stctg %c6, %c6, 0(%r15)
+ oi 4(%r15), 0xff
+ lctlg %c6, %c6, 0(%r15)
/* load enabled wait PSW */
larl %r1, enabled_wait_psw
lpswe 0(%r1)
@@ -112,7 +108,7 @@ disabled_wait_psw:
.quad PSW_MASK_DWAIT, 0x0000000000000000
enabled_wait_psw:
.quad PSW_MASK_EWAIT, 0x0000000000000000
-external_new_mask:
- .quad PSW_MASK_64
-io_new_mask:
- .quad PSW_MASK_64
+external_new_psw:
+ .quad PSW_MASK_64, external_new_code
+io_new_psw:
+ .quad PSW_MASK_64, io_new_code
--
2.25.1
On 7/15/20 11:40 AM, Janosch Frank wrote: > Absolutely no need to set them up every time before we enable our > interrupt masks. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> So, this one doesn't seem to be a great idea as a kernel loaded to 0x0 will overwrite the ext/io new PSWs and we'll then try to load a zero PSW when an IRQ hits. We have two options: * Add a piece of code that fixes the low core after a kernel has been loaded * Leave the code as is and add a comment, so other people don't fall into this trap. I'd be fine with both and for now I'd settle for the second option. > --- > pc-bios/s390-ccw/start.S | 30 +++++++++++++----------------- > 1 file changed, 13 insertions(+), 17 deletions(-) > > diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S > index 01c4c21b26..0059a15d21 100644 > --- a/pc-bios/s390-ccw/start.S > +++ b/pc-bios/s390-ccw/start.S > @@ -34,6 +34,12 @@ loop: > remainder: > larl %r2,memsetxc > ex %r3,0(%r2) > + /* Store io new PSW */ > + larl %r1,io_new_psw > + mvc 0x1f0(16),0(%r1) > + /* Store ext new PSW */ > + larl %r1,external_new_psw > + mvc 0x1b0(16),0(%r1) > done: > j main /* And call C */ > > @@ -64,11 +70,6 @@ consume_sclp_int: > stctg %c0,%c0,0(%r15) > 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) > @@ -81,14 +82,9 @@ consume_sclp_int: > .globl consume_io_int > consume_io_int: > /* enable I/O interrupts in cr6 */ > - stctg %c6,%c6,0(%r15) > - 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) > + stctg %c6, %c6, 0(%r15) > + oi 4(%r15), 0xff > + lctlg %c6, %c6, 0(%r15) > /* load enabled wait PSW */ > larl %r1, enabled_wait_psw > lpswe 0(%r1) > @@ -112,7 +108,7 @@ disabled_wait_psw: > .quad PSW_MASK_DWAIT, 0x0000000000000000 > enabled_wait_psw: > .quad PSW_MASK_EWAIT, 0x0000000000000000 > -external_new_mask: > - .quad PSW_MASK_64 > -io_new_mask: > - .quad PSW_MASK_64 > +external_new_psw: > + .quad PSW_MASK_64, external_new_code > +io_new_psw: > + .quad PSW_MASK_64, io_new_code >
On 15.07.20 15:13, Janosch Frank wrote: > On 7/15/20 11:40 AM, Janosch Frank wrote: >> Absolutely no need to set them up every time before we enable our >> interrupt masks. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > > So, this one doesn't seem to be a great idea as a kernel loaded to 0x0 > will overwrite the ext/io new PSWs and we'll then try to load a zero PSW > when an IRQ hits. > > We have two options: > * Add a piece of code that fixes the low core after a kernel has been loaded > * Leave the code as is and add a comment, so other people don't fall > into this trap. > > I'd be fine with both and for now I'd settle for the second option. Yes, leave it as is and add a comment. > >> --- >> pc-bios/s390-ccw/start.S | 30 +++++++++++++----------------- >> 1 file changed, 13 insertions(+), 17 deletions(-) >> >> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S >> index 01c4c21b26..0059a15d21 100644 >> --- a/pc-bios/s390-ccw/start.S >> +++ b/pc-bios/s390-ccw/start.S >> @@ -34,6 +34,12 @@ loop: >> remainder: >> larl %r2,memsetxc >> ex %r3,0(%r2) >> + /* Store io new PSW */ >> + larl %r1,io_new_psw >> + mvc 0x1f0(16),0(%r1) >> + /* Store ext new PSW */ >> + larl %r1,external_new_psw >> + mvc 0x1b0(16),0(%r1) >> done: >> j main /* And call C */ >> >> @@ -64,11 +70,6 @@ consume_sclp_int: >> stctg %c0,%c0,0(%r15) >> 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) >> @@ -81,14 +82,9 @@ consume_sclp_int: >> .globl consume_io_int >> consume_io_int: >> /* enable I/O interrupts in cr6 */ >> - stctg %c6,%c6,0(%r15) >> - 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) >> + stctg %c6, %c6, 0(%r15) >> + oi 4(%r15), 0xff >> + lctlg %c6, %c6, 0(%r15) >> /* load enabled wait PSW */ >> larl %r1, enabled_wait_psw >> lpswe 0(%r1) >> @@ -112,7 +108,7 @@ disabled_wait_psw: >> .quad PSW_MASK_DWAIT, 0x0000000000000000 >> enabled_wait_psw: >> .quad PSW_MASK_EWAIT, 0x0000000000000000 >> -external_new_mask: >> - .quad PSW_MASK_64 >> -io_new_mask: >> - .quad PSW_MASK_64 >> +external_new_psw: >> + .quad PSW_MASK_64, external_new_code >> +io_new_psw: >> + .quad PSW_MASK_64, io_new_code >> > >
Normally they don't need to be set up before waiting for an interrupt
but are set up on boot. The BIOS however might overwrite the lowcore
(and hence the PSWs) when loading a blob into memory and therefore
needs to set up those PSWs more often.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
pc-bios/s390-ccw/start.S | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index 01c4c21b26..b0fcb918cc 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -64,7 +64,10 @@ consume_sclp_int:
stctg %c0,%c0,0(%r15)
oi 6(%r15),0x2
lctlg %c0,%c0,0(%r15)
- /* prepare external call handler */
+ /*
+ * Prepare external new PSW as it might have been overwritten
+ * by a loaded blob
+ */
larl %r1, external_new_code
stg %r1, 0x1b8
larl %r1, external_new_mask
@@ -84,7 +87,10 @@ consume_io_int:
stctg %c6,%c6,0(%r15)
oi 4(%r15), 0xff
lctlg %c6,%c6,0(%r15)
- /* prepare i/o call handler */
+ /*
+ * Prepare i/o new PSW as it might have been overwritten
+ * by a loaded blob
+ */
larl %r1, io_new_code
stg %r1, 0x1f8
larl %r1, io_new_mask
--
2.25.1
On 15.07.20 16:08, Janosch Frank wrote: > Normally they don't need to be set up before waiting for an interrupt > but are set up on boot. The BIOS however might overwrite the lowcore > (and hence the PSWs) when loading a blob into memory and therefore > needs to set up those PSWs more often. Now when I read the new comment this actually inidicates a bug. When do we restore the original content? If the loaded program does have interrupt handlers in the original image and relies on that then we are broken, no? > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > pc-bios/s390-ccw/start.S | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S > index 01c4c21b26..b0fcb918cc 100644 > --- a/pc-bios/s390-ccw/start.S > +++ b/pc-bios/s390-ccw/start.S > @@ -64,7 +64,10 @@ consume_sclp_int: > stctg %c0,%c0,0(%r15) > oi 6(%r15),0x2 > lctlg %c0,%c0,0(%r15) > - /* prepare external call handler */ > + /* > + * Prepare external new PSW as it might have been overwritten > + * by a loaded blob > + */ > larl %r1, external_new_code > stg %r1, 0x1b8 > larl %r1, external_new_mask > @@ -84,7 +87,10 @@ consume_io_int: > stctg %c6,%c6,0(%r15) > oi 4(%r15), 0xff > lctlg %c6,%c6,0(%r15) > - /* prepare i/o call handler */ > + /* > + * Prepare i/o new PSW as it might have been overwritten > + * by a loaded blob > + */ > larl %r1, io_new_code > stg %r1, 0x1f8 > larl %r1, io_new_mask >
On 7/22/20 8:43 AM, Christian Borntraeger wrote: > > > On 15.07.20 16:08, Janosch Frank wrote: >> Normally they don't need to be set up before waiting for an interrupt >> but are set up on boot. The BIOS however might overwrite the lowcore >> (and hence the PSWs) when loading a blob into memory and therefore >> needs to set up those PSWs more often. > > Now when I read the new comment this actually inidicates a bug. > When do we restore the original content? If the loaded program > does have interrupt handlers in the original image and relies on that > then we are broken, no? I haven't seen references to a save/restore functionality for those PSWs. And I also think it's not that easy to do because we have multiple ways of loading data and if we want to print when loading we might end up overwriting and then saving the written value for a later restore. I need to have a closer look at how virtio works, but wouldn't we have a chicken - egg problem with IO interrupts for IO that writes the prefix? The BIOS often has "interesting" solutions to problems. If you have a quick fix, be my guest and send it. If not I'd put it on my todo list or let Stefan make it a proper dev item. > >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> pc-bios/s390-ccw/start.S | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S >> index 01c4c21b26..b0fcb918cc 100644 >> --- a/pc-bios/s390-ccw/start.S >> +++ b/pc-bios/s390-ccw/start.S >> @@ -64,7 +64,10 @@ consume_sclp_int: >> stctg %c0,%c0,0(%r15) >> oi 6(%r15),0x2 >> lctlg %c0,%c0,0(%r15) >> - /* prepare external call handler */ >> + /* >> + * Prepare external new PSW as it might have been overwritten >> + * by a loaded blob >> + */ >> larl %r1, external_new_code >> stg %r1, 0x1b8 >> larl %r1, external_new_mask >> @@ -84,7 +87,10 @@ consume_io_int: >> stctg %c6,%c6,0(%r15) >> oi 4(%r15), 0xff >> lctlg %c6,%c6,0(%r15) >> - /* prepare i/o call handler */ >> + /* >> + * Prepare i/o new PSW as it might have been overwritten >> + * by a loaded blob >> + */ >> larl %r1, io_new_code >> stg %r1, 0x1f8 >> larl %r1, io_new_mask >>
On 22.07.20 09:24, Janosch Frank wrote: > On 7/22/20 8:43 AM, Christian Borntraeger wrote: >> >> >> On 15.07.20 16:08, Janosch Frank wrote: >>> Normally they don't need to be set up before waiting for an interrupt >>> but are set up on boot. The BIOS however might overwrite the lowcore >>> (and hence the PSWs) when loading a blob into memory and therefore >>> needs to set up those PSWs more often. >> >> Now when I read the new comment this actually inidicates a bug. >> When do we restore the original content? If the loaded program >> does have interrupt handlers in the original image and relies on that >> then we are broken, no? > > I haven't seen references to a save/restore functionality for those > PSWs. And I also think it's not that easy to do because we have multiple > ways of loading data and if we want to print when loading we might end > up overwriting and then saving the written value for a later restore. > > I need to have a closer look at how virtio works, but wouldn't we have a > chicken - egg problem with IO interrupts for IO that writes the prefix? > > The BIOS often has "interesting" solutions to problems. > If you have a quick fix, be my guest and send it. If not I'd put it on > my todo list or let Stefan make it a proper dev item. Maybe a global fixup table in BIOS memory that restores all the memory that we messed with when we hand over control? Can you at least change the comment here to add a fixme? > >> >>> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> --- >>> pc-bios/s390-ccw/start.S | 10 ++++++++-- >>> 1 file changed, 8 insertions(+), 2 deletions(-) >>> >>> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S >>> index 01c4c21b26..b0fcb918cc 100644 >>> --- a/pc-bios/s390-ccw/start.S >>> +++ b/pc-bios/s390-ccw/start.S >>> @@ -64,7 +64,10 @@ consume_sclp_int: >>> stctg %c0,%c0,0(%r15) >>> oi 6(%r15),0x2 >>> lctlg %c0,%c0,0(%r15) >>> - /* prepare external call handler */ >>> + /* >>> + * Prepare external new PSW as it might have been overwritten >>> + * by a loaded blob >>> + */ >>> larl %r1, external_new_code >>> stg %r1, 0x1b8 >>> larl %r1, external_new_mask >>> @@ -84,7 +87,10 @@ consume_io_int: >>> stctg %c6,%c6,0(%r15) >>> oi 4(%r15), 0xff >>> lctlg %c6,%c6,0(%r15) >>> - /* prepare i/o call handler */ >>> + /* >>> + * Prepare i/o new PSW as it might have been overwritten >>> + * by a loaded blob >>> + */ >>> larl %r1, io_new_code >>> stg %r1, 0x1f8 >>> larl %r1, io_new_mask >>> > >
On 7/22/20 9:39 AM, Christian Borntraeger wrote: > > > On 22.07.20 09:24, Janosch Frank wrote: >> On 7/22/20 8:43 AM, Christian Borntraeger wrote: >>> >>> >>> On 15.07.20 16:08, Janosch Frank wrote: >>>> Normally they don't need to be set up before waiting for an interrupt >>>> but are set up on boot. The BIOS however might overwrite the lowcore >>>> (and hence the PSWs) when loading a blob into memory and therefore >>>> needs to set up those PSWs more often. >>> >>> Now when I read the new comment this actually inidicates a bug. >>> When do we restore the original content? If the loaded program >>> does have interrupt handlers in the original image and relies on that >>> then we are broken, no? >> >> I haven't seen references to a save/restore functionality for those >> PSWs. And I also think it's not that easy to do because we have multiple >> ways of loading data and if we want to print when loading we might end >> up overwriting and then saving the written value for a later restore. >> >> I need to have a closer look at how virtio works, but wouldn't we have a >> chicken - egg problem with IO interrupts for IO that writes the prefix? >> >> The BIOS often has "interesting" solutions to problems. >> If you have a quick fix, be my guest and send it. If not I'd put it on >> my todo list or let Stefan make it a proper dev item. > > Maybe a global fixup table in BIOS memory that restores all the memory that > we messed with when we hand over control? Can you at least change the comment > here to add a fixme? Sure > >> >>> >>>> >>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>> --- >>>> pc-bios/s390-ccw/start.S | 10 ++++++++-- >>>> 1 file changed, 8 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S >>>> index 01c4c21b26..b0fcb918cc 100644 >>>> --- a/pc-bios/s390-ccw/start.S >>>> +++ b/pc-bios/s390-ccw/start.S >>>> @@ -64,7 +64,10 @@ consume_sclp_int: >>>> stctg %c0,%c0,0(%r15) >>>> oi 6(%r15),0x2 >>>> lctlg %c0,%c0,0(%r15) >>>> - /* prepare external call handler */ >>>> + /* >>>> + * Prepare external new PSW as it might have been overwritten >>>> + * by a loaded blob >>>> + */ >>>> larl %r1, external_new_code >>>> stg %r1, 0x1b8 >>>> larl %r1, external_new_mask >>>> @@ -84,7 +87,10 @@ consume_io_int: >>>> stctg %c6,%c6,0(%r15) >>>> oi 4(%r15), 0xff >>>> lctlg %c6,%c6,0(%r15) >>>> - /* prepare i/o call handler */ >>>> + /* >>>> + * Prepare i/o new PSW as it might have been overwritten >>>> + * by a loaded blob >>>> + */ >>>> larl %r1, io_new_code >>>> stg %r1, 0x1f8 >>>> larl %r1, io_new_mask >>>> >> >>
On 22/07/2020 10.05, Janosch Frank wrote: > On 7/22/20 9:39 AM, Christian Borntraeger wrote: >> >> >> On 22.07.20 09:24, Janosch Frank wrote: >>> On 7/22/20 8:43 AM, Christian Borntraeger wrote: >>>> >>>> >>>> On 15.07.20 16:08, Janosch Frank wrote: >>>>> Normally they don't need to be set up before waiting for an interrupt >>>>> but are set up on boot. The BIOS however might overwrite the lowcore >>>>> (and hence the PSWs) when loading a blob into memory and therefore >>>>> needs to set up those PSWs more often. >>>> >>>> Now when I read the new comment this actually inidicates a bug. >>>> When do we restore the original content? If the loaded program >>>> does have interrupt handlers in the original image and relies on that >>>> then we are broken, no? >>> >>> I haven't seen references to a save/restore functionality for those >>> PSWs. And I also think it's not that easy to do because we have multiple >>> ways of loading data and if we want to print when loading we might end >>> up overwriting and then saving the written value for a later restore. >>> >>> I need to have a closer look at how virtio works, but wouldn't we have a >>> chicken - egg problem with IO interrupts for IO that writes the prefix? >>> >>> The BIOS often has "interesting" solutions to problems. >>> If you have a quick fix, be my guest and send it. If not I'd put it on >>> my todo list or let Stefan make it a proper dev item. >> >> Maybe a global fixup table in BIOS memory that restores all the memory that >> we messed with when we hand over control? Can you at least change the comment >> here to add a fixme? > > Sure Hi Janosch! Did you ever send a new version of this patch with the FIXME added? I can't find it right now... Or shall I add a FIXME when picking this up? (If so, could you please suggest a text?) Thomas
On 15/07/2020 16.08, Janosch Frank wrote: > Normally they don't need to be set up before waiting for an interrupt > but are set up on boot. The BIOS however might overwrite the lowcore > (and hence the PSWs) when loading a blob into memory and therefore > needs to set up those PSWs more often. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > pc-bios/s390-ccw/start.S | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S > index 01c4c21b26..b0fcb918cc 100644 > --- a/pc-bios/s390-ccw/start.S > +++ b/pc-bios/s390-ccw/start.S > @@ -64,7 +64,10 @@ consume_sclp_int: > stctg %c0,%c0,0(%r15) > oi 6(%r15),0x2 > lctlg %c0,%c0,0(%r15) > - /* prepare external call handler */ > + /* > + * Prepare external new PSW as it might have been overwritten > + * by a loaded blob > + */ > larl %r1, external_new_code > stg %r1, 0x1b8 > larl %r1, external_new_mask > @@ -84,7 +87,10 @@ consume_io_int: > stctg %c6,%c6,0(%r15) > oi 4(%r15), 0xff > lctlg %c6,%c6,0(%r15) > - /* prepare i/o call handler */ > + /* > + * Prepare i/o new PSW as it might have been overwritten > + * by a loaded blob > + */ > larl %r1, io_new_code > stg %r1, 0x1f8 > larl %r1, io_new_mask > Reviewed-by: Thomas Huth <thuth@redhat.com>
© 2016 - 2026 Red Hat, Inc.