Re: [RFC PATCH] hw/riscv: hart: allow other cpu instance

qemu-riscv

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH] hw/riscv: hart: allow other cpu instance

From:	Daniel Henrique Barboza
Subject:	Re: [RFC PATCH] hw/riscv: hart: allow other cpu instance
Date:	Mon, 31 Jul 2023 11:12:01 -0300
User-agent:	Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0



On 7/27/23 05:05, Nikita Shubin wrote:

From: Nikita Shubin <n.shubin@yadro.com>

Allow using instances derivative from RISCVCPU

Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
---
Currently it is not possible to overload instance of RISCVCPU,
i.e. something like this:

static const TypeInfo riscv_cpu_type_infos[] = {
      {
         .name = TYPE_ANOTHER_RISCV_CPU,
         .parent = TYPE_RISCV_CPU,
         .instance_size = sizeof(MyCPUState),
         .instance_init = riscv_my_cpu_init,
     }
};

Because we have RISCVHartArrayState.harts with exactly
the size of RISCVCPU.

Using own instances can be used to store some internal hart state.
---
  hw/riscv/boot.c               |  5 +++--
  hw/riscv/riscv_hart.c         | 20 ++++++++++++--------
  hw/riscv/sifive_u.c           |  7 +++++--
  hw/riscv/spike.c              |  4 +++-
  hw/riscv/virt-acpi-build.c    |  2 +-
  hw/riscv/virt.c               |  6 +++---
  include/hw/riscv/riscv_hart.h | 18 +++++++++++++++++-
  7 files changed, 44 insertions(+), 18 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 52bf8e67de..c0456dcc2e 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -36,7 +36,8 @@

bool riscv_is_32bit(RISCVHartArrayState *harts)

  {
-    return harts->harts[0].env.misa_mxl_max == MXL_RV32;
+    RISCVCPU *hart = riscv_array_get_hart(harts, 0);
+    return hart->env.misa_mxl_max == MXL_RV32;
  }

/*

@@ -414,7 +415,7 @@ void riscv_setup_rom_reset_vec(MachineState *machine, 
RISCVHartArrayState *harts
          reset_vec[4] = 0x0182b283;   /*     ld     t0, 24(t0) */
      }

