> -----Original Message-----
> From: Richard Henderson <richard.hender...@linaro.org>
> Sent: Thursday, March 20, 2025 10:34 AM
> To: Sid Manning <sidn...@quicinc.com>; ltaylorsimp...@gmail.com;
> 'Philippe Mathieu-Daudé' <phi...@linaro.org>; 'Brian Cain'
> <brian.c...@oss.qualcomm.com>; qemu-devel@nongnu.org
> Cc: Matheus Bernardino (QUIC) <quic_mathb...@quicinc.com>;
> a...@rev.ng; a...@rev.ng; Marco Liebel (QUIC)
> <quic_mlie...@quicinc.com>; alex.ben...@linaro.org; Mark Burton (QUIC)
> <quic_mbur...@quicinc.com>; Brian Cain <bc...@quicinc.com>
> Subject: Re: [PATCH 28/38] target/hexagon: Initialize htid, modectl regs
>
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
>
> On 3/19/25 09:08, Sid Manning wrote:
> >
> >
> >> -----Original Message-----
> >> From: Richard Henderson <richard.hender...@linaro.org>
> >> Sent: Thursday, March 13, 2025 2:07 PM
> >> To: ltaylorsimp...@gmail.com; 'Philippe Mathieu-Daudé'
> >> <phi...@linaro.org>; 'Brian Cain' <brian.c...@oss.qualcomm.com>;
> >> qemu- de...@nongnu.org
> >> Cc: Matheus Bernardino (QUIC) <quic_mathb...@quicinc.com>;
> >> a...@rev.ng; a...@rev.ng; Marco Liebel (QUIC)
> >> <quic_mlie...@quicinc.com>; alex.ben...@linaro.org; Mark Burton
> >> (QUIC) <quic_mbur...@quicinc.com>; Sid Manning
> <sidn...@quicinc.com>;
> >> Brian Cain <bc...@quicinc.com>
> >> Subject: Re: [PATCH 28/38] target/hexagon: Initialize htid, modectl
> >> regs
> >>
> >> WARNING: This email originated from outside of Qualcomm. Please be
> >> wary of any links or attachments, and do not enable macros.
> >>
> >> On 3/13/25 11:47, ltaylorsimp...@gmail.com wrote:
> >>> What we are trying to model is an instance of a Hexagon that has a
> >>> number
> >> of threads and some resources that are shared. The shared resources
> >> include the TLB and global S registers. The initial thought was to
> >> tie the shared resources to the thread with cpu_index == 0. If we
> >> were to model a Qualcomm SoC, there would be multiple ARM cores and
> >> multiple Hexagon instances. Each Hexagon instance would have
> >> distinct shared resources. So, you are correct that using cpu_index is not
> going to scale.
> >>>
> >>> What is the recommended way to model this? I see a "nr_threads"
> >>> field in
> >> CPUCore but no clear way to find the threads. PPC has some cores
> >> that add a "threads" field. Should we follow this approach?
> >>
> >> I recommend that the shared resources be modeled as a separate
> >> Object, which is linked via object_property_add_link to all of the cpus
> >> that
> use it.
> >>
> > [Sid Manning]
> > Hi Richard,
> > An example of shared resources would be the system registers. They are
> broken down into 2 regions. Each thread has its own copy of system
> registers 0-15 while registers 16-63 are global. Right now CPUHexagonState
> contains a pointer, g_sreg which points back to cpu[0]'s state thus keeping
> one copy of the global registers, accesses are done with BQL held to avoid
> race conditions.
> >
> > Your suggestion is to create a new object to represent the set of global
> system registers, I tried this:
> >
> > #define TYPE_HEXAGON_G_SREG "hexagon.global_sreg"
> > OBJECT_DECLARE_SIMPLE_TYPE(HexagonGlobalSREGState,
> HEXAGON_G_SREG)
> > struct HexagonGlobalSREGState {
> > SysBusDevice parent_obj;
>
> SysBusDevice is more than you need -- Object is sufficient here.
[Sid Manning]
Thanks! Will change that to Object.
>
> > uint32_t regs[64];
> > };
> >
> > In our virtual machine init:
> > vms->g_sreg =
> HEXAGON_G_SREG(qdev_new(TYPE_HEXAGON_G_SREG));
> >
> > and
> > object_property_set_link(OBJECT(cpu), "global-sreg",
> > OBJECT(vms->g_sreg), &error_abort);
> >
> > to attach the global regs to the cpu, but the above doesn't update cpu
> elements the same way calls to qdev_prop_set_uint32 will do,
> object_property_set_link doesn’t error out and returns true.
>
> Did you add the DEFINE_PROP_LINK to match? I'd expect something like
>
> DEFINE_PROP_LINK("global-sreg", HexagonCPU, g_sreg,
> TYPE_HEXAGON_G_SREG, HexagonGlobalSREGState *),
>
> Beyond that, I guess I'd have to see an actual patch to work out what's
> wrong.
[Sid Manning]
Yes, PROP_LINK above is almost exactly what I added.
Below is a patch representing what I tried. I hoped that cpu->global_sreg
would be updated after the call to object_property_set_link but it was not, in
the patch below I manually set it.
diff --git a/hw/hexagon/sysreg.h b/hw/hexagon/sysreg.h
new file mode 100644
index 0000000000..d7204896cf
--- /dev/null
+++ b/hw/hexagon/sysreg.h
@@ -0,0 +1,47 @@
+/*
+ * Hexagon system reg
+ * FIXME
+ */
+
+#ifndef HW_HEXAGON_HART_H
+#define HW_HEXAGON_HART_H
+#if !defined(CONFIG_USER_ONLY)
+#include "hw/sysbus.h"
+#include "qom/object.h"
+
+#define NUM_SREGS 64
+struct HexagonGlobalSREGState {
+ struct Object parent_obj;
+ uint32_t regs[NUM_SREGS];
+};
+
+#define TYPE_HEXAGON_G_SREG "hexagon.global_sreg"
+OBJECT_DECLARE_SIMPLE_TYPE(HexagonGlobalSREGState, HEXAGON_G_SREG)
+
+static void hexagon_global_sreg_init(Object *obj)
+{
+ HexagonGlobalSREGState *s = HEXAGON_G_SREG(obj);
+ /*
+ * The first 16 registers are thread local and should not come from
+ * this structure
+ */
+ for (int i = 0; i < 16; i++) {
+ s->regs[i] = 0xffffffff;
+ }
+}
+
+static const TypeInfo hexagon_sreg_info = {
+ .name = TYPE_HEXAGON_G_SREG,
+ .parent = TYPE_DEVICE,
+ .instance_size = sizeof(struct HexagonGlobalSREGState),
+ .instance_init = hexagon_global_sreg_init,
+};
+
+__attribute__ ((unused))
+static void hexagon_sreg_register_types(void)
+{
+ type_register_static(&hexagon_sreg_info);
+}
+#endif
+#endif
+
diff --git a/hw/hexagon/virt.c b/hw/hexagon/virt.c
index 1e7ac4e5b7..d2d599ac1d 100644
--- a/hw/hexagon/virt.c
+++ b/hw/hexagon/virt.c
@@ -10,12 +10,14 @@
#include "hw/char/pl011.h"
#include "hw/core/sysbus-fdt.h"
#include "hw/hexagon/hexagon.h"
+#include "hw/hexagon/sysreg.h"
#include "hw/hexagon/virt.h"
#include "hw/loader.h"
#include "hw/qdev-properties.h"
#include "hw/register.h"
#include "hw/timer/qct-qtimer.h"
#include "qemu/error-report.h"
+#include "qapi/error.h"
#include "qemu/guest-random.h"
#include "qemu/units.h"
#include "elf.h"
@@ -335,6 +337,7 @@ static void virt_init(MachineState *ms)
cpu_model = HEXAGON_CPU_TYPE_NAME("v73");
}
+ vms->g_sreg = HEXAGON_G_SREG(qdev_new(TYPE_HEXAGON_G_SREG));
HexagonCPU *cpu_0 = NULL;
for (int i = 0; i < ms->smp.cpus; i++) {
HexagonCPU *cpu = HEXAGON_CPU(object_new(ms->cpu_type));
@@ -356,6 +359,14 @@ static void virt_init(MachineState *ms)
qdev_prop_set_uint32(DEVICE(cpu), "qtimer-base-addr",
m_cfg->qtmr_region);
qdev_prop_set_uint32(DEVICE(cpu), "jtlb-entries",
m_cfg->cfgtable.jtlb_size_entries);
+ bool rc = object_property_set_link(OBJECT(cpu), "global-sreg",
+ OBJECT(vms->g_sreg), &error_abort);
+ g_assert(rc == true);
+
+ /* This is doing what I think object_property_set_link should do.*/
+ cpu->global_sreg = vms->g_sreg;
+
+
if (!qdev_realize_and_unref(DEVICE(cpu), NULL, errp)) {
return;
@@ -413,3 +424,5 @@ static const TypeInfo virt_machine_types[] = { {
} };
DEFINE_TYPES(virt_machine_types)
+
+type_init(hexagon_sreg_register_types)
diff --git a/include/hw/hexagon/virt.h b/include/hw/hexagon/virt.h
index 0c165a786d..dcd09d50b1 100644
--- a/include/hw/hexagon/virt.h
+++ b/include/hw/hexagon/virt.h
@@ -9,6 +9,7 @@
#define HW_HEXAGONVIRT_H
#include "hw/boards.h"
+#include "hw/hexagon/sysreg.h"
#include "target/hexagon/cpu.h"
struct HexagonVirtMachineState {
@@ -22,6 +23,7 @@ struct HexagonVirtMachineState {
MemoryRegion tcm;
MemoryRegion vtcm;
DeviceState *l2vic;
+ HexagonGlobalSREGState *g_sreg;
};
void hexagon_load_fdt(const struct HexagonVirtMachineState *vms);
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index c649aef99e..9773ee0be8 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -80,6 +80,8 @@ static const Property hexagon_cpu_properties[] = {
DEFINE_PROP_UINT32("exec-start-addr", HexagonCPU, boot_addr,
0xffffffffULL),
DEFINE_PROP_UINT64("config-table-addr", HexagonCPU, config_table_addr,
0xffffffffULL),
+ DEFINE_PROP_LINK("global-sreg", HexagonCPU, global_sreg,
+ TYPE_HEXAGON_G_SREG, HexagonGlobalSREGState *),
#endif
DEFINE_PROP_UINT32("dsp-rev", HexagonCPU, rev_reg, 0),
DEFINE_PROP_BOOL("lldb-compat", HexagonCPU, lldb_compat, false),
@@ -378,6 +380,11 @@ static void hexagon_cpu_reset_hold(Object *obj, ResetType
type)
CPUState *cs = CPU(obj);
HexagonCPUClass *mcc = HEXAGON_CPU_GET_CLASS(obj);
CPUHexagonState *env = cpu_env(cs);
+#ifndef CONFIG_USER_ONLY
+ HexagonCPU *cpu = HEXAGON_CPU(cs);
+ env->g_sreg = cpu->global_sreg->regs;
+#endif
+
if (mcc->parent_phases.hold) {
mcc->parent_phases.hold(obj, type);
@@ -389,11 +396,6 @@ static void hexagon_cpu_reset_hold(Object *obj, ResetType
type)
set_float_default_nan_pattern(0b11111111, &env->fp_status);
#ifndef CONFIG_USER_ONLY
- HexagonCPU *cpu = HEXAGON_CPU(cs);
-
- if (cs->cpu_index == 0) {
- memset(env->g_sreg, 0, sizeof(target_ulong) * NUM_SREGS);
- }
memset(env->t_sreg, 0, sizeof(target_ulong) * NUM_SREGS);
memset(env->greg, 0, sizeof(target_ulong) * NUM_GREGS);
@@ -468,13 +470,6 @@ static void hexagon_cpu_realize(DeviceState *dev, Error
**errp)
CPUHexagonState *env = cpu_env(cs);
#ifndef CONFIG_USER_ONLY
hex_mmu_realize(env);
- if (cs->cpu_index == 0) {
- env->g_sreg = g_new0(target_ulong, NUM_SREGS);
- } else {
- CPUState *cpu0 = qemu_get_cpu(0);
- CPUHexagonState *env0 = cpu_env(cpu0);
- env->g_sreg = env0->g_sreg;
- }
#endif
if (cs->cpu_index == 0) {
env->g_pcycle_base = g_malloc0(sizeof(*env->g_pcycle_base));
diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h
index 8b334068e2..716dd8253b 100644
--- a/target/hexagon/cpu.h
+++ b/target/hexagon/cpu.h
@@ -19,10 +19,10 @@
#define HEXAGON_CPU_H
#include "fpu/softfloat-types.h"
+#include "hw/hexagon/sysreg.h"
#define NUM_GREGS 32
#define GREG_WRITES_MAX 32
-#define NUM_SREGS 64
#define SREG_WRITES_MAX 64
#include "cpu-qom.h"
@@ -199,6 +199,7 @@ struct ArchCPU {
uint32_t hvx_contexts;
uint32_t boot_addr;
uint64_t config_table_addr;
+ HexagonGlobalSREGState *global_sreg;
#endif
};
>
>
> r~
