| Message ID | 20250301052628.1011210-29-brian.cain@oss.qualcomm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hexagon system emu, part 1/3 | expand |
> -----Original Message----- > From: Brian Cain <brian.cain@oss.qualcomm.com> > Sent: Friday, February 28, 2025 11:26 PM > To: qemu-devel@nongnu.org > Cc: brian.cain@oss.qualcomm.com; richard.henderson@linaro.org; > philmd@linaro.org; quic_mathbern@quicinc.com; ale@rev.ng; anjo@rev.ng; > quic_mliebel@quicinc.com; ltaylorsimpson@gmail.com; > alex.bennee@linaro.org; quic_mburton@quicinc.com; > sidneym@quicinc.com; Brian Cain <bcain@quicinc.com> > Subject: [PATCH 28/38] target/hexagon: Initialize htid, modectl regs > > From: Brian Cain <bcain@quicinc.com> > > Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com> > --- > target/hexagon/cpu.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c index > 36a93cc22f..2b6a707fca 100644 > --- a/target/hexagon/cpu.c > +++ b/target/hexagon/cpu.c > @@ -26,6 +26,7 @@ > #include "fpu/softfloat-helpers.h" > #include "tcg/tcg.h" > #include "exec/gdbstub.h" > +#include "cpu_helper.h" > > static void hexagon_v66_cpu_init(Object *obj) { } static void > hexagon_v67_cpu_init(Object *obj) { } @@ -290,11 +291,18 @@ 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); > + > + if (cs->cpu_index == 0) { > + arch_set_system_reg(env, HEX_SREG_MODECTL, 0x1); > + } Combine with previous check cs->cpu_index == 0? > + arch_set_system_reg(env, HEX_SREG_HTID, cs->cpu_index); > #endif > } Otherwise Reviewed-by: Taylor Simpson <ltaylorsimpson@gmail.com>
> -----Original Message----- > From: ltaylorsimpson@gmail.com <ltaylorsimpson@gmail.com> > Sent: Tuesday, March 11, 2025 6:27 PM > To: 'Brian Cain' <brian.cain@oss.qualcomm.com>; qemu-devel@nongnu.org > Cc: richard.henderson@linaro.org; philmd@linaro.org; Matheus Bernardino > (QUIC) <quic_mathbern@quicinc.com>; ale@rev.ng; anjo@rev.ng; Marco > Liebel (QUIC) <quic_mliebel@quicinc.com>; alex.bennee@linaro.org; Mark > Burton (QUIC) <quic_mburton@quicinc.com>; Sid Manning > <sidneym@quicinc.com>; Brian Cain <bcain@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. > > > -----Original Message----- > > From: Brian Cain <brian.cain@oss.qualcomm.com> > > Sent: Friday, February 28, 2025 11:26 PM > > To: qemu-devel@nongnu.org > > Cc: brian.cain@oss.qualcomm.com; richard.henderson@linaro.org; > > philmd@linaro.org; quic_mathbern@quicinc.com; ale@rev.ng; > anjo@rev.ng; > > quic_mliebel@quicinc.com; ltaylorsimpson@gmail.com; > > alex.bennee@linaro.org; quic_mburton@quicinc.com; > sidneym@quicinc.com; > > Brian Cain <bcain@quicinc.com> > > Subject: [PATCH 28/38] target/hexagon: Initialize htid, modectl regs > > > > From: Brian Cain <bcain@quicinc.com> > > > > Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com> > > --- > > target/hexagon/cpu.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c index > > 36a93cc22f..2b6a707fca 100644 > > --- a/target/hexagon/cpu.c > > +++ b/target/hexagon/cpu.c > > @@ -26,6 +26,7 @@ > > #include "fpu/softfloat-helpers.h" > > #include "tcg/tcg.h" > > #include "exec/gdbstub.h" > > +#include "cpu_helper.h" > > > > static void hexagon_v66_cpu_init(Object *obj) { } static void > > hexagon_v67_cpu_init(Object *obj) { } @@ -290,11 +291,18 @@ 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); > > + > > + if (cs->cpu_index == 0) { > > + arch_set_system_reg(env, HEX_SREG_MODECTL, 0x1); > > + } > > Combine with previous check cs->cpu_index == 0? [Sid Manning] Yes, will make that change, thanks! > > > + arch_set_system_reg(env, HEX_SREG_HTID, cs->cpu_index); > > #endif > > } > > Otherwise > Reviewed-by: Taylor Simpson <ltaylorsimpson@gmail.com> >
On 1/3/25 06:26, Brian Cain wrote: > From: Brian Cain <bcain@quicinc.com> > > Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com> > --- > target/hexagon/cpu.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c > index 36a93cc22f..2b6a707fca 100644 > --- a/target/hexagon/cpu.c > +++ b/target/hexagon/cpu.c > @@ -26,6 +26,7 @@ > #include "fpu/softfloat-helpers.h" > #include "tcg/tcg.h" > #include "exec/gdbstub.h" > +#include "cpu_helper.h" > > static void hexagon_v66_cpu_init(Object *obj) { } > static void hexagon_v67_cpu_init(Object *obj) { } > @@ -290,11 +291,18 @@ 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) { This doesn't scale to heterogeneous emulation. > 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); > + > + if (cs->cpu_index == 0) { Ditto. > + arch_set_system_reg(env, HEX_SREG_MODECTL, 0x1); > + } > + arch_set_system_reg(env, HEX_SREG_HTID, cs->cpu_index); > #endif > } >
On 3/12/2025 2:19 PM, Philippe Mathieu-Daudé wrote: > On 1/3/25 06:26, Brian Cain wrote: >> From: Brian Cain <bcain@quicinc.com> >> >> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com> >> --- >> target/hexagon/cpu.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c >> index 36a93cc22f..2b6a707fca 100644 >> --- a/target/hexagon/cpu.c >> +++ b/target/hexagon/cpu.c >> @@ -26,6 +26,7 @@ >> #include "fpu/softfloat-helpers.h" >> #include "tcg/tcg.h" >> #include "exec/gdbstub.h" >> +#include "cpu_helper.h" >> static void hexagon_v66_cpu_init(Object *obj) { } >> static void hexagon_v67_cpu_init(Object *obj) { } >> @@ -290,11 +291,18 @@ 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) { > > This doesn't scale to heterogeneous emulation. > If we have a target-specific index here (instead of cpu_index) guarding the "g_sreg" allocation shared by these Hexagon vCPUs, does that suffice? Or is the problem the shared allocation itself? Could a heterogeneous emulation configuration consist of multiple instances of (same-architecture) vCPU-groups? >> 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); >> + >> + if (cs->cpu_index == 0) { > > Ditto. > >> + arch_set_system_reg(env, HEX_SREG_MODECTL, 0x1); >> + } >> + arch_set_system_reg(env, HEX_SREG_HTID, cs->cpu_index); >> #endif >> } >
On 13/3/25 00:10, Brian Cain wrote: > > On 3/12/2025 2:19 PM, Philippe Mathieu-Daudé wrote: >> On 1/3/25 06:26, Brian Cain wrote: >>> From: Brian Cain <bcain@quicinc.com> >>> >>> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com> >>> --- >>> target/hexagon/cpu.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c >>> index 36a93cc22f..2b6a707fca 100644 >>> --- a/target/hexagon/cpu.c >>> +++ b/target/hexagon/cpu.c >>> @@ -26,6 +26,7 @@ >>> #include "fpu/softfloat-helpers.h" >>> #include "tcg/tcg.h" >>> #include "exec/gdbstub.h" >>> +#include "cpu_helper.h" >>> static void hexagon_v66_cpu_init(Object *obj) { } >>> static void hexagon_v67_cpu_init(Object *obj) { } >>> @@ -290,11 +291,18 @@ 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) { >> >> This doesn't scale to heterogeneous emulation. >> > > If we have a target-specific index here (instead of cpu_index) guarding > the "g_sreg" allocation shared by these Hexagon vCPUs, does that > suffice? Or is the problem the shared allocation itself? I'm not sure that suffices, but it is still better from my PoV. Let's assume we instantiate 4 ARM cores, then 2 HEX ones. The first Hexagon core has cpu_index=5. Now for the same example machine we instantiate first the Hexagon cores, then the ARM ones. The first Hexagon core has cpu_index=0. > Could a heterogeneous emulation configuration consist of multiple > instances of (same-architecture) vCPU-groups? Up to you if you want to model multiple hexagon SoCs in the same machine ;) Note in that case you could use a CPUClusterState to group.
> -----Original Message----- > From: Philippe Mathieu-Daudé <philmd@linaro.org> > Sent: Wednesday, March 12, 2025 6:40 PM > To: Brian Cain <brian.cain@oss.qualcomm.com>; qemu-devel@nongnu.org > Cc: richard.henderson@linaro.org; quic_mathbern@quicinc.com; > ale@rev.ng; anjo@rev.ng; quic_mliebel@quicinc.com; > ltaylorsimpson@gmail.com; alex.bennee@linaro.org; > quic_mburton@quicinc.com; sidneym@quicinc.com; Brian Cain > <bcain@quicinc.com> > Subject: Re: [PATCH 28/38] target/hexagon: Initialize htid, modectl regs > > On 13/3/25 00:10, Brian Cain wrote: > > > > On 3/12/2025 2:19 PM, Philippe Mathieu-Daudé wrote: > >> On 1/3/25 06:26, Brian Cain wrote: > >>> From: Brian Cain <bcain@quicinc.com> > >>> > >>> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com> > >>> --- > >>> target/hexagon/cpu.c | 8 ++++++++ > >>> 1 file changed, 8 insertions(+) > >>> > >>> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c index > >>> 36a93cc22f..2b6a707fca 100644 > >>> --- a/target/hexagon/cpu.c > >>> +++ b/target/hexagon/cpu.c > >>> @@ -26,6 +26,7 @@ > >>> #include "fpu/softfloat-helpers.h" > >>> #include "tcg/tcg.h" > >>> #include "exec/gdbstub.h" > >>> +#include "cpu_helper.h" > >>> static void hexagon_v66_cpu_init(Object *obj) { } > >>> static void hexagon_v67_cpu_init(Object *obj) { } @@ -290,11 > >>> +291,18 @@ 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) { > >> > >> This doesn't scale to heterogeneous emulation. > >> > > > > If we have a target-specific index here (instead of cpu_index) > > guarding the "g_sreg" allocation shared by these Hexagon vCPUs, does > > that suffice? Or is the problem the shared allocation itself? > > I'm not sure that suffices, but it is still better from my PoV. > > Let's assume we instantiate 4 ARM cores, then 2 HEX ones. The first Hexagon > core has cpu_index=5. Now for the same example machine we instantiate > first the Hexagon cores, then the ARM ones. The first Hexagon core has > cpu_index=0. > > > Could a heterogeneous emulation configuration consist of multiple > > instances of (same-architecture) vCPU-groups? > > Up to you if you want to model multiple hexagon SoCs in the same machine > ;) Note in that case you could use a CPUClusterState to group. 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? HTH, Taylor
On 3/13/25 11:47, ltaylorsimpson@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. The linking would be under the control of the board model and/or the SoC model. r~
> -----Original Message----- > From: Richard Henderson <richard.henderson@linaro.org> > Sent: Thursday, March 13, 2025 2:07 PM > To: ltaylorsimpson@gmail.com; 'Philippe Mathieu-Daudé' > <philmd@linaro.org>; 'Brian Cain' <brian.cain@oss.qualcomm.com>; qemu- > devel@nongnu.org > Cc: Matheus Bernardino (QUIC) <quic_mathbern@quicinc.com>; > ale@rev.ng; anjo@rev.ng; Marco Liebel (QUIC) > <quic_mliebel@quicinc.com>; alex.bennee@linaro.org; Mark Burton (QUIC) > <quic_mburton@quicinc.com>; Sid Manning <sidneym@quicinc.com>; Brian > Cain <bcain@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, ltaylorsimpson@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; 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. A straight assignment would work, cpu->global_sreg = vms->g_sreg; but that isn't what you are suggesting. Can you help me understand what I'm doing wrong. Thanks, > The linking would be under the control of the board model and/or the SoC > model. > > > r~
On 3/19/25 09:08, Sid Manning wrote: > > >> -----Original Message----- >> From: Richard Henderson <richard.henderson@linaro.org> >> Sent: Thursday, March 13, 2025 2:07 PM >> To: ltaylorsimpson@gmail.com; 'Philippe Mathieu-Daudé' >> <philmd@linaro.org>; 'Brian Cain' <brian.cain@oss.qualcomm.com>; qemu- >> devel@nongnu.org >> Cc: Matheus Bernardino (QUIC) <quic_mathbern@quicinc.com>; >> ale@rev.ng; anjo@rev.ng; Marco Liebel (QUIC) >> <quic_mliebel@quicinc.com>; alex.bennee@linaro.org; Mark Burton (QUIC) >> <quic_mburton@quicinc.com>; Sid Manning <sidneym@quicinc.com>; Brian >> Cain <bcain@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, ltaylorsimpson@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. > 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. r~
> -----Original Message----- > From: Richard Henderson <richard.henderson@linaro.org> > Sent: Thursday, March 20, 2025 10:34 AM > To: Sid Manning <sidneym@quicinc.com>; ltaylorsimpson@gmail.com; > 'Philippe Mathieu-Daudé' <philmd@linaro.org>; 'Brian Cain' > <brian.cain@oss.qualcomm.com>; qemu-devel@nongnu.org > Cc: Matheus Bernardino (QUIC) <quic_mathbern@quicinc.com>; > ale@rev.ng; anjo@rev.ng; Marco Liebel (QUIC) > <quic_mliebel@quicinc.com>; alex.bennee@linaro.org; Mark Burton (QUIC) > <quic_mburton@quicinc.com>; Brian Cain <bcain@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.henderson@linaro.org> > >> Sent: Thursday, March 13, 2025 2:07 PM > >> To: ltaylorsimpson@gmail.com; 'Philippe Mathieu-Daudé' > >> <philmd@linaro.org>; 'Brian Cain' <brian.cain@oss.qualcomm.com>; > >> qemu- devel@nongnu.org > >> Cc: Matheus Bernardino (QUIC) <quic_mathbern@quicinc.com>; > >> ale@rev.ng; anjo@rev.ng; Marco Liebel (QUIC) > >> <quic_mliebel@quicinc.com>; alex.bennee@linaro.org; Mark Burton > >> (QUIC) <quic_mburton@quicinc.com>; Sid Manning > <sidneym@quicinc.com>; > >> Brian Cain <bcain@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, ltaylorsimpson@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~
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c index 36a93cc22f..2b6a707fca 100644 --- a/target/hexagon/cpu.c +++ b/target/hexagon/cpu.c @@ -26,6 +26,7 @@ #include "fpu/softfloat-helpers.h" #include "tcg/tcg.h" #include "exec/gdbstub.h" +#include "cpu_helper.h" static void hexagon_v66_cpu_init(Object *obj) { } static void hexagon_v67_cpu_init(Object *obj) { } @@ -290,11 +291,18 @@ 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); + + if (cs->cpu_index == 0) { + arch_set_system_reg(env, HEX_SREG_MODECTL, 0x1); + } + arch_set_system_reg(env, HEX_SREG_HTID, cs->cpu_index); #endif }