Re: [PATCH v6 15/18] include/hw/core: Create struct CPUJumpCache

["Alex Bennée" <alex.bennee@linaro.org>]

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>
> ---
>  accel/tcg/tb-hash.h       |  1 +
>  accel/tcg/tb-jmp-cache.h  | 24 ++++++++++++++++++++++++
>  include/exec/cpu-common.h |  1 +
>  include/hw/core/cpu.h     | 15 +--------------
>  include/qemu/typedefs.h   |  1 +
>  accel/tcg/cpu-exec.c      | 10 +++++++---
>  accel/tcg/cputlb.c        |  9 +++++----
>  accel/tcg/translate-all.c | 28 +++++++++++++++++++++++++---
>  hw/core/cpu-common.c      |  3 +--
>  plugins/core.c            |  2 +-
>  trace/control-target.c    |  2 +-
>  11 files changed, 68 insertions(+), 28 deletions(-)
>  create mode 100644 accel/tcg/tb-jmp-cache.h
>
> diff --git a/accel/tcg/tb-hash.h b/accel/tcg/tb-hash.h
> index 0a273d9605..83dc610e4c 100644
> --- a/accel/tcg/tb-hash.h
> +++ b/accel/tcg/tb-hash.h
> @@ -23,6 +23,7 @@
>  #include "exec/cpu-defs.h"
>  #include "exec/exec-all.h"
>  #include "qemu/xxhash.h"
> +#include "tb-jmp-cache.h"
>  
>  #ifdef CONFIG_SOFTMMU
>  
> diff --git a/accel/tcg/tb-jmp-cache.h b/accel/tcg/tb-jmp-cache.h
> new file mode 100644
> index 0000000000..2d8fbb1bfe
> --- /dev/null
> +++ b/accel/tcg/tb-jmp-cache.h
> @@ -0,0 +1,24 @@
> +/*
> + * The per-CPU TranslationBlock jump cache.
> + *
> + *  Copyright (c) 2003 Fabrice Bellard
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef ACCEL_TCG_TB_JMP_CACHE_H
> +#define ACCEL_TCG_TB_JMP_CACHE_H
> +
> +#define TB_JMP_CACHE_BITS 12
> +#define TB_JMP_CACHE_SIZE (1 << TB_JMP_CACHE_BITS)
> +
> +/*
> + * Accessed in parallel; all accesses to 'tb' must be atomic.
> + */
> +struct CPUJumpCache {
> +    struct {
> +        TranslationBlock *tb;
> +    } array[TB_JMP_CACHE_SIZE];
> +};
> +
> +#endif /* ACCEL_TCG_TB_JMP_CACHE_H */

When I saw this I wondered if...

> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index d909429427..c493510ee9 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -38,6 +38,7 @@ void cpu_list_unlock(void);
>  unsigned int cpu_list_generation_id_get(void);
>  
>  void tcg_flush_softmmu_tlb(CPUState *cs);
> +void tcg_flush_jmp_cache(CPUState *cs);

this helper and ....

<snip>
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 3a63113c41..63ecc15236 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
<snip>

.. this one should be moved into accel/tcg/tb-jmp-cache.c so we can keep
all the jmp cache stuff nicely contained (and cut down the grab bag of
content to translate-all a bit)?

>  
> +/*
> + * Called by generic code at e.g. cpu reset after cpu creation,
> + * therefore we must be prepared to allocate the jump cache.
> + */
> +void tcg_flush_jmp_cache(CPUState *cpu)
> +{
> +    CPUJumpCache *jc = cpu->tb_jmp_cache;
> +
> +    if (likely(jc)) {
> +        for (int i = 0; i < TB_JMP_CACHE_SIZE; i++) {
> +            qatomic_set(&jc->array[i].tb, NULL);
> +        }
> +    } else {
> +        /* This should happen once during realize, and thus never race. */
> +        jc = g_new0(CPUJumpCache, 1);
> +        jc = qatomic_xchg(&cpu->tb_jmp_cache, jc);
> +        assert(jc == NULL);
> +    }
> +}
> +
<snip>

Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


-- 
Alex Bennée