On 17/3/26 18:50, Peter Maydell wrote:
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 <[email protected]>
---
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/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));
There is no point in allocating this 12-byte structure...
Another cleanup for tomorrow.
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
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/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),
