On 12.04.2018 21:26, David Hildenbrand 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 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. > > Signed-off-by: David Hildenbrand <da...@redhat.com> > --- > > Did only a quick check with TCG. I think this way it is way easier to > control what is getting reset. > > hw/s390x/ipl.c | 44 ++++++++++++++++++++++++--- > hw/s390x/ipl.h | 16 ++++++++-- > hw/s390x/s390-virtio-ccw.c | 49 +++++++++++++++++++++++++----- > 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(+), 79 deletions(-) > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index fb554ab156..f7589d20f1 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -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,11 +485,22 @@ 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 (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL) > { > + /* let's always use CPU 0 */ > + ipl->reset_cpu = NULL; > + } else { > + ipl->reset_cpu = cs; > + } > + ipl->reset_type = reset_type; > + > + if (reset_type != S390_RESET_REIPL) { > + goto no_reipl;
Could we please avoid goto in this case and simply put the code below into a "if (reset_type == S390_RESET_REIPL)" block? > + } > + > if (ipl->iplb_valid && > !ipl->netboot && > ipl->iplb.pbt == S390_IPL_TYPE_CCW && > @@ -505,7 +517,32 @@ void s390_reipl_request(void) > ipl->iplb_valid = s390_gen_initial_iplb(ipl); > } > } > +no_reipl: > 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); > + } > +} Thomas