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

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

[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 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 <david@redhat.com>
---

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(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index fb554ab156..e0d7265793 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,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)
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index 0570d0ad75..4e87b89418 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -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;
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 435f7c99e7..945a8c6fb8 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -93,7 +93,7 @@ static const char *const reset_dev_types[] = {
     "diag288",
 };
 
-void subsystem_reset(void)
+static void subsystem_reset(void)
 {
     DeviceState *dev;
     int i;
@@ -364,17 +364,54 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev,
     }
 }
 
+static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg)
+{
+    S390CPU *cpu = S390_CPU(cs);
+
+    s390_ipl_prepare_cpu(cpu);
+    s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
+}
+
 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_initial_reset, RUN_ON_CPU_NULL);
+        run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
+        break;
+    default:
+        g_assert_not_reached();
+    }
+    s390_ipl_clear_reset_request();
 }
 
 static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index ac896e31ea..ab88d49d10 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -53,6 +53,4 @@ bool cpu_model_allowed(void);
  */
 bool css_migration_enabled(void);
 
-void subsystem_reset(void);
-
 #endif
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 3ee40f08b7..6629a533f3 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -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);
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index a755837ad5..ac2c40f363 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -22,51 +22,6 @@
 #include "hw/s390x/ipl.h"
 #include "hw/s390x/s390-virtio-ccw.h"
 
-static int modified_clear_reset(S390CPU *cpu)
-{
-    S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
-    CPUState *t;
-
-    pause_all_vcpus();
-    cpu_synchronize_all_states();
-    CPU_FOREACH(t) {
-        run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
-    }
-    s390_cmma_reset();
-    subsystem_reset();
-    s390_crypto_reset();
-    scc->load_normal(CPU(cpu));
-    cpu_synchronize_all_post_reset();
-    resume_all_vcpus();
-    return 0;
-}
-
-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 int load_normal_reset(S390CPU *cpu)
-{
-    S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
-    CPUState *t;
-
-    pause_all_vcpus();
-    cpu_synchronize_all_states();
-    CPU_FOREACH(t) {
-        run_on_cpu(t, s390_do_cpu_reset, RUN_ON_CPU_NULL);
-    }
-    s390_cmma_reset();
-    subsystem_reset();
-    scc->initial_cpu_reset(CPU(cpu));
-    scc->load_normal(CPU(cpu));
-    cpu_synchronize_all_post_reset();
-    resume_all_vcpus();
-    return 0;
-}
-
 int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
 {
     uint64_t func = env->regs[r1];
@@ -101,6 +56,7 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
 
 void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
 {
+    CPUState *cs = CPU(s390_env_get_cpu(env));
     uint64_t addr =  env->regs[r1];
     uint64_t subcode = env->regs[r3];
     IplParameterBlock *iplb;
@@ -117,22 +73,13 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
 
     switch (subcode) {
     case 0:
-        modified_clear_reset(s390_env_get_cpu(env));
-        if (tcg_enabled()) {
-            cpu_loop_exit(CPU(s390_env_get_cpu(env)));
-        }
+        s390_ipl_reset_request(cs, S390_RESET_MODIFIED_CLEAR);
         break;
     case 1:
-        load_normal_reset(s390_env_get_cpu(env));
-        if (tcg_enabled()) {
-            cpu_loop_exit(CPU(s390_env_get_cpu(env)));
-        }
+        s390_ipl_reset_request(cs, S390_RESET_LOAD_NORMAL);
         break;
     case 3:
-        s390_reipl_request();
-        if (tcg_enabled()) {
-            cpu_loop_exit(CPU(s390_env_get_cpu(env)));
-        }
+        s390_ipl_reset_request(cs, S390_RESET_REIPL);
         break;
     case 5:
         if ((r1 & 1) || (addr & 0x0fffULL)) {
diff --git a/target/s390x/internal.h b/target/s390x/internal.h
index d911e84958..e392a02d12 100644
--- a/target/s390x/internal.h
+++ b/target/s390x/internal.h
@@ -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);
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index fb59d92def..b033d53f69 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -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);
-- 
2.14.3

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

[Cornelia Huck]

On Tue, 24 Apr 2018 12:18:59 +0200
David Hildenbrand <david@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 <david@redhat.com>
> ---
> 
> 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(-)

Looks good to me, and I'll apply it after I get some review.

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

[David Hildenbrand]

On 24.04.2018 12:18, 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 <david@redhat.com>
> ---
> 
> 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 :)


-- 

Thanks,

David / dhildenb

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

[Cornelia Huck]

On Wed, 9 May 2018 16:15:38 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 24.04.2018 12:18, 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 <david@redhat.com>
> > ---
> > 
> > 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 :)

