On 23.04.2018 12:42, Cornelia Huck wrote:
> 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 :)
Yes, that's the thing I was no realizing, while hacking on this. I
thought I would have to shift the whole block.
>
>> + 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?
Well, it's the same we already had just at a different place :)
>
> 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.
We will always create at least one CPU. The fist CPU will always be CPU
with ID 0. As cpu_index corresponds to ID for us, we will always get CPU
0 via qemu_get_cpu(). (and we don't have CPU unplug)
However I might just store the cpu id in there instead of an reference.
So we could "fallback" here to some random CPU that exists (in case
qemu_get_cpu() returns NULL).
>
>> + }
>> + *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'?
I keep making this spelling error over and over again :D (I think my
brain memorized how to type "initial" fast, the wrong way)
Thanks!
>
> (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?
Yes, makes sense!
>
>> + }
>> + s390_ipl_clear_reset_request();
>> }
>>
>> static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
Thanks!
--
Thanks,
David / dhildenb
