| Message ID | 20200715140820.3401-1-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pc-bios: s390x: Add a comment to the io and external new PSW setup | expand |
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>
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
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
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(-)