On Thu, 12 Apr 2018 21:26:02 +0200 David Hildenbrand <da...@redhat.com> 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. Yes, I like this patch. > > 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) { Pull the check for S390_RESET_REIPL into the if condition below? Gets rid of the goto :) > + goto no_reipl; > + } > + > 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); > + } > +} > + > +void s390_ipl_get_reset_request(CPUState **cs, enum s390_reset *reset_type) > +{ > + S390IPLState *ipl = get_ipl_device(); > + > + *cs = ipl->reset_cpu; > + if (!ipl->reset_cpu) { > + /* use CPU 0 as default */ > + *cs = qemu_get_cpu(0); That looks somewhat ugly. Maybe just stuff cpu 0 in there at init time? As an aside: Are we guaranteed to always have cpu 0? IIRC there was some code relying on that; but just grabbing an 'any' cpu looks more like what we want. > + } > + *reset_type = ipl->reset_type; > +} > static void s390_machine_reset(void) > { > - S390CPU *ipl_cpu = S390_CPU(qemu_get_cpu(0)); > + enum s390_reset reset_type; > + CPUState *cs, *t; > > + /* get the reset parameters, reset them once done */ > + s390_ipl_get_reset_request(&cs, &reset_type); > + > + /* all CPUs are paused and synchronized at this point */ > s390_cmma_reset(); > - qemu_devices_reset(); > - s390_crypto_reset(); > > - /* all cpus are stopped - configure and start the ipl cpu only */ > - s390_ipl_prepare_cpu(ipl_cpu); > - s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu); > + switch (reset_type) { > + case S390_RESET_EXTERNAL: > + case S390_RESET_REIPL: > + qemu_devices_reset(); > + s390_crypto_reset(); > + > + /* configure and start the ipl CPU only */ > + run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL); > + break; > + case S390_RESET_MODIFIED_CLEAR: > + CPU_FOREACH(t) { > + run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL); > + } > + subsystem_reset(); > + s390_crypto_reset(); > + run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL); > + break; > + case S390_RESET_LOAD_NORMAL: > + CPU_FOREACH(t) { > + run_on_cpu(t, s390_do_cpu_reset, RUN_ON_CPU_NULL); > + } > + subsystem_reset(); > + run_on_cpu(cs, s390_do_cpu_initital_reset, RUN_ON_CPU_NULL); 'initital' looks like a typo; 'initial'? (But you made the same typo twice, so it compiles :) > + run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL); > + break; Moan on default case hit? > + } > + s390_ipl_clear_reset_request(); > } > > static void s390_machine_device_plug(HotplugHandler *hotplug_dev,