[RFC PATCH 27/34] accel/tcg: Make translate-all.c target independent

Anton Johansson via posted 34 patches 10 months, 1 week ago
[RFC PATCH 27/34] accel/tcg: Make translate-all.c target independent
Posted by Anton Johansson via 10 months, 1 week ago
Makes translate-all.c independent of softmmu target by switching

    TARGET_LONG_BITS        -> target_long_bits()

    TARGET_INSN_START_WORDS -> tcg_ctx->insn_start_words,
                               target_insn_start_words(),

    TCG_GUEST_DEFAULT_MO    -> target_default_memory_order()

    MO_TE                   -> target_endian_memory_order()

Signed-off-by: Anton Johansson <anjo@rev.ng>
---
 accel/tcg/translate-all.c | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 9c981d1750..a3ae0c6910 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -65,7 +65,6 @@
 #include "internal-common.h"
 #include "internal-target.h"
 #include "perf.h"
-#include "tcg/insn-start-words.h"
 
 TBContext tb_ctx;
 
@@ -106,7 +105,7 @@ static int64_t decode_sleb128(const uint8_t **pp)
         val |= (int64_t)(byte & 0x7f) << shift;
         shift += 7;
     } while (byte & 0x80);
-    if (shift < TARGET_LONG_BITS && (byte & 0x40)) {
+    if (shift < target_long_bits() && (byte & 0x40)) {
         val |= -(int64_t)1 << shift;
     }
 
@@ -117,7 +116,7 @@ static int64_t decode_sleb128(const uint8_t **pp)
 /* Encode the data collected about the instructions while compiling TB.
    Place the data at BLOCK, and return the number of bytes consumed.
 
-   The logical table consists of TARGET_INSN_START_WORDS target_ulong's,
+   The logical table consists of tcg_ctx->insn_start_words target_ulong's,
    which come from the target's insn_start data, followed by a uintptr_t
    which comes from the host pc of the end of the code implementing the insn.
 
@@ -128,6 +127,7 @@ static int64_t decode_sleb128(const uint8_t **pp)
 
 static int encode_search(TranslationBlock *tb, uint8_t *block)
 {
+    const uint8_t insn_start_words = tcg_ctx->insn_start_words;
     uint8_t *highwater = tcg_ctx->code_gen_highwater;
     uint64_t *insn_data = tcg_ctx->gen_insn_data;
     uint16_t *insn_end_off = tcg_ctx->gen_insn_end_off;
@@ -137,13 +137,13 @@ static int encode_search(TranslationBlock *tb, uint8_t *block)
     for (i = 0, n = tb->icount; i < n; ++i) {
         uint64_t prev, curr;
 
-        for (j = 0; j < TARGET_INSN_START_WORDS; ++j) {
+        for (j = 0; j < insn_start_words; ++j) {
             if (i == 0) {
                 prev = (!(tb_cflags(tb) & CF_PCREL) && j == 0 ? tb->pc : 0);
             } else {
-                prev = insn_data[(i - 1) * TARGET_INSN_START_WORDS + j];
+                prev = insn_data[(i - 1) * insn_start_words + j];
             }
-            curr = insn_data[i * TARGET_INSN_START_WORDS + j];
+            curr = insn_data[i * insn_start_words + j];
             p = encode_sleb128(p, curr - prev);
         }
         prev = (i == 0 ? 0 : insn_end_off[i - 1]);
@@ -165,6 +165,7 @@ static int encode_search(TranslationBlock *tb, uint8_t *block)
 static int cpu_unwind_data_from_tb(TranslationBlock *tb, uintptr_t host_pc,
                                    uint64_t *data)
 {
+    const uint8_t insn_start_words = tcg_ctx->insn_start_words;
     uintptr_t iter_pc = (uintptr_t)tb->tc.ptr;
     const uint8_t *p = tb->tc.ptr + tb->tc.size;
     int i, j, num_insns = tb->icount;
@@ -175,7 +176,7 @@ static int cpu_unwind_data_from_tb(TranslationBlock *tb, uintptr_t host_pc,
         return -1;
     }
 
-    memset(data, 0, sizeof(uint64_t) * TARGET_INSN_START_WORDS);
+    memset(data, 0, sizeof(uint64_t) * insn_start_words);
     if (!(tb_cflags(tb) & CF_PCREL)) {
         data[0] = tb->pc;
     }
@@ -185,7 +186,7 @@ static int cpu_unwind_data_from_tb(TranslationBlock *tb, uintptr_t host_pc,
      * at which the end of the insn exceeds host_pc.
      */
     for (i = 0; i < num_insns; ++i) {
-        for (j = 0; j < TARGET_INSN_START_WORDS; ++j) {
+        for (j = 0; j < insn_start_words; ++j) {
             data[j] += decode_sleb128(&p);
         }
         iter_pc += decode_sleb128(&p);
@@ -203,7 +204,7 @@ static int cpu_unwind_data_from_tb(TranslationBlock *tb, uintptr_t host_pc,
 void cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
                                uintptr_t host_pc)
 {
-    uint64_t data[TARGET_INSN_START_WORDS];
+    uint64_t data[tcg_ctx->insn_start_words];
     int insns_left = cpu_unwind_data_from_tb(tb, host_pc, data);
 
     if (insns_left < 0) {
@@ -341,19 +342,15 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
     }
 
     tcg_ctx->gen_tb = tb;
-    tcg_ctx->addr_type = TARGET_LONG_BITS == 32 ? TCG_TYPE_I32 : TCG_TYPE_I64;
+    tcg_ctx->addr_type = target_long_bits() == 32 ? TCG_TYPE_I32 : TCG_TYPE_I64;
 #ifdef CONFIG_SOFTMMU
-    tcg_ctx->mo_te = MO_TE;
+    tcg_ctx->mo_te = target_endian_memory_order();
     tcg_ctx->page_bits = TARGET_PAGE_BITS;
     tcg_ctx->page_mask = TARGET_PAGE_MASK;
-    tcg_ctx->tlb_dyn_max_bits = CPU_TLB_DYN_MAX_BITS;
-#endif
-    tcg_ctx->insn_start_words = TARGET_INSN_START_WORDS;
-#ifdef TCG_GUEST_DEFAULT_MO
-    tcg_ctx->guest_mo = TCG_GUEST_DEFAULT_MO;
-#else
-    tcg_ctx->guest_mo = TCG_MO_ALL;
+    tcg_ctx->tlb_dyn_max_bits = target_tlb_dyn_max_bits();
 #endif
+    tcg_ctx->insn_start_words = target_insn_start_words();
+    tcg_ctx->guest_mo = target_default_memory_order();
 
  restart_translate:
     trace_translate_block(tb, pc, tb->tc.ptr);
@@ -441,6 +438,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
         qemu_log_in_addr_range(pc)) {
         FILE *logfile = qemu_log_trylock();
         if (logfile) {
+            const uint8_t insn_start_words = tcg_ctx->insn_start_words;
             int code_size, data_size;
             const tcg_target_ulong *rx_data_gen_ptr;
             size_t chunk_start;
@@ -460,7 +458,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
             fprintf(logfile, "OUT: [size=%d]\n", gen_code_size);
             fprintf(logfile,
                     "  -- guest addr 0x%016" PRIx64 " + tb prologue\n",
-                    tcg_ctx->gen_insn_data[insn * TARGET_INSN_START_WORDS]);
+                    tcg_ctx->gen_insn_data[insn * insn_start_words]);
             chunk_start = tcg_ctx->gen_insn_end_off[insn];
             disas(logfile, tb->tc.ptr, chunk_start);
 
@@ -473,7 +471,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
                 size_t chunk_end = tcg_ctx->gen_insn_end_off[insn];
                 if (chunk_end > chunk_start) {
                     fprintf(logfile, "  -- guest addr 0x%016" PRIx64 "\n",
-                            tcg_ctx->gen_insn_data[insn * TARGET_INSN_START_WORDS]);
+                            tcg_ctx->gen_insn_data[insn * insn_start_words]);
                     disas(logfile, tb->tc.ptr + chunk_start,
                           chunk_end - chunk_start);
                     chunk_start = chunk_end;
-- 
2.43.0
Re: [RFC PATCH 27/34] accel/tcg: Make translate-all.c target independent
Posted by Richard Henderson 10 months, 1 week ago
On 1/20/24 00:40, Anton Johansson wrote:
> Makes translate-all.c independent of softmmu target by switching
> 
>      TARGET_LONG_BITS        -> target_long_bits()
> 
>      TARGET_INSN_START_WORDS -> tcg_ctx->insn_start_words,
>                                 target_insn_start_words(),
> 
>      TCG_GUEST_DEFAULT_MO    -> target_default_memory_order()
> 
>      MO_TE                   -> target_endian_memory_order()
> 
> Signed-off-by: Anton Johansson <anjo@rev.ng>
> ---
>   accel/tcg/translate-all.c | 38 ++++++++++++++++++--------------------
>   1 file changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 9c981d1750..a3ae0c6910 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -65,7 +65,6 @@
>   #include "internal-common.h"
>   #include "internal-target.h"
>   #include "perf.h"
> -#include "tcg/insn-start-words.h"
>   
>   TBContext tb_ctx;
>   
> @@ -106,7 +105,7 @@ static int64_t decode_sleb128(const uint8_t **pp)
>           val |= (int64_t)(byte & 0x7f) << shift;
>           shift += 7;
>       } while (byte & 0x80);
> -    if (shift < TARGET_LONG_BITS && (byte & 0x40)) {
> +    if (shift < target_long_bits() && (byte & 0x40)) {

You just make TARGET_PAGE_SIZE etc target independent, right?
So you don't need this?  Or is this because of user-only still.

>   static int encode_search(TranslationBlock *tb, uint8_t *block)
>   {
> +    const uint8_t insn_start_words = tcg_ctx->insn_start_words;

Ok, because we're still inside the compilation context.

>   static int cpu_unwind_data_from_tb(TranslationBlock *tb, uintptr_t host_pc,
>                                      uint64_t *data)
>   {
> +    const uint8_t insn_start_words = tcg_ctx->insn_start_words;

Not ok, because we're outside of the compilation context.


r~
Re: [RFC PATCH 27/34] accel/tcg: Make translate-all.c target independent
Posted by Anton Johansson via 5 months, 2 weeks ago
On 24/01/24, Richard Henderson wrote:
> On 1/20/24 00:40, Anton Johansson wrote:
> > Makes translate-all.c independent of softmmu target by switching
> > 
> >      TARGET_LONG_BITS        -> target_long_bits()
> > 
> >      TARGET_INSN_START_WORDS -> tcg_ctx->insn_start_words,
> >                                 target_insn_start_words(),
> > 
> >      TCG_GUEST_DEFAULT_MO    -> target_default_memory_order()
> > 
> >      MO_TE                   -> target_endian_memory_order()
> > 
> > Signed-off-by: Anton Johansson <anjo@rev.ng>
> > ---
> >   accel/tcg/translate-all.c | 38 ++++++++++++++++++--------------------
> >   1 file changed, 18 insertions(+), 20 deletions(-)
> > 
> > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> > index 9c981d1750..a3ae0c6910 100644
> > --- a/accel/tcg/translate-all.c
> > +++ b/accel/tcg/translate-all.c
> > @@ -65,7 +65,6 @@
> >   #include "internal-common.h"
> >   #include "internal-target.h"
> >   #include "perf.h"
> > -#include "tcg/insn-start-words.h"
> >   TBContext tb_ctx;
> > @@ -106,7 +105,7 @@ static int64_t decode_sleb128(const uint8_t **pp)
> >           val |= (int64_t)(byte & 0x7f) << shift;
> >           shift += 7;
> >       } while (byte & 0x80);
> > -    if (shift < TARGET_LONG_BITS && (byte & 0x40)) {
> > +    if (shift < target_long_bits() && (byte & 0x40)) {
> 
> You just make TARGET_PAGE_SIZE etc target independent, right?
> So you don't need this?  Or is this because of user-only still.

Hi, Richard!

I missed this piece of feedback previously.  I don't see how
TARGET_PAGE_[SIZE|BITS] would be used here.  Are the values we're
encoding always TARGET_PAGE_BITS in size?

//Anton
Re: [RFC PATCH 27/34] accel/tcg: Make translate-all.c target independent
Posted by Richard Henderson 5 months, 1 week ago
On 6/13/24 02:50, Anton Johansson wrote:
> On 24/01/24, Richard Henderson wrote:
>> On 1/20/24 00:40, Anton Johansson wrote:
>>> Makes translate-all.c independent of softmmu target by switching
>>>
>>>       TARGET_LONG_BITS        -> target_long_bits()
>>>
>>>       TARGET_INSN_START_WORDS -> tcg_ctx->insn_start_words,
>>>                                  target_insn_start_words(),
>>>
>>>       TCG_GUEST_DEFAULT_MO    -> target_default_memory_order()
>>>
>>>       MO_TE                   -> target_endian_memory_order()
>>>
>>> Signed-off-by: Anton Johansson <anjo@rev.ng>
>>> ---
>>>    accel/tcg/translate-all.c | 38 ++++++++++++++++++--------------------
>>>    1 file changed, 18 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
>>> index 9c981d1750..a3ae0c6910 100644
>>> --- a/accel/tcg/translate-all.c
>>> +++ b/accel/tcg/translate-all.c
>>> @@ -65,7 +65,6 @@
>>>    #include "internal-common.h"
>>>    #include "internal-target.h"
>>>    #include "perf.h"
>>> -#include "tcg/insn-start-words.h"
>>>    TBContext tb_ctx;
>>> @@ -106,7 +105,7 @@ static int64_t decode_sleb128(const uint8_t **pp)
>>>            val |= (int64_t)(byte & 0x7f) << shift;
>>>            shift += 7;
>>>        } while (byte & 0x80);
>>> -    if (shift < TARGET_LONG_BITS && (byte & 0x40)) {
>>> +    if (shift < target_long_bits() && (byte & 0x40)) {
>>
>> You just make TARGET_PAGE_SIZE etc target independent, right?
>> So you don't need this?  Or is this because of user-only still.
> 
> Hi, Richard!
> 
> I missed this piece of feedback previously.  I don't see how
> TARGET_PAGE_[SIZE|BITS] would be used here.  Are the values we're
> encoding always TARGET_PAGE_BITS in size?

I was obviously tired here, since they're obviously unrelated.

However in this case I think TARGET_LONG_BITS is a red herring, and we should be using 
int64_t not target_long at all, and thus the shift count must be less than 64.


r~
Re: [RFC PATCH 27/34] accel/tcg: Make translate-all.c target independent
Posted by Anton Johansson via 5 months, 1 week ago
On 18/06/24, Richard Henderson wrote:
> On 6/13/24 02:50, Anton Johansson wrote:
> > On 24/01/24, Richard Henderson wrote:
> > > On 1/20/24 00:40, Anton Johansson wrote:
> > > > Makes translate-all.c independent of softmmu target by switching
> > > > 
> > > >       TARGET_LONG_BITS        -> target_long_bits()
> > > > 
> > > >       TARGET_INSN_START_WORDS -> tcg_ctx->insn_start_words,
> > > >                                  target_insn_start_words(),
> > > > 
> > > >       TCG_GUEST_DEFAULT_MO    -> target_default_memory_order()
> > > > 
> > > >       MO_TE                   -> target_endian_memory_order()
> > > > 
> > > > Signed-off-by: Anton Johansson <anjo@rev.ng>
> > > > ---
> > > >    accel/tcg/translate-all.c | 38 ++++++++++++++++++--------------------
> > > >    1 file changed, 18 insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> > > > index 9c981d1750..a3ae0c6910 100644
> > > > --- a/accel/tcg/translate-all.c
> > > > +++ b/accel/tcg/translate-all.c
> > > > @@ -65,7 +65,6 @@
> > > >    #include "internal-common.h"
> > > >    #include "internal-target.h"
> > > >    #include "perf.h"
> > > > -#include "tcg/insn-start-words.h"
> > > >    TBContext tb_ctx;
> > > > @@ -106,7 +105,7 @@ static int64_t decode_sleb128(const uint8_t **pp)
> > > >            val |= (int64_t)(byte & 0x7f) << shift;
> > > >            shift += 7;
> > > >        } while (byte & 0x80);
> > > > -    if (shift < TARGET_LONG_BITS && (byte & 0x40)) {
> > > > +    if (shift < target_long_bits() && (byte & 0x40)) {
> > > 
> > > You just make TARGET_PAGE_SIZE etc target independent, right?
> > > So you don't need this?  Or is this because of user-only still.
> > 
> > Hi, Richard!
> > 
> > I missed this piece of feedback previously.  I don't see how
> > TARGET_PAGE_[SIZE|BITS] would be used here.  Are the values we're
> > encoding always TARGET_PAGE_BITS in size?
> 
> I was obviously tired here, since they're obviously unrelated.
> 
> However in this case I think TARGET_LONG_BITS is a red herring, and we
> should be using int64_t not target_long at all, and thus the shift count
> must be less than 64.
> 
> 
> r~
> 

Ah I see, thanks!:) I'll give that a go then.

-- 
Anton Johansson
rev.ng Labs Srl.