1. Patchew
  2. QEMU
  3. View series

[PATCH for-5.0?] target/xtensa: Statically allocate xtensa_insnbufs

Richard Henderson posted 1 patch 5 years, 10 months ago
Failed in applying to current master (apply log)
target/xtensa/translate.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
[PATCH for-5.0?] target/xtensa: Statically allocate xtensa_insnbufs
Posted by Richard Henderson 5 years, 10 months ago 
Rather than dynamically allocate, and risk failing to free
when we longjmp out of the translator, allocate the maximum
buffer size from any of the supported cpus, which is 8:

core-dc232b/xtensa-modules.inc.c:  3 /* insn_size */, 0,
core-dc233c/xtensa-modules.inc.c:  3 /* insn_size */, 0,
core-de212/xtensa-modules.inc.c:  3 /* insn_size */, 0,
core-fsf/xtensa-modules.inc.c:  3 /* insn_size */, 0,
core-sample_controller/xtensa-modules.inc.c:  3 /* insn_size */, 0,
core-test_kc705_be/xtensa-modules.inc.c:  8 /* insn_size */, 0,
core-test_mmuhifi_c3/xtensa-modules.inc.c:  8 /* insn_size */, 0,

Cc: Max Filippov <jcmvbkbc@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/xtensa/translate.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index 37f65b1f03..86369aa623 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -72,8 +72,10 @@ struct DisasContext {
     unsigned cpenable;
 
     uint32_t op_flags;
-    xtensa_insnbuf insnbuf;
-    xtensa_insnbuf slotbuf;
+
+    /* The maximum of all supported cpus is 8. */
+    xtensa_insnbuf_word insnbuf[8];
+    xtensa_insnbuf_word slotbuf[8];
 };
 
 static TCGv_i32 cpu_pc;
@@ -1174,14 +1176,11 @@ static void xtensa_tr_init_disas_context(DisasContextBase *dcbase,
     dc->callinc = ((tb_flags & XTENSA_TBFLAG_CALLINC_MASK) >>
                    XTENSA_TBFLAG_CALLINC_SHIFT);
 
-    /*
-     * FIXME: This will leak when a failed instruction load or similar
-     * event causes us to longjump out of the translation loop and
-     * hence not clean-up in xtensa_tr_tb_stop
-     */
     if (dc->config->isa) {
-        dc->insnbuf = xtensa_insnbuf_alloc(dc->config->isa);
-        dc->slotbuf = xtensa_insnbuf_alloc(dc->config->isa);
+        size_t size = (xtensa_insnbuf_size(dc->config->isa) *
+                       sizeof(xtensa_insnbuf_word));
+        assert(sizeof(dc->insnbuf) >= size);
+        assert(sizeof(dc->slotbuf) >= size);
     }
     init_sar_tracker(dc);
 }
@@ -1272,10 +1271,6 @@ static void xtensa_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
     DisasContext *dc = container_of(dcbase, DisasContext, base);
 
     reset_sar_tracker(dc);
-    if (dc->config->isa) {
-        xtensa_insnbuf_free(dc->config->isa, dc->insnbuf);
-        xtensa_insnbuf_free(dc->config->isa, dc->slotbuf);
-    }
     if (dc->icount) {
         tcg_temp_free(dc->next_icount);
     }
-- 
2.20.1
Re: [PATCH for-5.0?] target/xtensa: Statically allocate xtensa_insnbufs
Posted by Max Filippov 5 years, 10 months ago 
On Mon, Apr 6, 2020 at 8:09 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Rather than dynamically allocate, and risk failing to free
> when we longjmp out of the translator, allocate the maximum
> buffer size from any of the supported cpus, which is 8:

There's macro MAX_INSN_LENGTH that defines maximal supported
instruction length in bytes. Maybe the following instead, along the lines
of what libisa does dynamically?:

--8<--
From 08cc91b0d51e244766d73aae23aebd194b598378 Mon Sep 17 00:00:00 2001
From: Max Filippov <jcmvbkbc@gmail.com>
Date: Mon, 6 Apr 2020 20:59:54 -0700
Subject: [PATCH] target/xtensa: statically allocate xtensa_insnbufs in
DisasContext

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 target/xtensa/cpu.h       |  3 +++
 target/xtensa/helper.c    |  1 +
 target/xtensa/translate.c | 12 ++----------
 3 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