- if (!harts->harts[0].cfg.ext_icsr) {

+    if (!riscv_array_get_hart(harts, 0)->cfg.ext_icsr) {
          /*
           * The Zicsr extension has been disabled, so let's ensure we don't
           * run the CSR instruction. Let's fill the address with a non
diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
index 613ea2aaa0..74fc10ef48 100644
--- a/hw/riscv/riscv_hart.c
+++ b/hw/riscv/riscv_hart.c
@@ -43,24 +43,28 @@ static void riscv_harts_cpu_reset(void *opaque)
  }

static bool riscv_hart_realize(RISCVHartArrayState *s, int idx,

-                               char *cpu_type, Error **errp)
+                               char *cpu_type, size_t size, Error **errp)
  {
-    object_initialize_child(OBJECT(s), "harts[*]", &s->harts[idx], cpu_type);
-    qdev_prop_set_uint64(DEVICE(&s->harts[idx]), "resetvec", s->resetvec);
-    s->harts[idx].env.mhartid = s->hartid_base + idx;
-    qemu_register_reset(riscv_harts_cpu_reset, &s->harts[idx]);
-    return qdev_realize(DEVICE(&s->harts[idx]), NULL, errp);
+    RISCVCPU *hart = riscv_array_get_hart(s, idx);
+    object_initialize_child_internal(OBJECT(s), "harts[*]",
+                                    hart, size, cpu_type);
+    qdev_prop_set_uint64(DEVICE(hart), "resetvec", s->resetvec);
+    hart->env.mhartid = s->hartid_base + idx;
+    qemu_register_reset(riscv_harts_cpu_reset, hart);
+    return qdev_realize(DEVICE(hart), NULL, errp);
  }

static void riscv_harts_realize(DeviceState *dev, Error **errp)

  {
      RISCVHartArrayState *s = RISCV_HART_ARRAY(dev);
+    size_t size = object_type_get_instance_size(s->cpu_type);
      int n;

- s->harts = g_new0(RISCVCPU, s->num_harts);

+    s->harts = g_new0(RISCVCPU *, s->num_harts);

for (n = 0; n < s->num_harts; n++) {

-        if (!riscv_hart_realize(s, n, s->cpu_type, errp)) {
+        s->harts[n] = RISCV_CPU(object_new(s->cpu_type));
+        if (!riscv_hart_realize(s, n, s->cpu_type, size, errp)) {
              return;


Not an issue with this patch but riscv_hart_realize() can use some review. I
I think that we're doing stuff in the wrong place. Perhaps I'll look into it.

          }
      }
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 35a335b8d0..b8a54db81b 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -104,6 +104,7 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry 
*memmap,
      char *nodename;
      uint32_t plic_phandle, prci_phandle, gpio_phandle, phandle = 1;
      uint32_t hfclk_phandle, rtcclk_phandle, phy_phandle;
+    RISCVCPU *hart;
      static const char * const ethclk_names[2] = { "pclk", "hclk" };
      static const char * const clint_compat[2] = {
          "sifive,clint0", "riscv,clint0"
@@ -180,9 +181,11 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry 
*memmap,
              } else {
                  qemu_fdt_setprop_string(fdt, nodename, "mmu-type", 
"riscv,sv48");
              }
-            isa = riscv_isa_string(&s->soc.u_cpus.harts[cpu - 1]);
+            hart = riscv_array_get_hart(&s->soc.u_cpus, cpu - 1);
+            isa = riscv_isa_string(hart);
          } else {
-            isa = riscv_isa_string(&s->soc.e_cpus.harts[0]);
+            hart = riscv_array_get_hart(&s->soc.e_cpus, 0);
+            isa = riscv_isa_string(hart);
          }
          qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", isa);
          qemu_fdt_setprop_string(fdt, nodename, "compatible", "riscv");
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 81f7e53aed..85b7568dad 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -61,6 +61,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry 
*memmap,
      uint32_t cpu_phandle, intc_phandle, phandle = 1;
      char *name, *mem_name, *clint_name, *clust_name;
      char *core_name, *cpu_name, *intc_name;
+    RISCVCPU *hart;
      static const char * const clint_compat[2] = {
          "sifive,clint0", "riscv,clint0"
      };
@@ -103,6 +104,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry 
*memmap,
          clint_cells =  g_new0(uint32_t, s->soc[socket].num_harts * 4);

for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) {

+            hart = riscv_array_get_hart(&s->soc[socket], cpu);
              cpu_phandle = phandle++;

cpu_name = g_strdup_printf("/cpus/cpu@%d",

@@ -113,7 +115,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry 
*memmap,
              } else {
                  qemu_fdt_setprop_string(fdt, cpu_name, "mmu-type", 
"riscv,sv48");
              }
-            name = riscv_isa_string(&s->soc[socket].harts[cpu]);
+            name = riscv_isa_string(hart);
              qemu_fdt_setprop_string(fdt, cpu_name, "riscv,isa", name);
              g_free(name);
              qemu_fdt_setprop_string(fdt, cpu_name, "compatible", "riscv");
diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
index 7331248f59..7cff4e4baf 100644
--- a/hw/riscv/virt-acpi-build.c
+++ b/hw/riscv/virt-acpi-build.c
@@ -158,7 +158,7 @@ static void build_rhct(GArray *table_data,
      isa_offset = table_data->len - table.table_offset;
      build_append_int_noprefix(table_data, 0, 2);   /* Type 0 */

- cpu = &s->soc[0].harts[0];

+    cpu = riscv_array_get_hart(&s->soc[0], 0);
      isa = riscv_isa_string(cpu);
      len = 8 + strlen(isa) + 1;
      aligned_len = (len % 2) ? (len + 1) : len;
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index d90286dc46..59b42cc5e4 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -236,7 +236,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int 
socket,
      uint8_t satp_mode_max;

for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) {

-        RISCVCPU *cpu_ptr = &s->soc[socket].harts[cpu];
+        RISCVCPU *cpu_ptr = riscv_array_get_hart(&s->soc[socket], cpu);

cpu_phandle = (*phandle)++;@@ -730,12 +730,12 @@ static void create_fdt_pmu(RISCVVirtState *s)

  {
      char *pmu_name;
      MachineState *ms = MACHINE(s);
-    RISCVCPU hart = s->soc[0].harts[0];
+    RISCVCPU *hart = riscv_array_get_hart(&s->soc[0], 0);

pmu_name = g_strdup_printf("/soc/pmu");

      qemu_fdt_add_subnode(ms->fdt, pmu_name);
      qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", "riscv,pmu");
-    riscv_pmu_generate_fdt_node(ms->fdt, hart.cfg.pmu_num, pmu_name);
+    riscv_pmu_generate_fdt_node(ms->fdt, hart->cfg.pmu_num, pmu_name);

g_free(pmu_name);

  }
diff --git a/include/hw/riscv/riscv_hart.h b/include/hw/riscv/riscv_hart.h
index bbc21cdc9a..a5393c361b 100644
--- a/include/hw/riscv/riscv_hart.h
+++ b/include/hw/riscv/riscv_hart.h
@@ -38,7 +38,23 @@ struct RISCVHartArrayState {
      uint32_t hartid_base;
      char *cpu_type;
      uint64_t resetvec;
-    RISCVCPU *harts;
+    RISCVCPU **harts;
  };

+/**

+ * riscv_array_get_hart:
+ */
+static inline RISCVCPU *riscv_array_get_hart(RISCVHartArrayState *harts, int i)
+{
+    return harts->harts[i];
+}


I don't see too much gain in this API because you'll still need a 
RISCVHartArrayState
instance anyways, which is the most annoying part. E.g:

-    cpu = &s->soc[0].harts[0];
+    cpu = riscv_array_get_hart(&s->soc[0], 0);

+
+/**
+ * riscv_array_get_num_harts:
+ */
+static inline unsigned riscv_array_get_num_harts(RISCVHartArrayState *harts)
+{
+    return harts->num_harts;
+}


Same with this API, which you didn't end up using in this patch. Thanks,


Daniel

+
  #endif

[Prev in Thread]

Current Thread

[Next in Thread]

[RFC PATCH] hw/riscv: hart: allow other cpu instance, Nikita Shubin, 2023/07/27
- Re: [RFC PATCH] hw/riscv: hart: allow other cpu instance, Daniel Henrique Barboza <=

Prev by Date: Re: [RFC PATCH 07/24] target/arm: Use GDBFeature for dynamic XML
Next by Date: [PATCH] disas/riscv: Further correction to LUI disassembly
Previous by thread: [RFC PATCH] hw/riscv: hart: allow other cpu instance
Next by thread: [PATCH v2] target/riscv: don't read write-only CSR
Index(es):
- Date
- Thread