> -----Original Message-----
> From: ltaylorsimpson@gmail.com <ltaylorsimpson@gmail.com>
> Sent: Monday, March 17, 2025 12:44 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 06/39] target/hexagon: Implement {g,s}etimask helpers
>
> 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:28 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 06/39] target/hexagon: Implement {g,s}etimask helpers
> >
> > From: Brian Cain <bcain@quicinc.com>
> >
> > Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> > ---
> > target/hexagon/op_helper.c | 31 +++++++++++++++++++++++++++++--
> > 1 file changed, 29 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
> > index 9f79b1a20c..83088cfaa3 100644
> > --- a/target/hexagon/op_helper.c
> > +++ b/target/hexagon/op_helper.c
> > @@ -1468,12 +1468,39 @@ void HELPER(resume)(CPUHexagonState *env,
> > uint32_t mask)
> >
> > uint32_t HELPER(getimask)(CPUHexagonState *env, uint32_t tid) {
> > - g_assert_not_reached();
> > + CPUState *cs;
> > + CPU_FOREACH(cs) {
> > + HexagonCPU *found_cpu = HEXAGON_CPU(cs);
> > + CPUHexagonState *found_env = &found_cpu->env;
> > + if (found_env->threadId == tid) {
> > + target_ulong imask = arch_get_system_reg(found_env,
> > HEX_SREG_IMASK);
> > + qemu_log_mask(CPU_LOG_INT, "%s: tid %d imask = 0x%x\n",
> > + __func__, env->threadId,
> > + (unsigned)GET_FIELD(IMASK_MASK, imask));
> > + return GET_FIELD(IMASK_MASK, imask);
> > + }
> > + }
> > + return 0;
> > }
> >
> > void HELPER(setimask)(CPUHexagonState *env, uint32_t pred, uint32_t
> > imask) {
>
> The name pred sounds like a predicate register. Use tid instead.
[Sid Manning]
In this case pred is referring to the predicate register.
setimask(Pt,Rs)
The setimask instruction writes the IMASK for the thread indicated by the low bits of
predicate Pt. Register Rs contains the 32-bit mask value to be written. For Pt values outside of
[0-NUM_THREADS-1], the results are undefined.
>
> > - g_assert_not_reached();
> > + CPUState *cs;
> > +
> > + BQL_LOCK_GUARD();
> > + CPU_FOREACH(cs) {
> > + HexagonCPU *found_cpu = HEXAGON_CPU(cs);
> > + CPUHexagonState *found_env = &found_cpu->env;
> > +
> > + if (pred == found_env->threadId) {
> > + SET_SYSTEM_FIELD(found_env, HEX_SREG_IMASK, IMASK_MASK,
> > imask);
> > + qemu_log_mask(CPU_LOG_INT, "%s: tid %d imask 0x%x\n",
> > + __func__, found_env->threadId, imask);
> > + hex_interrupt_update(env);
>
> Shouldn't this be found_env?
[Sid Manning]
It could be either, hex_interrupt_update is just using env to get the global ipend register.
>
> > + return;
> > + }
> > + }
> > + hex_interrupt_update(env);
>
> Do you need to update if the thread wasn't found?
[Sid Manning]
This may not be strictly needed since the only way to reach this line is for setimask to pass a predicate that is outside the range of valid htids. We can remove this and add a guest error.
>
> > }
> >
> > static bool handle_pmu_sreg_write(CPUHexagonState *env, uint32_t reg,
> > --
> > 2.34.1
>