[Qemu-devel] [RFC PATCH for 4.1?] target/ppc: move opcode decode tables to PowerPCCPU

Alex Bennée posted 1 patch 4 years, 8 months ago
Test asan failed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test FreeBSD passed
Test checkpatch passed
Test s390x passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190716121352.302-1-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(-)
[Qemu-devel] [RFC PATCH for 4.1?] target/ppc: move opcode decode tables to PowerPCCPU
Posted by Alex Bennée 4 years, 8 months ago
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


Re: [Qemu-devel] [RFC PATCH for 4.1?] target/ppc: move opcode decode tables to PowerPCCPU
Posted by Richard Henderson 4 years, 8 months ago
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~

Re: [Qemu-devel] [RFC PATCH for 4.1?] target/ppc: move opcode decode tables to PowerPCCPU
Posted by no-reply@patchew.org 4 years, 8 months ago
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
Re: [Qemu-devel] [RFC PATCH for 4.1?] target/ppc: move opcode decode tables to PowerPCCPU
Posted by David Gibson 4 years, 8 months ago
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
Re: [Qemu-devel] [RFC PATCH for 4.1?] target/ppc: move opcode decode tables to PowerPCCPU
Posted by Alex Bennée 4 years, 8 months ago
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