Wrap the bare TranslationBlock pointer into a structure.
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
include/hw/core/cpu.h | 8 ++++++--
accel/tcg/cpu-exec.c | 9 ++++++---
accel/tcg/cputlb.c | 2 +-
accel/tcg/translate-all.c | 4 ++--
4 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 9e47184513..ee5b75dea0 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -232,6 +232,10 @@ struct hvf_vcpu_state;
#define TB_JMP_CACHE_BITS 12
#define TB_JMP_CACHE_SIZE (1 << TB_JMP_CACHE_BITS)
+typedef struct {
+ TranslationBlock *tb;
+} CPUJumpCache;
+
/* work queue */
/* The union type allows passing of 64 bit target pointers on 32 bit
@@ -361,7 +365,7 @@ struct CPUState {
IcountDecr *icount_decr_ptr;
/* Accessed in parallel; all accesses must be atomic */
- TranslationBlock *tb_jmp_cache[TB_JMP_CACHE_SIZE];
+ CPUJumpCache tb_jmp_cache[TB_JMP_CACHE_SIZE];
struct GDBRegisterState *gdb_regs;
int gdb_num_regs;
@@ -452,7 +456,7 @@ static inline void cpu_tb_jmp_cache_clear(CPUState *cpu)
unsigned int i;
for (i = 0; i < TB_JMP_CACHE_SIZE; i++) {
- qatomic_set(&cpu->tb_jmp_cache[i], NULL);
+ qatomic_set(&cpu->tb_jmp_cache[i].tb, NULL);
}
}
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index dd58a144a8..c6283d5798 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -252,7 +252,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc,
tcg_debug_assert(!(cflags & CF_INVALID));
hash = tb_jmp_cache_hash_func(pc);
- tb = qatomic_rcu_read(&cpu->tb_jmp_cache[hash]);
+ tb = qatomic_rcu_read(&cpu->tb_jmp_cache[hash].tb);
if (likely(tb &&
tb->pc == pc &&
@@ -266,7 +266,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc,
if (tb == NULL) {
return NULL;
}
- qatomic_set(&cpu->tb_jmp_cache[hash], tb);
+ qatomic_set(&cpu->tb_jmp_cache[hash].tb, tb);
return tb;
}
@@ -987,6 +987,8 @@ int cpu_exec(CPUState *cpu)
tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
if (tb == NULL) {
+ uint32_t h;
+
mmap_lock();
tb = tb_gen_code(cpu, pc, cs_base, flags, cflags);
mmap_unlock();
@@ -994,7 +996,8 @@ int cpu_exec(CPUState *cpu)
* We add the TB in the virtual pc hash table
* for the fast lookup
*/
- qatomic_set(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)], tb);
+ h = tb_jmp_cache_hash_func(pc);
+ qatomic_set(&cpu->tb_jmp_cache[h].tb, tb);
}
#ifndef CONFIG_USER_ONLY
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index f5e6ca2da2..fb8f3087f1 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -103,7 +103,7 @@ static void tb_jmp_cache_clear_page(CPUState *cpu, target_ulong page_addr)
unsigned int i, i0 = tb_jmp_cache_hash_page(page_addr);
for (i = 0; i < TB_JMP_PAGE_SIZE; i++) {
- qatomic_set(&cpu->tb_jmp_cache[i0 + i], NULL);
+ qatomic_set(&cpu->tb_jmp_cache[i0 + i].tb, NULL);
}
}
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index f429d33981..efa479ccf3 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1187,8 +1187,8 @@ static void do_tb_phys_invalidate(TranslationBlock *tb, bool rm_from_page_list)
/* remove the TB from the hash list */
h = tb_jmp_cache_hash_func(tb->pc);
CPU_FOREACH(cpu) {
- if (qatomic_read(&cpu->tb_jmp_cache[h]) == tb) {
- qatomic_set(&cpu->tb_jmp_cache[h], NULL);
+ if (qatomic_read(&cpu->tb_jmp_cache[h].tb) == tb) {
+ qatomic_set(&cpu->tb_jmp_cache[h].tb, NULL);
}
}
--
2.34.1
Richard Henderson <richard.henderson@linaro.org> writes:
> Wrap the bare TranslationBlock pointer into a structure.
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> include/hw/core/cpu.h | 8 ++++++--
> accel/tcg/cpu-exec.c | 9 ++++++---
> accel/tcg/cputlb.c | 2 +-
> accel/tcg/translate-all.c | 4 ++--
> 4 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 9e47184513..ee5b75dea0 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -232,6 +232,10 @@ struct hvf_vcpu_state;
> #define TB_JMP_CACHE_BITS 12
> #define TB_JMP_CACHE_SIZE (1 << TB_JMP_CACHE_BITS)
>
> +typedef struct {
> + TranslationBlock *tb;
> +} CPUJumpCache;
> +
I don't quite follow whats going on here. I see we add vaddr pc in a
later patch but I don't quite see why a cache for looking up TBs gets a
sidechan value added later.
Is this because the vaddr will no longer match the tb->pc? Maybe a
comment on the structure is needed?
> /* work queue */
>
> /* The union type allows passing of 64 bit target pointers on 32 bit
> @@ -361,7 +365,7 @@ struct CPUState {
> IcountDecr *icount_decr_ptr;
>
> /* Accessed in parallel; all accesses must be atomic */
> - TranslationBlock *tb_jmp_cache[TB_JMP_CACHE_SIZE];
> + CPUJumpCache tb_jmp_cache[TB_JMP_CACHE_SIZE];
>
> struct GDBRegisterState *gdb_regs;
> int gdb_num_regs;
> @@ -452,7 +456,7 @@ static inline void cpu_tb_jmp_cache_clear(CPUState *cpu)
> unsigned int i;
>
> for (i = 0; i < TB_JMP_CACHE_SIZE; i++) {
> - qatomic_set(&cpu->tb_jmp_cache[i], NULL);
> + qatomic_set(&cpu->tb_jmp_cache[i].tb, NULL);
> }
> }
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index dd58a144a8..c6283d5798 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -252,7 +252,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc,
> tcg_debug_assert(!(cflags & CF_INVALID));
>
> hash = tb_jmp_cache_hash_func(pc);
> - tb = qatomic_rcu_read(&cpu->tb_jmp_cache[hash]);
> + tb = qatomic_rcu_read(&cpu->tb_jmp_cache[hash].tb);
>
> if (likely(tb &&
> tb->pc == pc &&
> @@ -266,7 +266,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc,
> if (tb == NULL) {
> return NULL;
> }
> - qatomic_set(&cpu->tb_jmp_cache[hash], tb);
> + qatomic_set(&cpu->tb_jmp_cache[hash].tb, tb);
> return tb;
> }
>
> @@ -987,6 +987,8 @@ int cpu_exec(CPUState *cpu)
>
> tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
> if (tb == NULL) {
> + uint32_t h;
> +
> mmap_lock();
> tb = tb_gen_code(cpu, pc, cs_base, flags, cflags);
> mmap_unlock();
> @@ -994,7 +996,8 @@ int cpu_exec(CPUState *cpu)
> * We add the TB in the virtual pc hash table
> * for the fast lookup
> */
> - qatomic_set(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)], tb);
> + h = tb_jmp_cache_hash_func(pc);
> + qatomic_set(&cpu->tb_jmp_cache[h].tb, tb);
> }
>
> #ifndef CONFIG_USER_ONLY
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index f5e6ca2da2..fb8f3087f1 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -103,7 +103,7 @@ static void tb_jmp_cache_clear_page(CPUState *cpu, target_ulong page_addr)
> unsigned int i, i0 = tb_jmp_cache_hash_page(page_addr);
>
> for (i = 0; i < TB_JMP_PAGE_SIZE; i++) {
> - qatomic_set(&cpu->tb_jmp_cache[i0 + i], NULL);
> + qatomic_set(&cpu->tb_jmp_cache[i0 + i].tb, NULL);
> }
> }
>
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index f429d33981..efa479ccf3 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -1187,8 +1187,8 @@ static void do_tb_phys_invalidate(TranslationBlock *tb, bool rm_from_page_list)
> /* remove the TB from the hash list */
> h = tb_jmp_cache_hash_func(tb->pc);
> CPU_FOREACH(cpu) {
> - if (qatomic_read(&cpu->tb_jmp_cache[h]) == tb) {
> - qatomic_set(&cpu->tb_jmp_cache[h], NULL);
> + if (qatomic_read(&cpu->tb_jmp_cache[h].tb) == tb) {
> + qatomic_set(&cpu->tb_jmp_cache[h].tb, NULL);
> }
> }
--
Alex Bennée
On 9/29/22 06:46, Alex Bennée wrote:
>
> Richard Henderson <richard.henderson@linaro.org> writes:
>
>> Wrap the bare TranslationBlock pointer into a structure.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> include/hw/core/cpu.h | 8 ++++++--
>> accel/tcg/cpu-exec.c | 9 ++++++---
>> accel/tcg/cputlb.c | 2 +-
>> accel/tcg/translate-all.c | 4 ++--
>> 4 files changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>> index 9e47184513..ee5b75dea0 100644
>> --- a/include/hw/core/cpu.h
>> +++ b/include/hw/core/cpu.h
>> @@ -232,6 +232,10 @@ struct hvf_vcpu_state;
>> #define TB_JMP_CACHE_BITS 12
>> #define TB_JMP_CACHE_SIZE (1 << TB_JMP_CACHE_BITS)
>>
>> +typedef struct {
>> + TranslationBlock *tb;
>> +} CPUJumpCache;
>> +
>
> I don't quite follow whats going on here. I see we add vaddr pc in a
> later patch but I don't quite see why a cache for looking up TBs gets a
> sidechan value added later.
>
> Is this because the vaddr will no longer match the tb->pc? Maybe a
> comment on the structure is needed?
Correct, there will be no tb->pc, so the cpu has to remember the virtual address itself.
This patch only wraps the current pointer into a structure.
r~
>
>> /* work queue */
>>
>> /* The union type allows passing of 64 bit target pointers on 32 bit
>> @@ -361,7 +365,7 @@ struct CPUState {
>> IcountDecr *icount_decr_ptr;
>>
>> /* Accessed in parallel; all accesses must be atomic */
>> - TranslationBlock *tb_jmp_cache[TB_JMP_CACHE_SIZE];
>> + CPUJumpCache tb_jmp_cache[TB_JMP_CACHE_SIZE];
>>
>> struct GDBRegisterState *gdb_regs;
>> int gdb_num_regs;
>> @@ -452,7 +456,7 @@ static inline void cpu_tb_jmp_cache_clear(CPUState *cpu)
>> unsigned int i;
>>
>> for (i = 0; i < TB_JMP_CACHE_SIZE; i++) {
>> - qatomic_set(&cpu->tb_jmp_cache[i], NULL);
>> + qatomic_set(&cpu->tb_jmp_cache[i].tb, NULL);
>> }
>> }
>>
>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>> index dd58a144a8..c6283d5798 100644
>> --- a/accel/tcg/cpu-exec.c
>> +++ b/accel/tcg/cpu-exec.c
>> @@ -252,7 +252,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc,
>> tcg_debug_assert(!(cflags & CF_INVALID));
>>
>> hash = tb_jmp_cache_hash_func(pc);
>> - tb = qatomic_rcu_read(&cpu->tb_jmp_cache[hash]);
>> + tb = qatomic_rcu_read(&cpu->tb_jmp_cache[hash].tb);
>>
>> if (likely(tb &&
>> tb->pc == pc &&
>> @@ -266,7 +266,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc,
>> if (tb == NULL) {
>> return NULL;
>> }
>> - qatomic_set(&cpu->tb_jmp_cache[hash], tb);
>> + qatomic_set(&cpu->tb_jmp_cache[hash].tb, tb);
>> return tb;
>> }
>>
>> @@ -987,6 +987,8 @@ int cpu_exec(CPUState *cpu)
>>
>> tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
>> if (tb == NULL) {
>> + uint32_t h;
>> +
>> mmap_lock();
>> tb = tb_gen_code(cpu, pc, cs_base, flags, cflags);
>> mmap_unlock();
>> @@ -994,7 +996,8 @@ int cpu_exec(CPUState *cpu)
>> * We add the TB in the virtual pc hash table
>> * for the fast lookup
>> */
>> - qatomic_set(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)], tb);
>> + h = tb_jmp_cache_hash_func(pc);
>> + qatomic_set(&cpu->tb_jmp_cache[h].tb, tb);
>> }
>>
>> #ifndef CONFIG_USER_ONLY
>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
>> index f5e6ca2da2..fb8f3087f1 100644
>> --- a/accel/tcg/cputlb.c
>> +++ b/accel/tcg/cputlb.c
>> @@ -103,7 +103,7 @@ static void tb_jmp_cache_clear_page(CPUState *cpu, target_ulong page_addr)
>> unsigned int i, i0 = tb_jmp_cache_hash_page(page_addr);
>>
>> for (i = 0; i < TB_JMP_PAGE_SIZE; i++) {
>> - qatomic_set(&cpu->tb_jmp_cache[i0 + i], NULL);
>> + qatomic_set(&cpu->tb_jmp_cache[i0 + i].tb, NULL);
>> }
>> }
>>
>> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
>> index f429d33981..efa479ccf3 100644
>> --- a/accel/tcg/translate-all.c
>> +++ b/accel/tcg/translate-all.c
>> @@ -1187,8 +1187,8 @@ static void do_tb_phys_invalidate(TranslationBlock *tb, bool rm_from_page_list)
>> /* remove the TB from the hash list */
>> h = tb_jmp_cache_hash_func(tb->pc);
>> CPU_FOREACH(cpu) {
>> - if (qatomic_read(&cpu->tb_jmp_cache[h]) == tb) {
>> - qatomic_set(&cpu->tb_jmp_cache[h], NULL);
>> + if (qatomic_read(&cpu->tb_jmp_cache[h].tb) == tb) {
>> + qatomic_set(&cpu->tb_jmp_cache[h].tb, NULL);
>> }
>> }
>
>
Richard Henderson <richard.henderson@linaro.org> writes:
> On 9/29/22 06:46, Alex Bennée wrote:
>> Richard Henderson <richard.henderson@linaro.org> writes:
>>
>>> Wrap the bare TranslationBlock pointer into a structure.
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>> include/hw/core/cpu.h | 8 ++++++--
>>> accel/tcg/cpu-exec.c | 9 ++++++---
>>> accel/tcg/cputlb.c | 2 +-
>>> accel/tcg/translate-all.c | 4 ++--
>>> 4 files changed, 15 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>>> index 9e47184513..ee5b75dea0 100644
>>> --- a/include/hw/core/cpu.h
>>> +++ b/include/hw/core/cpu.h
>>> @@ -232,6 +232,10 @@ struct hvf_vcpu_state;
>>> #define TB_JMP_CACHE_BITS 12
>>> #define TB_JMP_CACHE_SIZE (1 << TB_JMP_CACHE_BITS)
>>> +typedef struct {
>>> + TranslationBlock *tb;
>>> +} CPUJumpCache;
>>> +
>> I don't quite follow whats going on here. I see we add vaddr pc in a
>> later patch but I don't quite see why a cache for looking up TBs gets a
>> sidechan value added later.
>> Is this because the vaddr will no longer match the tb->pc? Maybe a
>> comment on the structure is needed?
>
> Correct, there will be no tb->pc, so the cpu has to remember the virtual address itself.
>
> This patch only wraps the current pointer into a structure.
Sure - however we don't expand the comment when vaddr is added later.
I'm also concerned that now we have two fields there is scope for skew.
I guess the acquire/release semantics are to ensure we may fail safe but
never get a false positive?
>
>
> r~
>
>>
>>> /* work queue */
>>> /* The union type allows passing of 64 bit target pointers on
>>> 32 bit
>>> @@ -361,7 +365,7 @@ struct CPUState {
>>> IcountDecr *icount_decr_ptr;
>>> /* Accessed in parallel; all accesses must be atomic */
>>> - TranslationBlock *tb_jmp_cache[TB_JMP_CACHE_SIZE];
>>> + CPUJumpCache tb_jmp_cache[TB_JMP_CACHE_SIZE];
>>> struct GDBRegisterState *gdb_regs;
>>> int gdb_num_regs;
>>> @@ -452,7 +456,7 @@ static inline void cpu_tb_jmp_cache_clear(CPUState *cpu)
>>> unsigned int i;
>>> for (i = 0; i < TB_JMP_CACHE_SIZE; i++) {
>>> - qatomic_set(&cpu->tb_jmp_cache[i], NULL);
>>> + qatomic_set(&cpu->tb_jmp_cache[i].tb, NULL);
>>> }
>>> }
>>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>>> index dd58a144a8..c6283d5798 100644
>>> --- a/accel/tcg/cpu-exec.c
>>> +++ b/accel/tcg/cpu-exec.c
>>> @@ -252,7 +252,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc,
>>> tcg_debug_assert(!(cflags & CF_INVALID));
>>> hash = tb_jmp_cache_hash_func(pc);
>>> - tb = qatomic_rcu_read(&cpu->tb_jmp_cache[hash]);
>>> + tb = qatomic_rcu_read(&cpu->tb_jmp_cache[hash].tb);
>>> if (likely(tb &&
>>> tb->pc == pc &&
>>> @@ -266,7 +266,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc,
>>> if (tb == NULL) {
>>> return NULL;
>>> }
>>> - qatomic_set(&cpu->tb_jmp_cache[hash], tb);
>>> + qatomic_set(&cpu->tb_jmp_cache[hash].tb, tb);
>>> return tb;
>>> }
>>> @@ -987,6 +987,8 @@ int cpu_exec(CPUState *cpu)
>>> tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
>>> if (tb == NULL) {
>>> + uint32_t h;
>>> +
>>> mmap_lock();
>>> tb = tb_gen_code(cpu, pc, cs_base, flags, cflags);
>>> mmap_unlock();
>>> @@ -994,7 +996,8 @@ int cpu_exec(CPUState *cpu)
>>> * We add the TB in the virtual pc hash table
>>> * for the fast lookup
>>> */
>>> - qatomic_set(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)], tb);
>>> + h = tb_jmp_cache_hash_func(pc);
>>> + qatomic_set(&cpu->tb_jmp_cache[h].tb, tb);
>>> }
>>> #ifndef CONFIG_USER_ONLY
>>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
>>> index f5e6ca2da2..fb8f3087f1 100644
>>> --- a/accel/tcg/cputlb.c
>>> +++ b/accel/tcg/cputlb.c
>>> @@ -103,7 +103,7 @@ static void tb_jmp_cache_clear_page(CPUState *cpu, target_ulong page_addr)
>>> unsigned int i, i0 = tb_jmp_cache_hash_page(page_addr);
>>> for (i = 0; i < TB_JMP_PAGE_SIZE; i++) {
>>> - qatomic_set(&cpu->tb_jmp_cache[i0 + i], NULL);
>>> + qatomic_set(&cpu->tb_jmp_cache[i0 + i].tb, NULL);
>>> }
>>> }
>>> diff --git a/accel/tcg/translate-all.c
>>> b/accel/tcg/translate-all.c
>>> index f429d33981..efa479ccf3 100644
>>> --- a/accel/tcg/translate-all.c
>>> +++ b/accel/tcg/translate-all.c
>>> @@ -1187,8 +1187,8 @@ static void do_tb_phys_invalidate(TranslationBlock *tb, bool rm_from_page_list)
>>> /* remove the TB from the hash list */
>>> h = tb_jmp_cache_hash_func(tb->pc);
>>> CPU_FOREACH(cpu) {
>>> - if (qatomic_read(&cpu->tb_jmp_cache[h]) == tb) {
>>> - qatomic_set(&cpu->tb_jmp_cache[h], NULL);
>>> + if (qatomic_read(&cpu->tb_jmp_cache[h].tb) == tb) {
>>> + qatomic_set(&cpu->tb_jmp_cache[h].tb, NULL);
>>> }
>>> }
>>
--
Alex Bennée
© 2016 - 2026 Red Hat, Inc.