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/debug/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/debug/tests/qtest/test-hmp --tap -p /mips64el/hmp/boston
  TAP version 14
  # random seed: R02S89554f0dc696ece515363e554b13b7f9
  # Start of mips64el tests
  # Start of hmp tests
  # starting QEMU: exec ./qemu-system-mips64el -qtest unix:/tmp/qtest-883372.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-883372.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -run-with exit-with-parent=on -S -M boston -accel qtest
  mips_cpu_reset_hold: dbg
  mips_gcr_init: 0x5600f2160050 - 0
  main_cpu_reset: dbg
  mips_cpu_reset_hold: dbg
  mps_reset: 000000001fbf8000
  mips_cpu_reset_hold: dbg
  ok 1 /mips64el/hmp/boston
  # End of hmp tests
  # End of mips64el tests
  1..1

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
---
 hw/mips/cps.c        | 22 +++++++++++++---------
 hw/misc/mips_cmgcr.c |  1 -
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 620ee972f8f..c91243599e0 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -55,6 +55,18 @@ static void main_cpu_reset(void *opaque)
     cpu_reset(cs);
 }
 
+static void mps_reset(void *opaque)
+{
+    DeviceState *dev = opaque;
+    MIPSCPSState *s = MIPS_CPS(dev);
+    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));
+}
+
 static bool cpu_mips_itu_supported(CPUMIPSState *env)
 {
     bool is_mt = (env->CP0_Config5 & (1 << CP0C5_VP)) || ase_mt_available(env);
@@ -65,7 +77,6 @@ static bool cpu_mips_itu_supported(CPUMIPSState *env)
 static void mips_cps_realize(DeviceState *dev, Error **errp)
 {
     MIPSCPSState *s = MIPS_CPS(dev);
-    target_ulong gcr_base;
     bool itu_present = false;
 
     if (!clock_get(s->clock)) {
@@ -144,16 +155,11 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(&s->container, 0,
                             sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gic), 0));
 
-    /* Global Configuration Registers */
-    gcr_base = MIPS_CPU(first_cpu)->env.CP0_CMGCRBase << 4;
-
     object_initialize_child(OBJECT(dev), "gcr", &s->gcr, TYPE_MIPS_GCR);
     object_property_set_uint(OBJECT(&s->gcr), "num-vp", s->num_vp,
                             &error_abort);
     object_property_set_int(OBJECT(&s->gcr), "gcr-rev", 0x800,
                             &error_abort);
-    object_property_set_int(OBJECT(&s->gcr), "gcr-base", gcr_base,
-                            &error_abort);
     object_property_set_link(OBJECT(&s->gcr), "gic", OBJECT(&s->gic.mr),
                              &error_abort);
     object_property_set_link(OBJECT(&s->gcr), "cpc", OBJECT(&s->cpc.mr),
@@ -161,9 +167,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->gcr), errp)) {
         return;
     }
