| Message ID | 20241206112213.88394-3-cohuck@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kvm/arm: Introduce a customizable aarch64 KVM host model | expand |
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? 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. Do we really need anything beyond the defined registers, or even the defined registers for which qemu knows how to do anything? 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. r~
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~ >
On 12/12/24 11:46, Eric Auger wrote: >> 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. Defined registers as in "have an architected definition". E.g. there's no need to set any fields in (op0=0 op1=0 crn=0, crm=0, op2=1), because that isn't a defined system register. It's in id register space sure, and almost certainly RAZ, but there's no call to either set it or represent it within qemu. Because you're working to a a symbolic command-line interface, with FEAT_FOO, ID_REG.FIELD names, qemu will (be extended to) handle every register named. > 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. Yes. And I'm suggesting the "hashtable" be defined by compile-time constants. >> 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. I haven't looked to see what KVM_ARM_GET_REG_WRITABLE_MASKS really returns, but I see no reason that it should not be trivial to map back and forth between the spaces. r~
On Thu, Dec 12 2024, Richard Henderson <richard.henderson@linaro.org> wrote: > On 12/12/24 11:46, Eric Auger wrote: >>> 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. > > Defined registers as in "have an architected definition". > > E.g. there's no need to set any fields in (op0=0 op1=0 crn=0, crm=0, op2=1), because that > isn't a defined system register. It's in id register space sure, and almost certainly > RAZ, but there's no call to either set it or represent it within qemu. > > Because you're working to a a symbolic command-line interface, with FEAT_FOO, ID_REG.FIELD > names, qemu will (be extended to) handle every register named. Going from the definitions, we have the potential to generate props for anything that has been named (do we have named registers/fields that are not architected?) Exposed on the command line are only those register fields that are actually writable with the current KVM interface (see patch 18.) I'm still not quite sure how to continue with FEAT_FOO, but I guess we're still going to need the ID_REG.FIELD names in any case to handle differences like DIC in CTR_EL0 mentioned elsewhere in this thread.
diff --git a/target/arm/cpu-sysregs.h b/target/arm/cpu-sysregs.h new file mode 100644 index 000000000000..f4b63a3af77b --- /dev/null +++ b/target/arm/cpu-sysregs.h @@ -0,0 +1,42 @@ +#ifndef ARM_CPU_SYSREGS_H +#define ARM_CPU_SYSREGS_H + +/* to be generated */ + +#define SYS_ID_AA64PFR0_EL1 sys_reg(3, 0, 0, 4, 0) +#define SYS_ID_AA64PFR1_EL1 sys_reg(3, 0, 0, 4, 1) +#define SYS_ID_AA64SMFR0_EL1 sys_reg(3, 0, 0, 4, 5) +#define SYS_ID_AA64DFR0_EL1 sys_reg(3, 0, 0, 5, 0) +#define SYS_ID_AA64DFR1_EL1 sys_reg(3, 0, 0, 5, 1) +#define SYS_ID_AA64ISAR0_EL1 sys_reg(3, 0, 0, 6, 0) +#define SYS_ID_AA64ISAR1_EL1 sys_reg(3, 0, 0, 6, 1) +#define SYS_ID_AA64ISAR2_EL1 sys_reg(3, 0, 0, 6, 2) +#define SYS_ID_AA64MMFR0_EL1 sys_reg(3, 0, 0, 7, 0) +#define SYS_ID_AA64MMFR1_EL1 sys_reg(3, 0, 0, 7, 1) +#define SYS_ID_AA64MMFR2_EL1 sys_reg(3, 0, 0, 7, 2) +#define SYS_ID_AA64MMFR3_EL1 sys_reg(3, 0, 0, 7, 3) + +#define SYS_ID_PFR0_EL1 sys_reg(3, 0, 0, 1, 0) +#define SYS_ID_PFR1_EL1 sys_reg(3, 0, 0, 1, 1) +#define SYS_ID_DFR0_EL1 sys_reg(3, 0, 0, 1, 2) +#define SYS_ID_MMFR0_EL1 sys_reg(3, 0, 0, 1, 4) +#define SYS_ID_MMFR1_EL1 sys_reg(3, 0, 0, 1, 5) +#define SYS_ID_MMFR2_EL1 sys_reg(3, 0, 0, 1, 6) +#define SYS_ID_MMFR3_EL1 sys_reg(3, 0, 0, 1, 7) +#define SYS_ID_ISAR0_EL1 sys_reg(3, 0, 0, 2, 0) +#define SYS_ID_ISAR1_EL1 sys_reg(3, 0, 0, 2, 1) +#define SYS_ID_ISAR2_EL1 sys_reg(3, 0, 0, 2, 2) +#define SYS_ID_ISAR3_EL1 sys_reg(3, 0, 0, 2, 3) +#define SYS_ID_ISAR4_EL1 sys_reg(3, 0, 0, 2, 4) +#define SYS_ID_ISAR5_EL1 sys_reg(3, 0, 0, 2, 5) +#define SYS_ID_MMFR4_EL1 sys_reg(3, 0, 0, 2, 6) +#define SYS_ID_ISAR6_EL1 sys_reg(3, 0, 0, 2, 7) +#define SYS_MVFR0_EL1 sys_reg(3, 0, 0, 3, 0) +#define SYS_MVFR1_EL1 sys_reg(3, 0, 0, 3, 1) +#define SYS_MVFR2_EL1 sys_reg(3, 0, 0, 3, 2) +#define SYS_ID_PFR2_EL1 sys_reg(3, 0, 0, 3, 4) +#define SYS_ID_DFR1_EL1 sys_reg(3, 0, 0, 3, 5) +#define SYS_ID_MMFR5_EL1 sys_reg(3, 0, 0, 3, 6) +#define SYS_ID_AA64ZFR0_EL1 sys_reg(3, 0, 0, 4, 4) + +#endif diff --git a/target/arm/cpu.h b/target/arm/cpu.h index e359152a4dbc..9e0403c9810a 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -144,6 +144,14 @@ typedef struct ARMGenericTimer { uint64_t ctl; /* Timer Control register */ } ARMGenericTimer; +typedef struct ARMSysReg { + int op0; + int op1; + int crn; + int crm; + int op2; +} ARMSysReg; + /* Define a maximum sized vector register. * For 32-bit, this is a 128-bit NEON/AdvSIMD register. * For 64-bit, this is a 2048-bit SVE register. @@ -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)); \ + }) + +static inline uint64_t _get_idreg(const IdRegMap *map, ARMSysReg sr) +{ + int index = ARM_FEATURE_ID_RANGE_IDX(sr.op0, sr.op1, sr.crn, sr.crm, sr.op2); + + return map->regs[index]; +} + +static inline void _set_idreg(IdRegMap *map, ARMSysReg sr, uint64_t value) +{ + int index = ARM_FEATURE_ID_RANGE_IDX(sr.op0, sr.op1, sr.crn, sr.crm, sr.op2); + + map->regs[index] = value; +} + +/* REG is ID_XXX */ +#define FIELD_DP64_IDREG(MAP, REG, FIELD, VALUE) \ +{ \ +uint64_t regval = _get_idreg(MAP, SYS_ ## REG ## _EL1); \ +regval = FIELD_DP64(regval, REG, FIELD, VALUE); \ +_set_idreg(MAP, SYS_ ## REG ## _EL1, regval); \ +} + +#define FIELD_EX64_IDREG(MAP, REG, FIELD) \ +FIELD_EX64(_get_idreg(MAP, SYS_ ## REG ## _EL1), REG, FIELD) \ + +#define SET_IDREG(MAP, REG, VALUE) \ +_set_idreg(MAP, SYS_ ## REG ## _EL1, VALUE) + +#define GET_IDREG(MAP, REG) \ +_get_idreg(MAP, SYS_ ## REG ## _EL1) + +static inline ARMSysReg sys_reg(int op0, int op1, int crn, int crm, int op2) +{ + ARMSysReg sr = {op0, op1, crn, crm, op2}; + + return sr; +} + /** * ARMCPU: * @env: #CPUARMState @@ -1052,6 +1105,7 @@ struct ArchCPU { uint64_t id_aa64zfr0; uint64_t id_aa64smfr0; uint64_t reset_pmcr_el0; + IdRegMap idregs; } isar; uint64_t midr; uint32_t revidr;