I'm currently inclined to just go ahead and merge it -- so if anyone
still wants to provide review, now's the time :)

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

[Thomas Huth]

On 24.04.2018 12:18, 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 <david@redhat.com>
> ---
> 
> 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(-)

I'm not an expert in all this s390x reset stuff, but as far as I can
see, this looks sane.

Acked-by: Thomas Huth <thuth@redhat.com>

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

[Cornelia Huck]

On Tue, 24 Apr 2018 12:18:59 +0200
David Hildenbrand <david@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 <david@redhat.com>
> ---
> 
> 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(-)

Thanks, applied.

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

[Christian Borntraeger]

On 04/24/2018 12:18 PM, 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.

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.

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 0d67349004..7b32698eaa 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -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);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index e893f72f3b..e9b11fd6cb 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -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;
 
diff --git a/vl.c b/vl.c
index b3426e03d0..18f379e833 100644
--- a/vl.c
+++ b/vl.c
@@ -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;
-- 
2.17.0

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

[David Hildenbrand]

On 21.06.2018 17:49, Christian Borntraeger wrote:
> 
> 
> On 04/24/2018 12:18 PM, 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.
> 
> 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/

> 
> 
> 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.
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0d67349004..7b32698eaa 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -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);
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index e893f72f3b..e9b11fd6cb 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -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.

>      SHUTDOWN_CAUSE__MAX,
>  } ShutdownCause;
>  
> diff --git a/vl.c b/vl.c
> index b3426e03d0..18f379e833 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -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;
> 


-- 

Thanks,

David / dhildenb

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

[Cornelia Huck]

On Thu, 21 Jun 2018 17:49:56 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 04/24/2018 12:18 PM, 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.  
> 
> 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.

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?

> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0d67349004..7b32698eaa 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -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);
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index e893f72f3b..e9b11fd6cb 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -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;
>  
> diff --git a/vl.c b/vl.c
> index b3426e03d0..18f379e833 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -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;

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

[Christian Borntraeger]

On 06/21/2018 06:04 PM, Cornelia Huck wrote:
> On Thu, 21 Jun 2018 17:49:56 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 04/24/2018 12:18 PM, 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.  
>>
>> 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.
> 
> 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?
> 
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 0d67349004..7b32698eaa 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -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);
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index e893f72f3b..e9b11fd6cb 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -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;
>>  
>> diff --git a/vl.c b/vl.c
>> index b3426e03d0..18f379e833 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -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;
> 

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

[David Hildenbrand]

On 21.06.2018 18:04, Cornelia Huck wrote:
> On Thu, 21 Jun 2018 17:49:56 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 04/24/2018 12:18 PM, 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.  
>>
>> 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.
> 
> 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 :)


-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 21.06.2018 18:06, Christian Borntraeger wrote:
> 
> 
> On 06/21/2018 06:04 PM, Cornelia Huck wrote:
>> On Thu, 21 Jun 2018 17:49:56 +0200
>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>
>>> On 04/24/2018 12:18 PM, 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.  
>>>
>>> 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.
>>
>> 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 :)

