Re: [PATCH 10/31] target/loongarch: Add loongarch interrupt and exception handle

[Richard Henderson]

On 10/19/21 12:34 AM, Xiaojuan Yang wrote:
> This patch Add loongarch interrupt and exception handle.
>
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> ---
>   target/loongarch/cpu.c | 293 +++++++++++++++++++++++++++++++++++++++++
>   target/loongarch/cpu.h |   6 +-
>   2 files changed, 298 insertions(+), 1 deletion(-)
>
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index 7fa3851251..3e3cf233db 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -45,7 +45,10 @@ static const char * const excp_names[EXCP_LAST + 1] = {
>       [EXCP_TLBPE] = "TLB priviledged error",
>       [EXCP_TLBNX] = "TLB execute-inhibit",
>       [EXCP_TLBNR] = "TLB read-inhibit",
> +    [EXCP_EXT_INTERRUPT] = "interrupt",
>       [EXCP_DBP] = "debug breakpoint",
> +    [EXCP_IBE] = "instruction bus error",
> +    [EXCP_DBE] = "data bus error",
>   };

More incomplete update from before.

> +/* Check if there is pending and not masked out interrupt */
> +static inline bool cpu_loongarch_hw_interrupts_pending(CPULoongArchState *env)
> +{
> +    uint32_t pending;
> +    uint32_t status;
> +    bool r;
> +
> +    pending = FIELD_EX64(env->CSR_ESTAT, CSR_ESTAT, IS);
> +    status  = FIELD_EX64(env->CSR_ECFG, CSR_ECFG, LIE);
> +
> +    r = (pending & status) != 0;
> +    return r;

Return the expression directly?

> +}
> +
> +static inline unsigned int get_vint_size(CPULoongArchState *env)
> +{
> +    unsigned int size = 0;
> +    uint64_t vs = FIELD_EX64(env->CSR_ECFG, CSR_ECFG, VS);
> +
> +    switch (vs) {
> +    case 0:
> +        break;
> +    case 1:
> +        size = 2 * 4;   /* #Insts * inst_size */
> +        break;
> +    case 2:
> +        size = 4 * 4;
> +        break;
> +    case 3:
> +        size = 8 * 4;
> +        break;
> +    case 4:
> +        size = 16 * 4;
> +        break;
> +    case 5:
> +        size = 32 * 4;
> +        break;
> +    case 6:
> +        size = 64 * 4;
> +        break;
> +    case 7:
> +        size = 128 * 4;
> +        break;

This is a pretty simple expression to turn into a switch.

> +#define is_refill(cs, env) (((cs->exception_index == EXCP_TLBL) \
> +        || (cs->exception_index == EXCP_TLBS))  \
> +        && (env->error_code & EXCP_TLB_NOMATCH))

This should be a function, not a macro.
It's probably worth computing once, not multiple times within 
loongarch_cpu_do_interrupt.

> +    default:
> +        qemu_log("Error: exception(%d) '%s' has not been supported\n",
> +                 cs->exception_index, excp_names[cs->exception_index]);

../qemu/target/loongarch/cpu.c: In function ‘loongarch_cpu_do_interrupt’:
../qemu/target/loongarch/cpu.c:250:9: error: array subscript [0, 16] is outside array 
bounds of ‘const char * const[17]’ [-Werror=array-bounds]
  250 |         qemu_log("Error: exception(%d) '%s' has not been supported\n",
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  251 |                  cs->exception_index, excp_names[cs->exception_index]);
      |                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../qemu/target/loongarch/cpu.c:36:27: note: while referencing ‘excp_names’
   36 | static const char * const excp_names[EXCP_LAST + 1] = {
      |                           ^~~~~~~~~~
cc1: all warnings being treated as errors

> @@ -223,6 +509,7 @@ static void loongarch_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>   
>   #ifndef CONFIG_USER_ONLY
>       ls3a5k_mmu_init(env);
> +    env->exception_base = 0x1C000000;
>   #endif

What is exception_base if it's not EENTRY?  Too much copying from MIPS?


r~