Re: [RFC PATCH v2 09/30] target/loongarch: Add TLB instruction support

[yangxiaojuan]

Hi, Richard:

On 11/12/2021 02:14 AM, Richard Henderson wrote:
> On 11/11/21 2:35 AM, Xiaojuan Yang wrote:
>> +static bool trans_tlbwr(DisasContext *ctx, arg_tlbwr *a)
>> +{
>> +    gen_helper_check_plv(cpu_env);
>> +    gen_helper_tlbwr(cpu_env);
>> +    tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next + 4);
>> +    ctx->base.is_jmp = DISAS_EXIT;
>> +    return true;
>> +}
> 
> I think you can skip the EXIT if paging is disabled, which it usually will be 
> in the software tlb handler.  You'd be able to tell with the mmu_idx being 
> the one you use for paging disabled.

The paging disabled only enabled at the bios startup, we can get the phys 
address directly, tlbwr instruction will not be used when paging enabled.

> 
>> +static void loongarch_invalidate_tlb_entry(CPULoongArchState *env,
>> +                                           loongarch_tlb *tlb)
>> +{
>> +    CPUState *cs = env_cpu(env);
>> +    target_ulong addr, end, mask;
>> +    int tlb_v0, tlb_v1;
>> +    uint64_t tlb_vppn;
>> +    uint8_t tlb_ps;
>> +
>> +    tlb_v0 = FIELD_EX64(tlb->tlb_entry0, ENTRY0, V);
>> +    tlb_v1 = FIELD_EX64(tlb->tlb_entry1, ENTRY1, V);
>> +    tlb_vppn = FIELD_EX64(tlb->tlb_misc, TLB_MISC, VPPN);
>> +    tlb_ps = FIELD_EX64(tlb->tlb_misc, TLB_MISC, PS);
>> +    mask = (1 << (1 + tlb_ps)) - 1;
> 
> MAKE_64BIT_MASK.
> 
>> +
>> +    if (tlb_v0) {
>> +        addr = tlb_vppn & ~mask;    /* xxx...xxx[0]000..0000 */
>> +        end = addr | (mask >> 1);   /* xxx...xxx[0]111..1111 */
>> +        while (addr < end) {
>> +            tlb_flush_page(cs, addr);
>> +            addr += TARGET_PAGE_SIZE;
> 
> tlb_flush_range_by_mmuidx.
> 
>> +    tlb->tlb_misc = FIELD_DP64(tlb->tlb_misc, TLB_MISC, VPPN, csr_vppn);
>> +    tlb->tlb_misc = FIELD_DP64(tlb->tlb_misc, TLB_MISC, E, 1);
>> +    csr_asid = FIELD_EX64(env->CSR_ASID, CSR_ASID, ASID);
>> +    tlb->tlb_misc = FIELD_DP64(tlb->tlb_misc, TLB_MISC, ASID, csr_asid);
>> +
>> +    csr_g = FIELD_EX64(env->CSR_TLBELO0, CSR_TLBELO0, G) &
>> +             FIELD_EX64(env->CSR_TLBELO1, CSR_TLBELO1, G);
>> +    tlb->tlb_misc = FIELD_DP64(tlb->tlb_misc, TLB_MISC, G, csr_g);
>> +
>> +    tlb->tlb_entry0 = FIELD_DP64(tlb->tlb_entry0, ENTRY0, V,
>> +                                 FIELD_EX64(lo0, CSR_TLBELO0, V));/* [0] */
>> +    tlb->tlb_entry0 = FIELD_DP64(tlb->tlb_entry0, ENTRY0, D,
>> +                                 FIELD_EX64(lo0, CSR_TLBELO0, D));/* [1] */
>> +    tlb->tlb_entry0 = FIELD_DP64(tlb->tlb_entry0, ENTRY0, PLV,
>> +                                 FIELD_EX64(lo0, CSR_TLBELO0, PLV));/* 
>> [3:2] */
>> +    tlb->tlb_entry0 = FIELD_DP64(tlb->tlb_entry0, ENTRY0, MAT,
>> +                                 FIELD_EX64(lo0, CSR_TLBELO0, MAT));/* 
>> [5:4] */
>> +    tlb->tlb_entry0 = FIELD_DP64(tlb->tlb_entry0, ENTRY0, PPN,
>> +                                 FIELD_EX64(lo0, CSR_TLBELO0, PPN));/* 
>> [47:12] */
>> +    tlb->tlb_entry0 = FIELD_DP64(tlb->tlb_entry0, ENTRY0, NR,
>> +                                 FIELD_EX64(lo0, CSR_TLBELO0, NR));/* [61] 
>> */
>> +    tlb->tlb_entry0 = FIELD_DP64(tlb->tlb_entry0, ENTRY0, NX,
>> +                                 FIELD_EX64(lo0, CSR_TLBELO0, NX));/* [62] 
>> */
>> +    tlb->tlb_entry0 = FIELD_DP64(tlb->tlb_entry0, ENTRY0, RPLV,
>> +                                 FIELD_EX64(lo0, CSR_TLBELO0, RPLV));/* 
>> [63] */
>> +
>> +    tlb->tlb_entry1 = FIELD_DP64(tlb->tlb_entry1, ENTRY1, V,
>> +                                 FIELD_EX64(lo1, CSR_TLBELO1, V));/* [0] */
>> +    tlb->tlb_entry1 = FIELD_DP64(tlb->tlb_entry1, ENTRY1, D,
>> +                                 FIELD_EX64(lo1, CSR_TLBELO1, D));/* [1] */
>> +    tlb->tlb_entry1 = FIELD_DP64(tlb->tlb_entry1, ENTRY1, PLV,
>> +                                 FIELD_EX64(lo1, CSR_TLBELO1, PLV));/* 
>> [3:2] */
>> +    tlb->tlb_entry1 = FIELD_DP64(tlb->tlb_entry1, ENTRY1, MAT,
>> +                                 FIELD_EX64(lo1, CSR_TLBELO1, MAT));/* 
>> [5:4] */
>> +    tlb->tlb_entry1 = FIELD_DP64(tlb->tlb_entry1, ENTRY1, PPN,
>> +                                 FIELD_EX64(lo1, CSR_TLBELO1, PPN));/* 
>> [47:12] */
>> +    tlb->tlb_entry1 = FIELD_DP64(tlb->tlb_entry1, ENTRY1, NR,
>> +                                 FIELD_EX64(lo1, CSR_TLBELO1, NR));/* [61] 
>> */
>> +    tlb->tlb_entry1 = FIELD_DP64(tlb->tlb_entry1, ENTRY1, NX,
>> +                                 FIELD_EX64(lo1, CSR_TLBELO1, NX));/* [62] 
>> */
>> +    tlb->tlb_entry1 = FIELD_DP64(tlb->tlb_entry1, ENTRY1, RPLV,
>> +                                 FIELD_EX64(lo1, CSR_TLBELO1, RPLV));/* 
>> [63] */
> 
> The point of making the two values have the same field layout is so that you 
> can just assign the whole value across, not extract and re-deposit each field.

Yes, it is much simpler when use the same field layout.

> 
>> +void helper_tlbsrch(CPULoongArchState *env)
>> +{
>> +    loongarch_tlb *tlb;
>> +    uint64_t vpn, tlb_vppn;
>> +    uint16_t csr_asid, tlb_asid, tlb_ps, tlb_e, tlb_g;
>> +
>> +    int stlb_size = env->stlb_size;
>> +    int mtlb_size = env->mtlb_size;
>> +    int i;
>> +    csr_asid = FIELD_EX64(env->CSR_ASID, CSR_ASID, ASID);
>> +
>> +    /* Search MTLB + STLB */
>> +    for (i = 0; i < stlb_size + mtlb_size; ++i) {
>> +        tlb = &env->tlb[i];
>> +        vpn = FIELD_EX64(env->CSR_TLBEHI, CSR_TLBEHI, VPPN);
>> +        tlb_asid = FIELD_EX64(tlb->tlb_misc, TLB_MISC, ASID);
>> +        tlb_ps = FIELD_EX64(tlb->tlb_misc, TLB_MISC, PS);
>> +        tlb_e = FIELD_EX64(tlb->tlb_misc, TLB_MISC, E);
>> +        tlb_g = FIELD_EX64(tlb->tlb_misc, TLB_MISC, G);
>> +        tlb_vppn = FIELD_EX64(tlb->tlb_misc, TLB_MISC, VPPN);
>> +
>> +        if ((tlb_g == 1 || tlb_asid == csr_asid) &&
>> +            (vpn >> (tlb_ps + 1 - 13) == tlb_vppn >> (tlb_ps + 1 - 13)) && 
>> tlb_e) {
>> +            env->CSR_TLBIDX = FIELD_DP64(env->CSR_TLBIDX, CSR_TLBIDX,
>> +                                         INDEX, (i & 0xfff));
>> +            env->CSR_TLBIDX = FIELD_DP64(env->CSR_TLBIDX, CSR_TLBIDX,
>> +                                         PS, (tlb_ps & 0x3f));
>> +            return;
>> +        }
>> +    }
>> +
>> +    env->CSR_TLBIDX = FIELD_DP64(env->CSR_TLBIDX, CSR_TLBIDX, NE, 1);
>> +}
> 
> Surely this should share code with loongarch_map_address.
> 
>> +
>> +void helper_tlbrd(CPULoongArchState *env)
>> +{
>> +    loongarch_tlb *tlb;
>> +    int idx;
>> +    uint16_t csr_asid, tlb_asid;
>> +    uint8_t tlb_ps, tlb_e, tlb_v0, tlb_v1, tlb_d0, tlb_d1;
>> +    uint8_t tlb_plv0, tlb_plv1, tlb_mat0, tlb_mat1, tlb_g;
>> +    uint64_t tlb_ppn0, tlb_ppn1;
>> +    uint8_t tlb_nr0, tlb_nr1, tlb_nx0, tlb_nx1, tlb_rplv0, tlb_rplv1;
>> +
>> +    idx = FIELD_EX64(env->CSR_TLBIDX, CSR_TLBIDX, INDEX);
>> +    tlb = &env->tlb[idx];
>> +
>> +    csr_asid = FIELD_EX64(env->CSR_ASID, CSR_ASID, ASID);
>> +    tlb_asid = FIELD_EX64(tlb->tlb_misc, TLB_MISC, ASID);
>> +    tlb_ps = FIELD_EX64(tlb->tlb_misc, TLB_MISC, PS);
>> +    tlb_e = FIELD_EX64(tlb->tlb_misc, TLB_MISC, E);
>> +    tlb_g = FIELD_EX64(tlb->tlb_misc, TLB_MISC, G);
>> +
>> +    tlb_v0 = FIELD_EX64(tlb->tlb_entry0, ENTRY0, V);
>> +    tlb_d0 = FIELD_EX64(tlb->tlb_entry0, ENTRY0, D);
>> +    tlb_plv0 = FIELD_EX64(tlb->tlb_entry0, ENTRY0, PLV);
>> +    tlb_mat0 = FIELD_EX64(tlb->tlb_entry0, ENTRY0, MAT);
>> +    tlb_ppn0 = FIELD_EX64(tlb->tlb_entry0, ENTRY0, PPN);
>> +    tlb_nr0 = FIELD_EX64(tlb->tlb_entry0, ENTRY0, NR);
>> +    tlb_nx0 = FIELD_EX64(tlb->tlb_entry0, ENTRY0, NX);
>> +    tlb_rplv0 = FIELD_EX64(tlb->tlb_entry0, ENTRY0, RPLV);
>> +
>> +    tlb_v1 = FIELD_EX64(tlb->tlb_entry1, ENTRY1, V);
>> +    tlb_d1 = FIELD_EX64(tlb->tlb_entry1, ENTRY1, D);
>> +    tlb_plv1 = FIELD_EX64(tlb->tlb_entry1, ENTRY1, PLV);
>> +    tlb_mat1 = FIELD_EX64(tlb->tlb_entry1, ENTRY1, MAT);
>> +    tlb_ppn1 = FIELD_EX64(tlb->tlb_entry1, ENTRY1, PPN);
>> +    tlb_nr1 = FIELD_EX64(tlb->tlb_entry1, ENTRY1, NR);
>> +    tlb_nx1 = FIELD_EX64(tlb->tlb_entry1, ENTRY1, NX);
>> +    tlb_rplv1 = FIELD_EX64(tlb->tlb_entry1, ENTRY1, RPLV);
>> +
>> +    if (csr_asid != tlb_asid) {
>> +        cpu_loongarch_tlb_flush(env);
> 
> Why?  Surely the index selected should not have matched on the previous 
> search?

yes, I will modify.

> 
>> +    } else {
>> +        /* Valid TLB entry */
>> +        env->CSR_TLBIDX = FIELD_DP64(env->CSR_TLBIDX, CSR_TLBIDX,
>> +                                     INDEX, (idx & 0xfff));
>> +        env->CSR_TLBIDX = FIELD_DP64(env->CSR_TLBIDX, CSR_TLBIDX,
>> +                                     PS, (tlb_ps & 0x3f));
>> +
>> +        env->CSR_TLBEHI = FIELD_EX64(tlb->tlb_misc, TLB_MISC, VPPN) << 13;
>> +
>> +        env->CSR_TLBELO0 = FIELD_DP64(0, CSR_TLBELO0, V, tlb_v0);
>> +        env->CSR_TLBELO0 = FIELD_DP64(env->CSR_TLBELO0, CSR_TLBELO0, D, 
>> tlb_d0);
>> +        env->CSR_TLBELO0 = FIELD_DP64(env->CSR_TLBELO0, CSR_TLBELO0, PLV, 
>> tlb_plv0);
>> +        env->CSR_TLBELO0 = FIELD_DP64(env->CSR_TLBELO0, CSR_TLBELO0, MAT, 
>> tlb_mat0);
>> +        env->CSR_TLBELO0 = FIELD_DP64(env->CSR_TLBELO0, CSR_TLBELO0, G, 
>> tlb_g);
>> +        env->CSR_TLBELO0 = FIELD_DP64(env->CSR_TLBELO0, CSR_TLBELO0, PPN, 
>> tlb_ppn0);
>> +        env->CSR_TLBELO0 = FIELD_DP64(env->CSR_TLBELO0, CSR_TLBELO0, NR, 
>> tlb_nr0);
>> +        env->CSR_TLBELO0 = FIELD_DP64(env->CSR_TLBELO0, CSR_TLBELO0, NX, 
>> tlb_nx0);
>> +        env->CSR_TLBELO0 = FIELD_DP64(env->CSR_TLBELO0, CSR_TLBELO0, RPLV, 
>> tlb_rplv0);
>> +
>> +        env->CSR_TLBELO1 = FIELD_DP64(0, CSR_TLBELO1, V, tlb_v1);
>> +        env->CSR_TLBELO1 = FIELD_DP64(env->CSR_TLBELO1, CSR_TLBELO1, D, 
>> tlb_d1);
>> +        env->CSR_TLBELO1 = FIELD_DP64(env->CSR_TLBELO1, CSR_TLBELO1, PLV, 
>> tlb_plv1);
>> +        env->CSR_TLBELO1 = FIELD_DP64(env->CSR_TLBELO1, CSR_TLBELO1, MAT, 
>> tlb_mat1);
>> +        env->CSR_TLBELO1 = FIELD_DP64(env->CSR_TLBELO1, CSR_TLBELO1, G, 
>> tlb_g);
>> +        env->CSR_TLBELO1 = FIELD_DP64(env->CSR_TLBELO1, CSR_TLBELO1, PPN, 
>> tlb_ppn1);
>> +        env->CSR_TLBELO1 = FIELD_DP64(env->CSR_TLBELO1, CSR_TLBELO1, NR, 
>> tlb_nr1);
>> +        env->CSR_TLBELO1 = FIELD_DP64(env->CSR_TLBELO1, CSR_TLBELO1, NX, 
>> tlb_nx1);
>> +        env->CSR_TLBELO1 = FIELD_DP64(env->CSR_TLBELO1, CSR_TLBELO1, RPLV, 
> 
> Again, these should easily copy across.
> 
>> +    env->CSR_ASID  = FIELD_DP64(env->CSR_ASID, CSR_ASID, ASID, tlb_asid);
> 
> The documentation for TLBRD does not mention modifications to the current 
> ASID.
> 
>> +    default:
>> +        do_raise_exception(env, EXCP_INE, GETPC());
> 
> You can detect this during translation, and dispatch to the appropriate 
> invtlb sub-function.
> 
oh, sorry, I don't quiet understand this. detect during the translation sees 
more complicated.

Xiaojuan,
Thanks
> 
> r~