-
-    memory_region_add_subregion(&s->container, gcr_base,
-                            sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gcr), 0));
+    qemu_register_reset(mps_reset, s);
 }
 
 static const Property mips_cps_properties[] = {
diff --git a/hw/misc/mips_cmgcr.c b/hw/misc/mips_cmgcr.c
index 3e262e828bc..9e1c8d26ea5 100644
--- a/hw/misc/mips_cmgcr.c
+++ b/hw/misc/mips_cmgcr.c
@@ -214,7 +214,6 @@ static const VMStateDescription vmstate_mips_gcr = {
 static const Property mips_gcr_properties[] = {
     DEFINE_PROP_UINT32("num-vp", MIPSGCRState, num_vps, 1),
     DEFINE_PROP_INT32("gcr-rev", MIPSGCRState, gcr_rev, 0x800),
-    DEFINE_PROP_UINT64("gcr-base", MIPSGCRState, gcr_base, GCR_BASE_ADDR),
     DEFINE_PROP_LINK("gic", MIPSGCRState, gic_mr, TYPE_MEMORY_REGION,
                      MemoryRegion *),
     DEFINE_PROP_LINK("cpc", MIPSGCRState, cpc_mr, TYPE_MEMORY_REGION,
-- 
2.47.3

On Thu, 8 Jan 2026 at 14:34, Alex Bennée <alex.bennee@linaro.org> 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/debug/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/debug/tests/qtest/test-hmp --tap -p /mips64el/hmp/boston
>   TAP version 14
>   # random seed: R02S89554f0dc696ece515363e554b13b7f9
>   # Start of mips64el tests
>   # Start of hmp tests
>   # starting QEMU: exec ./qemu-system-mips64el -qtest unix:/tmp/qtest-883372.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-883372.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -run-with exit-with-parent=on -S -M boston -accel qtest
>   mips_cpu_reset_hold: dbg
>   mips_gcr_init: 0x5600f2160050 - 0
>   main_cpu_reset: dbg
>   mips_cpu_reset_hold: dbg
>   mps_reset: 000000001fbf8000
>   mips_cpu_reset_hold: dbg
>   ok 1 /mips64el/hmp/boston
>   # End of hmp tests
>   # End of mips64el tests
>   1..1
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/mips/cps.c        | 22 +++++++++++++---------
>  hw/misc/mips_cmgcr.c |  1 -
>  2 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/hw/mips/cps.c b/hw/mips/cps.c
> index 620ee972f8f..c91243599e0 100644
> --- a/hw/mips/cps.c
> +++ b/hw/mips/cps.c
> @@ -55,6 +55,18 @@ static void main_cpu_reset(void *opaque)
>      cpu_reset(cs);
>  }
>
> +static void mps_reset(void *opaque)
> +{
> +    DeviceState *dev = opaque;
> +    MIPSCPSState *s = MIPS_CPS(dev);
> +    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));
> +}
> +
>  static bool cpu_mips_itu_supported(CPUMIPSState *env)
>  {
>      bool is_mt = (env->CP0_Config5 & (1 << CP0C5_VP)) || ase_mt_available(env);
> @@ -65,7 +77,6 @@ static bool cpu_mips_itu_supported(CPUMIPSState *env)
>  static void mips_cps_realize(DeviceState *dev, Error **errp)
>  {
>      MIPSCPSState *s = MIPS_CPS(dev);
> -    target_ulong gcr_base;
>      bool itu_present = false;
>
>      if (!clock_get(s->clock)) {
> @@ -144,16 +155,11 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>      memory_region_add_subregion(&s->container, 0,
>                              sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gic), 0));
>
> -    /* Global Configuration Registers */
> -    gcr_base = MIPS_CPU(first_cpu)->env.CP0_CMGCRBase << 4;
> -
>      object_initialize_child(OBJECT(dev), "gcr", &s->gcr, TYPE_MIPS_GCR);
>      object_property_set_uint(OBJECT(&s->gcr), "num-vp", s->num_vp,
>                              &error_abort);
>      object_property_set_int(OBJECT(&s->gcr), "gcr-rev", 0x800,
>                              &error_abort);
> -    object_property_set_int(OBJECT(&s->gcr), "gcr-base", gcr_base,
> -                            &error_abort);
>      object_property_set_link(OBJECT(&s->gcr), "gic", OBJECT(&s->gic.mr),
>                               &error_abort);
>      object_property_set_link(OBJECT(&s->gcr), "cpc", OBJECT(&s->cpc.mr),
> @@ -161,9 +167,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>      if (!sysbus_realize(SYS_BUS_DEVICE(&s->gcr), errp)) {
>          return;
>      }
> -
> -    memory_region_add_subregion(&s->container, gcr_base,
> -                            sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gcr), 0));
> +    qemu_register_reset(mps_reset, s);

Adding calls to qemu_register_reset() is adding more uses of
an API we'd ideally like to get rid of. It's particularly
non-ideal here where we're in an implementation of a sysbus
device, which has a perfectly good reset method we could
implement.

>  }
>
>  static const Property mips_cps_properties[] = {
> diff --git a/hw/misc/mips_cmgcr.c b/hw/misc/mips_cmgcr.c
> index 3e262e828bc..9e1c8d26ea5 100644
> --- a/hw/misc/mips_cmgcr.c
> +++ b/hw/misc/mips_cmgcr.c
> @@ -214,7 +214,6 @@ static const VMStateDescription vmstate_mips_gcr = {
>  static const Property mips_gcr_properties[] = {
>      DEFINE_PROP_UINT32("num-vp", MIPSGCRState, num_vps, 1),
>      DEFINE_PROP_INT32("gcr-rev", MIPSGCRState, gcr_rev, 0x800),
> -    DEFINE_PROP_UINT64("gcr-base", MIPSGCRState, gcr_base, GCR_BASE_ADDR),
>      DEFINE_PROP_LINK("gic", MIPSGCRState, gic_mr, TYPE_MEMORY_REGION,
>                       MemoryRegion *),
>      DEFINE_PROP_LINK("cpc", MIPSGCRState, cpc_mr, TYPE_MEMORY_REGION,

Something in these devices seems to be very weirdly modelled
and could probably do with being straightened out. Notably,
the GCR device itself has functionality for moving the
address of its memory region around when the guest writes
to the GCR_BASE register, so perhaps it should itself have
the job of making sure the MR is in the right place on reset?

If you have the GCR look at the CMGCRBase value of the CPU
and set the MR position in its reset exit phase method, then
you will probably find that sorts out your ordering issues,
because the CPU will set/reset its state in the 'hold' phase,
and then the GCR can look at it in the 'exit' phase.

thanks
-- PMM