Re: [PATCH 08/31] target/loongarch: Add tlb instruction support

[Richard Henderson <richard.henderson@linaro.org>]

On 10/19/21 12:34 AM, Xiaojuan Yang wrote:
> This includes:
> - TLBSRCH
> - TLBRD
> - TLBWR
> - TLBFILL
> - TLBCLR
> - TLBFLUSH
> - INVTLB
> 
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> ---
>   target/loongarch/cpu.c                   |  19 +
>   target/loongarch/helper.h                |   8 +
>   target/loongarch/insn_trans/trans_core.c |  54 +++
>   target/loongarch/insns.decode            |  14 +
>   target/loongarch/internals.h             |  18 +
>   target/loongarch/tlb_helper.c            | 468 +++++++++++++++++++++++
>   6 files changed, 581 insertions(+)
> 
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index f145afb603..afd186abac 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -118,6 +118,7 @@ static void set_loongarch_cpucfg(CPULoongArchState *env)
>   static void set_loongarch_csr(CPULoongArchState *env)
>   {
>       uint64_t t;
> +    CPUState *cs = env_cpu(env);
>   
>       t = FIELD_DP64(0, CSR_PRCFG1, SAVE_NUM, 8);
>       t = FIELD_DP64(t, CSR_PRCFG1, TIMER_BITS, 0x2f);
> @@ -145,6 +146,9 @@ static void set_loongarch_csr(CPULoongArchState *env)
>       env->CSR_RVACFG = 0x0;
>       env->CSR_ASID = 0xa0000;
>       env->CSR_ERA = env->pc;
> +    env->CSR_CPUID = (cs->cpu_index & 0x1ff);

Any reason to have a copy of cpu_index, as opposed to just using that field?  CSR_CPUID is 
read-only after all.

> +    env->CSR_EENTRY |= (uint64_t)0x80000000;
> +    env->CSR_TLBRENTRY |= (uint64_t)0x80000000;

Are there really a defined reset values?  The documentation doesn't say.  It would appear 
that the kernel must set these before enabling interrupts or turning on paging.

> +#ifndef CONFIG_USER_ONLY
> +    qemu_fprintf(f, "EUEN            0x%lx\n", env->CSR_EUEN);
> +    qemu_fprintf(f, "ESTAT           0x%lx\n", env->CSR_ESTAT);
> +    qemu_fprintf(f, "ERA             0x%lx\n", env->CSR_ERA);
> +    qemu_fprintf(f, "CRMD            0x%lx\n", env->CSR_CRMD);
> +    qemu_fprintf(f, "PRMD            0x%lx\n", env->CSR_PRMD);
> +    qemu_fprintf(f, "BadVAddr        0x%lx\n", env->CSR_BADV);
> +    qemu_fprintf(f, "TLB refill ERA  0x%lx\n", env->CSR_TLBRERA);
> +    qemu_fprintf(f, "TLB refill BadV 0x%lx\n", env->CSR_TLBRBADV);
> +    qemu_fprintf(f, "EENTRY            0x%lx\n", env->CSR_EENTRY);
> +    qemu_fprintf(f, "BadInstr        0x%lx\n", env->CSR_BADI);
> +    qemu_fprintf(f, "PRCFG1    0x%lx\nPRCFG2     0x%lx\nPRCFG3     0x%lx\n",
> +                 env->CSR_PRCFG1, env->CSR_PRCFG3, env->CSR_PRCFG3);
> +#endif

This probably belongs to a different patch?

> @@ -165,4 +172,51 @@ static bool trans_iocsrwr_d(DisasContext *ctx, arg_iocsrwr_d *a)
>       gen_helper_iocsr_write(cpu_env, addr, val, tcg_constant_i32(oi));
>       return true;
>   }
> +
> +static bool trans_tlbsrch(DisasContext *ctx, arg_tlbsrch *a)
> +{
> +    gen_helper_tlbsrch(cpu_env);
> +    return true;
> +}

Missing priv check, all functions.

> +static bool trans_invtlb(DisasContext *ctx, arg_invtlb *a)
> +{
> +    TCGv addr = gpr_src(ctx, a->addr, EXT_NONE);
> +    TCGv info = gpr_src(ctx, a->info, EXT_NONE);
> +    TCGv op = tcg_constant_tl(a->invop);
> +
> +    gen_helper_invtlb(cpu_env, addr, info, op);
> +    return true;
> +}

Decode op here -- there are only 7 defined opcodes.

Note that you'll need to end the TB after most TLB instructions, since the translation of 
PC could change between one insn and the next.


> +&fmt_invtlb         addr info invop
> +@fmt_invtlb          ...... ...... ..... ..... ..... .....    &fmt_invtlb         %addr %info %invop

Why are you using the names addr and info instead of rk and rj?

> diff --git a/target/loongarch/internals.h b/target/loongarch/internals.h
> index 1251e7f21c..916c675680 100644
> --- a/target/loongarch/internals.h
> +++ b/target/loongarch/internals.h
> @@ -76,6 +76,14 @@ struct CPULoongArchTLBContext {
>       int (*map_address)(struct CPULoongArchState *env, hwaddr *physical,
>                          int *prot, target_ulong address,
>                          MMUAccessType access_type);
> +    void (*helper_tlbwr)(struct CPULoongArchState *env);
> +    void (*helper_tlbfill)(struct CPULoongArchState *env);
> +    void (*helper_tlbsrch)(struct CPULoongArchState *env);
> +    void (*helper_tlbrd)(struct CPULoongArchState *env);
> +    void (*helper_tlbclr)(struct CPULoongArchState *env);
> +    void (*helper_tlbflush)(struct CPULoongArchState *env);
> +    void (*helper_invtlb)(struct CPULoongArchState *env, target_ulong addr,
> +                          target_ulong info, int op);

Again, function pointers are premature.

> +static uint64_t ls3a5k_pagesize_to_mask(int pagesize)
> +{
> +    /* 4KB - 1GB */
> +    if (pagesize < 12 && pagesize > 30) {
> +        qemu_log_mask(CPU_LOG_MMU, "unsupported page size %d\n", pagesize);
> +        exit(-1);

Do not call exit.  Make up something sensible that won't crash qemu.

> +/* return random value in [low, high] */
> +static uint32_t cpu_loongarch_get_random_ls3a5k_tlb(uint32_t low, uint32_t high)
> +{
> +    static uint32_t seed = 5;
> +    static uint32_t prev_idx;

No static variables like this, as they cannot be migrated, and are a race condition 
between multiple cpus.  That said...

> +    uint32_t idx;
> +    uint32_t nb_rand_tlb = high - low + 1;
> +
> +    do {
> +        seed = 1103515245 * seed + 12345;
> +        idx = (seed >> 16) % nb_rand_tlb + low;
> +    } while (idx == prev_idx);

... we have defined interfaces for getting random numbers.


r~