> 
> Shall I respin my patch as proper patch?

Yes, sure.

-- 

Thanks,

David / dhildenb

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

[Christian Borntraeger]

On 06/21/2018 06:07 PM, David Hildenbrand wrote:
> On 21.06.2018 18:04, Cornelia Huck wrote:
>> On Thu, 21 Jun 2018 17:49:56 +0200
>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>
>>> On 04/24/2018 12:18 PM, 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.  
>>>
>>> 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.
>>
>> 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.

> But definitely not for the BIOS :)
> 
> 

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

[David Hildenbrand]

On 21.06.2018 17:49, Christian Borntraeger wrote:
> 
> 
> On 04/24/2018 12:18 PM, 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.
> 
> 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.
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0d67349004..7b32698eaa 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -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);
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index e893f72f3b..e9b11fd6cb 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -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;
>  
> diff --git a/vl.c b/vl.c
> index b3426e03d0..18f379e833 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -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)

...

> +    } else if (no_reboot) {
>          shutdown_requested = reason;
>      } else {
>          reset_requested = reason;
> 


-- 

Thanks,

David / dhildenb

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

[Christian Borntraeger]

On 06/21/2018 06:15 PM, David Hildenbrand wrote:
> On 21.06.2018 17:49, Christian Borntraeger wrote:
>>
>>
>> On 04/24/2018 12:18 PM, 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.
>>
>> 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.
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 0d67349004..7b32698eaa 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -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);
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index e893f72f3b..e9b11fd6cb 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -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;
>>  
>> diff --git a/vl.c b/vl.c
>> index b3426e03d0..18f379e833 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -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)

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?

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

[Christian Borntraeger]

On 06/21/2018 06:20 PM, Christian Borntraeger wrote:
> 
> 
> On 06/21/2018 06:15 PM, David Hildenbrand wrote:
>> On 21.06.2018 17:49, Christian Borntraeger wrote:
>>>
>>>
>>> On 04/24/2018 12:18 PM, 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.
>>>
>>> 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.
>>>
>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>> index 0d67349004..7b32698eaa 100644
>>> --- a/hw/s390x/ipl.c
>>> +++ b/hw/s390x/ipl.c
>>> @@ -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);
>>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>>> index e893f72f3b..e9b11fd6cb 100644
>>> --- a/include/sysemu/sysemu.h
>>> +++ b/include/sysemu/sysemu.h
>>> @@ -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;
>>>  
>>> diff --git a/vl.c b/vl.c
>>> index b3426e03d0..18f379e833 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -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)
> 
> 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?

Now I see you point. You say nobody uses the value?

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

[David Hildenbrand]

On 21.06.2018 18:21, Christian Borntraeger wrote:
> 
> 
> On 06/21/2018 06:20 PM, Christian Borntraeger wrote:
>>
>>
>> On 06/21/2018 06:15 PM, David Hildenbrand wrote:
>>> On 21.06.2018 17:49, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 04/24/2018 12:18 PM, 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.
>>>>
>>>> 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.
>>>>
>>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>>> index 0d67349004..7b32698eaa 100644
>>>> --- a/hw/s390x/ipl.c
>>>> +++ b/hw/s390x/ipl.c
>>>> @@ -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);
>>>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>>>> index e893f72f3b..e9b11fd6cb 100644
>>>> --- a/include/sysemu/sysemu.h
>>>> +++ b/include/sysemu/sysemu.h
>>>> @@ -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;
>>>>  
>>>> diff --git a/vl.c b/vl.c
>>>> index b3426e03d0..18f379e833 100644
>>>> --- a/vl.c
>>>> +++ b/vl.c
>>>> @@ -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)
>>
>> 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?
> 
> 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.

-- 

Thanks,

David / dhildenb

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

[Paolo Bonzini]

On 21/06/2018 18:08, David Hildenbrand wrote:
>> 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