Re: [PATCH v2 03/22] target/loongarch: Add core definition

[Richard Henderson <richard.henderson@linaro.org>]

On 7/20/21 11:52 PM, Song Gao wrote:
> This patch add target state header, target definitions
> and initialization routines.
> 
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> ---
>   target/loongarch/cpu-param.h |  21 ++++
>   target/loongarch/cpu-qom.h   |  40 ++++++
>   target/loongarch/cpu.c       | 293 +++++++++++++++++++++++++++++++++++++++++++
>   target/loongarch/cpu.h       | 265 ++++++++++++++++++++++++++++++++++++++
>   4 files changed, 619 insertions(+)
>   create mode 100644 target/loongarch/cpu-param.h
>   create mode 100644 target/loongarch/cpu-qom.h
>   create mode 100644 target/loongarch/cpu.c
>   create mode 100644 target/loongarch/cpu.h
> 
> diff --git a/target/loongarch/cpu-param.h b/target/loongarch/cpu-param.h
> new file mode 100644
> index 0000000..582ee29
> --- /dev/null
> +++ b/target/loongarch/cpu-param.h
> @@ -0,0 +1,21 @@
> +/*
> + * LoongArch cpu parameters for qemu.
> + *
> + * Copyright (c) 2021 Loongson Technology Corporation Limited
> + *
> + * SPDX-License-Identifier: LGPL-2.1+
> + */
> +
> +#ifndef LOONGARCH_CPU_PARAM_H
> +#define LOONGARCH_CPU_PARAM_H 1
> +
> +#ifdef TARGET_LOONGARCH64
> +#define TARGET_LONG_BITS 64

Why the ifdef for TARGET_LOONGARCH64?
Nothing will compile without that set.

> +#ifdef CONFIG_TCG
> +static void loongarch_cpu_synchronize_from_tb(CPUState *cs,
> +                                              const TranslationBlock *tb)
> +{
> +    LoongArchCPU *cpu = LOONGARCH_CPU(cs);
> +    CPULoongArchState *env = &cpu->env;
> +
> +    env->active_tc.PC = tb->pc;
> +    env->hflags &= ~LOONGARCH_HFLAG_BMASK;
> +    env->hflags |= tb->flags & LOONGARCH_HFLAG_BMASK;
> +}

Loongarch has no branch delay slots, so you should not have replicated the mips branch 
delay slot handling.  There should be no BMASK at all.

> +#ifdef CONFIG_TCG
> +#include "hw/core/tcg-cpu-ops.h"
> +
> +static struct TCGCPUOps loongarch_tcg_ops = {
> +    .initialize = loongarch_tcg_init,
> +    .synchronize_from_tb = loongarch_cpu_synchronize_from_tb,
> +};
> +#endif /* CONFIG_TCG */

May I presume that Loongarch has virtualization hardware, and will eventually support KVM? 
  If not, there is no need for CONFIG_TCG anywhere.

> +#define TCG_GUEST_DEFAULT_MO (0)
> +#define UNASSIGNED_CPU_ID 0xFFFFFFFF
> +
> +typedef union fpr_t fpr_t;
> +union fpr_t {
> +    float64  fd;   /* ieee double precision */
> +    float32  fs[2];/* ieee single precision */
> +    uint64_t d;    /* binary double fixed-point */
> +    uint32_t w[2]; /* binary single fixed-point */
> +};

For what it's worth, we already have a CPU_DoubleU type that could be used.  But frankly, 
float64 *is* uint64_t, so there's very little use in putting them together into a union. 
It would seem that you don't even use fs and w for more than fpu_dump_state, and you're 
even doing it wrong there.

