[PATCH] target/hppa: Add CPU reset method

[PATCH] target/hppa: Add CPU reset method

[Helge Deller]

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));
+
+    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"

Re: [PATCH] target/hppa: Add CPU reset method

[Peter Maydell]

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

Re: [PATCH] target/hppa: Add CPU reset method

[Helge Deller]

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