[RFC PATCH 0/2] accel/tcg: replace phys_pc with asid in TB htable key

[RFC PATCH 0/2] accel/tcg: replace phys_pc with asid in TB htable key

[Oleg Vasilev]

See last patch in series.

Oleg Vasilev (2):
  target/arm: move regime_ttbr helper
  accel/tcg: replace phys_pc with asid in TB htable key

 accel/tcg/cpu-exec.c             | 38 ++++++++------------------------
 accel/tcg/tb-hash.h              |  4 ++--
 accel/tcg/translate-all.c        |  7 +++---
 hw/core/cpu-sysemu.c             |  6 +++++
 include/exec/exec-all.h          |  1 +
 include/hw/core/cpu.h            |  8 +++++++
 include/hw/core/sysemu-cpu-ops.h |  6 +++++
 target/arm/cpu.c                 | 13 +++++++++++
 target/arm/helper.c              | 17 --------------
 target/arm/internals.h           | 17 ++++++++++++++
 10 files changed, 65 insertions(+), 52 deletions(-)

-- 
2.33.1

[RFC PATCH 1/2] target/arm: move regime_ttbr helper

[Oleg Vasilev]

From: Oleg Vasilev <vasilev.oleg@huawei.com>

Now it is available for other files to use.

CC: qemu-arm@nongnu.org
Signed-off-by: Oleg Vasilev <vasilev.oleg@huawei.com>
Signed-off-by: Oleg Vasilev <me@svin.in>
---
 target/arm/helper.c    | 17 -----------------
 target/arm/internals.h | 17 +++++++++++++++++
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index db837d53bd..747c94fd95 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10275,23 +10275,6 @@ static inline bool regime_translation_big_endian(CPUARMState *env,
     return (regime_sctlr(env, mmu_idx) & SCTLR_EE) != 0;
 }
 
