On Thu, 19 Feb 2026 at 17:18, Alex Bennée <[email protected]> wrote: > > Currently the cpu_reset() in mips_cpu_realizefn() hides an implicit > sequencing requirement when setting gcr_base. Without it we barf > because we end up setting the region between 0x0-0x000000001fbfffff > which trips over a qtest that accesses the GCR during "memsave 0 4096 > /dev/null". > > By moving to the reset phase we have to drop the property lest we are > admonished for "Attempting to set...after it was realized" but there > doesn't seem to be a need to expose the property anyway. > > NB: it would be safer if I could guarantee the place in the reset tree > but I haven't quite grokked how to do that yet. Currently I see this > sequence when testing: > > env MALLOC_PERTURB_=43 > G_TEST_DBUS_DAEMON=/home/alex/lsrc/qemu.git/tests/dbus-vmstate-daemon.sh > UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 > QTEST_QEMU_IMG=./qemu-img QTEST_QEMU_BINARY=./qemu-system-mips64el > SPEED=thorough MESON_TEST_ITERATION=1 > MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 > PYTHON=/home/alex/lsrc/qemu.git/builds/all/pyvenv/bin/python3 > QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon > ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 G_TEST_SLOW=1 > RUST_BACKTRACE=1 /home/alex/lsrc/qemu.git/builds/all/tests/qtest/test-hmp > --tap -p /mips64el/hmp/boston > TAP version 14 > # random seed: R02S0d3b1a4f1aef5198107851bdee539e7d > # Start of mips64el tests > # Start of hmp tests > # starting QEMU: exec ./qemu-system-mips64el -qtest > unix:/tmp/qtest-530181.sock -qtest-log /dev/null -chardev > socket,path=/tmp/qtest-530181.qmp,id=char0 -mon chardev=char0,mode=control > -display none -audio none -run-with exit-with-parent=on -S -M boston -accel > qtest > main_cpu_reset: dbg > mips_gcr_reset: dbg > mps_reset_exit: dbg
This works because we call reset hooks in the order they're registered, and clearly the qemu_register_reset() in mips_cps_realize() gets in first. To actually get the three-phase ordering so that the 'exit' phase hook you add in this patch definitely runs after the 'hold' phase of the CPU, you would need to stop using cpu_reset() and instead use use qemu_register_resettable(OBJ(cpu));. > ok 1 /mips64el/hmp/boston > # End of hmp tests > # End of mips64el tests > 1..1 > > Cc: Peter Maydell <[email protected]> > Message-ID: <[email protected]> > Signed-off-by: Alex Bennée <[email protected]> > > --- > v2 > - use proper 3-phase reset > --- > include/hw/mips/cps.h | 14 +++++++++++++- > hw/mips/cps.c | 26 +++++++++++++++++--------- > hw/misc/mips_cmgcr.c | 1 - > 3 files changed, 30 insertions(+), 11 deletions(-) > > diff --git a/include/hw/mips/cps.h b/include/hw/mips/cps.h > index 878b4d819f4..1084a10de0f 100644 > --- a/include/hw/mips/cps.h > +++ b/include/hw/mips/cps.h > @@ -30,7 +30,7 @@ > #include "qom/object.h" > > #define TYPE_MIPS_CPS "mips-cps" > -OBJECT_DECLARE_SIMPLE_TYPE(MIPSCPSState, MIPS_CPS) > +OBJECT_DECLARE_TYPE(MIPSCPSState, MIPSCPSClass, MIPS_CPS) > > struct MIPSCPSState { > SysBusDevice parent_obj; > @@ -48,6 +48,18 @@ struct MIPSCPSState { > Clock *clock; > }; > > +/* > + * MIPSCPSClass: > + * @parent_phases: The parent class' reset phase handlers. > + * > + * A Coherent Processing System model. > + */ > +struct MIPSCPSClass { > + SysBusDeviceClass parent_class; > + > + ResettablePhases parent_phases; > +}; > + > qemu_irq get_cps_irq(MIPSCPSState *cps, int pin_number); > > #endif > diff --git a/hw/mips/cps.c b/hw/mips/cps.c > index 620ee972f8f..23918147276 100644 > --- a/hw/mips/cps.c > +++ b/hw/mips/cps.c > @@ -55,6 +55,17 @@ static void main_cpu_reset(void *opaque) > cpu_reset(cs); > } > > +static void mps_reset_exit(Object *obj, ResetType type) > +{ > + MIPSCPSState *s = MIPS_CPS(obj); > + hwaddr gcr_base; > + > + /* Global Configuration Registers - only valid once the CPU has been > reset */ > + gcr_base = MIPS_CPU(first_cpu)->env.CP0_CMGCRBase << 4; > + memory_region_add_subregion(&s->container, gcr_base, > + sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gcr), > 0)); It still seems odd to me that we are messing with the GCR device in this CPS reset method. The TYPE_MIPS_GCR device (in hw/misc/mips_cmgcr.c) is the one that owns these memory regions and it has code that adjusts their addresses at runtime based on guest register writes. It would seem more logical to have the CPs code just set things up (e.g. map them at address 0) in realize, and then have the GCR device's reset (exit phase) set the address/enabled of the MRs according to their required state at reset. thanks -- PMM
