Re: [Qemu-devel] [PATCH RFC] s390x: refactor reset/reipl handling

[David Hildenbrand]

On 23.04.2018 12:42, Cornelia Huck wrote:
> On Thu, 12 Apr 2018 21:26:02 +0200
> David Hildenbrand <address@hidden> 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 <address@hidden>
>> ---
>>
>> 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