-/* Return the TTBR associated with this translation regime */
-static inline uint64_t regime_ttbr(CPUARMState *env, ARMMMUIdx mmu_idx,
-                                   int ttbrn)
-{
-    if (mmu_idx == ARMMMUIdx_Stage2) {
-        return env->cp15.vttbr_el2;
-    }
-    if (mmu_idx == ARMMMUIdx_Stage2_S) {
-        return env->cp15.vsttbr_el2;
-    }
-    if (ttbrn == 0) {
-        return env->cp15.ttbr0_el[regime_el(env, mmu_idx)];
-    } else {
-        return env->cp15.ttbr1_el[regime_el(env, mmu_idx)];
-    }
-}
-
 #endif /* !CONFIG_USER_ONLY */
 
 /* Convert a possible stage1+2 MMU index into the appropriate
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 89f7610ebc..a288fe019b 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1132,6 +1132,23 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
 
 void arm_log_exception(int idx);
 
+/* Return the TTBR associated with this translation regime */
+static inline uint64_t regime_ttbr(CPUARMState *env, ARMMMUIdx mmu_idx,
+                                   int ttbrn)
+{
+    if (mmu_idx == ARMMMUIdx_Stage2) {
+        return env->cp15.vttbr_el2;
+    }
+    if (mmu_idx == ARMMMUIdx_Stage2_S) {
+        return env->cp15.vsttbr_el2;
+    }
+    if (ttbrn == 0) {
+        return env->cp15.ttbr0_el[regime_el(env, mmu_idx)];
+    } else {
+        return env->cp15.ttbr1_el[regime_el(env, mmu_idx)];
+    }
+}
+
 #endif /* !CONFIG_USER_ONLY */
 
 /*
-- 
2.33.1

[RFC PATCH 2/2] accel/tcg: replace phys_pc with asid in TB htable key

[Oleg Vasilev]

From: Oleg Vasilev <vasilev.oleg@huawei.com>

Using a physical pc requires to translate address every time next block
needs to be found and executed. This also contaminates TLB with code-related
records.

Instead, I suggest we introduce an architecture-specific address space
identifier, and use it to distinguish between different AS's
translation blocks.

CC: qemu-arm@nongnu.org
Signed-off-by: Oleg Vasilev <vasilev.oleg@huawei.com>
Signed-off-by: Oleg Vasilev <me@svin.in>
---
 accel/tcg/cpu-exec.c             | 38 ++++++++------------------------
 accel/tcg/tb-hash.h              |  4 ++--
 accel/tcg/translate-all.c        |  7 +++---
 hw/core/cpu-sysemu.c             |  6 +++++
 include/exec/exec-all.h          |  1 +
 include/hw/core/cpu.h            |  8 +++++++
 include/hw/core/sysemu-cpu-ops.h |  6 +++++
 target/arm/cpu.c                 | 13 +++++++++++
 8 files changed, 48 insertions(+), 35 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 409ec8c38c..01b0e67d9c 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -490,7 +490,7 @@ struct tb_desc {
     target_ulong pc;
     target_ulong cs_base;
     CPUArchState *env;
-    tb_page_addr_t phys_page1;
+    uint64_t asid;
     uint32_t flags;
     uint32_t cflags;
     uint32_t trace_vcpu_dstate;
@@ -501,34 +501,18 @@ static bool tb_lookup_cmp(const void *p, const void *d)
     const TranslationBlock *tb = p;
     const struct tb_desc *desc = d;
 
-    if (tb->pc == desc->pc &&
-        tb->page_addr[0] == desc->phys_page1 &&
-        tb->cs_base == desc->cs_base &&
-        tb->flags == desc->flags &&
-        tb->trace_vcpu_dstate == desc->trace_vcpu_dstate &&
-        tb_cflags(tb) == desc->cflags) {
-        /* check next page if needed */
-        if (tb->page_addr[1] == -1) {
-            return true;
-        } else {
-            tb_page_addr_t phys_page2;
-            target_ulong virt_page2;
-
-            virt_page2 = (desc->pc & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE;
-            phys_page2 = get_page_addr_code(desc->env, virt_page2);
-            if (tb->page_addr[1] == phys_page2) {
-                return true;
-            }
-        }
-    }
-    return false;
+    return (tb->pc == desc->pc &&
+            tb->asid == desc->asid &&
+            tb->cs_base == desc->cs_base &&
+            tb->flags == desc->flags &&
+            tb->trace_vcpu_dstate == desc->trace_vcpu_dstate &&
+            tb_cflags(tb) == desc->cflags);
 }
 
 TranslationBlock *tb_htable_lookup(CPUState *cpu, target_ulong pc,
                                    target_ulong cs_base, uint32_t flags,
                                    uint32_t cflags)
 {
-    tb_page_addr_t phys_pc;
     struct tb_desc desc;
     uint32_t h;
 
@@ -538,12 +522,8 @@ TranslationBlock *tb_htable_lookup(CPUState *cpu, target_ulong pc,
     desc.cflags = cflags;
     desc.trace_vcpu_dstate = *cpu->trace_dstate;
     desc.pc = pc;
-    phys_pc = get_page_addr_code(desc.env, pc);
-    if (phys_pc == -1) {
-        return NULL;
-    }
-    desc.phys_page1 = phys_pc & TARGET_PAGE_MASK;
-    h = tb_hash_func(phys_pc, pc, flags, cflags, *cpu->trace_dstate);
+    desc.asid = cpu_get_asid(cpu);
+    h = tb_hash_func(desc.asid, pc, flags, cflags, *cpu->trace_dstate);
     return qht_lookup_custom(&tb_ctx.htable, &desc, h, tb_lookup_cmp);
 }
 
diff --git a/accel/tcg/tb-hash.h b/accel/tcg/tb-hash.h
index 0a273d9605..b5c6f87711 100644
--- a/accel/tcg/tb-hash.h
+++ b/accel/tcg/tb-hash.h
@@ -60,10 +60,10 @@ static inline unsigned int tb_jmp_cache_hash_func(target_ulong pc)
 #endif /* CONFIG_SOFTMMU */
 
 static inline
-uint32_t tb_hash_func(tb_page_addr_t phys_pc, target_ulong pc, uint32_t flags,
+uint32_t tb_hash_func(uint64_t asid, target_ulong pc, uint32_t flags,
                       uint32_t cf_mask, uint32_t trace_vcpu_dstate)
 {
-    return qemu_xxhash7(phys_pc, pc, flags, cf_mask, trace_vcpu_dstate);
+    return qemu_xxhash7(asid, pc, flags, cf_mask, trace_vcpu_dstate);
 }
 
 #endif
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index bd71db59a9..8565691bfd 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1174,7 +1174,6 @@ static void do_tb_phys_invalidate(TranslationBlock *tb, bool rm_from_page_list)
     CPUState *cpu;
     PageDesc *p;
     uint32_t h;
-    tb_page_addr_t phys_pc;
     uint32_t orig_cflags = tb_cflags(tb);
 
     assert_memory_lock();
@@ -1185,8 +1184,7 @@ static void do_tb_phys_invalidate(TranslationBlock *tb, bool rm_from_page_list)
     qemu_spin_unlock(&tb->jmp_lock);
 
     /* remove the TB from the hash list */
-    phys_pc = tb->page_addr[0] + (tb->pc & ~TARGET_PAGE_MASK);
-    h = tb_hash_func(phys_pc, tb->pc, tb->flags, orig_cflags,
+    h = tb_hash_func(tb->asid, tb->pc, tb->flags, orig_cflags,
                      tb->trace_vcpu_dstate);
     if (!qht_remove(&tb_ctx.htable, tb, h)) {
         return;
@@ -1349,7 +1347,7 @@ tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
     }
 
     /* add in the hash table */
-    h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb->cflags,
+    h = tb_hash_func(tb->asid, tb->pc, tb->flags, tb->cflags,
                      tb->trace_vcpu_dstate);
     qht_insert(&tb_ctx.htable, tb, h, &existing_tb);
 
@@ -1427,6 +1425,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
     tb->flags = flags;
     tb->cflags = cflags;
     tb->trace_vcpu_dstate = *cpu->trace_dstate;
+    tb->asid = cpu_get_asid(cpu);
     tcg_ctx->tb_cflags = cflags;
  tb_overflow:
 
diff --git a/hw/core/cpu-sysemu.c b/hw/core/cpu-sysemu.c
index 00253f8929..915874ea7b 100644
--- a/hw/core/cpu-sysemu.c
+++ b/hw/core/cpu-sysemu.c
@@ -79,6 +79,12 @@ int cpu_asidx_from_attrs(CPUState *cpu, MemTxAttrs attrs)
     return ret;
 }
 
+uint64_t cpu_get_asid(CPUState *cpu) {
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    return cc->sysemu_ops->get_asid(cpu);
+}
+
 int cpu_write_elf32_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
                              void *opaque)
 {
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 35d8e93976..5554ab133d 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -560,6 +560,7 @@ struct TranslationBlock {
     uintptr_t jmp_list_head;
     uintptr_t jmp_list_next[2];
     uintptr_t jmp_dest[2];
+    uint64_t asid;
 };
 
 /* Hide the qatomic_read to make code a little easier on the eyes */
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index e948e81f1a..c935160153 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -579,6 +579,14 @@ hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
  */
 int cpu_asidx_from_attrs(CPUState *cpu, MemTxAttrs attrs);
 
+
+/** cpu_get_asid:
+ * @cpu: CPU
+ *
+ * Returns the identifier for a current address space.
+ */
+uint64_t cpu_get_asid(CPUState *cpu);
+
 /**
  * cpu_virtio_is_big_endian:
  * @cpu: CPU
diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
index a9ba39e5f2..919f941265 100644
--- a/include/hw/core/sysemu-cpu-ops.h
+++ b/include/hw/core/sysemu-cpu-ops.h
@@ -43,6 +43,12 @@ typedef struct SysemuCPUOps {
      *       a memory access with the specified memory transaction attributes.
      */
     int (*asidx_from_attrs)(CPUState *cpu, MemTxAttrs attrs);
+
+    /**
+     * @get_asid: Callback to return the identifier for a current address space.
+     */
+    uint64_t (*get_asid)(CPUState *cpu);
+
     /**
      * @get_crash_info: Callback for reporting guest crash information in
      * GUEST_PANICKED events.
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index a211804fd3..aa5440d960 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2017,9 +2017,22 @@ static gchar *arm_gdb_arch_name(CPUState *cs)
 #ifndef CONFIG_USER_ONLY
 #include "hw/core/sysemu-cpu-ops.h"
 
+/* Returns the identifier for a current address space. */
+static uint64_t arm_get_asid(CPUState *cs)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+    ARMMMUIdx mmu_idx = arm_mmu_idx(env);
+    uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
+
+#define TCR_A1     (1U << 22)
+    return regime_ttbr(env, mmu_idx, (tcr&TCR_A1)>0);
+}
+
 static const struct SysemuCPUOps arm_sysemu_ops = {
     .get_phys_page_attrs_debug = arm_cpu_get_phys_page_attrs_debug,
     .asidx_from_attrs = arm_asidx_from_attrs,
+    .get_asid = arm_get_asid,
     .write_elf32_note = arm_cpu_write_elf32_note,
     .write_elf64_note = arm_cpu_write_elf64_note,
     .virtio_is_big_endian = arm_cpu_virtio_is_big_endian,
-- 
2.33.1

Re: [RFC PATCH 2/2] accel/tcg: replace phys_pc with asid in TB htable key

[Richard Henderson]

On 12/22/21 8:50 AM, Oleg Vasilev wrote:
> From: Oleg Vasilev <vasilev.oleg@huawei.com>
> 
> Using a physical pc requires to translate address every time next block
> needs to be found and executed. This also contaminates TLB with code-related
> records.
> 
> Instead, I suggest we introduce an architecture-specific address space
> identifier, and use it to distinguish between different AS's
> translation blocks.

Why do you believe that asid is sufficient here?  You're not invalidating any more TBs 
that I can see.  What happens when the kernel re-uses an asid?

I believe this patch to be fundamentally flawed.

All that said,

> +/* Returns the identifier for a current address space. */
> +static uint64_t arm_get_asid(CPUState *cs)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    ARMMMUIdx mmu_idx = arm_mmu_idx(env);
> +    uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
> +
> +#define TCR_A1     (1U << 22)
> +    return regime_ttbr(env, mmu_idx, (tcr&TCR_A1)>0);
> +}

Why are you returning the entire ttbr, and not the asid in the top 16 bits?


r~

Re: [RFC PATCH 2/2] accel/tcg: replace phys_pc with asid in TB htable key

[Vasilev Oleg via]

On 12/23/2021 7:31 PM, Richard Henderson wrote:
> On 12/22/21 8:50 AM, Oleg Vasilev wrote:
>> From: Oleg Vasilev <vasilev.oleg@huawei.com>
>>
>> Using a physical pc requires to translate address every time next block
>> needs to be found and executed. This also contaminates TLB with code-related
>> records.
>>
>> Instead, I suggest we introduce an architecture-specific address space
>> identifier, and use it to distinguish between different AS's
>> translation blocks.
> 
> Why do you believe that asid is sufficient here?  You're not invalidating any more TBs 
> that I can see.  What happens when the kernel re-uses an asid?

Hi,

Sorry, I had some comments for the patch, but forgot to put it in.

So, I think I interpret the term "asid" in some other sense, namely, an
identifier, which is constant during whole lifespan of an address space.
Same as PID in that sense. Do you think this is a viable approach?

If we assume translation table wouldn't change during process life,
after the death of the process, all it address space would be anyway
unmapped and corresponding translation blocks would be invalidated.


> 
> I believe this patch to be fundamentally flawed.

Maybe it is, I just wanted to get feedback from you guys. Do you think
maybe exists some other way, which would not require translating va->pa
every time to look up next block?

More context is in:

 Subject: Suggestions for TCG performance improvements
 Date: Thu, 2 Dec 2021 09:47:13 +0000
 Message-ID: <c76bde31-8f3b-2d03-b7c7-9e026d4b5873@huawei.com>

> 
> All that said,
> 
>> +/* Returns the identifier for a current address space. */
>> +static uint64_t arm_get_asid(CPUState *cs)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(cs);
>> +    CPUARMState *env = &cpu->env;
>> +    ARMMMUIdx mmu_idx = arm_mmu_idx(env);
>> +    uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
>> +
>> +#define TCR_A1     (1U << 22)
>> +    return regime_ttbr(env, mmu_idx, (tcr&TCR_A1)>0);
>> +}
> 
> Why are you returning the entire ttbr, and not the asid in the top 16 bits?

