On Thu, Mar 12, 2026 at 4:37 PM Taylor Simpson <[email protected]> wrote:
> > > On Tue, Mar 10, 2026 at 10:08 PM Brian Cain <[email protected]> > wrote: > >> From: Brian Cain <[email protected]> >> >> Signed-off-by: Brian Cain <[email protected]> >> --- >> target/hexagon/cpu.h | 7 +++ >> target/hexagon/cpu_helper.h | 3 ++ >> target/hexagon/cpu.c | 26 +++++++++- >> target/hexagon/cpu_helper.c | 100 ++++++++++++++++++++++++++++++++++++ >> target/hexagon/op_helper.c | 4 +- >> 5 files changed, 136 insertions(+), 4 deletions(-) >> >> diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h >> index 0017be9dff7..1a3c9014455 100644 >> --- a/target/hexagon/cpu.h >> +++ b/target/hexagon/cpu.h >> @@ -207,6 +207,13 @@ G_NORETURN void >> hexagon_raise_exception_err(CPUHexagonState *env, >> uint32_t exception, >> uintptr_t pc); >> >> +#ifndef CONFIG_USER_ONLY >> +uint32_t hexagon_greg_read(CPUHexagonState *env, uint32_t reg); >> +uint32_t hexagon_sreg_read(CPUHexagonState *env, uint32_t reg); >> +void hexagon_gdb_sreg_write(CPUHexagonState *env, uint32_t reg, uint32_t >> val); >> +void hexagon_cpu_soft_reset(CPUHexagonState *env); >> +#endif >> + >> > > The first three above belong in a different patch. > I'll fix this. > > > >> diff --git a/target/hexagon/cpu_helper.c b/target/hexagon/cpu_helper.c >> index 6fbf5fc8e2f..c251790e5a5 100644 >> +static void set_enable_mask(CPUHexagonState *env) >> +{ >> + g_assert(bql_locked()); >> + >> + HexagonCPU *cpu = env_archcpu(env); >> + const uint32_t modectl = cpu->globalregs ? >> + hexagon_globalreg_read(cpu->globalregs, HEX_SREG_MODECTL, >> + env->threadId) : 0; >> + uint32_t thread_enabled_mask = GET_FIELD(MODECTL_E, modectl); >> + thread_enabled_mask |= 0x1 << env->threadId; >> + SET_SYSTEM_FIELD(env, HEX_SREG_MODECTL, MODECTL_E, >> thread_enabled_mask); >> > > Do you want to set modectl when cpu->globalregs is false? > IMO it's near meaningless to consider what it means to be a Hexagon DSP when globalregs is false. But since there's tests that exercise the "null" machine configuration without anything connected, it doesn't always make sense to assert globalregs either. So ... sure - I can skip modectl or just early return, because ... *shrug* what does this thing do without that integral piece? Who knows? > > >> +} >> + >> +static uint32_t clear_enable_mask(CPUHexagonState *env) >> +{ >> + g_assert(bql_locked()); >> + >> + HexagonCPU *cpu = env_archcpu(env); >> + const uint32_t modectl = cpu->globalregs ? >> + hexagon_globalreg_read(cpu->globalregs, HEX_SREG_MODECTL, >> + env->threadId) : 0; >> + uint32_t thread_enabled_mask = GET_FIELD(MODECTL_E, modectl); >> + thread_enabled_mask &= ~(0x1 << env->threadId); >> + SET_SYSTEM_FIELD(env, HEX_SREG_MODECTL, MODECTL_E, >> thread_enabled_mask); >> > > Ditto > > >> + return thread_enabled_mask; >> > > It's strange that set_enable_mask returns void and clear_enable_mask > returns the new mask. > > I'll rethink the interface and review the callers. > Thanks, > Taylor > >
