[Qemu-devel] [PATCH v3 1/1] s390/ipl: fix ipl with -no-reboot

[Qemu-devel] [PATCH v3 1/1] s390/ipl: fix ipl with -no-reboot

[Christian Borntraeger]

kexec/kdump as well as the bootloader use a subcode of diagnose 308
that is supposed to reset the subsystem but not comprise a full
"reboot". With the latest refactoring this is now broken when
-no-reboot is used. This for example breaks virt-install from
iso images.

We need to mark these "soft" reboots as ok for rebooting.

Fixes: a30fb811cbe9 (s390x: refactor reset/reipl handling)
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390x/ipl.c          | 8 +++++++-
 include/sysemu/sysemu.h | 3 +++
 vl.c                    | 4 ++--
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 0d67349004..ade6896af6 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -535,7 +535,13 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
             ipl->iplb_valid = s390_gen_initial_iplb(ipl);
         }
     }
-    qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+    if (reset_type == S390_RESET_MODIFIED_CLEAR ||
+        reset_type == S390_RESET_LOAD_NORMAL) {
+        /* ignore -no-reboot */
+        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET_FORCE);
+    } else {
+        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+    }
     /* as this is triggered by a CPU, make sure to exit the loop */
     if (tcg_enabled()) {
         cpu_loop_exit(cs);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index e893f72f3b..345369d924 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -44,6 +44,9 @@ typedef enum ShutdownCause {
                                      turns that into a shutdown */
     SHUTDOWN_CAUSE_GUEST_PANIC,   /* Guest panicked, and command line turns
                                      that into a shutdown */
+    SHUTDOWN_CAUSE_GUEST_RESET_FORCE,/* Guest reset that should ignore
+                                        --no-reboot. This is useful for reset
+                                        like actions as s390 kexec/kdump */
     SHUTDOWN_CAUSE__MAX,
 } ShutdownCause;
 
diff --git a/vl.c b/vl.c
index b3426e03d0..44e65071bc 100644
--- a/vl.c
+++ b/vl.c
@@ -1628,7 +1628,7 @@ void qemu_system_reset(ShutdownCause reason)
     } else {
         qemu_devices_reset();
     }