Actually, for my particular case I seem to need to return the lowest 40
bits, which is actual base for TT.

Thanks,
Oleg

> 
> 
> r~
> 

Re: [RFC PATCH 2/2] accel/tcg: replace phys_pc with asid in TB htable key

[Richard Henderson]

On 12/24/21 5:02 AM, Vasilev Oleg wrote:
> On 12/23/2021 7:31 PM, Richard Henderson wrote:
>> On 12/22/21 8:50 AM, Oleg Vasilev wrote:
>>> From: Oleg Vasilev <vasilev.oleg@huawei.com>
>>>
>>> Using a physical pc requires to translate address every time next block
>>> needs to be found and executed. This also contaminates TLB with code-related
>>> records.
>>>
>>> Instead, I suggest we introduce an architecture-specific address space
>>> identifier, and use it to distinguish between different AS's
>>> translation blocks.
>>
>> Why do you believe that asid is sufficient here?  You're not invalidating any more TBs
>> that I can see.  What happens when the kernel re-uses an asid?
> 
> Hi,
> 
> Sorry, I had some comments for the patch, but forgot to put it in.
> 
> So, I think I interpret the term "asid" in some other sense, namely, an
> identifier, which is constant during whole lifespan of an address space.
> Same as PID in that sense. Do you think this is a viable approach?

No, I do not.

> If we assume translation table wouldn't change during process life,
> after the death of the process, all it address space would be anyway
> unmapped and corresponding translation blocks would be invalidated.

While this assumption is often true, it certainly isn't universally true.
Consider the cases of dlclose, followed by another dlopen; or any JIT.


r~

Re: [RFC PATCH 1/2] target/arm: move regime_ttbr helper

[Vasilev Oleg via]