On 3/21/2025 4:48 PM, Sid Manning wrote:
>
>> -----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.
>
Fixed in v2 but I see now that we used the wrong format specifier - will
fix it for v3.
>>> }
>>>
>>> static bool handle_pmu_sreg_write(CPUHexagonState *env, uint32_t reg,
>>> --
>>> 2.34.1