| Message ID | 20200324150847.10476-6-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pc-bios: s390x: Cleanup part 1 | expand |
On Tue, 24 Mar 2020 11:08:44 -0400 Janosch Frank <frankja@linux.ibm.com> wrote: > panic() was defined for the ccw and net bios, i.e. twice, so it's > cleaner to rather put it into the header. They were also slightly different, so unifying them makes sense. > > Also let's add an infinite loop into the assembly of disabled_wait() so > the caller doesn't need to take care of it. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> > --- > pc-bios/s390-ccw/main.c | 7 ------- > pc-bios/s390-ccw/netmain.c | 8 -------- > pc-bios/s390-ccw/s390-ccw.h | 7 ++++++- > pc-bios/s390-ccw/start.S | 5 +++-- > 4 files changed, 9 insertions(+), 18 deletions(-) > > @@ -91,6 +90,12 @@ bool menu_is_enabled_enum(void); > > #define MAX_BOOT_ENTRIES 31 > > +static inline void panic(const char *string) > +{ > + sclp_print(string); > + disabled_wait(); > +} > + > static inline void fill_hex(char *out, unsigned char val) > { > const char hex[] = "0123456789abcdef"; > diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S > index aa8fceb19da2164a..35be141d8da38d07 100644 > --- a/pc-bios/s390-ccw/start.S > +++ b/pc-bios/s390-ccw/start.S > @@ -47,8 +47,9 @@ memsetxc: > */ > .globl disabled_wait > disabled_wait: > - larl %r1,disabled_wait_psw > - lpswe 0(%r1) > + larl %r1,disabled_wait_psw > + lpswe 0(%r1) > +1: j 1b > > > /* Possibly dumb question: Does checking code now figure out correctly that code flow does not continue after disabled_wait()?
On 24.03.20 16:08, Janosch Frank wrote: > panic() was defined for the ccw and net bios, i.e. twice, so it's > cleaner to rather put it into the header. > > Also let's add an infinite loop into the assembly of disabled_wait() so > the caller doesn't need to take care of it. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> > --- > pc-bios/s390-ccw/main.c | 7 ------- > pc-bios/s390-ccw/netmain.c | 8 -------- > pc-bios/s390-ccw/s390-ccw.h | 7 ++++++- > pc-bios/s390-ccw/start.S | 5 +++-- > 4 files changed, 9 insertions(+), 18 deletions(-) > > diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c > index 8b912454c940a390..146a50760bc70af7 100644 > --- a/pc-bios/s390-ccw/main.c > +++ b/pc-bios/s390-ccw/main.c > @@ -46,13 +46,6 @@ void write_iplb_location(void) > lowcore->ptr_iplb = ptr2u32(&iplb); > } > > -void panic(const char *string) > -{ > - sclp_print(string); > - disabled_wait(); > - while (1) { } > -} I remember there was a reason why to add the endless loop afterwards. Maybe because some special machine checks can actually wake it up? Or buggy hypervisor? Anyhow, the kernel also does __load_psw(psw); while (1); so it's best we keep that. With the endless loop re-added Reviewed-by: David Hildenbrand <david@redhat.com>
On 4/30/20 5:42 PM, David Hildenbrand wrote: > On 24.03.20 16:08, Janosch Frank wrote: >> panic() was defined for the ccw and net bios, i.e. twice, so it's >> cleaner to rather put it into the header. >> >> Also let's add an infinite loop into the assembly of disabled_wait() so >> the caller doesn't need to take care of it. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> pc-bios/s390-ccw/main.c | 7 ------- >> pc-bios/s390-ccw/netmain.c | 8 -------- >> pc-bios/s390-ccw/s390-ccw.h | 7 ++++++- >> pc-bios/s390-ccw/start.S | 5 +++-- >> 4 files changed, 9 insertions(+), 18 deletions(-) >> >> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c >> index 8b912454c940a390..146a50760bc70af7 100644 >> --- a/pc-bios/s390-ccw/main.c >> +++ b/pc-bios/s390-ccw/main.c >> @@ -46,13 +46,6 @@ void write_iplb_location(void) >> lowcore->ptr_iplb = ptr2u32(&iplb); >> } >> >> -void panic(const char *string) >> -{ >> - sclp_print(string); >> - disabled_wait(); >> - while (1) { } >> -} > > I remember there was a reason why to add the endless loop afterwards. > Maybe because some special machine checks can actually wake it up? Or > buggy hypervisor? > > Anyhow, the kernel also does > > __load_psw(psw); > while (1); > > so it's best we keep that. > > > With the endless loop re-added Well, I added a loop into the disabled_wait assembly and removed it from the C code. It's even in the commit message. > > Reviewed-by: David Hildenbrand <david@redhat.com> >
On 04.05.20 08:46, Janosch Frank wrote: > On 4/30/20 5:42 PM, David Hildenbrand wrote: >> On 24.03.20 16:08, Janosch Frank wrote: >>> panic() was defined for the ccw and net bios, i.e. twice, so it's >>> cleaner to rather put it into the header. >>> >>> Also let's add an infinite loop into the assembly of disabled_wait() so >>> the caller doesn't need to take care of it. >>> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> >>> --- >>> pc-bios/s390-ccw/main.c | 7 ------- >>> pc-bios/s390-ccw/netmain.c | 8 -------- >>> pc-bios/s390-ccw/s390-ccw.h | 7 ++++++- >>> pc-bios/s390-ccw/start.S | 5 +++-- >>> 4 files changed, 9 insertions(+), 18 deletions(-) >>> >>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c >>> index 8b912454c940a390..146a50760bc70af7 100644 >>> --- a/pc-bios/s390-ccw/main.c >>> +++ b/pc-bios/s390-ccw/main.c >>> @@ -46,13 +46,6 @@ void write_iplb_location(void) >>> lowcore->ptr_iplb = ptr2u32(&iplb); >>> } >>> >>> -void panic(const char *string) >>> -{ >>> - sclp_print(string); >>> - disabled_wait(); >>> - while (1) { } >>> -} >> >> I remember there was a reason why to add the endless loop afterwards. >> Maybe because some special machine checks can actually wake it up? Or >> buggy hypervisor? >> >> Anyhow, the kernel also does >> >> __load_psw(psw); >> while (1); >> >> so it's best we keep that. >> >> >> With the endless loop re-added > > Well, I added a loop into the disabled_wait assembly and removed it from > the C code. It's even in the commit message. Whops, missed that, looks good to me then!
On 4/7/20 9:25 AM, Cornelia Huck wrote: > On Tue, 24 Mar 2020 11:08:44 -0400 > Janosch Frank <frankja@linux.ibm.com> wrote: > >> panic() was defined for the ccw and net bios, i.e. twice, so it's >> cleaner to rather put it into the header. > > They were also slightly different, so unifying them makes sense. > >> >> Also let's add an infinite loop into the assembly of disabled_wait() so >> the caller doesn't need to take care of it. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> pc-bios/s390-ccw/main.c | 7 ------- >> pc-bios/s390-ccw/netmain.c | 8 -------- >> pc-bios/s390-ccw/s390-ccw.h | 7 ++++++- >> pc-bios/s390-ccw/start.S | 5 +++-- >> 4 files changed, 9 insertions(+), 18 deletions(-) >> > >> @@ -91,6 +90,12 @@ bool menu_is_enabled_enum(void); >> >> #define MAX_BOOT_ENTRIES 31 >> >> +static inline void panic(const char *string) >> +{ >> + sclp_print(string); >> + disabled_wait(); >> +} >> + >> static inline void fill_hex(char *out, unsigned char val) >> { >> const char hex[] = "0123456789abcdef"; >> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S >> index aa8fceb19da2164a..35be141d8da38d07 100644 >> --- a/pc-bios/s390-ccw/start.S >> +++ b/pc-bios/s390-ccw/start.S >> @@ -47,8 +47,9 @@ memsetxc: >> */ >> .globl disabled_wait >> disabled_wait: >> - larl %r1,disabled_wait_psw >> - lpswe 0(%r1) >> + larl %r1,disabled_wait_psw >> + lpswe 0(%r1) >> +1: j 1b >> >> >> /* > > Possibly dumb question: Does checking code now figure out correctly > that code flow does not continue after disabled_wait()? > Which checking code? I could certainly add "__attribute__ ((__noreturn__))" if needed.
On Thu, 14 May 2020 13:27:20 +0200 Janosch Frank <frankja@linux.ibm.com> wrote: > On 4/7/20 9:25 AM, Cornelia Huck wrote: > > On Tue, 24 Mar 2020 11:08:44 -0400 > > Janosch Frank <frankja@linux.ibm.com> wrote: > > > >> panic() was defined for the ccw and net bios, i.e. twice, so it's > >> cleaner to rather put it into the header. > > > > They were also slightly different, so unifying them makes sense. > > > >> > >> Also let's add an infinite loop into the assembly of disabled_wait() so > >> the caller doesn't need to take care of it. > >> > >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > >> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> > >> --- > >> pc-bios/s390-ccw/main.c | 7 ------- > >> pc-bios/s390-ccw/netmain.c | 8 -------- > >> pc-bios/s390-ccw/s390-ccw.h | 7 ++++++- > >> pc-bios/s390-ccw/start.S | 5 +++-- > >> 4 files changed, 9 insertions(+), 18 deletions(-) > >> > > > >> @@ -91,6 +90,12 @@ bool menu_is_enabled_enum(void); > >> > >> #define MAX_BOOT_ENTRIES 31 > >> > >> +static inline void panic(const char *string) > >> +{ > >> + sclp_print(string); > >> + disabled_wait(); > >> +} > >> + > >> static inline void fill_hex(char *out, unsigned char val) > >> { > >> const char hex[] = "0123456789abcdef"; > >> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S > >> index aa8fceb19da2164a..35be141d8da38d07 100644 > >> --- a/pc-bios/s390-ccw/start.S > >> +++ b/pc-bios/s390-ccw/start.S > >> @@ -47,8 +47,9 @@ memsetxc: > >> */ > >> .globl disabled_wait > >> disabled_wait: > >> - larl %r1,disabled_wait_psw > >> - lpswe 0(%r1) > >> + larl %r1,disabled_wait_psw > >> + lpswe 0(%r1) > >> +1: j 1b > >> > >> > >> /* > > > > Possibly dumb question: Does checking code now figure out correctly > > that code flow does not continue after disabled_wait()? > > > > Which checking code? Things like e.g. Coverity. > I could certainly add "__attribute__ ((__noreturn__))" if needed. Probably would not hurt.
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c index 8b912454c940a390..146a50760bc70af7 100644 --- a/pc-bios/s390-ccw/main.c +++ b/pc-bios/s390-ccw/main.c @@ -46,13 +46,6 @@ void write_iplb_location(void) lowcore->ptr_iplb = ptr2u32(&iplb); } -void panic(const char *string) -{ - sclp_print(string); - disabled_wait(); - while (1) { } -} - unsigned int get_loadparm_index(void) { return atoui(loadparm_str); diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c index a8e2b1b6233735d8..ca23f9bb19a3e04c 100644 --- a/pc-bios/s390-ccw/netmain.c +++ b/pc-bios/s390-ccw/netmain.c @@ -439,14 +439,6 @@ static int net_try_direct_tftp_load(filename_ip_t *fn_ip) return rc; } -void panic(const char *string) -{ - sclp_print(string); - for (;;) { - disabled_wait(); - } -} - void write_subsystem_identification(void) { SubChannelId *schid = (SubChannelId *) 184; diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h index c5820e43aed143d0..b3dcfbc3c9b3814c 100644 --- a/pc-bios/s390-ccw/s390-ccw.h +++ b/pc-bios/s390-ccw/s390-ccw.h @@ -55,7 +55,6 @@ void consume_sclp_int(void); void consume_io_int(void); /* main.c */ -void panic(const char *string); void write_subsystem_identification(void); void write_iplb_location(void); extern char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE))); @@ -91,6 +90,12 @@ bool menu_is_enabled_enum(void); #define MAX_BOOT_ENTRIES 31 +static inline void panic(const char *string) +{ + sclp_print(string); + disabled_wait(); +} + static inline void fill_hex(char *out, unsigned char val) { const char hex[] = "0123456789abcdef"; diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S index aa8fceb19da2164a..35be141d8da38d07 100644 --- a/pc-bios/s390-ccw/start.S +++ b/pc-bios/s390-ccw/start.S @@ -47,8 +47,9 @@ memsetxc: */ .globl disabled_wait disabled_wait: - larl %r1,disabled_wait_psw - lpswe 0(%r1) + larl %r1,disabled_wait_psw + lpswe 0(%r1) +1: j 1b /*