Re: [PATCH v1 23/43] target/loongarch: Add LoongArch interrupt and exception handle

[Richard Henderson]

On 4/15/22 02:40, Xiaojuan Yang wrote:
> +    if (level) {
> +        env->CSR_ESTAT |= 1 << irq;
> +    } else {
> +        env->CSR_ESTAT &= ~(1 << irq);
> +    }

This is

    env->CSR_ESTAT = deposit64(env->CSR_ESTAT, irq, 1, level != 0);

> +static inline unsigned int get_vint_size(CPULoongArchState *env)
> +{
> +    uint64_t vs = FIELD_EX64(env->CSR_ECFG, CSR_ECFG, VS);
> +    uint64_t size = 0;
> +
> +    if (vs == 0) {
> +        return 0;
> +    }
> +
> +    if (vs < 8) {
> +        size = 1 << (vs + 2);
> +    }
> +
> +    if (vs > 8) {
> +        qemu_log("%s: unexpected value", __func__);
> +        assert(0);
> +    }
> +
> +    return size;
> +}

Given that VS is 3 bits, the two tests against 8 are unnecessary.

> +
> +static void loongarch_cpu_do_interrupt(CPUState *cs)
> +{
> +    LoongArchCPU *cpu = LOONGARCH_CPU(cs);
> +    CPULoongArchState *env = &cpu->env;
> +    bool update_badinstr = 1;
> +    int cause = -1;
> +    const char *name;
> +    bool tlbfill = FIELD_EX64(env->CSR_TLBRERA, CSR_TLBRERA, ISTLBR);
> +
> +    if (cs->exception_index != EXCCODE_INT) {
> +        if (cs->exception_index < 0 ||
> +            cs->exception_index > ARRAY_SIZE(excp_names)) {
> +            name = "unknown";
> +        } else {
> +            name = excp_names[cs->exception_index];
> +        }
> +
> +        qemu_log_mask(CPU_LOG_INT,
> +                     "%s enter: pc " TARGET_FMT_lx " ERA " TARGET_FMT_lx
> +                     " TLBRERA " TARGET_FMT_lx " %s exception\n", __func__,
> +                     env->pc, env->CSR_ERA, env->CSR_TLBRERA, name);
> +    }
> +    if (cs->exception_index == EXCCODE_INT &&
> +       (FIELD_EX64(env->CSR_DBG, CSR_DBG, DST))) {
> +        env->CSR_DBG = FIELD_DP64(env->CSR_DBG, CSR_DBG, DEI, 1);
> +        goto set_DERA;
> +    }

Seems like this block could be moved into the switch, and fall through into 
EXCCODE_PIF.

> +    uint32_t vec_size = get_vint_size(env);
> +    env->pc = env->CSR_EENTRY;
> +    env->pc += cause * vec_size;
> +    if (tlbfill) {
> +        /* TLB Refill */
> +        env->pc = env->CSR_TLBRENTRY;
> +    }

The first 3 statements seem better placed as an else to the if (tlbfill).
Also, probably all of this should be pushed down, into the non-interrupt branch.


> +    if  (cs->exception_index == EXCCODE_INT) {
> +        /* Interrupt */
> +        uint32_t vector = 0;
> +        uint32_t pending = FIELD_EX64(env->CSR_ESTAT, CSR_ESTAT, IS);
> +        pending &= FIELD_EX64(env->CSR_ECFG, CSR_ECFG, LIE);
> +
> +        /* Find the highest-priority interrupt. */
> +        while (pending >>= 1) {
> +            vector++;
> +        }

Use clz32 to compute this, not a loop.

> +        env->pc = env->CSR_EENTRY + (EXCCODE_EXTERNAL_INT + vector) * vec_size;
> +        qemu_log_mask(CPU_LOG_INT,
> +                      "%s: PC " TARGET_FMT_lx " ERA " TARGET_FMT_lx
> +                      " cause %d\n" "    A " TARGET_FMT_lx " D "
> +                      TARGET_FMT_lx " vector = %d ExC %08lx ExS %08lx\n",
> +                      __func__, env->pc, env->CSR_ERA,
> +                      cause, env->CSR_BADV, env->CSR_DERA, vector,
> +                      env->CSR_ECFG, env->CSR_ESTAT);
> +    }
> +
> +    if (!tlbfill) {
> +        env->CSR_ESTAT = FIELD_DP64(env->CSR_ESTAT, CSR_ESTAT, ECODE, cause);
> +    }

This second if appears to be misplaced, and needs to be above the first if -- in 
particular, we will have just logged an incorrect value for ESTAT.

After that is moved...

> +    if (cs->exception_index != EXCCODE_INT) {
> +        qemu_log_mask(CPU_LOG_INT,
> +                      "%s: PC " TARGET_FMT_lx " ERA " TARGET_FMT_lx
> +                      " cause %d%s\n, ESTAT " TARGET_FMT_lx
> +                      " EXCFG " TARGET_FMT_lx " BADVA " TARGET_FMT_lx
> +                      "BADI " TARGET_FMT_lx " SYS_NUM " TARGET_FMT_lu
> +                      " cpu %d asid 0x%lx" "\n", __func__, env->pc,
> +                      tlbfill ? env->CSR_TLBRERA : env->CSR_ERA,
> +                      cause, tlbfill ? "(refill)" : "", env->CSR_ESTAT,
> +                      env->CSR_ECFG,
> +                      tlbfill ? env->CSR_TLBRBADV : env->CSR_BADV,
> +                      env->CSR_BADI, env->gpr[11], cs->cpu_index,
> +                      env->CSR_ASID);
> +    }

... this becomes an else for the first if.


r~