Re: [RFC PATCH v2 07/30] target/loongarch: Add MMU support for LoongArch CPU.

[yangxiaojuan]

Hi, Richard:

On 11/11/2021 11:53 PM, Richard Henderson wrote:
> On 11/11/21 2:35 AM, Xiaojuan Yang wrote:
>> This patch introduces basic TLB interfaces.
>>
>> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
>> Signed-off-by: Song Gao <gaosong@loongson.cn>
>> ---
>>   target/loongarch/cpu-param.h  |   3 +
>>   target/loongarch/cpu.c        |  36 ++++
>>   target/loongarch/cpu.h        |  57 ++++++
>>   target/loongarch/internals.h  |   7 +
>>   target/loongarch/machine.c    |  56 ++++++
>>   target/loongarch/meson.build  |   1 +
>>   target/loongarch/tlb_helper.c | 339 ++++++++++++++++++++++++++++++++++
>>   7 files changed, 499 insertions(+)
>>   create mode 100644 target/loongarch/tlb_helper.c
>>
>> diff --git a/target/loongarch/cpu-param.h b/target/loongarch/cpu-param.h
>> index 9a769b67e0..5a2147fb90 100644
>> --- a/target/loongarch/cpu-param.h
>> +++ b/target/loongarch/cpu-param.h
>> @@ -12,6 +12,9 @@
>>   #define TARGET_PHYS_ADDR_SPACE_BITS 48
>>   #define TARGET_VIRT_ADDR_SPACE_BITS 48
>>   +#define TARGET_PHYS_MASK ((1UL << TARGET_PHYS_ADDR_SPACE_BITS) - 1)
>> +#define TARGET_VIRT_MASK ((1UL << TARGET_VIRT_ADDR_SPACE_BITS) - 1)
> 
> As before, unsigned long is wrong; use MAKE_64BIT_MASK.
> 
> These do not belong in cpu-param.h anyway; probably only tlb_helper.c needs 
> them.
> 
>> +#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);
> 
> %lx is wrong; use PRIx64.
> 
>> +#define LOONGARCH_TLB_MAX      2112 /* 2048 STLB + 64 MTLB */
> 
> Better to write (2048 + 64).
> 
>> +FIELD(TLB_MISC, E, 0, 1)
>> +FIELD(TLB_MISC, ASID, 1, 10)
>> +FIELD(TLB_MISC, G, 11, 1)
>> +FIELD(TLB_MISC, PS, 12, 6)
>> +FIELD(TLB_MISC, VPPN, 18, 35)
>> +
>> +/* Corresponding to CSR_TLBELO0/1 */
>> +FIELD(ENTRY0, V, 0, 1)
>> +FIELD(ENTRY0, D, 1, 1)
>> +FIELD(ENTRY0, NR, 2, 1)
>> +FIELD(ENTRY0, NX, 3, 1)
>> +FIELD(ENTRY0, MAT, 4, 2)
>> +FIELD(ENTRY0, PLV, 6, 2)
>> +FIELD(ENTRY0, RPLV, 8, 1)
>> +FIELD(ENTRY0, PPN, 9, 36)
>> +
>> +FIELD(ENTRY1, V, 0, 1)
>> +FIELD(ENTRY1, D, 1, 1)
>> +FIELD(ENTRY1, NR, 2, 1)
>> +FIELD(ENTRY1, NX, 3, 1)
>> +FIELD(ENTRY1, MAT, 4, 2)
>> +FIELD(ENTRY1, PLV, 6, 2)
>> +FIELD(ENTRY1, RPLV, 8, 1)
>> +FIELD(ENTRY1, PPN, 9, 36)
> 
> Why are you duplicating the CSR_TLBELO* fields?
> 
>> +const VMStateInfo vmstate_info_tlb = {
>> +    .name = "tlb_entry",
>> +    .get  = get_tlb,
>> +    .put  = put_tlb,
>> +};
> 
> Better to use .fields.
> 
>> +#define VMSTATE_TLB_ARRAY_V(_f, _s, _n, _v)                     \
>> +    VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_tlb, loongarch_tlb)
>> +
>> +#define VMSTATE_TLB_ARRAY(_f, _s, _n)                           \
>> +    VMSTATE_TLB_ARRAY_V(_f, _s, _n, 0)
> 
> Don't need these.
> 
>> +
>> +const VMStateDescription vmstate_tlb = {
>> +    .name = "cpu/tlb",
>> +    .version_id = 0,
>> +    .minimum_version_id = 0,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_TLB_ARRAY(env.tlb, LoongArchCPU, LOONGARCH_TLB_MAX),
> 
> VMSTATE_STRUCT_ARRAY.
> 
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>>     /* LoongArch CPU state */
>>   @@ -22,6 +70,10 @@ const VMStateDescription vmstate_loongarch_cpu = {
>>           VMSTATE_UINT64_ARRAY(env.fpr, LoongArchCPU, 32),
>>           VMSTATE_UINT32(env.fcsr0, LoongArchCPU),
>>   +        /* TLB */
>> +        VMSTATE_UINT32(env.stlb_size, LoongArchCPU),
>> +        VMSTATE_UINT32(env.mtlb_size, LoongArchCPU),
> 
> Might as well keep these in vmstate_tlb.

All of the vmstate has been modified.

>> +/* TLB address map */
>> +static int loongarch_map_tlb_entry(CPULoongArchState *env, hwaddr *physical,
>> +                                   int *prot, target_ulong address,
>> +                                   int access_type, loongarch_tlb *tlb)
>> +{
>> +    uint64_t plv = FIELD_EX64(env->CSR_CRMD, CSR_CRMD, PLV);
> 
> Incorrect.  PLV associated with mmu_idx, so you need to use that.
> 
>> +    uint8_t tlb_ps, n, tlb_v0, tlb_v1, tlb_d0, tlb_d1;
>> +    uint8_t tlb_nx0, tlb_nx1, tlb_nr0, tlb_nr1;
>> +    uint64_t tlb_ppn0, tlb_ppn1;
>> +    uint8_t tlb_rplv0, tlb_rplv1, tlb_plv0, tlb_plv1;
>> +
>> +    tlb_ps = FIELD_EX64(tlb->tlb_misc, TLB_MISC, PS);
>> +    n = (address >> tlb_ps) & 0x1;/* Odd or even */
> 
> Surely you need to pass in tlb_ps, since it's not present in the STLB entries.
> 
>> +
>> +    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_ppn0 = FIELD_EX64(tlb->tlb_entry0, ENTRY0, PPN);
>> +    tlb_nx0 = FIELD_EX64(tlb->tlb_entry0, ENTRY0, NX);
>> +    tlb_nr0 = FIELD_EX64(tlb->tlb_entry0, ENTRY0, NR);
>> +    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_ppn1 = FIELD_EX64(tlb->tlb_entry1, ENTRY1, PPN);
>> +    tlb_nx1 = FIELD_EX64(tlb->tlb_entry1, ENTRY1, NX);
>> +    tlb_nr1 = FIELD_EX64(tlb->tlb_entry1, ENTRY1, NR);
>> +    tlb_rplv1 = FIELD_EX64(tlb->tlb_entry1, ENTRY1, RPLV);
> 
> Better to check N first.
> 
>     entry = n ? tlb->tlb_entry1 : tlb->tlb_entry0;
>     v = FIELD_EX64(entry, TLBENTRY, V);
> 
> etc.

oh, yes , it is Much simpler when check N first. Your advice is always so 
appropriate. Thanks

> 
>> +        *physical = (tlb_ppn1 << 12) | (address & ((1 << tlb_ps) - 1));
> 
> TARGET_PAGE_SIZE.

Maybe TARGET_PAGE_SIZE is not fit for a huge page. MAKE_64BIT_MASK(0, tlb_ps) 
is ok?

> 
>> +/* LoongArch 3A5000 -style MMU emulation */
> 
> There's no mention of 3A5000 in "LoongArch Reference Manual", which defines 
> this.  I think you've copied this from MIPS, which talks about r4k here.
> 
>> +static int loongarch_map_address(CPULoongArchState *env, hwaddr *physical,
>> +                                 int *prot,
>> +                                 target_ulong address,
>> +                                 MMUAccessType access_type)
>> +{
>> +    loongarch_tlb *tlb;
>> +    uint16_t csr_asid, tlb_asid, stlb_idx;
>> +    uint8_t tlb_e, stlb_ps, tlb_ps, tlb_g;
>> +    int i, stlb_size, mtlb_size;
>> +    uint64_t vpn, tlb_vppn;   /* Address to map */
>> +
>> +    stlb_size = env->stlb_size;
>> +    mtlb_size = env->mtlb_size;
>> +    csr_asid = FIELD_EX64(env->CSR_ASID, CSR_ASID, ASID);
>> +
>> +    /* Search MTLB */
>> +    for (i = stlb_size; i < stlb_size + mtlb_size; ++i) {
> 
> Bit of a shame to have a linear search.  I guess it's ok for a start, but 
> you'll find that this function is critical to the emulation speed of qemu, so 
> you should think about other ways to organize the data.

The stlb search by the set, the mtlb uses the linear search, I have no other 
idea, do you have some advice? 

> 
>> +        tlb = &env->tlb[i];
>> +        tlb_vppn = FIELD_EX64(tlb->tlb_misc, TLB_MISC, VPPN);
>> +        tlb_ps = FIELD_EX64(tlb->tlb_misc, TLB_MISC, PS);
>> +
>> +        vpn = (address & TARGET_VIRT_MASK) >> (tlb_ps + 1);
> 
> Why +1?

One tlb entry holds a adjacent odd/even pair, the vpn is the content of the 
virtual page number divided by 2.
So the compare vpn is bit[47:15] for 16KB page. while the vppn field in tlb 
entry contains bit[47:13], so need adjust.

> 
>> +        tlb_asid = FIELD_EX64(tlb->tlb_misc, TLB_MISC, ASID);
>> +        tlb_e = FIELD_EX64(tlb->tlb_misc, TLB_MISC, E);
>> +        tlb_g = FIELD_EX64(tlb->tlb_misc, TLB_MISC, G);
>> +
>> +        if ((tlb_g == 1 || tlb_asid == csr_asid) &&
>> +            (vpn == (tlb_vppn >> (tlb_ps + 1 - 13))) && tlb_e) {
> 
> Surely extract and test the enable bit E before anything else, just to speed 
> up the lookup.
> 
> The -13 is a bit of a magic number.  Surely TARGET_PAGE_BITS + 1.

OK, I will use the R_TLB_MISC_VPPN_SHIFT replace the magic 13.

> 
>> +static int get_physical_address(CPULoongArchState *env, hwaddr *physical,
>> +                                int *prot, target_ulong real_address,
>> +                                MMUAccessType access_type, int mmu_idx)
>> +{
>> +    int user_mode = mmu_idx == LOONGARCH_HFLAG_UM;
>> +    int kernel_mode = !user_mode;
> 
> Incorrect.  PLV == mmu_idx, as defined by cpu_mmu_index.
> 
>> +    unsigned plv, base_c, base_v, tmp;
>> +    uint64_t pg = FIELD_EX64(env->CSR_CRMD, CSR_CRMD, PG);
>> +
>> +    /* Effective address */
>> +    target_ulong address = real_address;
>> +
>> +    /* Check PG */
>> +    if (!pg) {
>> +        /* DA mode */
>> +        *physical = address & TARGET_PHYS_MASK;
>> +        *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>> +        return TLBRET_MATCH;
>> +    }
> 
> You need to define a fifth mmu_index for paging disabled, because the 
> software tlb handler runs in this mode.  If you don't define that, you'll 
> have to flush the softmmu tlb every time you have a tlb miss.
> 
>> +    plv = kernel_mode | (user_mode << 3);
> 
> plv_mask = 1 << plv;
> 
>> +    base_v = address >> CSR_DMW_BASE_SH;
>> +    /* Check direct map window 0 */
>> +    base_c = env->CSR_DMWIN0 >> CSR_DMW_BASE_SH;
>> +    if ((plv & env->CSR_DMWIN0) && (base_c == base_v)) {
>> +        *physical = dmwin_va2pa(address);
>> +        *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>> +        return TLBRET_MATCH;
>> +    }
>> +    /* Check direct map window 1 */
>> +    base_c = env->CSR_DMWIN1 >> CSR_DMW_BASE_SH;
>> +    if ((plv & env->CSR_DMWIN1) && (base_c == base_v)) {
>> +        *physical = dmwin_va2pa(address);
>> +        *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>> +        return TLBRET_MATCH;
>> +    }
> 
> There are more than 2 of these, which is why I suggested putting them in an 
> array, so that you can loop.  Maybe pull this out to a separate function, 
> like the tlb lookup?
> 
>> +    /* Check valid extension */
>> +    tmp = address >> (TARGET_VIRT_ADDR_SPACE_BITS - 1);
>> +    if (!(tmp == 0 || tmp == 0x1ffff)) {
> 
> Better to cast to int64_t first, so that this becomes 0 or -1, without 
> knowing that 64 - TARGET_VIRT_ADDR_SPACE_BITS - 1 -> 0x1ffff.
> 
>> +void loongarch_mmu_init(CPULoongArchState *env)
>> +{
>> +    /* Number of MTLB */
>> +    env->mtlb_size = 64;
>> +
>> +    /* Number of STLB */
>> +    env->stlb_size = 2048;
>> +
>> +    /* For 16KB, ps = 14, compare the bit [47:15] */
>> +    for (int i = 0; i < LOONGARCH_TLB_MAX; i++) {
>> +        env->tlb[i].tlb_misc = FIELD_DP64(env->tlb[i].tlb_misc, TLB_MISC, 
>> E, 0);
>> +    }
> 
> Just memset the whole thing?

yes, the tlb is empty at the start.

> 
>> +bool loongarch_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>> +                            MMUAccessType access_type, int mmu_idx,
>> +                            bool probe, uintptr_t retaddr)
>> +{
>> +    LoongArchCPU *cpu = LOONGARCH_CPU(cs);
>> +    CPULoongArchState *env = &cpu->env;
>> +    hwaddr physical;
>> +    int prot;
>> +    int ret = TLBRET_BADADDR;
>> +
>> +    /* Data access */
>> +    /* XXX: put correct access by using cpu_restore_state() correctly */
>> +    ret = get_physical_address(env, &physical, &prot, address,
>> +                               access_type, mmu_idx);
>> +    switch (ret) {
>> +    case TLBRET_MATCH:
>> +        qemu_log_mask(CPU_LOG_MMU,
>> +                      "%s address=%" VADDR_PRIx " physical " TARGET_FMT_plx
>> +                      " prot %d\n", __func__, address, physical, prot);
>> +        break;
>> +    default:
>> +        qemu_log_mask(CPU_LOG_MMU,
>> +                      "%s address=%" VADDR_PRIx " ret %d\n", __func__, 
>> address,
>> +                      ret);
>> +        break;
>> +    }
>> +    if (ret == TLBRET_MATCH) {
>> +        tlb_set_page(cs, address & TARGET_PAGE_MASK,
>> +                     physical & TARGET_PAGE_MASK, prot,
>> +                     mmu_idx, TARGET_PAGE_SIZE);
>> +        return true;
> 
> Merge the above switch into this?
> 
>> +    }
>> +    if (probe) {
>> +        return false;
>> +    } else {
>> +        raise_mmu_exception(env, address, access_type, ret);
>> +        do_raise_exception(env, cs->exception_index, retaddr);
>> +    }
>> +}
>>
> 
> 
> r~