[PATCH 0/3] target/mips: Move non-shallow-copy fields out of env to fix leaks

[PATCH 0/3] target/mips: Move non-shallow-copy fields out of env to fix leaks

[Peter Maydell]

At the moment the linux-user cpu_copy() implementation assumes that it
can clone a CPU object for a new guest thread by first creating and
resetting a fresh CPU object and then doing a shallow copy with
memcpy() of the CPU's env struct.  This is obviously a very dubious
assumption, but for the most part it works.

One place where it doesn't currently work is MIPS, where we have a few
pointer fields at the end of the CPU struct.  Currently we don't ever
try to free the objects we point to when the CPU is destroyed, so this
is a leak but not incorrect behaviour. However, we can't fix the leak
by freeing memory in CPU unrealize, because then we would get a
double-free because both old and new thread point to the same memory.

In the long term we should really reimplement cpu_copy by having CPU
objects have a method for doing this, rather than accumulating
architecture-specific hacks. But since in practice I only see leaks in
'make check-tcg' for MIPS, this series takes the simpler approach of
moving the fields of CPUMIPSState that cannot be shallow-copied and
that are used in user-only mode out to the MIPSCPU struct.  There are
only two to move: mvp_init and count_clock.

The remaining pointer fields in CPUMIPSState are:
 * several fields inside the !defined(CONFIG_USER_ONLY) block
 * cpu_model, which is always equal to MIPSCPUClass::cpu_def
   and so can be shallow-copied
 * timer, which is only set in system-emulation and is
   NULL in user-only mode

This patchset does the two moves of fields out of the env
struct, and adds an unrealize method where we can free the
mvp struct that we would otherwise leak.

thanks
-- PMM

Peter Maydell (3):
  target/mips: Move 'mvp' field from CPUMIPSState to MIPSCPU
  target/mips: Free mvp in unrealize
  target/mips: Move count_clock to MIPSCPU struct

 hw/mips/malta.c                     |  4 ++--
 target/mips/cpu-defs.c.inc          | 10 +++++----
 target/mips/cpu.c                   | 18 ++++++++++++---
 target/mips/cpu.h                   |  6 +++--
 target/mips/internal.h              |  3 ++-
 target/mips/system/cp0_timer.c      | 12 ++++++----
 target/mips/system/machine.c        |  2 +-
 target/mips/tcg/system/cp0_helper.c | 35 ++++++++++++++++++-----------
 target/mips/tcg/translate.c         |  6 +++--
 9 files changed, 64 insertions(+), 32 deletions(-)

-- 
2.43.0

[PATCH 1/3] target/mips: Move 'mvp' field from CPUMIPSState to MIPSCPU

[Peter Maydell]

The 'mvp' field in the CPUMIPSState is a pointer to memory allocated
in mvp_init().  This is in theory fine, but in practice it clashes
with the current linux-user implementation of cpu_copy(), which
assumes it can do a shallow memcpy() copy of the CPU env struct in
order to clone the CPU when creating a new thread.

Almost all of the MIPS env struct is actually memcpy() copyable;
one of the exceptions is the mvp pointer. We don't need this
to be in the env struct; move it to the CPU object struct instead.

At the moment the memcpy() of the env->mvp pointer doesn't have any
obvious ill-effects, because we never free the memory and it
doesn't contain anything that varies at runtime for user-mode.
So thread 2 ends up pointing at thread 1's mvp struct, but it
still works OK. However, we would like to free the mvp memory to
avoid a leak when a user-mode thread exits, and unless we avoid
the shallow copy this will end up with a double-free when both
thread 1 and thread 2 free the same mvp struct.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/mips/malta.c                     |  4 ++--
 target/mips/cpu-defs.c.inc          | 10 +++++----
 target/mips/cpu.c                   |  2 +-
 target/mips/cpu.h                   |  3 ++-
 target/mips/internal.h              |  3 ++-
 target/mips/system/machine.c        |  2 +-
 target/mips/tcg/system/cp0_helper.c | 35 ++++++++++++++++++-----------
 target/mips/tcg/translate.c         |  6 +++--
 8 files changed, 40 insertions(+), 25 deletions(-)

diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index 812ff64d83..dfd537f44a 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -968,10 +968,10 @@ static void malta_mips_config(MIPSCPU *cpu)
     CPUState *cs = CPU(cpu);
 
     if (ase_mt_available(env)) {
-        env->mvp->CP0_MVPConf0 = deposit32(env->mvp->CP0_MVPConf0,
+        cpu->mvp->CP0_MVPConf0 = deposit32(cpu->mvp->CP0_MVPConf0,
                                            CP0MVPC0_PTC, 8,
                                            smp_cpus * cs->nr_threads - 1);
-        env->mvp->CP0_MVPConf0 = deposit32(env->mvp->CP0_MVPConf0,
+        cpu->mvp->CP0_MVPConf0 = deposit32(cpu->mvp->CP0_MVPConf0,
                                            CP0MVPC0_PVPE, 4, smp_cpus - 1);
     }
 }
diff --git a/target/mips/cpu-defs.c.inc b/target/mips/cpu-defs.c.inc
index d93b9d341a..faefab0473 100644
--- a/target/mips/cpu-defs.c.inc
+++ b/target/mips/cpu-defs.c.inc
@@ -1034,7 +1034,9 @@ static void fpu_init (CPUMIPSState *env, const mips_def_t *def)
 
 static void mvp_init(CPUMIPSState *env)
 {
-    env->mvp = g_malloc0(sizeof(CPUMIPSMVPContext));
+    MIPSCPU *cpu = env_archcpu(env);
+
+    cpu->mvp = g_malloc0(sizeof(CPUMIPSMVPContext));
 
     if (!ase_mt_available(env)) {
         return;
@@ -1044,7 +1046,7 @@ static void mvp_init(CPUMIPSState *env)
        programmable cache partitioning implemented, number of allocatable
        and shareable TLB entries, MVP has allocatable TCs, 2 VPEs
        implemented, 5 TCs implemented. */
-    env->mvp->CP0_MVPConf0 = (1U << CP0MVPC0_M) | (1 << CP0MVPC0_TLBS) |
+    cpu->mvp->CP0_MVPConf0 = (1U << CP0MVPC0_M) | (1 << CP0MVPC0_TLBS) |
                              (0 << CP0MVPC0_GS) | (1 << CP0MVPC0_PCP) |
 // TODO: actually do 2 VPEs.
 //                             (1 << CP0MVPC0_TCA) | (0x1 << CP0MVPC0_PVPE) |
@@ -1053,12 +1055,12 @@ static void mvp_init(CPUMIPSState *env)
                              (0x00 << CP0MVPC0_PTC);
 #if !defined(CONFIG_USER_ONLY)
     /* Usermode has no TLB support */
-    env->mvp->CP0_MVPConf0 |= (env->tlb->nb_tlb << CP0MVPC0_PTLBE);
+    cpu->mvp->CP0_MVPConf0 |= (env->tlb->nb_tlb << CP0MVPC0_PTLBE);
 #endif
 
     /* Allocatable CP1 have media extensions, allocatable CP1 have FP support,
        no UDI implemented, no CP2 implemented, 1 CP1 implemented. */
-    env->mvp->CP0_MVPConf1 = (1U << CP0MVPC1_CIM) | (1 << CP0MVPC1_CIF) |
+    cpu->mvp->CP0_MVPConf1 = (1U << CP0MVPC1_CIM) | (1 << CP0MVPC1_CIF) |
                              (0x0 << CP0MVPC1_PCX) | (0x0 << CP0MVPC1_PCP2) |
                              (0x1 << CP0MVPC1_PCP1);
 }
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 5f88c077db..789ca188b5 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -339,7 +339,7 @@ static void mips_cpu_reset_hold(Object *obj, ResetType type)
 
         if (cs->cpu_index == 0) {
             /* VPE0 starts up enabled.  */
-            env->mvp->CP0_MVPControl |= (1 << CP0MVPCo_EVP);
+            cpu->mvp->CP0_MVPControl |= (1 << CP0MVPCo_EVP);
             env->CP0_VPEConf0 |= (1 << CP0VPEC0_MVP) | (1 << CP0VPEC0_VPA);
 
             /* TC0 starts up unhalted.  */
diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index ed662135cb..8de3178b6d 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -1174,7 +1174,6 @@ typedef struct CPUArchState {
     struct {} end_reset_fields;
 
     /* Fields from here on are preserved across CPU reset. */
-    CPUMIPSMVPContext *mvp;
 #if !defined(CONFIG_USER_ONLY)
     CPUMIPSTLBContext *tlb;
     qemu_irq irq[8];
@@ -1209,6 +1208,8 @@ struct ArchCPU {
     Clock *clock;
     Clock *count_div; /* Divider for CP0_Count clock */
 
+    CPUMIPSMVPContext *mvp;
+
     /* Properties */
     bool is_big_endian;
 };
diff --git a/target/mips/internal.h b/target/mips/internal.h
index 28eb28936b..95b8b7bb9c 100644
--- a/target/mips/internal.h
+++ b/target/mips/internal.h
@@ -246,10 +246,11 @@ static inline void restore_pamask(CPUMIPSState *env)
 
 static inline int mips_vpe_active(CPUMIPSState *env)
 {
+    MIPSCPU *cpu = env_archcpu(env);
     int active = 1;
 
     /* Check that the VPE is enabled.  */
-    if (!(env->mvp->CP0_MVPControl & (1 << CP0MVPCo_EVP))) {
+    if (!(cpu->mvp->CP0_MVPControl & (1 << CP0MVPCo_EVP))) {
         active = 0;
     }
     /* Check that the VPE is activated.  */
diff --git a/target/mips/system/machine.c b/target/mips/system/machine.c
index 8af11fd896..67f6f414d9 100644
--- a/target/mips/system/machine.c
+++ b/target/mips/system/machine.c
@@ -233,7 +233,7 @@ const VMStateDescription vmstate_mips_cpu = {
                        CPUMIPSFPUContext),
 
         /* MVP */
-        VMSTATE_STRUCT_POINTER(env.mvp, MIPSCPU, vmstate_mvp,
+        VMSTATE_STRUCT_POINTER(mvp, MIPSCPU, vmstate_mvp,
                                CPUMIPSMVPContext),
 
         /* TLB */
diff --git a/target/mips/tcg/system/cp0_helper.c b/target/mips/tcg/system/cp0_helper.c
index b69e70d7fc..123d5c217c 100644
--- a/target/mips/tcg/system/cp0_helper.c
+++ b/target/mips/tcg/system/cp0_helper.c
@@ -229,17 +229,20 @@ uint32_t cpu_mips_get_random(CPUMIPSState *env)
 /* CP0 helpers */
 target_ulong helper_mfc0_mvpcontrol(CPUMIPSState *env)
 {
-    return env->mvp->CP0_MVPControl;
+    MIPSCPU *cpu = env_archcpu(env);
+    return cpu->mvp->CP0_MVPControl;
 }
 
 target_ulong helper_mfc0_mvpconf0(CPUMIPSState *env)
 {
-    return env->mvp->CP0_MVPConf0;
+    MIPSCPU *cpu = env_archcpu(env);
+    return cpu->mvp->CP0_MVPConf0;
 }
 
 target_ulong helper_mfc0_mvpconf1(CPUMIPSState *env)
 {
-    return env->mvp->CP0_MVPConf1;
+    MIPSCPU *cpu = env_archcpu(env);
+    return cpu->mvp->CP0_MVPConf1;
 }
 
 target_ulong helper_mfc0_random(CPUMIPSState *env)
@@ -514,6 +517,7 @@ void helper_mtc0_index(CPUMIPSState *env, target_ulong arg1)
 
 void helper_mtc0_mvpcontrol(CPUMIPSState *env, target_ulong arg1)
 {
+    MIPSCPU *cpu = env_archcpu(env);
     uint32_t mask = 0;
     uint32_t newval;
 
@@ -521,14 +525,14 @@ void helper_mtc0_mvpcontrol(CPUMIPSState *env, target_ulong arg1)
         mask |= (1 << CP0MVPCo_CPA) | (1 << CP0MVPCo_VPC) |
                 (1 << CP0MVPCo_EVP);
     }
-    if (env->mvp->CP0_MVPControl & (1 << CP0MVPCo_VPC)) {
+    if (cpu->mvp->CP0_MVPControl & (1 << CP0MVPCo_VPC)) {
         mask |= (1 << CP0MVPCo_STLB);
     }
-    newval = (env->mvp->CP0_MVPControl & ~mask) | (arg1 & mask);
+    newval = (cpu->mvp->CP0_MVPControl & ~mask) | (arg1 & mask);
 
     /* TODO: Enable/disable shared TLB, enable/disable VPEs. */
 
-    env->mvp->CP0_MVPControl = newval;
+    cpu->mvp->CP0_MVPControl = newval;
 }
 
 void helper_mtc0_vpecontrol(CPUMIPSState *env, target_ulong arg1)
@@ -616,10 +620,11 @@ void helper_mttc0_vpeconf0(CPUMIPSState *env, target_ulong arg1)
 
 void helper_mtc0_vpeconf1(CPUMIPSState *env, target_ulong arg1)
 {
+    MIPSCPU *cpu = env_archcpu(env);
     uint32_t mask = 0;
     uint32_t newval;
 
-    if (env->mvp->CP0_MVPControl & (1 << CP0MVPCo_VPC))
+    if (cpu->mvp->CP0_MVPControl & (1 << CP0MVPCo_VPC))
         mask |= (0xff << CP0VPEC1_NCX) | (0xff << CP0VPEC1_NCP2) |
                 (0xff << CP0VPEC1_NCP1);
     newval = (env->CP0_VPEConf1 & ~mask) | (arg1 & mask);
@@ -689,10 +694,11 @@ void helper_mttc0_tcstatus(CPUMIPSState *env, target_ulong arg1)
 
 void helper_mtc0_tcbind(CPUMIPSState *env, target_ulong arg1)
 {
+    MIPSCPU *cpu = env_archcpu(env);
     uint32_t mask = (1 << CP0TCBd_TBE);
     uint32_t newval;
 
-    if (env->mvp->CP0_MVPControl & (1 << CP0MVPCo_VPC)) {
+    if (cpu->mvp->CP0_MVPControl & (1 << CP0MVPCo_VPC)) {
         mask |= (1 << CP0TCBd_CurVPE);
     }
     newval = (env->active_tc.CP0_TCBind & ~mask) | (arg1 & mask);
@@ -705,8 +711,9 @@ void helper_mttc0_tcbind(CPUMIPSState *env, target_ulong arg1)
     uint32_t mask = (1 << CP0TCBd_TBE);
     uint32_t newval;
     CPUMIPSState *other = mips_cpu_map_tc(env, &other_tc);
+    MIPSCPU *other_cpu = env_archcpu(other);
 
-    if (other->mvp->CP0_MVPControl & (1 << CP0MVPCo_VPC)) {
+    if (other_cpu->mvp->CP0_MVPControl & (1 << CP0MVPCo_VPC)) {
         mask |= (1 << CP0TCBd_CurVPE);
     }
     if (other_tc == other->current_tc) {
@@ -1560,14 +1567,15 @@ target_ulong helper_emt(void)
 target_ulong helper_dvpe(CPUMIPSState *env)
 {
     CPUState *other_cs = first_cpu;
-    target_ulong prev = env->mvp->CP0_MVPControl;
+    MIPSCPU *cpu = env_archcpu(env);
+    target_ulong prev = cpu->mvp->CP0_MVPControl;
 
     if (env->CP0_VPEConf0 & (1 << CP0VPEC0_MVP)) {
         CPU_FOREACH(other_cs) {
             MIPSCPU *other_cpu = MIPS_CPU(other_cs);
             /* Turn off all VPEs except the one executing the dvpe.  */
             if (&other_cpu->env != env) {
-                other_cpu->env.mvp->CP0_MVPControl &= ~(1 << CP0MVPCo_EVP);
+                other_cpu->mvp->CP0_MVPControl &= ~(1 << CP0MVPCo_EVP);
                 mips_vpe_sleep(other_cpu);
             }
         }
@@ -1578,7 +1586,8 @@ target_ulong helper_dvpe(CPUMIPSState *env)
 target_ulong helper_evpe(CPUMIPSState *env)
 {
     CPUState *other_cs = first_cpu;
-    target_ulong prev = env->mvp->CP0_MVPControl;
+    MIPSCPU *cpu = env_archcpu(env);
+    target_ulong prev = cpu->mvp->CP0_MVPControl;
 
     if (env->CP0_VPEConf0 & (1 << CP0VPEC0_MVP)) {
         CPU_FOREACH(other_cs) {
@@ -1588,7 +1597,7 @@ target_ulong helper_evpe(CPUMIPSState *env)
                 /* If the VPE is WFI, don't disturb its sleep.  */
                 && !mips_vpe_is_wfi(other_cpu)) {
                 /* Enable the VPE.  */
-                other_cpu->env.mvp->CP0_MVPControl |= (1 << CP0MVPCo_EVP);
+                other_cpu->mvp->CP0_MVPControl |= (1 << CP0MVPCo_EVP);
                 mips_vpe_wake(other_cpu); /* And wake it up.  */
             }
         }
diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
index 54849e9ff1..6991f0a521 100644
--- a/target/mips/tcg/translate.c
+++ b/target/mips/tcg/translate.c
@@ -8085,6 +8085,7 @@ cp0_unimplemented:
 static void gen_mftr(CPUMIPSState *env, DisasContext *ctx, int rt, int rd,
                      int u, int sel, int h)
 {
+    MIPSCPU *cpu = env_archcpu(env);
     int other_tc = env->CP0_VPEControl & (0xff << CP0VPECo_TargTC);
     TCGv t0 = tcg_temp_new();
 
@@ -8093,7 +8094,7 @@ static void gen_mftr(CPUMIPSState *env, DisasContext *ctx, int rt, int rd,
          (env->active_tc.CP0_TCBind & (0xf << CP0TCBd_CurVPE)))) {
         tcg_gen_movi_tl(t0, -1);
     } else if ((env->CP0_VPEControl & (0xff << CP0VPECo_TargTC)) >
-               (env->mvp->CP0_MVPConf0 & (0xff << CP0MVPC0_PTC))) {
+               (cpu->mvp->CP0_MVPConf0 & (0xff << CP0MVPC0_PTC))) {
         tcg_gen_movi_tl(t0, -1);
     } else if (u == 0) {
         switch (rt) {
@@ -8309,6 +8310,7 @@ die:
 static void gen_mttr(CPUMIPSState *env, DisasContext *ctx, int rd, int rt,
                      int u, int sel, int h)
 {
+    MIPSCPU *cpu = env_archcpu(env);
     int other_tc = env->CP0_VPEControl & (0xff << CP0VPECo_TargTC);
     TCGv t0 = tcg_temp_new();
 
@@ -8319,7 +8321,7 @@ static void gen_mttr(CPUMIPSState *env, DisasContext *ctx, int rd, int rt,
         /* NOP */
         ;
     } else if ((env->CP0_VPEControl & (0xff << CP0VPECo_TargTC)) >
-             (env->mvp->CP0_MVPConf0 & (0xff << CP0MVPC0_PTC))) {
+             (cpu->mvp->CP0_MVPConf0 & (0xff << CP0MVPC0_PTC))) {
         /* NOP */
         ;
     } else if (u == 0) {
-- 
2.43.0

[PATCH 2/3] target/mips: Free mvp in unrealize

[Peter Maydell]

We allocate memory for cpu->mvp in mips_cpu_realizefn(), but we
never free it, which causes memory leaks like this:

    Direct leak of 24 byte(s) in 2 object(s) allocated from:
        #0 0x5f9458e61c8d in calloc (/home/pm215/qemu/build/san/qemu-mips+0x4d8c8d) (BuildId: 4153e33b3d08657a71ce2a04a82d0c2954966d9c)
        #1 0x74761891a771 in g_malloc0 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x63771) (BuildId: 116e142b9b52c8a4dfd403e759e71ab8f95d8bb3)
        #2 0x5f94590687aa in mvp_init /home/pm215/qemu/build/san/../../target/mips/cpu-defs.c.inc:1037:16
        #3 0x5f94590687aa in mips_cpu_realizefn /home/pm215/qemu/build/san/../../target/mips/cpu.c:489:5
        #4 0x5f9459366a3a in device_set_realized /home/pm215/qemu/build/san/../../hw/core/qdev.c:523:13
        #5 0x5f9459380a49 in property_set_bool /home/pm215/qemu/build/san/../../qom/object.c:2376:5
        #6 0x5f945937bace in object_property_set /home/pm215/qemu/build/san/../../qom/object.c:1450:5
        #7 0x5f945938816c in object_property_set_qobject /home/pm215/qemu/build/san/../../qom/qom-qobject.c:28:10
        #8 0x5f94592cc100 in cpu_copy /home/pm215/qemu/build/san/../../linux-user/main.c:240:25
        #9 0x5f9459309931 in do_syscall1 /home/pm215/qemu/build/san/../../linux-user/syscall.c
        #10 0x5f94593058d8 in do_syscall /home/pm215/qemu/build/san/../../linux-user/syscall.c:14422:15
        #11 0x5f945905c73e in cpu_loop /home/pm215/qemu/build/san/../../linux-user/mips/cpu_loop.c:124:23

for linux-user, where each new guest thread is a new CPU object that
we need to destroy on thread exit.

Add an unrealize method which frees this memory.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/mips/cpu.c | 12 ++++++++++++
 target/mips/cpu.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 789ca188b5..0663cda003 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -502,6 +502,16 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
     mcc->parent_realize(dev, errp);
 }
 
+static void mips_cpu_unrealizefn(DeviceState *dev)
+{
+    MIPSCPU *cpu = MIPS_CPU(dev);
+    MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(dev);
+
+    g_free(cpu->mvp);
+
+    mcc->parent_unrealize(dev);
+}
+
 static void mips_cpu_initfn(Object *obj)
 {
     MIPSCPU *cpu = MIPS_CPU(obj);
@@ -606,6 +616,8 @@ static void mips_cpu_class_init(ObjectClass *c, const void *data)
     device_class_set_props(dc, mips_cpu_properties);
     device_class_set_parent_realize(dc, mips_cpu_realizefn,
                                     &mcc->parent_realize);
+    device_class_set_parent_unrealize(dc, mips_cpu_unrealizefn,
+                                      &mcc->parent_unrealize);
     resettable_class_set_parent_phases(rc, NULL, mips_cpu_reset_hold, NULL,
                                        &mcc->parent_phases);
 
diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 8de3178b6d..ca36ca0d6f 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -1225,6 +1225,7 @@ struct MIPSCPUClass {
     CPUClass parent_class;
 
     DeviceRealize parent_realize;
+    DeviceUnrealize parent_unrealize;
     ResettablePhases parent_phases;
     const struct mips_def_t *cpu_def;
 
-- 
2.43.0

[PATCH 3/3] target/mips: Move count_clock to MIPSCPU struct

[Peter Maydell]

The count_clock pointer is not something we can do a shallow copy of,
as linux-user cpu_copy() does, and although it is a system-mode piece
of state we unconditionally create it, so it is present also in
user-mode.

There isn't any need to keep this in the env struct rather than the
CPU struct, so move it to avoid possible memory leaks or
double-usage. This also puts it next to the other Clocks that this
CPU has.

I haven't seen any sanitizer reports about this field, so this is
averting a possible problem rather than correcting an observed one.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/mips/cpu.c              |  4 ++--
 target/mips/cpu.h              |  2 +-
 target/mips/system/cp0_timer.c | 12 ++++++++----
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 0663cda003..f803d47763 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -449,7 +449,7 @@ static void mips_cp0_period_set(MIPSCPU *cpu)
 
     clock_set_mul_div(cpu->count_div, env->cpu_model->CCRes, 1);
     clock_set_source(cpu->count_div, cpu->clock);
-    clock_set_source(env->count_clock, cpu->count_div);
+    clock_set_source(cpu->count_clock, cpu->count_div);
 }
 
 static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
@@ -520,7 +520,7 @@ static void mips_cpu_initfn(Object *obj)
 
     cpu->clock = qdev_init_clock_in(DEVICE(obj), "clk-in", NULL, cpu, 0);
     cpu->count_div = clock_new(OBJECT(obj), "clk-div-count");
-    env->count_clock = clock_new(OBJECT(obj), "clk-count");
+    cpu->count_clock = clock_new(OBJECT(obj), "clk-count");
     env->cpu_model = mcc->cpu_def;
 }
 
diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index ca36ca0d6f..cb72be9336 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -1188,7 +1188,6 @@ typedef struct CPUArchState {
 
     const mips_def_t *cpu_model;
     QEMUTimer *timer; /* Internal timer */
-    Clock *count_clock; /* CP0_Count clock */
     target_ulong exception_base; /* ExceptionBase input to the core */
 } CPUMIPSState;
 
@@ -1206,6 +1205,7 @@ struct ArchCPU {
     CPUMIPSState env;
 
     Clock *clock;
+    Clock *count_clock; /* CP0_Count clock */
     Clock *count_div; /* Divider for CP0_Count clock */
 
     CPUMIPSMVPContext *mvp;
diff --git a/target/mips/system/cp0_timer.c b/target/mips/system/cp0_timer.c
index afa163c319..634c2a66bb 100644
--- a/target/mips/system/cp0_timer.c
+++ b/target/mips/system/cp0_timer.c
@@ -29,14 +29,16 @@
 /* MIPS R4K timer */
 static uint32_t cpu_mips_get_count_val(CPUMIPSState *env)
 {
+    MIPSCPU *cpu = env_archcpu(env);
     int64_t now_ns;
     now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     return env->CP0_Count +
-            (uint32_t)clock_ns_to_ticks(env->count_clock, now_ns);
+            (uint32_t)clock_ns_to_ticks(cpu->count_clock, now_ns);
 }
 
 static void cpu_mips_timer_update(CPUMIPSState *env)
 {
+    MIPSCPU *cpu = env_archcpu(env);
     uint64_t now_ns, next_ns;
     uint32_t wait;
 
@@ -46,7 +48,7 @@ static void cpu_mips_timer_update(CPUMIPSState *env)
     if (!wait) {
         wait = UINT32_MAX;
     }
-    next_ns = now_ns + clock_ticks_to_ns(env->count_clock, wait);
+    next_ns = now_ns + clock_ticks_to_ns(cpu->count_clock, wait);
     timer_mod(env->timer, next_ns);
 }
 
@@ -85,11 +87,12 @@ void cpu_mips_store_count(CPUMIPSState *env, uint32_t count)
      * So env->timer may be NULL, which is also the case with KVM enabled so
      * treat timer as disabled in that case.
      */
+    MIPSCPU *cpu = env_archcpu(env);
     if (env->CP0_Cause & (1 << CP0Ca_DC) || !env->timer) {
         env->CP0_Count = count;
     } else {
         /* Store new count register */
-        env->CP0_Count = count - (uint32_t)clock_ns_to_ticks(env->count_clock,
+        env->CP0_Count = count - (uint32_t)clock_ns_to_ticks(cpu->count_clock,
                         qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
         /* Update timer timer */
         cpu_mips_timer_update(env);
@@ -116,7 +119,8 @@ void cpu_mips_start_count(CPUMIPSState *env)
 void cpu_mips_stop_count(CPUMIPSState *env)
 {
     /* Store the current value */
-    env->CP0_Count += (uint32_t)clock_ns_to_ticks(env->count_clock,
+    MIPSCPU *cpu = env_archcpu(env);
+    env->CP0_Count += (uint32_t)clock_ns_to_ticks(cpu->count_clock,
                         qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
 }
 
-- 
2.43.0

Re: [PATCH 3/3] target/mips: Move count_clock to MIPSCPU struct

[Philippe Mathieu-Daudé]

On 17/3/26 18:50, Peter Maydell wrote:
> The count_clock pointer is not something we can do a shallow copy of,
> as linux-user cpu_copy() does, and although it is a system-mode piece
> of state we unconditionally create it, so it is present also in
> user-mode.
> 
> There isn't any need to keep this in the env struct rather than the
> CPU struct, so move it to avoid possible memory leaks or
> double-usage. This also puts it next to the other Clocks that this
> CPU has.
> 
> I haven't seen any sanitizer reports about this field, so this is
> averting a possible problem rather than correcting an observed one.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/mips/cpu.c              |  4 ++--
>   target/mips/cpu.h              |  2 +-
>   target/mips/system/cp0_timer.c | 12 ++++++++----
>   3 files changed, 11 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>