Re: [PATCH v2 24/36] target/arm: Add key parameter to add_cpreg_to_hashtable

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 25/9/25 16:51, Peter Maydell wrote:
> On Thu, 25 Sept 2025 at 12:07, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 16/9/25 16:22, Richard Henderson wrote:
>>> Hoist the computation of key into the caller, where
>>> state is a known constant.
> 
>>> @@ -7587,18 +7582,22 @@ static void add_cpreg_to_hashtable_aa64(ARMCPU *cpu, const ARMCPRegInfo *r,
>>>             */
>>>            ARMCPRegInfo nxs_ri = *r;
>>>            g_autofree char *name = g_strdup_printf("%sNXS", r->name);
>>> +        uint32_t nxs_key;
>>>
>>>            assert(nxs_ri.crn < 0xf);
>>>            nxs_ri.crn++;
>>> +        nxs_key = key + (1 << CP_REG_ARM64_SYSREG_CRN_SHIFT);
>>
>> This is new but not mentioned. While the CRN bit is know to be 0,
>> we usually use '|' to set a bit, not '+'. Preferably using '|':
> 
> I thought so too at first glance -- but what we're doing here
> is adding one to crn (there's a comment in this function that's
> just outside the context of the diff that explains this).
> Since crn is both in the reginfo field and also encoded into
> the key, we need to increment both the crn and the bitfield
> inside the key. As it happens, at the moment all the regdefs
> with ARM_CP_ADD_TLBI_NXS have crn == 8 and so whether we add
> or OR makes no difference, but conceptually the addition is
> correct.
> 
> I have added a comment
>   /* Also increment the CRN field inside the key value */
> to hopefully make it a bit clearer that we're doing an
> increment operation here.

Thanks!