Re: [PATCH RFCv2 02/20] arm/cpu: Add sysreg definitions in cpu-sysregs.h

[Eric Auger]

Hi Richard,
On 12/12/24 15:37, Richard Henderson wrote:
> On 12/6/24 05:21, Cornelia Huck wrote:
>> +#define SYS_ID_AA64PFR0_EL1                             sys_reg(3,
>> 0, 0, 4, 0)
> ...
>> +typedef struct ARMSysReg {
>> +    int op0;
>> +    int op1;
>> +    int crn;
>> +    int crm;
>> +    int op2;
>> +} ARMSysReg;
> ...
>> +static inline ARMSysReg sys_reg(int op0, int op1, int crn, int crm,
>> int op2)
>> +{
>> +        ARMSysReg sr = {op0, op1, crn, crm, op2};
>> +
>> +        return sr;
>> +}
>
> Not a fan.  Why take 20 bytes to represent these?
sure we can optimize it
>
> Our existing ENCODE_CP_REG and ENCODE_AA64_CP_REG macros seem much
> better, even if the argument ordering doesn't match the column
> ordering in Table D22-2.
>
>> @@ -841,6 +849,51 @@ typedef struct IdRegMap {
>>       uint64_t regs[NR_ID_REGS];
>>   } IdRegMap;
>>   +#define ARM_FEATURE_ID_RANGE_IDX(op0, op1, crn, crm,
>> op2)               \
>> +       
>> ({                                                              \
>> +                __u64 __op1 = (op1) &
>> 3;                                \
>> +                __op1 -= (__op1 ==
>> 3);                                  \
>> +                (__op1 << 6 | ((crm) & 7) << 3 |
>> (op2));                \
>> +        })
>
> Ah, well, this answers my question re patch 1.
>
> It seems a shame to use 128 slots to represent all 9 id registers in
> the op1={1,3} space.
wouldn't it make sense to use a hashtable then as we don't have
consecutive indexes?
>
> Do we really need anything beyond the defined registers, or even the
> defined registers for which qemu knows how to do anything?
what do you mean by "defined registers". The end goal is to be able to
tune any id reg that the kernel allows to write. So I guess we shall
encompass more regs than qemu currently handles.

Wrt op1={1,3}, tbh I initially sticked to the KVM API. Now looking at
D22-2, effectively we have very few ID regs there. If we were to use a
hashtable we may be more flexible in picking up the indexes that are
relevant for us.
>
> I'm certainly happy to replace ARMISARegisters fields with an array,
> but more like
>
> enum ARMIDRegisterIdx {
>     ID_AA64ISAR0_IDX,
>     etc
>     ordering arbitrary, either machine or macro generated,
>     but every register has a symbolic index.
>     NUM_ID_IDX,
> };
>
> enum ARMSysregs {
>     SYS_ID_AA64PFR0_EL1 = ENCODE_AA64_CP_REG(...),
>     etc
> };
>
> const uint32_t id_register_sysreg[NUM_ID_IDX] = {
>     [ID_AA64ISAR0_IDX] = SYS_ID_AA64PFR0_EL1,
>     etc
> };
>
> struct ARMISARegisters {
>     uint64_t id[NUM_ID_IDX];
> };
>
> This seems trivial to automate, and wastes no space.

Sure we will study such rework. As long as the key (ID_AA64ISAR0_IDX)
can be matched against the index used by the KVM API we should be fine.

Thanks

Eric
>
>
> r~
>