[PATCH 0/4] target/loongarch: Cleanups in preparation of loongarch32 support

[PATCH 0/4] target/loongarch: Cleanups in preparation of loongarch32 support

[Philippe Mathieu-Daudé]

Jiajie, this series contains few notes I took while
reviewing your series adding loongarch32 support [*].

If your series isn't merged, consider rebasing it on
this one.

Regards,

Phil.

[*] https://lore.kernel.org/qemu-devel/20230817093121.1053890-1-gaosong@loongson.cn/

Philippe Mathieu-Daudé (4):
  target/loongarch: Log I/O write accesses to CSR registers
  target/loongarch: Remove duplicated disas_set_info assignment
  target/loongarch: Rename 64-bit specific functions
  target/loongarch: Extract 64-bit specifics to
    loongarch64_cpu_class_init

 target/loongarch/cpu.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

-- 
2.41.0

[PATCH 1/4] target/loongarch: Log I/O write accesses to CSR registers

[Philippe Mathieu-Daudé]

Various CSR registers have Read/Write fields. We might
want to see guest trying to change such registers.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/loongarch/cpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index ad93ecac92..7107968699 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -544,6 +544,8 @@ static void loongarch_cpu_realizefn(DeviceState *dev, Error **errp)
 static void loongarch_qemu_write(void *opaque, hwaddr addr,
                                  uint64_t val, unsigned size)
 {
+    qemu_log_mask(LOG_UNIMP, "[%s]: Unimplemented reg 0x%" HWADDR_PRIx "\n",
+                  __func__, addr);
 }
 
 static uint64_t loongarch_qemu_read(void *opaque, hwaddr addr, unsigned size)
-- 
2.41.0

[PATCH 2/4] target/loongarch: Remove duplicated disas_set_info assignment

[Philippe Mathieu-Daudé]

