Re: [PATCH 06/31] target/loongarch: Add mmu support for Loongarch CPU.

[Richard Henderson]

On 10/19/21 12:34 AM, Xiaojuan Yang wrote:
> @@ -272,6 +288,7 @@ static const struct SysemuCPUOps loongarch_sysemu_ops = {
>   #ifdef CONFIG_TCG
>   #include "hw/core/tcg-cpu-ops.h"
>   
> +#ifdef CONFIG_USER_ONLY
>   static bool loongarch_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                          MMUAccessType access_type, int mmu_idx,
>                          bool probe, uintptr_t retaddr)
> @@ -280,9 +297,14 @@ static bool loongarch_cpu_tlb_fill(CPUState *cs, vaddr 
> address, int size,
>       CPULoongArchState *env = &cpu->env;
>   
>       env->badaddr = address;
> -    cs->exception_index = EXCP_ADE;
> +    if (access_type == MMU_DATA_STORE) {
> +        cs->exception_index = EXCP_ADES;
> +    } else {
> +        cs->exception_index = EXCP_ADEL;
> +    }
>       do_raise_exception(env, cs->exception_index, retaddr);
>   }
> +#endif

It's too early to add this ifdef.  With what's upstream at the moment, you've broken 
loongarch-linux-user build by removing loongarch_cpu_tlb_fill.

There are patches out for review that would require tlb_fill be a system-only hook, but 
they have not landed yet.

> +#define LOONGARCH_HFLAG_KU     0x00003 /* kernel/user mode mask   */
> +#define LOONGARCH_HFLAG_UM     0x00003 /* user mode flag                     */
> +#define LOONGARCH_HFLAG_KM     0x00000 /* kernel mode flag                   */

I think you might as well represent all 3 priv levels: it's not a "kernel/user" 
mask.

> +#define EXCP_TLB_NOMATCH   0x1
> +#define EXCP_INST_NOTAVAIL 0x2 /* No valid instruction word for BadInstr */

These should be with the other EXCP values in the enum.
At the moment you're overlapping EXCP_ADES and EXCP_SYSCALL.

