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 <yangxiaoj...@loongson.cn>
>> Signed-off-by: Song Gao <gaos...@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~
