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

[yangxiaojuan]

Hi, Richard:

On 10/20/2021 12:19 PM, Richard Henderson wrote:
> 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.
> 
Yes, we need this value, the uefi code read this CPUID when Start slave cores.

>> +    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.
> 
OK, it can be removed.

>> +#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.
> 
>
Do you mean the qemu_guest_getrandom function? It gets random values that do 
not limit the range.
But I need a random in a fixed range, I cannot find the  Similar interface. 
Thanks. 

 
> r~