target/ppc/cpu.h | 8 ++++---- target/ppc/translate.c | 3 ++- target/ppc/translate_init.inc.c | 16 +++++++--------- 3 files changed, 13 insertions(+), 14 deletions(-)
The opcode decode tables aren't really part of the CPUPPCState but an
internal implementation detail for the translator. This can cause
problems with memcpy in cpu_copy as any table created during
ppc_cpu_realize get written over causing a memory leak. To avoid this
move the tables into PowerPCCPU which is better suited to hold
internal implementation details.
Attempts to fix: https://bugs.launchpad.net/qemu/+bug/1836558
Cc: 1836558@bugs.launchpad.net
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
target/ppc/cpu.h | 8 ++++----
target/ppc/translate.c | 3 ++-
target/ppc/translate_init.inc.c | 16 +++++++---------
3 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index c9beba2a5c0..10e34b69b75 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1104,10 +1104,6 @@ struct CPUPPCState {
bool resume_as_sreset;
#endif
- /* Those resources are used only during code translation */
- /* opcode handlers */
- opc_handler_t *opcodes[PPC_CPU_OPCODES_LEN];
-
/* Those resources are used only in QEMU core */
target_ulong hflags; /* hflags is a MSR & HFLAGS_MASK */
target_ulong hflags_nmsr; /* specific hflags, not coming from MSR */
@@ -1191,6 +1187,10 @@ struct PowerPCCPU {
int32_t node_id; /* NUMA node this CPU belongs to */
PPCHash64Options *hash64_opts;
+ /* Those resources are used only during code translation */
+ /* opcode handlers */
+ opc_handler_t *opcodes[PPC_CPU_OPCODES_LEN];
+
/* Fields related to migration compatibility hacks */
bool pre_2_8_migration;
target_ulong mig_msr_mask;
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 4a5de280365..c0faab8a824 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -7857,6 +7857,7 @@ static bool ppc_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
static void ppc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
{
DisasContext *ctx = container_of(dcbase, DisasContext, base);
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
CPUPPCState *env = cs->env_ptr;
opc_handler_t **table, *handler;
@@ -7874,7 +7875,7 @@ static void ppc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
opc3(ctx->opcode), opc4(ctx->opcode),
ctx->le_mode ? "little" : "big");
ctx->base.pc_next += 4;
- table = env->opcodes;
+ table = cpu->opcodes;
handler = table[opc1(ctx->opcode)];
if (is_indirect_opcode(handler)) {
table = ind_table(handler);
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 86fc8f2e316..9cd2033bb92 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -9440,14 +9440,13 @@ static void fix_opcode_tables(opc_handler_t **ppc_opcodes)
static void create_ppc_opcodes(PowerPCCPU *cpu, Error **errp)
{
PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
- CPUPPCState *env = &cpu->env;
opcode_t *opc;
- fill_new_table(env->opcodes, PPC_CPU_OPCODES_LEN);
+ fill_new_table(cpu->opcodes, PPC_CPU_OPCODES_LEN);
for (opc = opcodes; opc < &opcodes[ARRAY_SIZE(opcodes)]; opc++) {
if (((opc->handler.type & pcc->insns_flags) != 0) ||
((opc->handler.type2 & pcc->insns_flags2) != 0)) {
- if (register_insn(env->opcodes, opc) < 0) {
+ if (register_insn(cpu->opcodes, opc) < 0) {
error_setg(errp, "ERROR initializing PowerPC instruction "
"0x%02x 0x%02x 0x%02x", opc->opc1, opc->opc2,
opc->opc3);
@@ -9455,7 +9454,7 @@ static void create_ppc_opcodes(PowerPCCPU *cpu, Error **errp)
}
}
}
- fix_opcode_tables(env->opcodes);
+ fix_opcode_tables(cpu->opcodes);
fflush(stdout);
fflush(stderr);
}
@@ -10023,7 +10022,6 @@ static void ppc_cpu_unrealize(DeviceState *dev, Error **errp)
{
PowerPCCPU *cpu = POWERPC_CPU(dev);
PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
- CPUPPCState *env = &cpu->env;
Error *local_err = NULL;
opc_handler_t **table, **table_2;
int i, j, k;
@@ -10035,11 +10033,11 @@ static void ppc_cpu_unrealize(DeviceState *dev, Error **errp)
}
for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) {
- if (env->opcodes[i] == &invalid_handler) {
+ if (cpu->opcodes[i] == &invalid_handler) {
continue;
}
- if (is_indirect_opcode(env->opcodes[i])) {
- table = ind_table(env->opcodes[i]);
+ if (is_indirect_opcode(cpu->opcodes[i])) {
+ table = ind_table(cpu->opcodes[i]);
for (j = 0; j < PPC_CPU_INDIRECT_OPCODES_LEN; j++) {
if (table[j] == &invalid_handler) {
continue;
@@ -10057,7 +10055,7 @@ static void ppc_cpu_unrealize(DeviceState *dev, Error **errp)
~PPC_INDIRECT));
}
}
- g_free((opc_handler_t *)((uintptr_t)env->opcodes[i] &
+ g_free((opc_handler_t *)((uintptr_t)cpu->opcodes[i] &
~PPC_INDIRECT));
}
}
--
2.20.1
On 7/16/19 12:13 PM, Alex Bennée wrote: > The opcode decode tables aren't really part of the CPUPPCState but an > internal implementation detail for the translator. This can cause > problems with memcpy in cpu_copy as any table created during > ppc_cpu_realize get written over causing a memory leak. To avoid this > move the tables into PowerPCCPU which is better suited to hold > internal implementation details. > > Attempts to fix: https://bugs.launchpad.net/qemu/+bug/1836558 > Cc: 1836558@bugs.launchpad.net > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > target/ppc/cpu.h | 8 ++++---- > target/ppc/translate.c | 3 ++- > target/ppc/translate_init.inc.c | 16 +++++++--------- > 3 files changed, 13 insertions(+), 14 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Patchew URL: https://patchew.org/QEMU/20190716121352.302-1-alex.bennee@linaro.org/ Hi, This series failed the asan build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-fedora V=1 NETWORK=1 time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20190716121352.302-1-alex.bennee@linaro.org/testing.asan/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On Tue, Jul 16, 2019 at 01:13:52PM +0100, Alex Bennée wrote: > The opcode decode tables aren't really part of the CPUPPCState but an > internal implementation detail for the translator. This can cause > problems with memcpy in cpu_copy as any table created during > ppc_cpu_realize get written over causing a memory leak. To avoid this > move the tables into PowerPCCPU which is better suited to hold > internal implementation details. > > Attempts to fix: https://bugs.launchpad.net/qemu/+bug/1836558 > Cc: 1836558@bugs.launchpad.net > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> I've applied this now to ppc-for-4.2. If there's an argument for including it in 4.1 during hard freeze, you'll need to spell it out for me. > --- > target/ppc/cpu.h | 8 ++++---- > target/ppc/translate.c | 3 ++- > target/ppc/translate_init.inc.c | 16 +++++++--------- > 3 files changed, 13 insertions(+), 14 deletions(-) > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index c9beba2a5c0..10e34b69b75 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -1104,10 +1104,6 @@ struct CPUPPCState { > bool resume_as_sreset; > #endif > > - /* Those resources are used only during code translation */ > - /* opcode handlers */ > - opc_handler_t *opcodes[PPC_CPU_OPCODES_LEN]; > - > /* Those resources are used only in QEMU core */ > target_ulong hflags; /* hflags is a MSR & HFLAGS_MASK */ > target_ulong hflags_nmsr; /* specific hflags, not coming from MSR */ > @@ -1191,6 +1187,10 @@ struct PowerPCCPU { > int32_t node_id; /* NUMA node this CPU belongs to */ > PPCHash64Options *hash64_opts; > > + /* Those resources are used only during code translation */ > + /* opcode handlers */ > + opc_handler_t *opcodes[PPC_CPU_OPCODES_LEN]; > + > /* Fields related to migration compatibility hacks */ > bool pre_2_8_migration; > target_ulong mig_msr_mask; > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > index 4a5de280365..c0faab8a824 100644 > --- a/target/ppc/translate.c > +++ b/target/ppc/translate.c > @@ -7857,6 +7857,7 @@ static bool ppc_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs, > static void ppc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs) > { > DisasContext *ctx = container_of(dcbase, DisasContext, base); > + PowerPCCPU *cpu = POWERPC_CPU(cs); > CPUPPCState *env = cs->env_ptr; > opc_handler_t **table, *handler; > > @@ -7874,7 +7875,7 @@ static void ppc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs) > opc3(ctx->opcode), opc4(ctx->opcode), > ctx->le_mode ? "little" : "big"); > ctx->base.pc_next += 4; > - table = env->opcodes; > + table = cpu->opcodes; > handler = table[opc1(ctx->opcode)]; > if (is_indirect_opcode(handler)) { > table = ind_table(handler); > diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c > index 86fc8f2e316..9cd2033bb92 100644 > --- a/target/ppc/translate_init.inc.c > +++ b/target/ppc/translate_init.inc.c > @@ -9440,14 +9440,13 @@ static void fix_opcode_tables(opc_handler_t **ppc_opcodes) > static void create_ppc_opcodes(PowerPCCPU *cpu, Error **errp) > { > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); > - CPUPPCState *env = &cpu->env; > opcode_t *opc; > > - fill_new_table(env->opcodes, PPC_CPU_OPCODES_LEN); > + fill_new_table(cpu->opcodes, PPC_CPU_OPCODES_LEN); > for (opc = opcodes; opc < &opcodes[ARRAY_SIZE(opcodes)]; opc++) { > if (((opc->handler.type & pcc->insns_flags) != 0) || > ((opc->handler.type2 & pcc->insns_flags2) != 0)) { > - if (register_insn(env->opcodes, opc) < 0) { > + if (register_insn(cpu->opcodes, opc) < 0) { > error_setg(errp, "ERROR initializing PowerPC instruction " > "0x%02x 0x%02x 0x%02x", opc->opc1, opc->opc2, > opc->opc3); > @@ -9455,7 +9454,7 @@ static void create_ppc_opcodes(PowerPCCPU *cpu, Error **errp) > } > } > } > - fix_opcode_tables(env->opcodes); > + fix_opcode_tables(cpu->opcodes); > fflush(stdout); > fflush(stderr); > } > @@ -10023,7 +10022,6 @@ static void ppc_cpu_unrealize(DeviceState *dev, Error **errp) > { > PowerPCCPU *cpu = POWERPC_CPU(dev); > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); > - CPUPPCState *env = &cpu->env; > Error *local_err = NULL; > opc_handler_t **table, **table_2; > int i, j, k; > @@ -10035,11 +10033,11 @@ static void ppc_cpu_unrealize(DeviceState *dev, Error **errp) > } > > for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) { > - if (env->opcodes[i] == &invalid_handler) { > + if (cpu->opcodes[i] == &invalid_handler) { > continue; > } > - if (is_indirect_opcode(env->opcodes[i])) { > - table = ind_table(env->opcodes[i]); > + if (is_indirect_opcode(cpu->opcodes[i])) { > + table = ind_table(cpu->opcodes[i]); > for (j = 0; j < PPC_CPU_INDIRECT_OPCODES_LEN; j++) { > if (table[j] == &invalid_handler) { > continue; > @@ -10057,7 +10055,7 @@ static void ppc_cpu_unrealize(DeviceState *dev, Error **errp) > ~PPC_INDIRECT)); > } > } > - g_free((opc_handler_t *)((uintptr_t)env->opcodes[i] & > + g_free((opc_handler_t *)((uintptr_t)cpu->opcodes[i] & > ~PPC_INDIRECT)); > } > } -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
David Gibson <david@gibson.dropbear.id.au> writes: > On Tue, Jul 16, 2019 at 01:13:52PM +0100, Alex Bennée wrote: >> The opcode decode tables aren't really part of the CPUPPCState but an >> internal implementation detail for the translator. This can cause >> problems with memcpy in cpu_copy as any table created during >> ppc_cpu_realize get written over causing a memory leak. To avoid this >> move the tables into PowerPCCPU which is better suited to hold >> internal implementation details. >> >> Attempts to fix: https://bugs.launchpad.net/qemu/+bug/1836558 >> Cc: 1836558@bugs.launchpad.net >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > I've applied this now to ppc-for-4.2. If there's an argument for > including it in 4.1 during hard freeze, you'll need to spell it out > for me. Well without: Subject: [RFC PATCH for 4.1] linux-user: unparent CPU object before unref Date: Tue, 16 Jul 2019 15:01:33 +0100 Message-Id: <20190716140133.8578-1-alex.bennee@linaro.org> it doesn't matter as we never attempt to free the memory once a thread is destroyed. This causes all linux-user guests that create and destroy threads quickly to slowly leak memory. However due to the dynamic opcode table ppc/ppc64-linux-user guests leak a lot faster than most, in the order of ~50k each time a thread is created and destroyed. However I'm happy to defer to you as the maintainer :-) -- Alex Bennée
© 2016 - 2024 Red Hat, Inc.