| Message ID | Zxvinao2jcZgyAVG@p100 (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | target/hppa: Add CPU reset method | expand |
On Fri, 25 Oct 2024 at 19:25, Helge Deller <deller@kernel.org> wrote: > > Add the missing CPU reset method, which resets all CPU registers and the > TLB to zero. Then the CPU will switch to 32-bit mode (PSW_W bit is not > set) and start execution at address 0xf0000004. > > Signed-off-by: Helge Deller <deller@gmx.de> > > diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c > index c38439c180..0cc696ccd3 100644 > --- a/target/hppa/cpu.c > +++ b/target/hppa/cpu.c > @@ -235,15 +235,39 @@ static const TCGCPUOps hppa_tcg_ops = { > #endif /* !CONFIG_USER_ONLY */ > }; > > +static void hppa_cpu_reset_hold(Object *obj, ResetType type) > +{ > + HPPACPU *cpu = HPPA_CPU(obj); > + HPPACPUClass *scc = HPPA_CPU_GET_CLASS(cpu); > + CPUHPPAState *env = &cpu->env; > + CPUState *cs = CPU(cpu); > + > + if (scc->parent_phases.hold) { > + scc->parent_phases.hold(obj, type); > + } > + > + memset(env, 0, sizeof(*env)); I would recommend doing what the other CPU classes do and having an end_reset_fields marker in your state struct to mark the last point which is zeroed out /* Fields up to this point are cleared by a CPU reset */ struct {} end_reset_fields; which you then do with: memset(env, 0, offsetof(CPUHPPAState, end_reset_fields)); In particular, I'm pretty sure you don't want to zero out pointer fields like tlb_partial. (That kind of data-structure piece of the cpu state struct either needs by-hand code to reset it to power-on state, or in some cases may be OK to simply leave alone across reset, depending on what it is.) > + > + cpu_set_pc(cs, 0xf0000004); > + env->psw = PSW_Q; > + > + cs->exception_index = -1; > + cs->halted = 0; hppa_cpu_initfn() currently does these: cs->exception_index = -1; cpu_hppa_loaded_fr0(env); cpu_hppa_put_psw(env, PSW_W); They should probably be moved to reset (or deleted, for the cases where the reset code above already does that work). PS: the PSW reset value looks like a behaviour change. If that's intentional you probably want to do it in a separate patch. > +} > + > static void hppa_cpu_class_init(ObjectClass *oc, void *data) > { > DeviceClass *dc = DEVICE_CLASS(oc); > CPUClass *cc = CPU_CLASS(oc); > HPPACPUClass *acc = HPPA_CPU_CLASS(oc); > + ResettableClass *rc = RESETTABLE_CLASS(oc); > > device_class_set_parent_realize(dc, hppa_cpu_realizefn, > &acc->parent_realize); > > + resettable_class_set_parent_phases(rc, NULL, hppa_cpu_reset_hold, NULL, > + &acc->parent_phases); > + > cc->class_by_name = hppa_cpu_class_by_name; > cc->has_work = hppa_cpu_has_work; > cc->mmu_index = hppa_cpu_mmu_index; The machinery for registering the reset handler function all looks good. thanks -- PMM
On 10/29/24 13:44, Peter Maydell wrote: > On Fri, 25 Oct 2024 at 19:25, Helge Deller <deller@kernel.org> wrote: >> >> Add the missing CPU reset method, which resets all CPU registers and the >> TLB to zero. Then the CPU will switch to 32-bit mode (PSW_W bit is not >> set) and start execution at address 0xf0000004. >> >> Signed-off-by: Helge Deller <deller@gmx.de> >> >> diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c >> index c38439c180..0cc696ccd3 100644 >> --- a/target/hppa/cpu.c >> +++ b/target/hppa/cpu.c >> @@ -235,15 +235,39 @@ static const TCGCPUOps hppa_tcg_ops = { >> #endif /* !CONFIG_USER_ONLY */ >> }; >> >> +static void hppa_cpu_reset_hold(Object *obj, ResetType type) >> +{ >> + HPPACPU *cpu = HPPA_CPU(obj); >> + HPPACPUClass *scc = HPPA_CPU_GET_CLASS(cpu); >> + CPUHPPAState *env = &cpu->env; >> + CPUState *cs = CPU(cpu); >> + >> + if (scc->parent_phases.hold) { >> + scc->parent_phases.hold(obj, type); >> + } >> + >> + memset(env, 0, sizeof(*env)); > > I would recommend doing what the other CPU classes do > and having an end_reset_fields marker in your state > struct to mark the last point which is zeroed out > > /* Fields up to this point are cleared by a CPU reset */ > struct {} end_reset_fields; > > which you then do with: > memset(env, 0, offsetof(CPUHPPAState, end_reset_fields)); I did implemented it initially like this... > In particular, I'm pretty sure you don't want to zero out > pointer fields like tlb_partial. (That kind of data-structure > piece of the cpu state struct either needs by-hand code to > reset it to power-on state, or in some cases may be OK to > simply leave alone across reset, depending on what it is.) ... and I did check the various pointers that it's correct that they get zeroed out on reset. This is true for tlb_partial as well. So, zero out the whole struct is OK currently. >> + cpu_set_pc(cs, 0xf0000004); >> + env->psw = PSW_Q; >> + >> + cs->exception_index = -1; >> + cs->halted = 0; > > hppa_cpu_initfn() currently does these: > > cs->exception_index = -1; > cpu_hppa_loaded_fr0(env); > cpu_hppa_put_psw(env, PSW_W); > > They should probably be moved to reset (or deleted, for > the cases where the reset code above already does that work). Yes. > PS: the PSW reset value looks like a behaviour change. If that's > intentional you probably want to do it in a separate patch. That change actually doesn't really matter. On reset the SeaBIOS firmware will immediately be executed and sets the correct bit width. >> +} >> + >> static void hppa_cpu_class_init(ObjectClass *oc, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(oc); >> CPUClass *cc = CPU_CLASS(oc); >> HPPACPUClass *acc = HPPA_CPU_CLASS(oc); >> + ResettableClass *rc = RESETTABLE_CLASS(oc); >> >> device_class_set_parent_realize(dc, hppa_cpu_realizefn, >> &acc->parent_realize); >> >> + resettable_class_set_parent_phases(rc, NULL, hppa_cpu_reset_hold, NULL, >> + &acc->parent_phases); >> + >> cc->class_by_name = hppa_cpu_class_by_name; >> cc->has_work = hppa_cpu_has_work; >> cc->mmu_index = hppa_cpu_mmu_index; > > The machinery for registering the reset handler function > all looks good. Thanks for review! I'll send a new patch series. Helge
diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c index c38439c180..0cc696ccd3 100644 --- a/target/hppa/cpu.c +++ b/target/hppa/cpu.c @@ -235,15 +235,39 @@ static const TCGCPUOps hppa_tcg_ops = { #endif /* !CONFIG_USER_ONLY */ }; +static void hppa_cpu_reset_hold(Object *obj, ResetType type) +{ + HPPACPU *cpu = HPPA_CPU(obj); + HPPACPUClass *scc = HPPA_CPU_GET_CLASS(cpu); + CPUHPPAState *env = &cpu->env; + CPUState *cs = CPU(cpu); + + if (scc->parent_phases.hold) { + scc->parent_phases.hold(obj, type); + } + + memset(env, 0, sizeof(*env)); + + cpu_set_pc(cs, 0xf0000004); + env->psw = PSW_Q; + + cs->exception_index = -1; + cs->halted = 0; +} + static void hppa_cpu_class_init(ObjectClass *oc, void *data) { DeviceClass *dc = DEVICE_CLASS(oc); CPUClass *cc = CPU_CLASS(oc); HPPACPUClass *acc = HPPA_CPU_CLASS(oc); + ResettableClass *rc = RESETTABLE_CLASS(oc); device_class_set_parent_realize(dc, hppa_cpu_realizefn, &acc->parent_realize); + resettable_class_set_parent_phases(rc, NULL, hppa_cpu_reset_hold, NULL, + &acc->parent_phases); + cc->class_by_name = hppa_cpu_class_by_name; cc->has_work = hppa_cpu_has_work; cc->mmu_index = hppa_cpu_mmu_index; diff --git a/target/hppa/cpu.h b/target/hppa/cpu.h index e45ba50a59..44ee115139 100644 --- a/target/hppa/cpu.h +++ b/target/hppa/cpu.h @@ -281,6 +281,7 @@ struct ArchCPU { /** * HPPACPUClass: * @parent_realize: The parent class' realize handler. + * @parent_phases: The parent class' reset phase handlers. * * An HPPA CPU model. */ @@ -288,6 +289,7 @@ struct HPPACPUClass { CPUClass parent_class; DeviceRealize parent_realize; + ResettablePhases parent_phases; }; #include "exec/cpu-all.h"
Add the missing CPU reset method, which resets all CPU registers and the TLB to zero. Then the CPU will switch to 32-bit mode (PSW_W bit is not set) and start execution at address 0xf0000004. Signed-off-by: Helge Deller <deller@gmx.de>