-    if (reason) {
+    if (reason != SHUTDOWN_CAUSE_GUEST_RESET_FORCE) {
         qapi_event_send_reset(shutdown_caused_by_guest(reason),
                               &error_abort);
     }
@@ -1674,7 +1674,7 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
 
 void qemu_system_reset_request(ShutdownCause reason)
 {
-    if (no_reboot) {
+    if (no_reboot && reason != SHUTDOWN_CAUSE_GUEST_RESET_FORCE) {
         shutdown_requested = reason;
     } else {
         reset_requested = reason;
-- 
2.17.0

Re: [Qemu-devel] [PATCH v3 1/1] s390/ipl: fix ipl with -no-reboot

[Paolo Bonzini]

On 21/06/2018 19:01, Christian Borntraeger wrote:
> kexec/kdump as well as the bootloader use a subcode of diagnose 308
> that is supposed to reset the subsystem but not comprise a full
> "reboot". With the latest refactoring this is now broken when
> -no-reboot is used. This for example breaks virt-install from
> iso images.
> 
> We need to mark these "soft" reboots as ok for rebooting.
> 
> Fixes: a30fb811cbe9 (s390x: refactor reset/reipl handling)
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

Ok, then my suggestion made even more sense. :)  No other objections
apart from the name of the constant.

Paolo

> ---
>  hw/s390x/ipl.c          | 8 +++++++-
>  include/sysemu/sysemu.h | 3 +++
>  vl.c                    | 4 ++--
>  3 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0d67349004..ade6896af6 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -535,7 +535,13 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
>              ipl->iplb_valid = s390_gen_initial_iplb(ipl);
>          }
>      }
> -    qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +    if (reset_type == S390_RESET_MODIFIED_CLEAR ||
> +        reset_type == S390_RESET_LOAD_NORMAL) {
> +        /* ignore -no-reboot */
> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET_FORCE);
> +    } else {
> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +    }
>      /* as this is triggered by a CPU, make sure to exit the loop */
>      if (tcg_enabled()) {
>          cpu_loop_exit(cs);
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index e893f72f3b..345369d924 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -44,6 +44,9 @@ typedef enum ShutdownCause {
>                                       turns that into a shutdown */
>      SHUTDOWN_CAUSE_GUEST_PANIC,   /* Guest panicked, and command line turns
>                                       that into a shutdown */
> +    SHUTDOWN_CAUSE_GUEST_RESET_FORCE,/* Guest reset that should ignore
> +                                        --no-reboot. This is useful for reset
> +                                        like actions as s390 kexec/kdump */
>      SHUTDOWN_CAUSE__MAX,
>  } ShutdownCause;
>  
> diff --git a/vl.c b/vl.c
> index b3426e03d0..44e65071bc 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1628,7 +1628,7 @@ void qemu_system_reset(ShutdownCause reason)
>      } else {
>          qemu_devices_reset();
>      }
> -    if (reason) {
> +    if (reason != SHUTDOWN_CAUSE_GUEST_RESET_FORCE) {
>          qapi_event_send_reset(shutdown_caused_by_guest(reason),
>                                &error_abort);
>      }
> @@ -1674,7 +1674,7 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
>  
>  void qemu_system_reset_request(ShutdownCause reason)
>  {
> -    if (no_reboot) {
> +    if (no_reboot && reason != SHUTDOWN_CAUSE_GUEST_RESET_FORCE) {
>          shutdown_requested = reason;
>      } else {
>          reset_requested = reason;
> 

Re: [Qemu-devel] [PATCH v3 1/1] s390/ipl: fix ipl with -no-reboot

[Christian Borntraeger]

On 06/21/2018 07:08 PM, Paolo Bonzini wrote:
> On 21/06/2018 19:01, Christian Borntraeger wrote:
>> kexec/kdump as well as the bootloader use a subcode of diagnose 308
>> that is supposed to reset the subsystem but not comprise a full
>> "reboot". With the latest refactoring this is now broken when
>> -no-reboot is used. This for example breaks virt-install from
>> iso images.
>>
>> We need to mark these "soft" reboots as ok for rebooting.
>>
>> Fixes: a30fb811cbe9 (s390x: refactor reset/reipl handling)
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> Ok, then my suggestion made even more sense. :)  No other objections
> apart from the name of the constant.
> 
> Paolo

SHUTDOWN_CAUSE_S390_PARTIAL ?

SHUTDOWN_CAUSE_S390_SUBSYS_RESET?


> 
>> ---
>>  hw/s390x/ipl.c          | 8 +++++++-
>>  include/sysemu/sysemu.h | 3 +++
>>  vl.c                    | 4 ++--
>>  3 files changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 0d67349004..ade6896af6 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -535,7 +535,13 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
>>              ipl->iplb_valid = s390_gen_initial_iplb(ipl);
>>          }
>>      }
>> -    qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>> +    if (reset_type == S390_RESET_MODIFIED_CLEAR ||
>> +        reset_type == S390_RESET_LOAD_NORMAL) {
>> +        /* ignore -no-reboot */
>> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET_FORCE);
>> +    } else {
>> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>> +    }
>>      /* as this is triggered by a CPU, make sure to exit the loop */
>>      if (tcg_enabled()) {
>>          cpu_loop_exit(cs);
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index e893f72f3b..345369d924 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -44,6 +44,9 @@ typedef enum ShutdownCause {
>>                                       turns that into a shutdown */
>>      SHUTDOWN_CAUSE_GUEST_PANIC,   /* Guest panicked, and command line turns
>>                                       that into a shutdown */
>> +    SHUTDOWN_CAUSE_GUEST_RESET_FORCE,/* Guest reset that should ignore
>> +                                        --no-reboot. This is useful for reset
>> +                                        like actions as s390 kexec/kdump */
>>      SHUTDOWN_CAUSE__MAX,
>>  } ShutdownCause;
>>  
>> diff --git a/vl.c b/vl.c
>> index b3426e03d0..44e65071bc 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1628,7 +1628,7 @@ void qemu_system_reset(ShutdownCause reason)
>>      } else {
>>          qemu_devices_reset();
>>      }
>> -    if (reason) {
>> +    if (reason != SHUTDOWN_CAUSE_GUEST_RESET_FORCE) {
>>          qapi_event_send_reset(shutdown_caused_by_guest(reason),
>>                                &error_abort);
>>      }
>> @@ -1674,7 +1674,7 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
>>  
>>  void qemu_system_reset_request(ShutdownCause reason)
>>  {
>> -    if (no_reboot) {
>> +    if (no_reboot && reason != SHUTDOWN_CAUSE_GUEST_RESET_FORCE) {
>>          shutdown_requested = reason;
>>      } else {
>>          reset_requested = reason;
>>
> 

Re: [Qemu-devel] [PATCH v3 1/1] s390/ipl: fix ipl with -no-reboot

[David Hildenbrand]

On 21.06.2018 19:01, Christian Borntraeger wrote:
> kexec/kdump as well as the bootloader use a subcode of diagnose 308
> that is supposed to reset the subsystem but not comprise a full
> "reboot". With the latest refactoring this is now broken when
> -no-reboot is used. This for example breaks virt-install from
> iso images.
> 
> We need to mark these "soft" reboots as ok for rebooting.
> 
> Fixes: a30fb811cbe9 (s390x: refactor reset/reipl handling)
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/s390x/ipl.c          | 8 +++++++-
>  include/sysemu/sysemu.h | 3 +++
>  vl.c                    | 4 ++--
>  3 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0d67349004..ade6896af6 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -535,7 +535,13 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
>              ipl->iplb_valid = s390_gen_initial_iplb(ipl);
>          }
>      }
> -    qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +    if (reset_type == S390_RESET_MODIFIED_CLEAR ||
> +        reset_type == S390_RESET_LOAD_NORMAL) {
> +        /* ignore -no-reboot */
> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET_FORCE);
> +    } else {
> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +    }
>      /* as this is triggered by a CPU, make sure to exit the loop */
>      if (tcg_enabled()) {
>          cpu_loop_exit(cs);
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index e893f72f3b..345369d924 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -44,6 +44,9 @@ typedef enum ShutdownCause {
>                                       turns that into a shutdown */
>      SHUTDOWN_CAUSE_GUEST_PANIC,   /* Guest panicked, and command line turns
>                                       that into a shutdown */
> +    SHUTDOWN_CAUSE_GUEST_RESET_FORCE,/* Guest reset that should ignore
> +                                        --no-reboot. This is useful for reset
> +                                        like actions as s390 kexec/kdump */
>      SHUTDOWN_CAUSE__MAX,
>  } ShutdownCause;
>  
> diff --git a/vl.c b/vl.c
> index b3426e03d0..44e65071bc 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1628,7 +1628,7 @@ void qemu_system_reset(ShutdownCause reason)
>      } else {
>          qemu_devices_reset();
>      }
> -    if (reason) {
> +    if (reason != SHUTDOWN_CAUSE_GUEST_RESET_FORCE) {
>          qapi_event_send_reset(shutdown_caused_by_guest(reason),
>                                &error_abort);
>      }
> @@ -1674,7 +1674,7 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
>  
>  void qemu_system_reset_request(ShutdownCause reason)
>  {
> -    if (no_reboot) {
> +    if (no_reboot && reason != SHUTDOWN_CAUSE_GUEST_RESET_FORCE) {
>          shutdown_requested = reason;
>      } else {
>          reset_requested = reason;
> 

With whatever name you will end up using for the constant

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v3 1/1] s390/ipl: fix ipl with -no-reboot

[Cornelia Huck]

On Thu, 21 Jun 2018 22:34:49 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 06/21/2018 07:08 PM, Paolo Bonzini wrote:
> > On 21/06/2018 19:01, Christian Borntraeger wrote:  
> >> kexec/kdump as well as the bootloader use a subcode of diagnose 308
> >> that is supposed to reset the subsystem but not comprise a full
> >> "reboot". With the latest refactoring this is now broken when
> >> -no-reboot is used. This for example breaks virt-install from
> >> iso images.
> >>
> >> We need to mark these "soft" reboots as ok for rebooting.
> >>
> >> Fixes: a30fb811cbe9 (s390x: refactor reset/reipl handling)
> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>  
> > 
> > Ok, then my suggestion made even more sense. :)  No other objections
> > apart from the name of the constant.
> > 
> > Paolo  
> 
> SHUTDOWN_CAUSE_S390_PARTIAL ?

Don't like that one much.

> 
> SHUTDOWN_CAUSE_S390_SUBSYS_RESET?

Either that, or drop the 'S390' (is it conceivable that other
architectures have something similar)?

> 
> 
> >   
> >> ---
> >>  hw/s390x/ipl.c          | 8 +++++++-
> >>  include/sysemu/sysemu.h | 3 +++
> >>  vl.c                    | 4 ++--
> >>  3 files changed, 12 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> >> index 0d67349004..ade6896af6 100644
> >> --- a/hw/s390x/ipl.c
> >> +++ b/hw/s390x/ipl.c
> >> @@ -535,7 +535,13 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
> >>              ipl->iplb_valid = s390_gen_initial_iplb(ipl);
> >>          }
> >>      }
> >> -    qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> >> +    if (reset_type == S390_RESET_MODIFIED_CLEAR ||
> >> +        reset_type == S390_RESET_LOAD_NORMAL) {
> >> +        /* ignore -no-reboot */
> >> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET_FORCE);
> >> +    } else {
> >> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> >> +    }
> >>      /* as this is triggered by a CPU, make sure to exit the loop */
> >>      if (tcg_enabled()) {
> >>          cpu_loop_exit(cs);
> >> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> >> index e893f72f3b..345369d924 100644
> >> --- a/include/sysemu/sysemu.h
> >> +++ b/include/sysemu/sysemu.h
> >> @@ -44,6 +44,9 @@ typedef enum ShutdownCause {
> >>                                       turns that into a shutdown */
> >>      SHUTDOWN_CAUSE_GUEST_PANIC,   /* Guest panicked, and command line turns
> >>                                       that into a shutdown */
> >> +    SHUTDOWN_CAUSE_GUEST_RESET_FORCE,/* Guest reset that should ignore
> >> +                                        --no-reboot. This is useful for reset
> >> +                                        like actions as s390 kexec/kdump */
> >>      SHUTDOWN_CAUSE__MAX,
> >>  } ShutdownCause;
> >>  
> >> diff --git a/vl.c b/vl.c
> >> index b3426e03d0..44e65071bc 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> >> @@ -1628,7 +1628,7 @@ void qemu_system_reset(ShutdownCause reason)
> >>      } else {
> >>          qemu_devices_reset();
> >>      }
> >> -    if (reason) {
> >> +    if (reason != SHUTDOWN_CAUSE_GUEST_RESET_FORCE) {
> >>          qapi_event_send_reset(shutdown_caused_by_guest(reason),
> >>                                &error_abort);
> >>      }
> >> @@ -1674,7 +1674,7 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
> >>  
> >>  void qemu_system_reset_request(ShutdownCause reason)
> >>  {
> >> -    if (no_reboot) {
> >> +    if (no_reboot && reason != SHUTDOWN_CAUSE_GUEST_RESET_FORCE) {
> >>          shutdown_requested = reason;
> >>      } else {
> >>          reset_requested = reason;
> >>  
> >   
> 

The patch looks sane to me.

Re: [Qemu-devel] [PATCH v3 1/1] s390/ipl: fix ipl with -no-reboot

[Paolo Bonzini]

On 22/06/2018 11:46, Cornelia Huck wrote:
>>> Ok, then my suggestion made even more sense. :)  No other objections
>>> apart from the name of the constant.
>>>
>>> Paolo  
>> SHUTDOWN_CAUSE_S390_PARTIAL ?
> Don't like that one much.
> 
>> SHUTDOWN_CAUSE_S390_SUBSYS_RESET?
> Either that, or drop the 'S390' (is it conceivable that other
> architectures have something similar)?
> 

I don't know exactly what is going on here, so I'm not very suited to
answer this.  The difference just from reading the C code seems to be
that it calls s390_cpu_load_normal instead of s390_ipl_prepare_cpu, and
that it calls subsystem_reset instead of qemu_devices_reset.

For a more generic name I'd go with SHUTDOWN_CAUSE_OS_BOOT (or
OS_BOOTING, or BOOT_OS), but an s390-specific name is of course fine as
well, especially if you have a specific moniker for the "thing that IPL
gives control to".

Paolo

Re: [Qemu-devel] [PATCH v3 1/1] s390/ipl: fix ipl with -no-reboot

[Christian Borntraeger]

On 06/22/2018 11:59 AM, Paolo Bonzini wrote:
> On 22/06/2018 11:46, Cornelia Huck wrote:
>>>> Ok, then my suggestion made even more sense. :)  No other objections
>>>> apart from the name of the constant.
>>>>
>>>> Paolo  
>>> SHUTDOWN_CAUSE_S390_PARTIAL ?
>> Don't like that one much.
>>
>>> SHUTDOWN_CAUSE_S390_SUBSYS_RESET?
>> Either that, or drop the 'S390' (is it conceivable that other
>> architectures have something similar)?
>>
> 
> I don't know exactly what is going on here, so I'm not very suited to
> answer this.  The difference just from reading the C code seems to be
> that it calls s390_cpu_load_normal instead of s390_ipl_prepare_cpu, and
> that it calls subsystem_reset instead of qemu_devices_reset.
> 
> For a more generic name I'd go with SHUTDOWN_CAUSE_OS_BOOT (or
> OS_BOOTING, or BOOT_OS), but an s390-specific name is of course fine as
> well, especially if you have a specific moniker for the "thing that IPL

