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

[Song Gao <gaosong@loongson.cn>]

Hi, Richard.

On 07/23/2021 06:43 AM, Richard Henderson wrote:
> 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.
> 

OK, I'll remove it.

>> +#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.
>
OK
 
>> +#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.
>
Yes, Loongarch has virtualization hardware,  We plan to support KVM in QEMU in the future.  
 
>> +#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.
>
OK， I'll correct it.
 
>> +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.
> 

OK.

>> +
>> +    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?
>
Oh, It semms so. 

> 
>> +
>> +    /* 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,
>

No.
 
>> +#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.
OK, I'll remove them.

> 
>> +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.

Right, We copied this from 3.10 version, the version is really old.  

> 
>> +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,Drop all of the branch delay slot stuff.
Use a common routine and pass in the TCGCond
>> +                                          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.
>

Hmm,  but where can we declared in ? such as ARM architecture declared in internals.h, is that OK?


Thanks 
Song Gao