Re: [RFC PATCH v7 07/29] target/loongarch: Add LoongArch CSR instruction

[yangxiaojuan]

On 2022/3/29 上午2:34, Richard Henderson wrote:

> > +target_ulong
> >         helper_csr_rdq(CPULoongArchState *env, uint64_t csr)
> >         
> >         +{
> >         
> >         +    LoongArchCPU *cpu;
> >         
> >         +    int64_t v;
> >         
> >         +
> >         
> >         +    switch (csr) {
> >         
> >         +    case LOONGARCH_CSR_PGD:
> >         
> >         +        if (env->CSR_TLBRERA & 0x1) {
> >         
> >         +            v = env->CSR_TLBRBADV;
> >         
> >         +        } else {
> >         
> >         +            v = env->CSR_BADV;
> >         
> >         +        }
> >         
> >         +
> >         
> >         +        if ((v >> 63) & 0x1) {
> >         
> >         +            v = env->CSR_PGDH;
> >         
> >         +        } else {
> >         
> >         +            v = env->CSR_PGDL;
> >         
> >         +        }
> >         
> >         +        break;
> >         
> >         +    case LOONGARCH_CSR_CPUID:
> >         
> >         +        v = (env_cpu(env))->cpu_index;
> >         
> >         +        break;
> >         
> >         +    case LOONGARCH_CSR_TVAL:
> >         
> >         +        cpu = LOONGARCH_CPU(env_cpu(env));
> >         
> >         +        v = cpu_loongarch_get_constant_timer_ticks(cpu);
> >         
> >         +        break;
> >         
> >         +    default:
> >         
> >         +        break;
> >         
> >         +    }
> >         
> >         +
> >         
> >         +    return v;
> >         
> >         +}
> >         
> >       
>       
>       You should have seen a compiler warning for 'v' uninitialized
>       here, via the default path.
>       
>       
>       The default path should not be reachable, because of code in
>       trans_csrrd, and so could be written as g_assert_not_reachable(). 
>       However, I strongly suggest you split this function so that you do
>       not need a switch here at all.  With CPUID now handled via
>       cpu_csr_offset, there are only two helpers needed.
>     
    trans_csrrd {
    ...
        switch(a->csr) {
        case LOONGARCH_CSR_PGD:
            gen_helper_csrrd_pgd();
            break;
        case LOONGARCH_CSR_TVAL:
            gen_helper_csrrd_tval();
            break;
        case LOONGARCH_CSR_CPUID:
            ...
        default:
            ...
        }
    }
    And the same in trans_csrwr, is this right?
    
    Thanks.
    Xiaojuan