It is not a BOOT, it is in fact an reset just to the devices.
Right now I favor

SHUTDOWN_CAUSE_SUBSYSTEM_RESET

Re: [Qemu-devel] [PATCH v3 1/1] s390/ipl: fix ipl with -no-reboot

[David Hildenbrand]

On 22.06.2018 12:10, Christian Borntraeger wrote:
> 
> 
> On 06/22/2018 11:59 AM, Paolo Bonzini wrote:
>> On 22/06/2018 11:46, Cornelia Huck wrote:
>>>>> Ok, then my suggestion made even more sense. :)  No other objections
>>>>> apart from the name of the constant.
>>>>>
>>>>> Paolo  
>>>> SHUTDOWN_CAUSE_S390_PARTIAL ?
>>> Don't like that one much.
>>>
>>>> SHUTDOWN_CAUSE_S390_SUBSYS_RESET?
>>> Either that, or drop the 'S390' (is it conceivable that other
>>> architectures have something similar)?
>>>
>>
>> I don't know exactly what is going on here, so I'm not very suited to
>> answer this.  The difference just from reading the C code seems to be
>> that it calls s390_cpu_load_normal instead of s390_ipl_prepare_cpu, and
>> that it calls subsystem_reset instead of qemu_devices_reset.
>>
>> For a more generic name I'd go with SHUTDOWN_CAUSE_OS_BOOT (or
>> OS_BOOTING, or BOOT_OS), but an s390-specific name is of course fine as
>> well, especially if you have a specific moniker for the "thing that IPL
> 
> It is not a BOOT, it is in fact an reset just to the devices.
> Right now I favor
> 
> SHUTDOWN_CAUSE_SUBSYSTEM_RESET
> 

+1

-- 

Thanks,

David / dhildenb