Re: [PATCH v2 1/3] arm: handle demuxed ID registers

[Cornelia Huck <cohuck@redhat.com>]

On Thu, Feb 05 2026, Richard Henderson <richard.henderson@linaro.org> wrote:

> On 2/4/26 23:32, Cornelia Huck wrote:
>> For some registers, we do not have a single ID register, but actually
>> an array of values (e.g. CCSIDR_EL1, where the actual value is
>> determined by whatever CSSELR_EL1 points to.) If we want to avoid
>> using a different way to handle registers like that for every
>> instance, we should provide some kind of infrastructure. Therefore,
>> add accessors {GET,SET}_IDREG_DEMUX that are similar to the accessors
>> we already use for regular ID registers.
>> 
>> Tested-by: Alireza Sanaee <alireza.sanaee@huawei.com>
>> Reviewed-by: Sebastian Ott <sebott@redhat.com>
>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>   target/arm/cpu-sysregs.h | 13 +++++++++++++
>>   target/arm/cpu.h         | 20 ++++++++++++++++++++
>>   2 files changed, 33 insertions(+)
>> 
>> diff --git a/target/arm/cpu-sysregs.h b/target/arm/cpu-sysregs.h
>> index 7877a3b06a8e..911f54bc8a4f 100644
>> --- a/target/arm/cpu-sysregs.h
>> +++ b/target/arm/cpu-sysregs.h
>> @@ -35,6 +35,19 @@ typedef enum ARMSysRegs {
>>   
>>   #undef DEF
>>   
>> +/* ID registers that vary based upon another register */
>> +typedef enum ARMIDRegisterDemuxIdx {
>> +    NUM_ID_DEMUX_IDX,
>> +} ARMIDRegisterDemuxIdx;
>> +
>> +/*
>> + * Number of register variants per demuxed register, trying to accommodate
>> + * possible use cases.
>> + * CCSIDR_EL1 currently needs 7*2, could be 7 more with FEAT_MTE2, in which
>> + * case we would need to bump this number.
>> + */
>> +#define ID_DEMUX_ARRAYLEN 16
>> +
>>   extern const uint32_t id_register_sysreg[NUM_ID_IDX];
>>   
>>   int get_sysreg_idx(ARMSysRegs sysreg);
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index 21fee5e840b7..f9d51c0fc187 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -906,6 +906,25 @@ typedef struct {
>>           i_->idregs[REG ## _EL1_IDX];                                    \
>>       })
>>   
>> +#define SET_IDREG_DEMUX(ISAR, REG, INDEX, VALUE)                        \
>> +    ({                                                                  \
>> +        ARMISARegisters *i_ = (ISAR);                                   \
>> +        i_->idregs_demux[REG ## _EL1_DEMUX_IDX][INDEX] = VALUE;         \
>> +    })
>> +
>> +#define GET_IDREG_DEMUX(ISAR, REG, INDEX)                               \
>> +    ({                                                                  \
>> +        ARMISARegisters *i_ = (ISAR);                                   \
>> +        i_->idregs_demux[REG ## _EL1_DEMUX_IDX][INDEX];                 \
>> +    })
>> +
>> +#define COPY_IDREG_DEMUX(ISAR, REG, FROM_INDEX, TO_INDEX)               \
>> +    ({                                                                  \
>> +        ARMISARegisters *i_ = (ISAR);                                   \
>> +        i_->idregs_demux[REG ## _EL1_DEMUX_IDX][TO_INDEX] =             \
>> +            i_->idregs_demux[REG ## _EL1_DEMUX_IDX][FROM_INDEX];        \
>> +    })
>> +
>>   /**
>>    * ARMCPU:
>>    * @env: #CPUARMState
>> @@ -1084,6 +1103,7 @@ struct ArchCPU {
>>           uint32_t dbgdevid1;
>>           uint64_t reset_pmcr_el0;
>>           uint64_t idregs[NUM_ID_IDX];
>> +        uint64_t idregs_demux[NUM_ID_DEMUX_IDX][ID_DEMUX_ARRAYLEN];

[Oops, totally overlooked this email! Sorry about that.]

>
> I'm not keen on this.  Surely we don't expect all multiplexed ID registers to have the 
> same number of sub-registers?  A rectangular matrix isn't a great representation.  Also, 
> you're wasting a hole in idregs[] with CSSIDR_EL1_IDX.
>
> Why not just have the N sub-registers occupy N entries within the regular idregs[]? 
> Something like
>
> #define DEF_MUX(NAME, OP0, OP1, CRN, CRM, OP2, NUM) \
>      NAME##_DEMUX_IDX,                               \
>      NAME##_DEMUX_LAST = NAME##_DEMUX_IDX + NUM - 1,
>
> #define SET_IDREG_DEMUX(ISAR, REG, INDEX, VALUE)                        \
>      ({                                                                  \
>          ARMISARegisters *i_ = (ISAR);                                   \
>          i_->idregs_demux[REG ## _EL1_DEMUX_IDX + INDEX] = VALUE;        \
>      })
>
> etc.

Yeah, I was not overly excited about my solution, either. I'll go and
play with this, thanks.