index c0d69fad96c5..7a46dccbe11b 100644
--- a/target/xtensa/cpu.h
+++ b/target/xtensa/cpu.h
@@ -213,6 +213,9 @@ enum {
 #define MEMCTL_IL0EN 0x1

 #define MAX_INSN_LENGTH 64
+#define MAX_INSNBUF_LENGTH \
+    ((MAX_INSN_LENGTH + sizeof(xtensa_insnbuf_word) - 1) / \
+     sizeof(xtensa_insnbuf_word))
 #define MAX_INSN_SLOTS 32
 #define MAX_OPCODE_ARGS 16
 #define MAX_NAREG 64
diff --git a/target/xtensa/helper.c b/target/xtensa/helper.c
index 376a61f3397c..278415ae0e06 100644
--- a/target/xtensa/helper.c
+++ b/target/xtensa/helper.c
@@ -96,6 +96,7 @@ static void init_libisa(XtensaConfig *config)

     config->isa = xtensa_isa_init(config->isa_internal, NULL, NULL);
     assert(xtensa_isa_maxlength(config->isa) <= MAX_INSN_LENGTH);
+    assert(xtensa_insnbuf_size(dc->config->isa) <= MAX_INSNBUF_LENGTH);
     opcodes = xtensa_isa_num_opcodes(config->isa);
     formats = xtensa_isa_num_formats(config->isa);
     regfiles = xtensa_isa_num_regfiles(config->isa);
diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index 8aa972cafdf3..91c7776c2544 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -72,8 +72,8 @@ struct DisasContext {
     unsigned cpenable;

     uint32_t op_flags;
-    xtensa_insnbuf insnbuf;
-    xtensa_insnbuf slotbuf;
+    xtensa_insnbuf_word insnbuf[MAX_INSNBUF_LENGTH];
+    xtensa_insnbuf_word slotbuf[MAX_INSNBUF_LENGTH];
 };

 static TCGv_i32 cpu_pc;
@@ -1174,10 +1174,6 @@ static void
xtensa_tr_init_disas_context(DisasContextBase *dcbase,
     dc->callinc = ((tb_flags & XTENSA_TBFLAG_CALLINC_MASK) >>
                    XTENSA_TBFLAG_CALLINC_SHIFT);

-    if (dc->config->isa) {
-        dc->insnbuf = xtensa_insnbuf_alloc(dc->config->isa);
-        dc->slotbuf = xtensa_insnbuf_alloc(dc->config->isa);
-    }
     init_sar_tracker(dc);
 }

@@ -1267,10 +1263,6 @@ static void xtensa_tr_tb_stop(DisasContextBase
*dcbase, CPUState *cpu)
     DisasContext *dc = container_of(dcbase, DisasContext, base);

     reset_sar_tracker(dc);
-    if (dc->config->isa) {
-        xtensa_insnbuf_free(dc->config->isa, dc->insnbuf);
-        xtensa_insnbuf_free(dc->config->isa, dc->slotbuf);
-    }
     if (dc->icount) {
         tcg_temp_free(dc->next_icount);
     }
--8<--
-- 
Thanks.
-- Max
Re: [PATCH for-5.0?] target/xtensa: Statically allocate xtensa_insnbufs
Posted by Richard Henderson 5 years, 10 months ago 
On 4/6/20 9:04 PM, Max Filippov wrote:
> On Mon, Apr 6, 2020 at 8:09 PM Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Rather than dynamically allocate, and risk failing to free
>> when we longjmp out of the translator, allocate the maximum
>> buffer size from any of the supported cpus, which is 8:
> 
> There's macro MAX_INSN_LENGTH that defines maximal supported
> instruction length in bytes. Maybe the following instead, along the lines
> of what libisa does dynamically?:

Thanks for the pointer.  Looks better than mine.

>  #define MAX_INSN_LENGTH 64
> +#define MAX_INSNBUF_LENGTH \
> +    ((MAX_INSN_LENGTH + sizeof(xtensa_insnbuf_word) - 1) / \
> +     sizeof(xtensa_insnbuf_word))

There is a ROUND_UP macro, but it seems unnecessary.


r~
Re: [PATCH for-5.0?] target/xtensa: Statically allocate xtensa_insnbufs
Posted by Max Filippov 5 years, 10 months ago 
On Mon, Apr 6, 2020 at 9:04 PM Max Filippov <jcmvbkbc@gmail.com> wrote:
> diff --git a/target/xtensa/helper.c b/target/xtensa/helper.c
> index 376a61f3397c..278415ae0e06 100644
> --- a/target/xtensa/helper.c
> +++ b/target/xtensa/helper.c
> @@ -96,6 +96,7 @@ static void init_libisa(XtensaConfig *config)
>
>      config->isa = xtensa_isa_init(config->isa_internal, NULL, NULL);
>      assert(xtensa_isa_maxlength(config->isa) <= MAX_INSN_LENGTH);
> +    assert(xtensa_insnbuf_size(dc->config->isa) <= MAX_INSNBUF_LENGTH);

No 'dc->' here...

-- 
Thanks.
-- Max

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.