s390x: refactor reset/reipl handling
PATCH 1/1 s390x: refactor reset/reipl handling
Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might not be the best idea. As pause_all_vcpus() temporarily drops the qemu mutex, two parallel calls to pause_all_vcpus() can be active at a time, resulting in a deadlock. (either by two VCPUs or by the main thread and a VCPU)
Let's handle it via the main loop instead, as suggested by Paolo. If we would have two parallel reset requests by two different VCPUs at the same time, the last one would win.
We use the existing ipl device to handle it. The nice side effect is that we can get rid of reipl_requested.
This change implies that all reset handling now goes via the common path, so "no-reboot" handling is now active for all kinds of reboots.
Let's execute any CPU initialization code on the target CPU using run_on_cpu.
---
RFC -> v1:
- initital -> initial
- get rid of goto
- store CPU index instead of CPU. Fallback to any CPU in case not found
- handle default case in switch-case
hw/s390x/ipl.c | 43 +++++++++++++++++++++++---- hw/s390x/ipl.h | 16 ++++++++-- hw/s390x/s390-virtio-ccw.c | 51 ++++++++++++++++++++++++++----- include/hw/s390x/s390-virtio-ccw.h | 2 -- target/s390x/cpu.h | 26 ++++++++++++++++ target/s390x/diag.c | 61 +++----------------------------------- target/s390x/internal.h | 6 ---- target/s390x/kvm.c | 2 +- 8 files changed, 127 insertions(+), 80 deletions(-)
hw/s390x/ipl.c +38 -5
@@ -26,6 +26,7 @@#include "qemu/config-file.h" #include "qemu/cutils.h" #include "qemu/option.h" +#include "exec/exec-all.h" #define KERN_IMAGE_START 0x010000UL#define KERN_PARM_AREA 0x010480UL
@@ -484,12 +485,20 @@ IplParameterBlock *s390_ipl_get_iplb(void)return &ipl->iplb; } -void s390_reipl_request (void ) +void s390_ipl_reset_request (CPUState *cs,enum s390_reset reset_type) {S390IPLState *ipl =get_ipl_device (); - ipl->reipl_requested = true; -if (ipl->iplb_valid && +if (reset_type ==S390_RESET_EXTERNAL || reset_type ==S390_RESET_REIPL ) { +/* use CPU 0 for full resets */ + ipl->reset_cpu_index =0 ; + }else { + ipl->reset_cpu_index = cs->cpu_index; + } + ipl->reset_type = reset_type; + +if (reset_type ==S390_RESET_REIPL && + ipl->iplb_valid && !ipl->netboot && ipl->iplb.pbt ==S390_IPL_TYPE_CCW &&is_virtio_scsi_device (&ipl->iplb)) {
@@ -506,6 +515,31 @@ void s390_reipl_request(void) } }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); + } +} + +void s390_ipl_get_reset_request (CPUState **cs,enum s390_reset *reset_type) +{ +S390IPLState *ipl =get_ipl_device (); + + *cs =qemu_get_cpu (ipl->reset_cpu_index); +if (!*cs) { +/* use any CPU */ + *cs = first_cpu; + } + *reset_type = ipl->reset_type; +} + +void s390_ipl_clear_reset_request (void ) +{ +S390IPLState *ipl =get_ipl_device (); + + ipl->reset_type =S390_RESET_EXTERNAL ; +/* use CPU 0 for full resets */ + ipl->reset_cpu_index =0 ; }static void s390_ipl_prepare_qipl (S390CPU *cpu)
@@ -552,11 +586,10 @@ static void s390_ipl_reset(DeviceState *dev) {S390IPLState *ipl =S390_IPL (dev); -if (!ipl->reipl_requested) { +if (ipl->reset_type !=S390_RESET_REIPL ) { ipl->iplb_valid = false;memset (&ipl->iplb,0 ,sizeof (IplParameterBlock)); } - ipl->reipl_requested = false; }static void s390_ipl_class_init (ObjectClass *klass,void *data)
hw/s390x/ipl.h +14 -2
@@ -87,7 +87,17 @@ int s390_ipl_set_loadparm(uint8_t *loadparm);void s390_ipl_update_diag308 (IplParameterBlock *iplb);void s390_ipl_prepare_cpu (S390CPU *cpu);IplParameterBlock *s390_ipl_get_iplb (void ); -void s390_reipl_request (void ); + +enum s390_reset { +/* default is a reset not triggered by a CPU e.g. issued by QMP */ +S390_RESET_EXTERNAL =0 , +S390_RESET_REIPL , +S390_RESET_MODIFIED_CLEAR , +S390_RESET_LOAD_NORMAL , +}; +void s390_ipl_reset_request (CPUState *cs,enum s390_reset reset_type); +void s390_ipl_get_reset_request (CPUState **cs,enum s390_reset *reset_type); +void s390_ipl_clear_reset_request (void );#define QIPL_ADDRESS 0xcc
@@ -129,9 +139,11 @@ struct S390IPLState {bool enforce_bios;IplParameterBlock iplb;bool iplb_valid; -bool reipl_requested;bool netboot;QemuIplParameters qipl; +/* reset related properties don't have to be migrated or reset */ +enum s390_reset reset_type; +int reset_cpu_index;/*< public >*/ char *kernel;
hw/s390x/s390-virtio-ccw.c +44 -7
68 lines not on this page · raw message
include/hw/s390x/s390-virtio-ccw.h +0 -2
@@ -53,6 +53,4 @@ bool cpu_model_allowed(void);*/ bool css_migration_enabled (void ); -void subsystem_reset (void ); -#endif
target/s390x/cpu.h +26 -0
@@ -686,6 +686,32 @@ static inline uint64_t s390_build_validity_mcic(void)return mcic; } +static inline void s390_do_cpu_full_reset (CPUState *cs,run_on_cpu_data arg) +{ +cpu_reset (cs); +} + +static inline void s390_do_cpu_reset (CPUState *cs,run_on_cpu_data arg) +{ +S390CPUClass *scc =S390_CPU_GET_CLASS (cs); + + scc->cpu_reset (cs); +} + +static inline void s390_do_cpu_initial_reset (CPUState *cs,run_on_cpu_data arg) +{ +S390CPUClass *scc =S390_CPU_GET_CLASS (cs); + + scc->initial_cpu_reset (cs); +} + +static inline void s390_do_cpu_load_normal (CPUState *cs,run_on_cpu_data arg) +{ +S390CPUClass *scc =S390_CPU_GET_CLASS (cs); + + scc->load_normal (cs); +} +/* cpu.c */ int s390_get_clock (uint8_t *tod_high,uint64_t *tod_low);
target/s390x/diag.c +4 -57
83 lines not on this page · raw message
target/s390x/internal.h +0 -6
@@ -273,12 +273,6 @@ static inline hwaddr decode_basedisp_s(CPUS390XState *env, uint32_t ipb,/* Base/displacement are at the same locations. */ #define decode_basedisp_rs decode_basedisp_s -static inline void s390_do_cpu_full_reset (CPUState *cs,run_on_cpu_data arg) -{ -cpu_reset (cs); -} - -/* arch_dump.c */ int s390_cpu_write_elf64_note (WriteCoreDumpFunction f,CPUState *cs,int cpuid,void *opaque);
target/s390x/kvm.c +1 -1
@@ -1785,7 +1785,7 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run) ret =handle_intercept (cpu);break ;case KVM_EXIT_S390_RESET : -s390_reipl_request (); +s390_ipl_reset_request (cs,S390_RESET_REIPL );break ;case KVM_EXIT_S390_TSCH : ret =handle_tsch (cpu);
Signature · 1 line
2.14.3
Quoted 28 lines · On Tue, 24 Apr 2018 12:18:59 +0200 David Hildenbrand <[email protected]> wrote:
Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might not be the best idea. As
pause_all_vcpus()temporarily drops the qemu mutex, two parallel calls topause_all_vcpus()can be active at a time, resulting in a deadlock. (either by two VCPUs or by the main thread and a VCPU)Let's handle it via the main loop instead, as suggested by Paolo. If we would have two parallel reset requests by two different VCPUs at the same time, the last one would win.
We use the existing ipl device to handle it. The nice side effect is that we can get rid of
reipl_requested.This change implies that all reset handling now goes via the common path, so "no-reboot" handling is now active for all kinds of reboots.
Let's execute any CPU initialization code on the target CPU using
run_on_cpu.Signed-off-by David Hildenbrand <[email protected]>---
RFC -> v1:
- initital -> initial
- get rid of goto
- store CPU index instead of CPU. Fallback to any CPU in case not found
- handle default case in switch-case
Quoted 9 lines
hw/s390x/ipl.c | 43 +++++++++++++++++++++++---- hw/s390x/ipl.h | 16 ++++++++-- hw/s390x/s390-virtio-ccw.c | 51 ++++++++++++++++++++++++++----- include/hw/s390x/s390-virtio-ccw.h | 2 -- target/s390x/cpu.h | 26 ++++++++++++++++ target/s390x/diag.c | 61 +++----------------------------------- target/s390x/internal.h | 6 ---- target/s390x/kvm.c | 2 +- 8 files changed, 127 insertions(+), 80 deletions(-)
Looks good to me, and I'll apply it after I get some review.
Quoted 19 lines · David Hildenbrand
Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might not be the best idea. As
pause_all_vcpus()temporarily drops the qemu mutex, two parallel calls topause_all_vcpus()can be active at a time, resulting in a deadlock. (either by two VCPUs or by the main thread and a VCPU)Let's handle it via the main loop instead, as suggested by Paolo. If we would have two parallel reset requests by two different VCPUs at the same time, the last one would win.
We use the existing ipl device to handle it. The nice side effect is that we can get rid of
reipl_requested.This change implies that all reset handling now goes via the common path, so "no-reboot" handling is now active for all kinds of reboots.
Let's execute any CPU initialization code on the target CPU using
run_on_cpu.
Quoted 8 lines
Signed-off-by David Hildenbrand <[email protected]>---
RFC -> v1:
- initital -> initial
- get rid of goto
- store CPU index instead of CPU. Fallback to any CPU in case not found
- handle default case in switch-case
Very friendly ping :)
Signature · 3 lines
Thanks, David / dhildenb
Quoted 23 lines · On Wed, 9 May 2018 16:15:38 +0200 David Hildenbrand <[email protected]> wrote:
Quoted 22 lines · David Hildenbrand
Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might not be the best idea. As
pause_all_vcpus()temporarily drops the qemu mutex, two parallel calls topause_all_vcpus()can be active at a time, resulting in a deadlock. (either by two VCPUs or by the main thread and a VCPU)Let's handle it via the main loop instead, as suggested by Paolo. If we would have two parallel reset requests by two different VCPUs at the same time, the last one would win.
We use the existing ipl device to handle it. The nice side effect is that we can get rid of
reipl_requested.This change implies that all reset handling now goes via the common path, so "no-reboot" handling is now active for all kinds of reboots.
Let's execute any CPU initialization code on the target CPU using
run_on_cpu.Signed-off-by David Hildenbrand <[email protected]>---
Quoted 7 lines
Quoted 5 lines
RFC -> v1:
- initital -> initial
- get rid of goto
- store CPU index instead of CPU. Fallback to any CPU in case not found
- handle default case in switch-caseVery friendly ping :)
I'm currently inclined to just go ahead and merge it -- so if anyone still wants to provide review, now's the time :)
Quoted 28 lines · David Hildenbrand
Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might not be the best idea. As
pause_all_vcpus()temporarily drops the qemu mutex, two parallel calls topause_all_vcpus()can be active at a time, resulting in a deadlock. (either by two VCPUs or by the main thread and a VCPU)Let's handle it via the main loop instead, as suggested by Paolo. If we would have two parallel reset requests by two different VCPUs at the same time, the last one would win.
We use the existing ipl device to handle it. The nice side effect is that we can get rid of
reipl_requested.This change implies that all reset handling now goes via the common path, so "no-reboot" handling is now active for all kinds of reboots.
Let's execute any CPU initialization code on the target CPU using
run_on_cpu.Signed-off-by David Hildenbrand <[email protected]>---
RFC -> v1:
- initital -> initial
- get rid of goto
- store CPU index instead of CPU. Fallback to any CPU in case not found
- handle default case in switch-case
Quoted 9 lines
hw/s390x/ipl.c | 43 +++++++++++++++++++++++---- hw/s390x/ipl.h | 16 ++++++++-- hw/s390x/s390-virtio-ccw.c | 51 ++++++++++++++++++++++++++----- include/hw/s390x/s390-virtio-ccw.h | 2 -- target/s390x/cpu.h | 26 ++++++++++++++++ target/s390x/diag.c | 61 +++----------------------------------- target/s390x/internal.h | 6 ---- target/s390x/kvm.c | 2 +- 8 files changed, 127 insertions(+), 80 deletions(-)
I'm not an expert in all this s390x reset stuff, but as far as I can see, this looks sane.
Cornelia Huck
AppliedQuoted 28 lines · On Tue, 24 Apr 2018 12:18:59 +0200 David Hildenbrand <[email protected]> wrote:
Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might not be the best idea. As
pause_all_vcpus()temporarily drops the qemu mutex, two parallel calls topause_all_vcpus()can be active at a time, resulting in a deadlock. (either by two VCPUs or by the main thread and a VCPU)Let's handle it via the main loop instead, as suggested by Paolo. If we would have two parallel reset requests by two different VCPUs at the same time, the last one would win.
We use the existing ipl device to handle it. The nice side effect is that we can get rid of
reipl_requested.This change implies that all reset handling now goes via the common path, so "no-reboot" handling is now active for all kinds of reboots.
Let's execute any CPU initialization code on the target CPU using
run_on_cpu.Signed-off-by David Hildenbrand <[email protected]>---
RFC -> v1:
- initital -> initial
- get rid of goto
- store CPU index instead of CPU. Fallback to any CPU in case not found
- handle default case in switch-case
Quoted 9 lines
hw/s390x/ipl.c | 43 +++++++++++++++++++++++---- hw/s390x/ipl.h | 16 ++++++++-- hw/s390x/s390-virtio-ccw.c | 51 ++++++++++++++++++++++++++----- include/hw/s390x/s390-virtio-ccw.h | 2 -- target/s390x/cpu.h | 26 ++++++++++++++++ target/s390x/diag.c | 61 +++----------------------------------- target/s390x/internal.h | 6 ---- target/s390x/kvm.c | 2 +- 8 files changed, 127 insertions(+), 80 deletions(-)
Thanks, applied.
Quoted 10 lines · David Hildenbrand
Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might not be the best idea. As
pause_all_vcpus()temporarily drops the qemu mutex, two parallel calls topause_all_vcpus()can be active at a time, resulting in a deadlock. (either by two VCPUs or by the main thread and a VCPU)Let's handle it via the main loop instead, as suggested by Paolo. If we would have two parallel reset requests by two different VCPUs at the same time, the last one would win.
Quoted 5 lines
We use the existing ipl device to handle it. The nice side effect is that we can get rid of
reipl_requested.This change implies that all reset handling now goes via the common path, so "no-reboot" handling is now active for all kinds of reboots.
Ok, this breaks the s390 IPL process when -no-reboot is specified. The bios does a diagnose 308 subcode 1 to jump to the final image while at the same time resetting all devices. This is now blocked with -no-reboot (although it is actually the boot)
I have noticed that with virt-install on iso images since virt-install specifies -no-reboot.
Something like this seems to help but it is not a nice solution.
hw/s390x/ipl.c +7 -1
@@ -534,8 +534,14 @@ 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 ); + }else 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 ); } -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);
include/sysemu/sysemu.h +3 -0
@@ -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 +useful for the subsystem resets on s390 done +for kdump and kexec */ SHUTDOWN_CAUSE__MAX , } ShutdownCause;
vl.c +3 -1
@@ -1674,7 +1674,9 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)void qemu_system_reset_request (ShutdownCause reason) { -if (no_reboot) { +if (reason ==SHUTDOWN_CAUSE_GUEST_RESET_FORCE ) { + reset_requested =SHUTDOWN_CAUSE_GUEST_RESET ; + }else if (no_reboot) { shutdown_requested = reason; }else { reset_requested = reason;
Signature · 1 line
2.17.0
Quoted 16 lines · Christian Borntraeger
Quoted 13 lines · David Hildenbrand
Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might not be the best idea. As
pause_all_vcpus()temporarily drops the qemu mutex, two parallel calls topause_all_vcpus()can be active at a time, resulting in a deadlock. (either by two VCPUs or by the main thread and a VCPU)Let's handle it via the main loop instead, as suggested by Paolo. If we would have two parallel reset requests by two different VCPUs at the same time, the last one would win.
We use the existing ipl device to handle it. The nice side effect is that we can get rid of
reipl_requested.
Quoted 7 lines
Quoted 2 lines
This change implies that all reset handling now goes via the common path, so "no-reboot" handling is now active for all kinds of reboots.
Ok, this breaks the s390 IPL process when -no-reboot is specified. The bios does a diagnose 308 subcode 1 to jump to the final image while at the same time resetting all devices. This is now blocked with -no-reboot (although it is actually the boot)
Indeed, one corner case I missed, thanks for looking into it.
I wonder if we could turn that into a allow "one S390_RESET_MODIFIED_CLEAR ignoring -no-reboot" if booting from bios using a simple flag.
Then s/SHUTDOWN_CAUSE_GUEST_RESET/SHUTDOWN_CAUSE_BIOS_RESET/
Quoted 32 lines
I have noticed that with virt-install on iso images since virt-install specifies -no-reboot.
Something like this seems to help but it is not a nice solution.
hw/s390x/ipl.c +7
-1@@ -534,8 +534,14 @@ 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 ); + }else 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 ); } -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);include/sysemu/sysemu.h +0
-0
include/sysemu/sysemu.h typedef enum ShutdownCause { · line 49
Quoted 6 lines
include/sysemu/sysemu.h +3
-0@@ -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 +useful for the subsystem resets on s390 done +for kdump and kexec */
I don't think it is that ugly.
Quoted 19 lines
include/sysemu/sysemu.h +0
-0SHUTDOWN_CAUSE__MAX, } ShutdownCause;vl.c +3
-1@@ -1674,7 +1674,9 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)void qemu_system_reset_request (ShutdownCause reason) { -if (no_reboot) { +if (reason ==SHUTDOWN_CAUSE_GUEST_RESET_FORCE ) { + reset_requested =SHUTDOWN_CAUSE_GUEST_RESET ; + }else if (no_reboot) { shutdown_requested = reason; }else { reset_requested = reason;
Signature · 3 lines
Thanks, David / dhildenb
Quoted 23 lines · On Thu, 21 Jun 2018 17:49:56 +0200 Christian Borntraeger…
Quoted 10 lines · David Hildenbrand
Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might not be the best idea. As
pause_all_vcpus()temporarily drops the qemu mutex, two parallel calls topause_all_vcpus()can be active at a time, resulting in a deadlock. (either by two VCPUs or by the main thread and a VCPU)Let's handle it via the main loop instead, as suggested by Paolo. If we would have two parallel reset requests by two different VCPUs at the same time, the last one would win.
Quoted 5 lines
We use the existing ipl device to handle it. The nice side effect is that we can get rid of
reipl_requested.This change implies that all reset handling now goes via the common path, so "no-reboot" handling is now active for all kinds of reboots.
Ok, this breaks the s390 IPL process when -no-reboot is specified. The bios does a diagnose 308 subcode 1 to jump to the final image while at the same time resetting all devices. This is now blocked with -no-reboot (although it is actually the boot)
Quoted 4 lines
I have noticed that with virt-install on iso images since virt-install specifies -no-reboot.
Something like this seems to help but it is not a nice solution.
It's a bit ugly, but allows us to accommodate the different semantics for the different diag subcodes... the -no-reboot parameter is only supposed to suppress normal guest reboot requests, right?
Quoted 50 lines
hw/s390x/ipl.c +7
-1@@ -534,8 +534,14 @@ 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 ); + }else 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 ); } -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);include/sysemu/sysemu.h +3
-0@@ -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 +useful for the subsystem resets on s390 done +for kdump and kexec */ SHUTDOWN_CAUSE__MAX , } ShutdownCause;vl.c +3
-1@@ -1674,7 +1674,9 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)void qemu_system_reset_request (ShutdownCause reason) { -if (no_reboot) { +if (reason ==SHUTDOWN_CAUSE_GUEST_RESET_FORCE ) { + reset_requested =SHUTDOWN_CAUSE_GUEST_RESET ; + }else if (no_reboot) { shutdown_requested = reason; }else { reset_requested = reason;
Quoted 26 lines · Cornelia Huck
Quoted 23 lines · On Thu, 21 Jun 2018 17:49:56 +0200 Christian Borntraeger…
Quoted 10 lines · David Hildenbrand
Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might not be the best idea. As
pause_all_vcpus()temporarily drops the qemu mutex, two parallel calls topause_all_vcpus()can be active at a time, resulting in a deadlock. (either by two VCPUs or by the main thread and a VCPU)Let's handle it via the main loop instead, as suggested by Paolo. If we would have two parallel reset requests by two different VCPUs at the same time, the last one would win.
Quoted 5 lines
We use the existing ipl device to handle it. The nice side effect is that we can get rid of
reipl_requested.This change implies that all reset handling now goes via the common path, so "no-reboot" handling is now active for all kinds of reboots.
Ok, this breaks the s390 IPL process when -no-reboot is specified. The bios does a diagnose 308 subcode 1 to jump to the final image while at the same time resetting all devices. This is now blocked with -no-reboot (although it is actually the boot)
Quoted 8 lines
Quoted 4 lines
I have noticed that with virt-install on iso images since virt-install specifies -no-reboot.
Something like this seems to help but it is not a nice solution.
It's a bit ugly, but allows us to accommodate the different semantics for the different diag subcodes... the -no-reboot parameter is only supposed to suppress normal guest reboot requests, right?
At least thats how it worked until 2.12. Without this we have no kdump/kexec/ipl as soon as somebody uses -no-reboot.
Shall I respin my patch as proper patch?
Quoted 52 lines
Quoted 51 lines
hw/s390x/ipl.c +7
-1@@ -534,8 +534,14 @@ 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 ); + }else 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 ); } -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);include/sysemu/sysemu.h +3
-0@@ -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 +useful for the subsystem resets on s390 done +for kdump and kexec */ SHUTDOWN_CAUSE__MAX , } ShutdownCause;vl.c +3
-1@@ -1674,7 +1674,9 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)void qemu_system_reset_request (ShutdownCause reason) { -if (no_reboot) { +if (reason ==SHUTDOWN_CAUSE_GUEST_RESET_FORCE ) { + reset_requested =SHUTDOWN_CAUSE_GUEST_RESET ; + }else if (no_reboot) { shutdown_requested = reason; }else { reset_requested = reason;
Quoted 32 lines · Christian Borntraeger
Quoted 29 lines · Cornelia Huck
Quoted 26 lines · On Thu, 21 Jun 2018 17:49:56 +0200 Christian Borntraeger…
Quoted 10 lines · David Hildenbrand
Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might not be the best idea. As
pause_all_vcpus()temporarily drops the qemu mutex, two parallel calls topause_all_vcpus()can be active at a time, resulting in a deadlock. (either by two VCPUs or by the main thread and a VCPU)Let's handle it via the main loop instead, as suggested by Paolo. If we would have two parallel reset requests by two different VCPUs at the same time, the last one would win.
Quoted 5 lines
We use the existing ipl device to handle it. The nice side effect is that we can get rid of
reipl_requested.This change implies that all reset handling now goes via the common path, so "no-reboot" handling is now active for all kinds of reboots.
Ok, this breaks the s390 IPL process when -no-reboot is specified. The bios does a diagnose 308 subcode 1 to jump to the final image while at the same time resetting all devices. This is now blocked with -no-reboot (although it is actually the boot)
I have noticed that with virt-install on iso images since virt-install specifies -no-reboot.
Quoted 9 lines
Quoted 5 lines
Quoted 1 line
Something like this seems to help but it is not a nice solution.
It's a bit ugly, but allows us to accommodate the different semantics for the different diag subcodes... the -no-reboot parameter is only supposed to suppress normal guest reboot requests, right?
At least thats how it worked until 2.12. Without this we have no kdump/kexec/ipl as soon as somebody uses -no-reboot.
Paolo even said that consistent -no-reboot handling would be one of the benefits :)
Quoted 2 lines
Shall I respin my patch as proper patch?
Yes, sure.
Signature · 3 lines
Thanks, David / dhildenb
Quoted 6 lines · David Hildenbrand
Quoted 3 lines
Without this we have no kdump/kexec/ipl
as soon as somebody uses -no-reboot.Paolo even said that consistent -no-reboot handling would be one of the benefits :)
kdump/kexec (and of course "ipl") do not reboot on x86, so fixing the bug does help in making things consistent. :)
Do we really want a reset event for ipl? Maybe for kexec/kdump, but perhaps just using SHUTDOWN_CAUSE_NONE for MODIFIED_CLEAR and LOAD_NORMAL (not sure about the difference) is the way to go.
For example, x86 suspend is a "reboot" but it is already special cased to use SHUTDOWN_CAUSE_NONE.
Paolo
Quoted 26 lines · Cornelia Huck
Quoted 23 lines · On Thu, 21 Jun 2018 17:49:56 +0200 Christian Borntraeger…
Quoted 10 lines · David Hildenbrand
Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might not be the best idea. As
pause_all_vcpus()temporarily drops the qemu mutex, two parallel calls topause_all_vcpus()can be active at a time, resulting in a deadlock. (either by two VCPUs or by the main thread and a VCPU)Let's handle it via the main loop instead, as suggested by Paolo. If we would have two parallel reset requests by two different VCPUs at the same time, the last one would win.
Quoted 5 lines
We use the existing ipl device to handle it. The nice side effect is that we can get rid of
reipl_requested.This change implies that all reset handling now goes via the common path, so "no-reboot" handling is now active for all kinds of reboots.
Ok, this breaks the s390 IPL process when -no-reboot is specified. The bios does a diagnose 308 subcode 1 to jump to the final image while at the same time resetting all devices. This is now blocked with -no-reboot (although it is actually the boot)
Quoted 8 lines
Quoted 4 lines
I have noticed that with virt-install on iso images since virt-install specifies -no-reboot.
Something like this seems to help but it is not a nice solution.
It's a bit ugly, but allows us to accommodate the different semantics for the different diag subcodes... the -no-reboot parameter is only supposed to suppress normal guest reboot requests, right?
The question is if -no-reboot should also be considered for kexec. There are (at least on x86, not sure about s390x) "fast reboot techniques using kexec". So it feels like -no-reboot should also apply for kexec. But definitely not for the BIOS :)
Signature · 3 lines
Thanks, David / dhildenb
Quoted 32 lines · David Hildenbrand
Quoted 31 lines · Cornelia Huck
Quoted 27 lines · On Thu, 21 Jun 2018 17:49:56 +0200 Christian Borntraeger…
Quoted 10 lines · David Hildenbrand
Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might not be the best idea. As
pause_all_vcpus()temporarily drops the qemu mutex, two parallel calls topause_all_vcpus()can be active at a time, resulting in a deadlock. (either by two VCPUs or by the main thread and a VCPU)Let's handle it via the main loop instead, as suggested by Paolo. If we would have two parallel reset requests by two different VCPUs at the same time, the last one would win.
Quoted 5 lines
We use the existing ipl device to handle it. The nice side effect is that we can get rid of
reipl_requested.This change implies that all reset handling now goes via the common path, so "no-reboot" handling is now active for all kinds of reboots.
Ok, this breaks the s390 IPL process when -no-reboot is specified. The bios does a diagnose 308 subcode 1 to jump to the final image while at the same time resetting all devices. This is now blocked with -no-reboot (although it is actually the boot)
I have noticed that with virt-install on iso images since virt-install specifies -no-reboot.
Something like this seems to help but it is not a nice solution.
Quoted 7 lines
Quoted 3 lines
It's a bit ugly, but allows us to accommodate the different semantics for the different diag subcodes... the -no-reboot parameter is only supposed to suppress normal guest reboot requests, right?
The question is if -no-reboot should also be considered for kexec. There are (at least on x86, not sure about s390x) "fast reboot techniques using kexec". So it feels like -no-reboot should also apply for kexec.
these fast reboots via kexec also work on s390.
But from a machine perspective kexec/kdump is not a reboot. It should not be blocked by -no-reboot - if it does it would be wrong from a user perspective.
Quoted 3 lines
But definitely not for the BIOS :)
Quoted 69 lines · Christian Borntraeger
Quoted 10 lines · David Hildenbrand
Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might not be the best idea. As
pause_all_vcpus()temporarily drops the qemu mutex, two parallel calls topause_all_vcpus()can be active at a time, resulting in a deadlock. (either by two VCPUs or by the main thread and a VCPU)Let's handle it via the main loop instead, as suggested by Paolo. If we would have two parallel reset requests by two different VCPUs at the same time, the last one would win.
Quoted 5 lines
We use the existing ipl device to handle it. The nice side effect is that we can get rid of
reipl_requested.This change implies that all reset handling now goes via the common path, so "no-reboot" handling is now active for all kinds of reboots.
Ok, this breaks the s390 IPL process when -no-reboot is specified. The bios does a diagnose 308 subcode 1 to jump to the final image while at the same time resetting all devices. This is now blocked with -no-reboot (although it is actually the boot)
I have noticed that with virt-install on iso images since virt-install specifies -no-reboot.
Something like this seems to help but it is not a nice solution.
hw/s390x/ipl.c +7
-1@@ -534,8 +534,14 @@ 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 ); + }else 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 ); } -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);include/sysemu/sysemu.h +3
-0@@ -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 +useful for the subsystem resets on s390 done +for kdump and kexec */ SHUTDOWN_CAUSE__MAX , } ShutdownCause;vl.c +0
-0
vl.c void qemu_system_guest_panicked(GuestPanicInformation *info) · line 1678
Quoted 6 lines
vl.c +2
-1@@ -1674,7 +1674,9 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)void qemu_system_reset_request (ShutdownCause reason) { -if (no_reboot) { +if (reason ==SHUTDOWN_CAUSE_GUEST_RESET_FORCE ) { + reset_requested =SHUTDOWN_CAUSE_GUEST_RESET ;
As the value is not use anywhere, you can make this less ugly by not setting it like this maybe
if (reason != SHUTDOWN_CAUSE_GUEST_RESET_FORCE && no_reboot)
...
Quoted 5 lines
vl.c +1
-0+ }else if (no_reboot) { shutdown_requested = reason; }else { reset_requested = reason;
Signature · 3 lines
Thanks, David / dhildenb
Quoted 80 lines · David Hildenbrand
Quoted 69 lines · Christian Borntraeger
Quoted 10 lines · David Hildenbrand
Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might not be the best idea. As
pause_all_vcpus()temporarily drops the qemu mutex, two parallel calls topause_all_vcpus()can be active at a time, resulting in a deadlock. (either by two VCPUs or by the main thread and a VCPU)Let's handle it via the main loop instead, as suggested by Paolo. If we would have two parallel reset requests by two different VCPUs at the same time, the last one would win.
Quoted 5 lines
We use the existing ipl device to handle it. The nice side effect is that we can get rid of
reipl_requested.This change implies that all reset handling now goes via the common path, so "no-reboot" handling is now active for all kinds of reboots.
Ok, this breaks the s390 IPL process when -no-reboot is specified. The bios does a diagnose 308 subcode 1 to jump to the final image while at the same time resetting all devices. This is now blocked with -no-reboot (although it is actually the boot)
I have noticed that with virt-install on iso images since virt-install specifies -no-reboot.
Something like this seems to help but it is not a nice solution.
hw/s390x/ipl.c +7
-1@@ -534,8 +534,14 @@ 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 ); + }else 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 ); } -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);include/sysemu/sysemu.h +3
-0@@ -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 +useful for the subsystem resets on s390 done +for kdump and kexec */ SHUTDOWN_CAUSE__MAX , } ShutdownCause;vl.c +0
-0vl.c void qemu_system_guest_panicked(GuestPanicInformation *info) · line 1678
Quoted 6 lines
vl.c +2
-1@@ -1674,7 +1674,9 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)void qemu_system_reset_request (ShutdownCause reason) { -if (no_reboot) { +if (reason ==SHUTDOWN_CAUSE_GUEST_RESET_FORCE ) { + reset_requested =SHUTDOWN_CAUSE_GUEST_RESET ;As the value is not use anywhere, you can make this less ugly by not setting it like this maybe
Quoted 1 line
if (reason !=
SHUTDOWN_CAUSE_GUEST_RESET_FORCE&&no_reboot)
I also change the reason from SHUTDOWN_CAUSE_GUEST_RESET_FORCE back to SHUTDOWN_CAUSE_GUEST_RESET. I think I have to, so that the handler do not need to be modified. No?
Quoted 83 lines · Christian Borntraeger
Quoted 80 lines · David Hildenbrand
Quoted 69 lines · Christian Borntraeger
Quoted 10 lines · David Hildenbrand
Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might not be the best idea. As
pause_all_vcpus()temporarily drops the qemu mutex, two parallel calls topause_all_vcpus()can be active at a time, resulting in a deadlock. (either by two VCPUs or by the main thread and a VCPU)Let's handle it via the main loop instead, as suggested by Paolo. If we would have two parallel reset requests by two different VCPUs at the same time, the last one would win.
Quoted 5 lines
We use the existing ipl device to handle it. The nice side effect is that we can get rid of
reipl_requested.This change implies that all reset handling now goes via the common path, so "no-reboot" handling is now active for all kinds of reboots.
Ok, this breaks the s390 IPL process when -no-reboot is specified. The bios does a diagnose 308 subcode 1 to jump to the final image while at the same time resetting all devices. This is now blocked with -no-reboot (although it is actually the boot)
I have noticed that with virt-install on iso images since virt-install specifies -no-reboot.
Something like this seems to help but it is not a nice solution.
hw/s390x/ipl.c +7
-1@@ -534,8 +534,14 @@ 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 ); + }else 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 ); } -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);include/sysemu/sysemu.h +3
-0@@ -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 +useful for the subsystem resets on s390 done +for kdump and kexec */ SHUTDOWN_CAUSE__MAX , } ShutdownCause;vl.c +0
-0vl.c void qemu_system_guest_panicked(GuestPanicInformation *info) · line 1678
Quoted 6 lines
vl.c +2
-1@@ -1674,7 +1674,9 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)void qemu_system_reset_request (ShutdownCause reason) { -if (no_reboot) { +if (reason ==SHUTDOWN_CAUSE_GUEST_RESET_FORCE ) { + reset_requested =SHUTDOWN_CAUSE_GUEST_RESET ;As the value is not use anywhere, you can make this less ugly by not setting it like this maybe
Quoted 5 lines
Quoted 1 line
if (reason !=
SHUTDOWN_CAUSE_GUEST_RESET_FORCE&&no_reboot)I also change the reason from
SHUTDOWN_CAUSE_GUEST_RESET_FORCEback toSHUTDOWN_CAUSE_GUEST_RESET. I think I have to, so that the handler do not need to be modified. No?
Now I see you point. You say nobody uses the value?
Quoted 86 lines · Christian Borntraeger
Quoted 83 lines · Christian Borntraeger
Quoted 80 lines · David Hildenbrand
Quoted 69 lines · Christian Borntraeger
Quoted 10 lines · David Hildenbrand
Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might not be the best idea. As
pause_all_vcpus()temporarily drops the qemu mutex, two parallel calls topause_all_vcpus()can be active at a time, resulting in a deadlock. (either by two VCPUs or by the main thread and a VCPU)Let's handle it via the main loop instead, as suggested by Paolo. If we would have two parallel reset requests by two different VCPUs at the same time, the last one would win.
Quoted 5 lines
We use the existing ipl device to handle it. The nice side effect is that we can get rid of
reipl_requested.This change implies that all reset handling now goes via the common path, so "no-reboot" handling is now active for all kinds of reboots.
Ok, this breaks the s390 IPL process when -no-reboot is specified. The bios does a diagnose 308 subcode 1 to jump to the final image while at the same time resetting all devices. This is now blocked with -no-reboot (although it is actually the boot)
I have noticed that with virt-install on iso images since virt-install specifies -no-reboot.
Something like this seems to help but it is not a nice solution.
hw/s390x/ipl.c +7
-1@@ -534,8 +534,14 @@ 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 ); + }else 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 ); } -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);include/sysemu/sysemu.h +3
-0@@ -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 +useful for the subsystem resets on s390 done +for kdump and kexec */ SHUTDOWN_CAUSE__MAX , } ShutdownCause;vl.c +0
-0vl.c void qemu_system_guest_panicked(GuestPanicInformation *info) · line 1678
Quoted 6 lines
vl.c +2
-1@@ -1674,7 +1674,9 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)void qemu_system_reset_request (ShutdownCause reason) { -if (no_reboot) { +if (reason ==SHUTDOWN_CAUSE_GUEST_RESET_FORCE ) { + reset_requested =SHUTDOWN_CAUSE_GUEST_RESET ;As the value is not use anywhere, you can make this less ugly by not setting it like this maybe
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
if (reason !=
SHUTDOWN_CAUSE_GUEST_RESET_FORCE&&no_reboot)I also change the reason from
SHUTDOWN_CAUSE_GUEST_RESET_FORCEback toSHUTDOWN_CAUSE_GUEST_RESET. I think I have to, so that the handler do not need to be modified. No?Now I see you point. You say nobody uses the value?
Yes, the only relevant place I see is
shutdown_caused_by_guest()
and that seems to work just fine with that change.
Signature · 3 lines
Thanks, David / dhildenb