On Thu, 8 Jan 2026 at 14:34, 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/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 <[email protected]>
> Cc: Peter Maydell <[email protected]>
> ---
> 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