Commit 228021f05e ("target/loongarch: Add core definition") sets
disas_set_info to loongarch_cpu_disas_set_info. Probably due to
a failed git-rebase, commit ca61e75071 ("target/loongarch: Add gdb
support") also sets it to the same value. Remove the duplication.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/loongarch/cpu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index 7107968699..dc617be36f 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -723,7 +723,6 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
     cc->disas_set_info = loongarch_cpu_disas_set_info;
     cc->gdb_read_register = loongarch_cpu_gdb_read_register;
     cc->gdb_write_register = loongarch_cpu_gdb_write_register;
-    cc->disas_set_info = loongarch_cpu_disas_set_info;
     cc->gdb_num_core_regs = 35;
     cc->gdb_core_xml_file = "loongarch-base64.xml";
     cc->gdb_stop_before_watchpoint = true;
-- 
2.41.0

Re: [PATCH 2/4] target/loongarch: Remove duplicated disas_set_info assignment

[Richard Henderson]

On 8/18/23 03:34, Philippe Mathieu-Daudé wrote:
> Commit 228021f05e ("target/loongarch: Add core definition") sets
> disas_set_info to loongarch_cpu_disas_set_info. Probably due to
> a failed git-rebase, commit ca61e75071 ("target/loongarch: Add gdb
> support") also sets it to the same value. Remove the duplication.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/loongarch/cpu.c | 1 -
>   1 file changed, 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 3/4] target/loongarch: Rename 64-bit specific functions

[Philippe Mathieu-Daudé]

These functions are specific to loongarch64 cores. Rename
including the '64' suffix in preparation of supporting
loongarch32 cores.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/loongarch/cpu.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index dc617be36f..19572e37ad 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -76,7 +76,7 @@ void G_NORETURN do_raise_exception(CPULoongArchState *env,
     cpu_loop_exit_restore(cs, pc);
 }
 
-static void loongarch_cpu_set_pc(CPUState *cs, vaddr value)
+static void loongarch64_cpu_set_pc(CPUState *cs, vaddr value)
 {
     LoongArchCPU *cpu = LOONGARCH_CPU(cs);
     CPULoongArchState *env = &cpu->env;
@@ -84,7 +84,7 @@ static void loongarch_cpu_set_pc(CPUState *cs, vaddr value)
     env->pc = value;
 }
 
-static vaddr loongarch_cpu_get_pc(CPUState *cs)
+static vaddr loongarch64_cpu_get_pc(CPUState *cs)
 {
     LoongArchCPU *cpu = LOONGARCH_CPU(cs);
     CPULoongArchState *env = &cpu->env;
@@ -356,7 +356,7 @@ static bool loongarch_cpu_has_work(CPUState *cs)
 #endif
 }
 
-static void loongarch_la464_initfn(Object *obj)
+static void loongarch64_la464_initfn(Object *obj)
 {
     LoongArchCPU *cpu = LOONGARCH_CPU(obj);
     CPULoongArchState *env = &cpu->env;
@@ -694,7 +694,7 @@ static const struct SysemuCPUOps loongarch_sysemu_ops = {
 };
 #endif
 
-static gchar *loongarch_gdb_arch_name(CPUState *cs)
+static gchar *loongarch64_gdb_arch_name(CPUState *cs)
 {
     return g_strdup("loongarch64");
 }
@@ -714,8 +714,8 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
     cc->class_by_name = loongarch_cpu_class_by_name;
     cc->has_work = loongarch_cpu_has_work;
     cc->dump_state = loongarch_cpu_dump_state;
-    cc->set_pc = loongarch_cpu_set_pc;
-    cc->get_pc = loongarch_cpu_get_pc;
+    cc->set_pc = loongarch64_cpu_set_pc;
+    cc->get_pc = loongarch64_cpu_get_pc;
 #ifndef CONFIG_USER_ONLY
     dc->vmsd = &vmstate_loongarch_cpu;
     cc->sysemu_ops = &loongarch_sysemu_ops;
@@ -726,14 +726,14 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
     cc->gdb_num_core_regs = 35;
     cc->gdb_core_xml_file = "loongarch-base64.xml";
     cc->gdb_stop_before_watchpoint = true;
-    cc->gdb_arch_name = loongarch_gdb_arch_name;
+    cc->gdb_arch_name = loongarch64_gdb_arch_name;
 
 #ifdef CONFIG_TCG
     cc->tcg_ops = &loongarch_tcg_ops;
 #endif
 }
 
-#define DEFINE_LOONGARCH_CPU_TYPE(model, initfn) \
+#define DEFINE_LOONGARCH64_CPU_TYPE(model, initfn) \
     { \
         .parent = TYPE_LOONGARCH_CPU, \
         .instance_init = initfn, \
@@ -751,7 +751,7 @@ static const TypeInfo loongarch_cpu_type_infos[] = {
         .class_size = sizeof(LoongArchCPUClass),
         .class_init = loongarch_cpu_class_init,
     },
-    DEFINE_LOONGARCH_CPU_TYPE("la464", loongarch_la464_initfn),
+    DEFINE_LOONGARCH64_CPU_TYPE("la464", loongarch64_la464_initfn),
 };
 
 DEFINE_TYPES(loongarch_cpu_type_infos)
-- 
2.41.0

Re: [PATCH 3/4] target/loongarch: Rename 64-bit specific functions

[Richard Henderson]

On 8/18/23 03:34, Philippe Mathieu-Daudé wrote:
> These functions are specific to loongarch64 cores. Rename
> including the '64' suffix in preparation of supporting
> loongarch32 cores.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/loongarch/cpu.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index dc617be36f..19572e37ad 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -76,7 +76,7 @@ void G_NORETURN do_raise_exception(CPULoongArchState *env,
>       cpu_loop_exit_restore(cs, pc);
>   }
>   
> -static void loongarch_cpu_set_pc(CPUState *cs, vaddr value)
> +static void loongarch64_cpu_set_pc(CPUState *cs, vaddr value)
>   {
>       LoongArchCPU *cpu = LOONGARCH_CPU(cs);
>       CPULoongArchState *env = &cpu->env;
> @@ -84,7 +84,7 @@ static void loongarch_cpu_set_pc(CPUState *cs, vaddr value)
>       env->pc = value;
>   }
>   
> -static vaddr loongarch_cpu_get_pc(CPUState *cs)
> +static vaddr loongarch64_cpu_get_pc(CPUState *cs)
>   {
>       LoongArchCPU *cpu = LOONGARCH_CPU(cs);
>       CPULoongArchState *env = &cpu->env;
> @@ -356,7 +356,7 @@ static bool loongarch_cpu_has_work(CPUState *cs)
>   #endif
>   }

Not 64-bit specific.


r~

[PATCH 4/4] target/loongarch: Extract 64-bit specifics to loongarch64_cpu_class_init

[Philippe Mathieu-Daudé]

Extract loongarch64 specific code from loongarch_cpu_class_init()
to a new loongarch64_cpu_class_init().
Adapt DEFINE_LOONGARCH64_CPU_TYPE() macro.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/loongarch/cpu.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index 19572e37ad..6bd5ca93fd 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -694,11 +694,6 @@ static const struct SysemuCPUOps loongarch_sysemu_ops = {
 };
 #endif
 
-static gchar *loongarch64_gdb_arch_name(CPUState *cs)
-{
-    return g_strdup("loongarch64");
-}
-
 static void loongarch_cpu_class_init(ObjectClass *c, void *data)
 {
     LoongArchCPUClass *lacc = LOONGARCH_CPU_CLASS(c);
@@ -714,8 +709,6 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
     cc->class_by_name = loongarch_cpu_class_by_name;
     cc->has_work = loongarch_cpu_has_work;
     cc->dump_state = loongarch_cpu_dump_state;
-    cc->set_pc = loongarch64_cpu_set_pc;
-    cc->get_pc = loongarch64_cpu_get_pc;
 #ifndef CONFIG_USER_ONLY
     dc->vmsd = &vmstate_loongarch_cpu;
     cc->sysemu_ops = &loongarch_sysemu_ops;
@@ -723,21 +716,35 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
     cc->disas_set_info = loongarch_cpu_disas_set_info;
     cc->gdb_read_register = loongarch_cpu_gdb_read_register;
     cc->gdb_write_register = loongarch_cpu_gdb_write_register;
-    cc->gdb_num_core_regs = 35;
-    cc->gdb_core_xml_file = "loongarch-base64.xml";
     cc->gdb_stop_before_watchpoint = true;
-    cc->gdb_arch_name = loongarch64_gdb_arch_name;
 
 #ifdef CONFIG_TCG
     cc->tcg_ops = &loongarch_tcg_ops;
 #endif
 }
 
+static gchar *loongarch64_gdb_arch_name(CPUState *cs)
+{
+    return g_strdup("loongarch64");
+}
+
+static void loongarch64_cpu_class_init(ObjectClass *c, void *data)
+{
+    CPUClass *cc = CPU_CLASS(c);
+
+    cc->set_pc = loongarch64_cpu_set_pc;
+    cc->get_pc = loongarch64_cpu_get_pc;
+    cc->gdb_num_core_regs = 35;
+    cc->gdb_core_xml_file = "loongarch-base64.xml";
+    cc->gdb_arch_name = loongarch64_gdb_arch_name;
+}
+
 #define DEFINE_LOONGARCH64_CPU_TYPE(model, initfn) \
     { \
         .parent = TYPE_LOONGARCH_CPU, \
-        .instance_init = initfn, \
         .name = LOONGARCH_CPU_TYPE_NAME(model), \
+        .instance_init = initfn, \
+        .class_init = loongarch64_cpu_class_init, \
     }
 
 static const TypeInfo loongarch_cpu_type_infos[] = {
-- 
2.41.0