> +typedef struct CPULoongArchFPUContext CPULoongArchFPUContext;
> +struct CPULoongArchFPUContext {
> +    /* Floating point registers */
> +    fpr_t fpr[32];
> +    float_status fp_status;
> +
> +    bool cf[8];
> +    /*
> +     * fcsr0
> +     * 31:29 |28:24 |23:21 |20:16 |15:10 |9:8 |7  |6  |5 |4:0
> +     *        Cause         Flags         RM   DAE TM     Enables
> +     */
> +    uint32_t fcsr0;
> +    uint32_t fcsr0_mask;
> +    uint32_t vcsr16;
> +
> +#define FCSR0_M1    0xdf         /* FCSR1 mask, DAE, TM and Enables */
> +#define FCSR0_M2    0x1f1f0000   /* FCSR2 mask, Cause and Flags */
> +#define FCSR0_M3    0x300        /* FCSR3 mask, Round Mode */
> +#define FCSR0_RM    8            /* Round Mode bit num on fcsr0 */
> +#define GET_FP_CAUSE(reg)        (((reg) >> 24) & 0x1f)
> +#define GET_FP_ENABLE(reg)       (((reg) >>  0) & 0x1f)
> +#define GET_FP_FLAGS(reg)        (((reg) >> 16) & 0x1f)
> +#define SET_FP_CAUSE(reg, v)      do { (reg) = ((reg) & ~(0x1f << 24)) | \
> +                                               ((v & 0x1f) << 24);       \
> +                                     } while (0)
> +#define SET_FP_ENABLE(reg, v)     do { (reg) = ((reg) & ~(0x1f <<  0)) | \
> +                                               ((v & 0x1f) << 0);        \
> +                                     } while (0)
> +#define SET_FP_FLAGS(reg, v)      do { (reg) = ((reg) & ~(0x1f << 16)) | \
> +                                               ((v & 0x1f) << 16);       \
> +                                     } while (0)
> +#define UPDATE_FP_FLAGS(reg, v)   do { (reg) |= ((v & 0x1f) << 16); } while (0)
> +#define FP_INEXACT        1
> +#define FP_UNDERFLOW      2
> +#define FP_OVERFLOW       4
> +#define FP_DIV0           8
> +#define FP_INVALID        16
> +};
> +
> +#define TARGET_INSN_START_EXTRA_WORDS 2
> +#define LOONGARCH_FPU_MAX 1
> +#define N_IRQS      14
> +
> +enum loongarch_feature {
> +    LA_FEATURE_3A5000,
> +};
> +
> +typedef struct TCState TCState;
> +struct TCState {
> +    target_ulong gpr[32];
> +    target_ulong PC;
> +};
> +
> +typedef struct CPULoongArchState CPULoongArchState;
> +struct CPULoongArchState {
> +    TCState active_tc;
> +    CPULoongArchFPUContext active_fpu;

Please don't replicate the mips foolishness with active_tc and active_fpu.  There is no 
inactive_fpu with which to contrast this.  Just include these fields directly into the 
main CPULoongArchState structure.

> +
> +    uint32_t current_tc;
> +    uint64_t scr[4];
> +    uint32_t current_fpu;
> +
> +    /* LoongArch CSR register */
> +    CPU_LOONGARCH_CSR
> +    target_ulong lladdr; /* LL virtual address compared against SC */
> +    target_ulong llval;
> +
> +    CPULoongArchFPUContext fpus[LOONGARCH_FPU_MAX];

More copying from MIPS?  What is this for?


> +
> +    /* QEMU */
> +    int error_code;
> +    uint32_t hflags;    /* CPU State */
> +#define TLB_NOMATCH   0x1
> +#define INST_INAVAIL  0x2 /* Invalid instruction word for BadInstr */
> +    /* TMASK defines different execution modes */
> +#define LOONGARCH_HFLAG_TMASK  0x1F5807FF
> +#define LOONGARCH_HFLAG_KU     0x00003 /* kernel/supervisor/user mode mask   */
> +#define LOONGARCH_HFLAG_UM     0x00003 /* user mode flag                     */
> +#define LOONGARCH_HFLAG_KM     0x00000 /* kernel mode flag                   */
> +#define LOONGARCH_HFLAG_64     0x00008 /* 64-bit instructions enabled        */

Is there a 32-bit mode for LoongArch?  I don't see this big in CRMD.  This big overlaps 
the "Direct address translation mode enable bit".  Which does sound like it should be 
present in tb->flags,

> +#define LOONGARCH_HFLAG_FPU    0x00020 /* FPU enabled                        */
> +#define LOONGARCH_HFLAG_F64    0x00040 /* 64-bit FPU enabled                 */

I don't see that there is a mode-switch for a 32-bit fpu either.

> +#define LOONGARCH_HFLAG_BMASK  0x3800
> +#define LOONGARCH_HFLAG_B      0x00800 /* Unconditional branch               */
> +#define LOONGARCH_HFLAG_BC     0x01000 /* Conditional branch                 */
> +#define LOONGARCH_HFLAG_BR     0x02000 /* branch to register (can't link TB) */

None of the BMASK stuff applies to LoongArch.


> +#define LOONGARCH_HFLAG_FRE   0x2000000 /* FRE enabled */
> +#define LOONGARCH_HFLAG_ELPA  0x4000000
> +    target_ulong btarget;        /* Jump / branch target               */
> +    target_ulong bcond;          /* Branch condition (if needed)       */

Nor this.

> +static inline LoongArchCPU *loongarch_env_get_cpu(CPULoongArchState *env)
> +{
> +    return container_of(env, LoongArchCPU, env);
> +}
> +
> +#define ENV_GET_CPU(e) CPU(loongarch_env_get_cpu(e))

You have copied this from a very old version of qemu.  These were replaced by generic 
functions in include/exec/cpu-all.h.

> +void loongarch_tcg_init(void);
> +
> +void loongarch_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
> +
> +void QEMU_NORETURN do_raise_exception_err(CPULoongArchState *env,
> +                                          uint32_t exception,
> +                                          int error_code,
> +                                          uintptr_t pc);
> +
> +static inline void QEMU_NORETURN do_raise_exception(CPULoongArchState *env,
> +                                                    uint32_t exception,
> +                                                    uintptr_t pc)
> +{
> +    do_raise_exception_err(env, exception, 0, pc);
> +}
> +
> +static inline void compute_hflags(CPULoongArchState *env)
> +{
> +    env->hflags &= ~(LOONGARCH_HFLAG_64 | LOONGARCH_HFLAG_FPU |
> +                     LOONGARCH_HFLAG_KU | LOONGARCH_HFLAG_ELPA);
> +
> +    env->hflags |= (env->CSR_CRMD & CSR_CRMD_PLV);
> +    env->hflags |= LOONGARCH_HFLAG_64;
> +
> +    if (env->CSR_EUEN & CSR_EUEN_FPEN) {
> +        env->hflags |= LOONGARCH_HFLAG_FPU;
> +    }
> +}
> +
> +const char *loongarch_exception_name(int32_t exception);

These should not be declared in cpu.h.


r~