1. Patchew
  2. QEMU
  3. [PATCH 0/8] pc-bios: s390x: Cleanup part 1
  4. View patch

[PATCH 5/8] pc-bios: s390x: Move panic() into header and add infinite loop

Janosch Frank posted 8 patches 5 years, 10 months ago
Maintainers: Thomas Huth <thuth@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Christian Borntraeger <borntraeger@de.ibm.com>
There is a newer version of this series
[PATCH 5/8] pc-bios: s390x: Move panic() into header and add infinite loop
Posted by Janosch Frank 5 years, 10 months ago 
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) { }
-}
-
 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
 
 
 /*
-- 
2.25.1
Re: [PATCH 5/8] pc-bios: s390x: Move panic() into header and add infinite loop
Posted by Cornelia Huck 5 years, 10 months ago 
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()?
Re: [PATCH 5/8] pc-bios: s390x: Move panic() into header and add infinite loop
Posted by Janosch Frank 5 years, 9 months ago 
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.
Re: [PATCH 5/8] pc-bios: s390x: Move panic() into header and add infinite loop
Posted by Cornelia Huck 5 years, 9 months ago 
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.
Re: [PATCH 5/8] pc-bios: s390x: Move panic() into header and add infinite loop
Posted by David Hildenbrand 5 years, 9 months ago 
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>

-- 
Thanks,

David / dhildenb
Re: [PATCH 5/8] pc-bios: s390x: Move panic() into header and add infinite loop
Posted by Janosch Frank 5 years, 9 months ago 
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>
>
Re: [PATCH 5/8] pc-bios: s390x: Move panic() into header and add infinite loop
Posted by David Hildenbrand 5 years, 9 months ago 
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!


-- 
Thanks,

David / dhildenb

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.