| Message ID | 20240813165250.2717650-3-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390: Convert virtio-ccw, cpu to three-phase reset, and followup cleanup | expand |
On 8/14/24 02:52, Peter Maydell wrote: > static void sigp_cpu_reset(CPUState *cs, run_on_cpu_data arg) > { > - S390CPU *cpu = S390_CPU(cs); > - S390CPUClass *scc = S390_CPU_GET_CLASS(cpu); > SigpInfo *si = arg.host_ptr; > > cpu_synchronize_state(cs); > - scc->reset(cs, S390_CPU_RESET_NORMAL); > + resettable_reset(OBJECT(cs), RESET_TYPE_COLD); NORMAL, not COLD here? Otherwise, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 14/08/2024 01.03, Richard Henderson wrote: > On 8/14/24 02:52, Peter Maydell wrote: >> static void sigp_cpu_reset(CPUState *cs, run_on_cpu_data arg) >> { >> - S390CPU *cpu = S390_CPU(cs); >> - S390CPUClass *scc = S390_CPU_GET_CLASS(cpu); >> SigpInfo *si = arg.host_ptr; >> cpu_synchronize_state(cs); >> - scc->reset(cs, S390_CPU_RESET_NORMAL); >> + resettable_reset(OBJECT(cs), RESET_TYPE_COLD); > > NORMAL, not COLD here? > > Otherwise, > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> With that fixed: Acked-by: Thomas Huth <thuth@redhat.com>
On Tue, 2024-08-13 at 17:52 +0100, Peter Maydell wrote: > Convert the s390 CPU to the Resettable interface. This is slightly > more involved than the other CPU types were (see commits > 9130cade5fc22..d66e64dd006df) because S390 has its own set of > different kinds of reset with different behaviours that it needs to > trigger. > > We handle this by adding these reset types to the Resettable > ResetType enum. Now instead of having an underlying implementation > of reset that is s390-specific and which might be called either > directly or via the DeviceClass::reset method, we can implement only > the Resettable hold phase method, and have the places that need to > trigger an s390-specific reset type do so by calling > resettable_reset(). > > The other option would have been to smuggle in the s390 reset > type via, for instance, a field in the CPU state that we set > in s390_do_cpu_initial_reset() etc and then examined in the > reset method, but doing it this way seems cleaner. > > The motivation for this change is that this is the last caller > of the legacy device_class_set_parent_reset() function, and > removing that will let us clean up some glue code that we added > for the transition to three-phase reset. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > Tested with 'make check' and 'make check-avocado' only. The > descriptions of the reset types are borrowed from the commit > message of f5ae2a4fd8d573cfeba; please check them as I haven't > got a clue what s390 does ;-) > --- With the already mentioned fix: Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c > index 0fbfcd35d83..4e41a3dff59 100644 > --- a/target/s390x/cpu.c > +++ b/target/s390x/cpu.c > [...] > -/* S390CPUClass::reset() */ > -static void s390_cpu_reset(CPUState *s, cpu_reset_type type) > +/* S390CPUClass Resettable reset_hold phase method */ > +static void s390_cpu_reset_hold(Object *obj, ResetType type) > { [...] > > switch (type) { > - case S390_CPU_RESET_CLEAR: > + default: > + /* RESET_TYPE_COLD: power on or "clear" reset */ I'd prefer case RESET_TYPE_COLD: case RESET_TYPE_SNAPSHOT_LOAD: and keeping the default unreachable assert. > memset(env, 0, offsetof(CPUS390XState, start_initial_reset_fields)); > /* fall through */ > - case S390_CPU_RESET_INITIAL: > + case RESET_TYPE_S390_CPU_INITIAL: > /* initial reset does not clear everything! */ > memset(&env->start_initial_reset_fields, 0, > offsetof(CPUS390XState, start_normal_reset_fields) - > @@ -203,7 +206,7 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type) > set_float_detect_tininess(float_tininess_before_rounding, > &env->fpu_status); > /* fall through */ > - case S390_CPU_RESET_NORMAL: > + case RESET_TYPE_S390_CPU_NORMAL: > env->psw.mask &= ~PSW_MASK_RI; > memset(&env->start_normal_reset_fields, 0, > offsetof(CPUS390XState, end_reset_fields) - > @@ -212,20 +215,18 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type) > env->pfault_token = -1UL; > env->bpbc = false; > break; > - default: > - g_assert_not_reached(); > } > > /* Reset state inside the kernel that we cannot access yet from QEMU. */ > if (kvm_enabled()) { > switch (type) { > - case S390_CPU_RESET_CLEAR: > + default: And the same here. > kvm_s390_reset_vcpu_clear(cpu); > break; > - case S390_CPU_RESET_INITIAL: > + case RESET_TYPE_S390_CPU_INITIAL: > kvm_s390_reset_vcpu_initial(cpu); > break; > - case S390_CPU_RESET_NORMAL: > + case RESET_TYPE_S390_CPU_NORMAL: > kvm_s390_reset_vcpu_normal(cpu); > break; > } > @@ -315,12 +316,6 @@ static Property s390x_cpu_properties[] = { > DEFINE_PROP_END_OF_LIST() > }; [...]
On Fri, 23 Aug 2024 at 18:45, Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote: > > On Tue, 2024-08-13 at 17:52 +0100, Peter Maydell wrote: > > Convert the s390 CPU to the Resettable interface. This is slightly > > more involved than the other CPU types were (see commits > > 9130cade5fc22..d66e64dd006df) because S390 has its own set of > > different kinds of reset with different behaviours that it needs to > > trigger. > > > > We handle this by adding these reset types to the Resettable > > ResetType enum. Now instead of having an underlying implementation > > of reset that is s390-specific and which might be called either > > directly or via the DeviceClass::reset method, we can implement only > > the Resettable hold phase method, and have the places that need to > > trigger an s390-specific reset type do so by calling > > resettable_reset(). > > > > The other option would have been to smuggle in the s390 reset > > type via, for instance, a field in the CPU state that we set > > in s390_do_cpu_initial_reset() etc and then examined in the > > reset method, but doing it this way seems cleaner. > > > > The motivation for this change is that this is the last caller > > of the legacy device_class_set_parent_reset() function, and > > removing that will let us clean up some glue code that we added > > for the transition to three-phase reset. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > Tested with 'make check' and 'make check-avocado' only. The > > descriptions of the reset types are borrowed from the commit > > message of f5ae2a4fd8d573cfeba; please check them as I haven't > > got a clue what s390 does ;-) > > --- > > With the already mentioned fix: > Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> Thanks for the review. > > switch (type) { > > - case S390_CPU_RESET_CLEAR: > > + default: > > + /* RESET_TYPE_COLD: power on or "clear" reset */ > > I'd prefer > case RESET_TYPE_COLD: > case RESET_TYPE_SNAPSHOT_LOAD: > > and keeping the default unreachable assert. The reset API (docs/devel/reset.rst) says: # Devices which implement reset methods must treat any unknown ``ResetType`` # as equivalent to ``RESET_TYPE_COLD``; this will reduce the amount of # existing code we need to change if we add more types in future. So making an unknown reset type behave like "cold" reset is deliberate. Otherwise every time we added a new reset type to the system we'd need to go through every device that had a switch on the reset type to add a new case for it. thanks -- PMM
diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst index 9746a4e8a0b..ed41e09f16d 100644 --- a/docs/devel/reset.rst +++ b/docs/devel/reset.rst @@ -44,6 +44,16 @@ The Resettable interface handles reset types with an enum ``ResetType``: value on each cold reset, such as RNG seed information, and which they must not reinitialize on a snapshot-load reset. +``RESET_TYPE_S390_CPU_NORMAL`` + This is only used for S390 CPU objects; it clears interrupts, stops + processing, and clears the TLB, but does not touch register contents. + +``RESET_TYPE_S390_CPU_INITIAL`` + This is only used for S390 CPU objects; it does everything + ``RESET_TYPE_S390_CPU_NORMAL`` does and also clears the PSW, prefix, + FPC, timer and control registers. It does not touch gprs, fprs or acrs. + + Devices which implement reset methods must treat any unknown ``ResetType`` as equivalent to ``RESET_TYPE_COLD``; this will reduce the amount of existing code we need to change if we add more types in future. diff --git a/include/hw/resettable.h b/include/hw/resettable.h index 7e249deb8b5..83b561fc830 100644 --- a/include/hw/resettable.h +++ b/include/hw/resettable.h @@ -36,6 +36,8 @@ typedef struct ResettableState ResettableState; typedef enum ResetType { RESET_TYPE_COLD, RESET_TYPE_SNAPSHOT_LOAD, + RESET_TYPE_S390_CPU_INITIAL, + RESET_TYPE_S390_CPU_NORMAL, } ResetType; /* diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index d6b75ad0e08..6a644724038 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -177,19 +177,11 @@ struct ArchCPU { uint32_t irqstate_saved_size; }; -typedef enum cpu_reset_type { - S390_CPU_RESET_NORMAL, - S390_CPU_RESET_INITIAL, - S390_CPU_RESET_CLEAR, -} cpu_reset_type; - /** * S390CPUClass: * @parent_realize: The parent class' realize handler. - * @parent_reset: The parent class' reset handler. + * @parent_phases: The parent class' reset phase handlers. * @load_normal: Performs a load normal. - * @cpu_reset: Performs a CPU reset. - * @initial_cpu_reset: Performs an initial CPU reset. * * An S/390 CPU model. */ @@ -203,9 +195,8 @@ struct S390CPUClass { const char *desc; DeviceRealize parent_realize; - DeviceReset parent_reset; + ResettablePhases parent_phases; void (*load_normal)(CPUState *cpu); - void (*reset)(CPUState *cpu, cpu_reset_type type); }; #ifndef CONFIG_USER_ONLY @@ -872,16 +863,12 @@ static inline void s390_do_cpu_full_reset(CPUState *cs, run_on_cpu_data arg) static inline void s390_do_cpu_reset(CPUState *cs, run_on_cpu_data arg) { - S390CPUClass *scc = S390_CPU_GET_CLASS(cs); - - scc->reset(cs, S390_CPU_RESET_NORMAL); + resettable_reset(OBJECT(cs), RESET_TYPE_S390_CPU_NORMAL); } static inline void s390_do_cpu_initial_reset(CPUState *cs, run_on_cpu_data arg) { - S390CPUClass *scc = S390_CPU_GET_CLASS(cs); - - scc->reset(cs, S390_CPU_RESET_INITIAL); + resettable_reset(OBJECT(cs), RESET_TYPE_S390_CPU_INITIAL); } static inline void s390_do_cpu_load_normal(CPUState *cs, run_on_cpu_data arg) diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c index 0fbfcd35d83..4e41a3dff59 100644 --- a/target/s390x/cpu.c +++ b/target/s390x/cpu.c @@ -32,6 +32,7 @@ #include "sysemu/hw_accel.h" #include "hw/qdev-properties.h" #include "hw/qdev-properties-system.h" +#include "hw/resettable.h" #include "fpu/softfloat-helpers.h" #include "disas/capstone.h" #include "sysemu/tcg.h" @@ -162,23 +163,25 @@ static void s390_query_cpu_fast(CPUState *cpu, CpuInfoFast *value) #endif } -/* S390CPUClass::reset() */ -static void s390_cpu_reset(CPUState *s, cpu_reset_type type) +/* S390CPUClass Resettable reset_hold phase method */ +static void s390_cpu_reset_hold(Object *obj, ResetType type) { - S390CPU *cpu = S390_CPU(s); + S390CPU *cpu = S390_CPU(obj); S390CPUClass *scc = S390_CPU_GET_CLASS(cpu); CPUS390XState *env = &cpu->env; - DeviceState *dev = DEVICE(s); - scc->parent_reset(dev); + if (scc->parent_phases.hold) { + scc->parent_phases.hold(obj, type); + } cpu->env.sigp_order = 0; s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu); switch (type) { - case S390_CPU_RESET_CLEAR: + default: + /* RESET_TYPE_COLD: power on or "clear" reset */ memset(env, 0, offsetof(CPUS390XState, start_initial_reset_fields)); /* fall through */ - case S390_CPU_RESET_INITIAL: + case RESET_TYPE_S390_CPU_INITIAL: /* initial reset does not clear everything! */ memset(&env->start_initial_reset_fields, 0, offsetof(CPUS390XState, start_normal_reset_fields) - @@ -203,7 +206,7 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type) set_float_detect_tininess(float_tininess_before_rounding, &env->fpu_status); /* fall through */ - case S390_CPU_RESET_NORMAL: + case RESET_TYPE_S390_CPU_NORMAL: env->psw.mask &= ~PSW_MASK_RI; memset(&env->start_normal_reset_fields, 0, offsetof(CPUS390XState, end_reset_fields) - @@ -212,20 +215,18 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type) env->pfault_token = -1UL; env->bpbc = false; break; - default: - g_assert_not_reached(); } /* Reset state inside the kernel that we cannot access yet from QEMU. */ if (kvm_enabled()) { switch (type) { - case S390_CPU_RESET_CLEAR: + default: kvm_s390_reset_vcpu_clear(cpu); break; - case S390_CPU_RESET_INITIAL: + case RESET_TYPE_S390_CPU_INITIAL: kvm_s390_reset_vcpu_initial(cpu); break; - case S390_CPU_RESET_NORMAL: + case RESET_TYPE_S390_CPU_NORMAL: kvm_s390_reset_vcpu_normal(cpu); break; } @@ -315,12 +316,6 @@ static Property s390x_cpu_properties[] = { DEFINE_PROP_END_OF_LIST() }; -static void s390_cpu_reset_full(DeviceState *dev) -{ - CPUState *s = CPU(dev); - return s390_cpu_reset(s, S390_CPU_RESET_CLEAR); -} - #ifdef CONFIG_TCG #include "hw/core/tcg-cpu-ops.h" @@ -383,15 +378,16 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data) S390CPUClass *scc = S390_CPU_CLASS(oc); CPUClass *cc = CPU_CLASS(scc); DeviceClass *dc = DEVICE_CLASS(oc); + ResettableClass *rc = RESETTABLE_CLASS(oc); device_class_set_parent_realize(dc, s390_cpu_realizefn, &scc->parent_realize); device_class_set_props(dc, s390x_cpu_properties); dc->user_creatable = true; - device_class_set_parent_reset(dc, s390_cpu_reset_full, &scc->parent_reset); + resettable_class_set_parent_phases(rc, NULL, s390_cpu_reset_hold, NULL, + &scc->parent_phases); - scc->reset = s390_cpu_reset; cc->class_by_name = s390_cpu_class_by_name, cc->has_work = s390_cpu_has_work; cc->mmu_index = s390x_cpu_mmu_index; diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c index ad0ad61177d..1c99ec88009 100644 --- a/target/s390x/sigp.c +++ b/target/s390x/sigp.c @@ -251,24 +251,20 @@ static void sigp_restart(CPUState *cs, run_on_cpu_data arg) static void sigp_initial_cpu_reset(CPUState *cs, run_on_cpu_data arg) { - S390CPU *cpu = S390_CPU(cs); - S390CPUClass *scc = S390_CPU_GET_CLASS(cpu); SigpInfo *si = arg.host_ptr; cpu_synchronize_state(cs); - scc->reset(cs, S390_CPU_RESET_INITIAL); + resettable_reset(OBJECT(cs), RESET_TYPE_S390_CPU_INITIAL); cpu_synchronize_post_reset(cs); si->cc = SIGP_CC_ORDER_CODE_ACCEPTED; } static void sigp_cpu_reset(CPUState *cs, run_on_cpu_data arg) { - S390CPU *cpu = S390_CPU(cs); - S390CPUClass *scc = S390_CPU_GET_CLASS(cpu); SigpInfo *si = arg.host_ptr; cpu_synchronize_state(cs); - scc->reset(cs, S390_CPU_RESET_NORMAL); + resettable_reset(OBJECT(cs), RESET_TYPE_COLD); cpu_synchronize_post_reset(cs); si->cc = SIGP_CC_ORDER_CODE_ACCEPTED; }
Convert the s390 CPU to the Resettable interface. This is slightly more involved than the other CPU types were (see commits 9130cade5fc22..d66e64dd006df) because S390 has its own set of different kinds of reset with different behaviours that it needs to trigger. We handle this by adding these reset types to the Resettable ResetType enum. Now instead of having an underlying implementation of reset that is s390-specific and which might be called either directly or via the DeviceClass::reset method, we can implement only the Resettable hold phase method, and have the places that need to trigger an s390-specific reset type do so by calling resettable_reset(). The other option would have been to smuggle in the s390 reset type via, for instance, a field in the CPU state that we set in s390_do_cpu_initial_reset() etc and then examined in the reset method, but doing it this way seems cleaner. The motivation for this change is that this is the last caller of the legacy device_class_set_parent_reset() function, and removing that will let us clean up some glue code that we added for the transition to three-phase reset. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- Tested with 'make check' and 'make check-avocado' only. The descriptions of the reset types are borrowed from the commit message of f5ae2a4fd8d573cfeba; please check them as I haven't got a clue what s390 does ;-) --- docs/devel/reset.rst | 10 ++++++++++ include/hw/resettable.h | 2 ++ target/s390x/cpu.h | 21 ++++----------------- target/s390x/cpu.c | 38 +++++++++++++++++--------------------- target/s390x/sigp.c | 8 ++------ 5 files changed, 35 insertions(+), 44 deletions(-)