diff mbox series

[v4,12/45] target/arm: Merge allocation of the cpreg and its name

Message ID 20220501055028.646596-13-richard.henderson@linaro.org
State Superseded
Headers show
Series target/arm: Cleanups, new features, new cpus | expand

Commit Message

Richard Henderson May 1, 2022, 5:49 a.m. UTC 
Simplify freeing cp_regs hash table entries by using a single
allocation for the entire value.

This fixes a theoretical bug if we were to ever free the entire
hash table, because we've been installing string literal constants
into the cpreg structure in define_arm_vh_e2h_redirects_aliases.
However, at present we only free entries created for AArch32
wildcard cpregs which get overwritten by more specific cpregs,
so this bug is never exposed.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.c    | 16 +---------------
 target/arm/helper.c | 10 ++++++++--
 2 files changed, 9 insertions(+), 17 deletions(-)

Comments

Peter Maydell May 3, 2022, 4:07 p.m. UTC | #1 
On Sun, 1 May 2022 at 07:18, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Simplify freeing cp_regs hash table entries by using a single
> allocation for the entire value.
>
> This fixes a theoretical bug if we were to ever free the entire
> hash table, because we've been installing string literal constants
> into the cpreg structure in define_arm_vh_e2h_redirects_aliases.
> However, at present we only free entries created for AArch32
> wildcard cpregs which get overwritten by more specific cpregs,
> so this bug is never exposed.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM
diff mbox series

Patch

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index e81d96c3c1..6e8b39dc9e 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1069,27 +1069,13 @@  uint64_t arm_cpu_mp_affinity(int idx, uint8_t clustersz)
     return (Aff1 << ARM_AFF1_SHIFT) | Aff0;
 }
 
-static void cpreg_hashtable_data_destroy(gpointer data)
-{
-    /*
-     * Destroy function for cpu->cp_regs hashtable data entries.
-     * We must free the name string because it was g_strdup()ed in
-     * add_cpreg_to_hashtable(). It's OK to cast away the 'const'
-     * from r->name because we know we definitely allocated it.
-     */
-    ARMCPRegInfo *r = data;
-
-    g_free((void *)r->name);
-    g_free(r);
-}
-
 static void arm_cpu_initfn(Object *obj)
 {
     ARMCPU *cpu = ARM_CPU(obj);
 
     cpu_set_cpustate_pointers(cpu);
     cpu->cp_regs = g_hash_table_new_full(g_direct_hash, g_direct_equal,
-                                         NULL, cpreg_hashtable_data_destroy);
+                                         NULL, g_free);
 
     QLIST_INIT(&cpu->pre_el_change_hooks);
     QLIST_INIT(&cpu->el_change_hooks);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index edfb5c3d38..44c05deb5b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8506,11 +8506,17 @@  static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
      * add a single reginfo struct to the hash table.
      */
     uint32_t key;
-    ARMCPRegInfo *r2 = g_memdup(r, sizeof(ARMCPRegInfo));
+    ARMCPRegInfo *r2;
     int is64 = (r->type & ARM_CP_64BIT) ? 1 : 0;
     int ns = (secstate & ARM_CP_SECSTATE_NS) ? 1 : 0;
+    size_t name_len;
+
+    /* Combine cpreg and name into one allocation. */
+    name_len = strlen(name) + 1;
+    r2 = g_malloc(sizeof(*r2) + name_len);
+    *r2 = *r;
+    r2->name = memcpy(r2 + 1, name, name_len);
 
-    r2->name = g_strdup(name);
     /* Reset the secure state to the specific incoming state.  This is
      * necessary as the register may have been defined with both states.
      */