Re: [PATCH v2 03/22] target/loongarch: Add core definition
On 7/25/21 10:47 PM, Song Gao wrote: Hmm, but where can we declared in ? such as ARM architecture declared in internals.h, is that OK? Yes. It is preferable that only things that are used outside of target/arch/ go into cpu.h, and that everything that is private to target/arch/ go into some other local header (with internals.h being a good catch-all). r~
Re: [PATCH v2 03/22] target/loongarch: Add core definition
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
>> ---
>> 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 000..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 = >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 0x
>> +
>> +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 0x1f1f /* 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
Re: [PATCH v2 03/22] target/loongarch: Add core definition
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
---
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 000..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 = >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 0x
+
+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_M10xdf /* FCSR1 mask, DAE, TM and Enables */
+#define FCSR0_M20x1f1f /* FCSR2 mask, Cause and Flags */
+#define FCSR0_M30x300/* FCSR3 mask, Round Mode */
+#define FCSR0_RM8/* 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_INEXACT1
+#define FP_UNDERFLOW 2
+#define FP_OVERFLOW 4
+#define FP_DIV0 8
+#define FP_INVALID16
+};
+
+#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
