Message ID | 20180621170143.113370-1-borntraeger@de.ibm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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; >
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; >> >
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>
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.
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
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
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
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;
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(-)