> @@ -130,7 +139,11 @@ void loongarch_cpu_list(void);
>   
>   static inline int cpu_mmu_index(CPULoongArchState *env, bool ifetch)
>   {
> +#ifdef CONFIG_USER_ONLY
>       return MMU_USER_IDX;
> +#else
> +    return env->CSR_CRMD & LOONGARCH_HFLAG_KU;

Better would be

    return FIELD_EX64(env->CSR_CRMD, CSR_CRMD, PLV);

since that's the field you're extracting from CRMD.

> +typedef struct ls3a5k_tlb_t ls3a5k_tlb_t;

Types should be in CamelCase, without _t suffix.

> +struct ls3a5k_tlb_t {
> +    target_ulong VPN;
> +    uint64_t PageMask;
> +    uint32_t PageSize;
> +    uint16_t ASID;
> +    unsigned int V0:1;     /* CSR_TLBLO[0] */
> +    unsigned int V1:1;
> +
> +    unsigned int D0:1;     /* CSR_TLBLO[1] */
> +    unsigned int D1:1;
> +
> +    unsigned int PLV0:2;   /* CSR_TLBLO[3:2] */
> +    unsigned int PLV1:2;
> +
> +    unsigned int MAT0:3;   /* CSR_TLBLO[5:4] */
> +    unsigned int MAT1:3;
> +
> +    unsigned int G:1;      /* CSR_TLBLO[6] */
> +
> +    uint64_t PPN0;         /* CSR_TLBLO[47:12] */
> +    uint64_t PPN1;
> +
> +    unsigned int NR0:1;    /* CSR_TLBLO[61] */
> +    unsigned int NR1:1;
> +
> +    unsigned int NX0:1;    /* CSR_TLBLO[62] */
> +    unsigned int NX1:1;
> +
> +    unsigned int NE:1;     /* CSR_TLBIDX[31] */
> +
> +    unsigned int RPLV0:1;
> +    unsigned int RPLV1:1;  /* CSR_TLBLO[63] */
> +};

It would be much better if you didn't use bitfields at all.  This was a bad idea when mips 
did it; let us not compound the error.

Just use the format defined by the architecture for the CSRs: a couple of uint64_t.  Use 
FIELD definitions to give the parts intelligible names.

> +typedef struct ls3a5k_tlb_t ls3a5k_tlb_t;
> +
> +struct CPULoongArchTLBContext {
> +    uint32_t nb_tlb;
> +    int (*map_address)(struct CPULoongArchState *env, hwaddr *physical,
> +                       int *prot, target_ulong address,
> +                       MMUAccessType access_type);
> +    struct {
> +        uint64_t     stlb_mask;
> +        uint32_t     stlb_size; /* at most : 8 * 256 = 2048 */
> +        uint32_t     mtlb_size; /* at most : 64 */
> +        ls3a5k_tlb_t tlb[LOONGARCH_TLB_MAX];
> +    } ls3a5k;
> +};

There's probably no point in using an indirect function call until you've got more than 
one mmu implementation.  You're copying too much from mips.

> +/* TLB state */
> +static int get_tlb(QEMUFile *f, void *pv, size_t size,
> +                   const VMStateField *field)
> +{
> +    ls3a5k_tlb_t *v = pv;
> +    uint32_t flags;
> +
> +    qemu_get_betls(f, &v->VPN);
> +    qemu_get_be64s(f, &v->PageMask);
> +    qemu_get_be32s(f, &v->PageSize);
> +    qemu_get_be16s(f, &v->ASID);
> +    qemu_get_be32s(f, &flags);
> +    v->RPLV1 = (flags >> 21) & 1;
> +    v->RPLV0 = (flags >> 20) & 1;
> +    v->PLV1 = (flags >> 18) & 3;
> +    v->PLV0 = (flags >> 16) & 3;
> +    v->NE = (flags >> 15) & 1;
> +    v->NR1 = (flags >> 14) & 1;
> +    v->NR0 = (flags >> 13) & 1;
> +    v->NX1 = (flags >> 12) & 1;
> +    v->NX0 = (flags >> 11) & 1;
> +    v->D1 = (flags >> 10) & 1;
> +    v->D0 = (flags >> 9) & 1;
> +    v->V1 = (flags >> 8) & 1;
> +    v->V0 = (flags >> 7) & 1;
> +    v->MAT1 = (flags >> 4) & 7;
> +    v->MAT0 = (flags >> 1) & 7;
> +    v->G = (flags >> 0) & 1;
> +    qemu_get_be64s(f, &v->PPN0);
> +    qemu_get_be64s(f, &v->PPN1);

Some of the ugly things that go away if you don't use bitfields.

> +const VMStateDescription vmstate_tlb = {
> +    .name = "cpu/tlb",
> +    .version_id = 2,
> +    .minimum_version_id = 2,

Too much copying again: version numbers do not start at 2.

> +void ls3a5k_mmu_init(CPULoongArchState *env)
> +{
> +    env->tlb = g_malloc0(sizeof(CPULoongArchTLBContext));

I think you should not make this a separate structure, and instead allocate this with 
CPULoongArchState.

> diff --git a/target/loongarch/translate.c b/target/loongarch/translate.c
> index bea290df66..0be29994f9 100644
> --- a/target/loongarch/translate.c
> +++ b/target/loongarch/translate.c
> @@ -61,9 +61,10 @@ static void loongarch_tr_init_disas_context(DisasContextBase 
> *dcbase,
>   {
>       int64_t bound;
>       DisasContext *ctx = container_of(dcbase, DisasContext, base);
> +    CPULoongArchState *env = cs->env_ptr;
>   
>       ctx->page_start = ctx->base.pc_first & TARGET_PAGE_MASK;
> -    ctx->mem_idx = MMU_USER_IDX;
> +    ctx->mem_idx = cpu_mmu_index(env, false);

This is incorrect.  You want

    tb_flags = ctx->base.tb->flags;
    mem_idx = tb_flags & LOONGARCH_HFLAG_PRIV.

It is almost always incorrect to dereference env at this point.  Everything should have 
been encoded into tb_flags so that when we do the hashing of the TranslationBlocks we find 
the one that has been compiled for the correct privilege level, etc.


r~