The system register definitions in the arm64 get-reg-list are all done with directly specified magic numbers rather than using the definitions we import from the main kernel. This is error prone, and requires us to audit the additions to get-reg-list separately to what we do when specifying the registers for the main kernel. Since Marc has indicated that this isn't a deliberate or desired choice let's start using the constants we have defined.
We first manually update the data used to filter registers based on ID register fields to use a simplified macro that specifies the register and ID field in a muc more compact fashion. This is done first since there is an error in the ID register field for the S1PIE registers. We then replace all the remaining named system register specifications with use of the existing KVM_ARM64_SYS_REG() macro.
Signed-off-by: Mark Brown broonie@kernel.org --- Mark Brown (2): KVM: selftests: arm64: Simplify specification of filtered registers KVM: selftests: arm64: Use generated defines for named system registers
tools/testing/selftests/kvm/aarch64/get-reg-list.c | 237 ++++++++++----------- 1 file changed, 115 insertions(+), 122 deletions(-) --- base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b change-id: 20240802-kvm-arm64-get-reg-list-a86a37460bdd
Best regards,
Since we already import the generated sysreg definitions from the main kernel and reference them in processor.h for use in other KVM tests we can also make use of them for get-reg-list as well instead of having hard coded magic numbers in the program. Do this for the table defining which registers should be gated on ID register values, using a macro which allows us to specify the register and ID register field in a much more compact and direct fashion.
In the process we fix the ID register checked for S1PIE specific registers which was using an incorrect shift of 4, checking SCTLRX support instead. No other change is seen in the generated data.
Fixes: 5f0419a0083b ("KVM: selftests: get-reg-list: add Permission Indirection registers") Signed-off-by: Mark Brown broonie@kernel.org --- tools/testing/selftests/kvm/aarch64/get-reg-list.c | 29 ++++++++-------------- 1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c index 709d7d721760..a00322970578 100644 --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c @@ -22,25 +22,18 @@ struct feature_id_reg { __u64 feat_min; };
-static struct feature_id_reg feat_id_regs[] = { - { - ARM64_SYS_REG(3, 0, 2, 0, 3), /* TCR2_EL1 */ - ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */ - 0, - 1 - }, - { - ARM64_SYS_REG(3, 0, 10, 2, 2), /* PIRE0_EL1 */ - ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */ - 4, - 1 - }, - { - ARM64_SYS_REG(3, 0, 10, 2, 3), /* PIR_EL1 */ - ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */ - 4, - 1 +#define FEAT_ID_CHECK(reg, id_reg, id_field, id_val) \ + { \ + KVM_ARM64_SYS_REG(SYS_##reg), \ + KVM_ARM64_SYS_REG(SYS_##id_reg), \ + id_reg##_##id_field##_SHIFT, \ + id_reg##_##id_field##_##id_val, \ } + +static struct feature_id_reg feat_id_regs[] = { + FEAT_ID_CHECK(TCR2_EL1, ID_AA64MMFR3_EL1, TCRX, IMP), + FEAT_ID_CHECK(PIRE0_EL1, ID_AA64MMFR3_EL1, S1PIE, IMP), + FEAT_ID_CHECK(PIR_EL1, ID_AA64MMFR3_EL1, S1PIE, IMP), };
bool filter_reg(__u64 reg)
On Fri, 02 Aug 2024 22:57:53 +0100, Mark Brown broonie@kernel.org wrote:
Since we already import the generated sysreg definitions from the main kernel and reference them in processor.h for use in other KVM tests we can also make use of them for get-reg-list as well instead of having hard coded magic numbers in the program. Do this for the table defining which registers should be gated on ID register values, using a macro which allows us to specify the register and ID register field in a much more compact and direct fashion.
In the process we fix the ID register checked for S1PIE specific registers which was using an incorrect shift of 4, checking SCTLRX support instead. No other change is seen in the generated data.
Fixes: 5f0419a0083b ("KVM: selftests: get-reg-list: add Permission Indirection registers") Signed-off-by: Mark Brown broonie@kernel.org
tools/testing/selftests/kvm/aarch64/get-reg-list.c | 29 ++++++++-------------- 1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c index 709d7d721760..a00322970578 100644 --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c @@ -22,25 +22,18 @@ struct feature_id_reg { __u64 feat_min; }; -static struct feature_id_reg feat_id_regs[] = {
- {
ARM64_SYS_REG(3, 0, 2, 0, 3), /* TCR2_EL1 */ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */0,1- },
- {
ARM64_SYS_REG(3, 0, 10, 2, 2), /* PIRE0_EL1 */ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */4,1- },
- {
ARM64_SYS_REG(3, 0, 10, 2, 3), /* PIR_EL1 */ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */4,1+#define FEAT_ID_CHECK(reg, id_reg, id_field, id_val) \
- { \
KVM_ARM64_SYS_REG(SYS_##reg), \KVM_ARM64_SYS_REG(SYS_##id_reg), \id_reg##_##id_field##_SHIFT, \id_reg##_##id_field##_##id_val, \
Please use designated initialisers.
}
+static struct feature_id_reg feat_id_regs[] = {
- FEAT_ID_CHECK(TCR2_EL1, ID_AA64MMFR3_EL1, TCRX, IMP),
- FEAT_ID_CHECK(PIRE0_EL1, ID_AA64MMFR3_EL1, S1PIE, IMP),
- FEAT_ID_CHECK(PIR_EL1, ID_AA64MMFR3_EL1, S1PIE, IMP),
}; bool filter_reg(__u64 reg)
Thanks,
M.
Currently the get-reg-list test uses directly specified numeric values to define system registers to validate. Since we already have a macro which allows us to use the generated system register definitions from the main kernel easily let's update all the registers where we have specified the name in a comment to just use that macro. This reduces the number of places where we need to validate the name to number mapping.
This conversion was done with the sed command:
sed -i -E 's-ARM64_SYS_REG.*/* (.*) */-KVM_ARM64_SYS_REG(SYS_\1),-' tools/testing/selftests/kvm/aarch64/get-reg-list.c
We still have a number of numerically specified registers, some of these are reserved registers without defined names (eg, unallocated ID registers) and others don't have kernel macro definitions yet.
No change in the generated output.
Suggested-by: Marc Zyngier maz@kernel.org Signed-off-by: Mark Brown broonie@kernel.org --- tools/testing/selftests/kvm/aarch64/get-reg-list.c | 208 ++++++++++----------- 1 file changed, 104 insertions(+), 104 deletions(-)
diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c index a00322970578..4d786c4ab28a 100644 --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c @@ -313,14 +313,14 @@ static __u64 base_regs[] = { KVM_REG_ARM_FW_FEAT_BMAP_REG(0), /* KVM_REG_ARM_STD_BMAP */ KVM_REG_ARM_FW_FEAT_BMAP_REG(1), /* KVM_REG_ARM_STD_HYP_BMAP */ KVM_REG_ARM_FW_FEAT_BMAP_REG(2), /* KVM_REG_ARM_VENDOR_HYP_BMAP */ - ARM64_SYS_REG(3, 3, 14, 3, 1), /* CNTV_CTL_EL0 */ - ARM64_SYS_REG(3, 3, 14, 3, 2), /* CNTV_CVAL_EL0 */ + KVM_ARM64_SYS_REG(SYS_CNTV_CTL_EL0), + KVM_ARM64_SYS_REG(SYS_CNTV_CVAL_EL0), ARM64_SYS_REG(3, 3, 14, 0, 2), - ARM64_SYS_REG(3, 0, 0, 0, 0), /* MIDR_EL1 */ - ARM64_SYS_REG(3, 0, 0, 0, 6), /* REVIDR_EL1 */ - ARM64_SYS_REG(3, 1, 0, 0, 1), /* CLIDR_EL1 */ - ARM64_SYS_REG(3, 1, 0, 0, 7), /* AIDR_EL1 */ - ARM64_SYS_REG(3, 3, 0, 0, 1), /* CTR_EL0 */ + KVM_ARM64_SYS_REG(SYS_MIDR_EL1), + KVM_ARM64_SYS_REG(SYS_REVIDR_EL1), + KVM_ARM64_SYS_REG(SYS_CLIDR_EL1), + KVM_ARM64_SYS_REG(SYS_AIDR_EL1), + KVM_ARM64_SYS_REG(SYS_CTR_EL0), ARM64_SYS_REG(2, 0, 0, 0, 4), ARM64_SYS_REG(2, 0, 0, 0, 5), ARM64_SYS_REG(2, 0, 0, 0, 6), @@ -329,8 +329,8 @@ static __u64 base_regs[] = { ARM64_SYS_REG(2, 0, 0, 1, 5), ARM64_SYS_REG(2, 0, 0, 1, 6), ARM64_SYS_REG(2, 0, 0, 1, 7), - ARM64_SYS_REG(2, 0, 0, 2, 0), /* MDCCINT_EL1 */ - ARM64_SYS_REG(2, 0, 0, 2, 2), /* MDSCR_EL1 */ + KVM_ARM64_SYS_REG(SYS_MDCCINT_EL1), + KVM_ARM64_SYS_REG(SYS_MDSCR_EL1), ARM64_SYS_REG(2, 0, 0, 2, 4), ARM64_SYS_REG(2, 0, 0, 2, 5), ARM64_SYS_REG(2, 0, 0, 2, 6), @@ -387,109 +387,109 @@ static __u64 base_regs[] = { ARM64_SYS_REG(2, 0, 0, 15, 5), ARM64_SYS_REG(2, 0, 0, 15, 6), ARM64_SYS_REG(2, 0, 0, 15, 7), - ARM64_SYS_REG(2, 0, 1, 1, 4), /* OSLSR_EL1 */ - ARM64_SYS_REG(2, 4, 0, 7, 0), /* DBGVCR32_EL2 */ - ARM64_SYS_REG(3, 0, 0, 0, 5), /* MPIDR_EL1 */ - ARM64_SYS_REG(3, 0, 0, 1, 0), /* ID_PFR0_EL1 */ - ARM64_SYS_REG(3, 0, 0, 1, 1), /* ID_PFR1_EL1 */ - ARM64_SYS_REG(3, 0, 0, 1, 2), /* ID_DFR0_EL1 */ - ARM64_SYS_REG(3, 0, 0, 1, 3), /* ID_AFR0_EL1 */ - ARM64_SYS_REG(3, 0, 0, 1, 4), /* ID_MMFR0_EL1 */ - ARM64_SYS_REG(3, 0, 0, 1, 5), /* ID_MMFR1_EL1 */ - ARM64_SYS_REG(3, 0, 0, 1, 6), /* ID_MMFR2_EL1 */ - ARM64_SYS_REG(3, 0, 0, 1, 7), /* ID_MMFR3_EL1 */ - ARM64_SYS_REG(3, 0, 0, 2, 0), /* ID_ISAR0_EL1 */ - ARM64_SYS_REG(3, 0, 0, 2, 1), /* ID_ISAR1_EL1 */ - ARM64_SYS_REG(3, 0, 0, 2, 2), /* ID_ISAR2_EL1 */ - ARM64_SYS_REG(3, 0, 0, 2, 3), /* ID_ISAR3_EL1 */ - ARM64_SYS_REG(3, 0, 0, 2, 4), /* ID_ISAR4_EL1 */ - ARM64_SYS_REG(3, 0, 0, 2, 5), /* ID_ISAR5_EL1 */ - ARM64_SYS_REG(3, 0, 0, 2, 6), /* ID_MMFR4_EL1 */ - ARM64_SYS_REG(3, 0, 0, 2, 7), /* ID_ISAR6_EL1 */ - ARM64_SYS_REG(3, 0, 0, 3, 0), /* MVFR0_EL1 */ - ARM64_SYS_REG(3, 0, 0, 3, 1), /* MVFR1_EL1 */ - ARM64_SYS_REG(3, 0, 0, 3, 2), /* MVFR2_EL1 */ + KVM_ARM64_SYS_REG(SYS_OSLSR_EL1), + KVM_ARM64_SYS_REG(SYS_DBGVCR32_EL2), + KVM_ARM64_SYS_REG(SYS_MPIDR_EL1), + KVM_ARM64_SYS_REG(SYS_ID_PFR0_EL1), + KVM_ARM64_SYS_REG(SYS_ID_PFR1_EL1), + KVM_ARM64_SYS_REG(SYS_ID_DFR0_EL1), + KVM_ARM64_SYS_REG(SYS_ID_AFR0_EL1), + KVM_ARM64_SYS_REG(SYS_ID_MMFR0_EL1), + KVM_ARM64_SYS_REG(SYS_ID_MMFR1_EL1), + KVM_ARM64_SYS_REG(SYS_ID_MMFR2_EL1), + KVM_ARM64_SYS_REG(SYS_ID_MMFR3_EL1), + KVM_ARM64_SYS_REG(SYS_ID_ISAR0_EL1), + KVM_ARM64_SYS_REG(SYS_ID_ISAR1_EL1), + KVM_ARM64_SYS_REG(SYS_ID_ISAR2_EL1), + KVM_ARM64_SYS_REG(SYS_ID_ISAR3_EL1), + KVM_ARM64_SYS_REG(SYS_ID_ISAR4_EL1), + KVM_ARM64_SYS_REG(SYS_ID_ISAR5_EL1), + KVM_ARM64_SYS_REG(SYS_ID_MMFR4_EL1), + KVM_ARM64_SYS_REG(SYS_ID_ISAR6_EL1), + KVM_ARM64_SYS_REG(SYS_MVFR0_EL1), + KVM_ARM64_SYS_REG(SYS_MVFR1_EL1), + KVM_ARM64_SYS_REG(SYS_MVFR2_EL1), ARM64_SYS_REG(3, 0, 0, 3, 3), - ARM64_SYS_REG(3, 0, 0, 3, 4), /* ID_PFR2_EL1 */ - ARM64_SYS_REG(3, 0, 0, 3, 5), /* ID_DFR1_EL1 */ - ARM64_SYS_REG(3, 0, 0, 3, 6), /* ID_MMFR5_EL1 */ + KVM_ARM64_SYS_REG(SYS_ID_PFR2_EL1), + KVM_ARM64_SYS_REG(SYS_ID_DFR1_EL1), + KVM_ARM64_SYS_REG(SYS_ID_MMFR5_EL1), ARM64_SYS_REG(3, 0, 0, 3, 7), - ARM64_SYS_REG(3, 0, 0, 4, 0), /* ID_AA64PFR0_EL1 */ - ARM64_SYS_REG(3, 0, 0, 4, 1), /* ID_AA64PFR1_EL1 */ - ARM64_SYS_REG(3, 0, 0, 4, 2), /* ID_AA64PFR2_EL1 */ + KVM_ARM64_SYS_REG(SYS_ID_AA64PFR0_EL1), + KVM_ARM64_SYS_REG(SYS_ID_AA64PFR1_EL1), + KVM_ARM64_SYS_REG(SYS_ID_AA64PFR2_EL1), ARM64_SYS_REG(3, 0, 0, 4, 3), - ARM64_SYS_REG(3, 0, 0, 4, 4), /* ID_AA64ZFR0_EL1 */ - ARM64_SYS_REG(3, 0, 0, 4, 5), /* ID_AA64SMFR0_EL1 */ + KVM_ARM64_SYS_REG(SYS_ID_AA64ZFR0_EL1), + KVM_ARM64_SYS_REG(SYS_ID_AA64SMFR0_EL1), ARM64_SYS_REG(3, 0, 0, 4, 6), ARM64_SYS_REG(3, 0, 0, 4, 7), - ARM64_SYS_REG(3, 0, 0, 5, 0), /* ID_AA64DFR0_EL1 */ - ARM64_SYS_REG(3, 0, 0, 5, 1), /* ID_AA64DFR1_EL1 */ + KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), + KVM_ARM64_SYS_REG(SYS_ID_AA64DFR1_EL1), ARM64_SYS_REG(3, 0, 0, 5, 2), ARM64_SYS_REG(3, 0, 0, 5, 3), - ARM64_SYS_REG(3, 0, 0, 5, 4), /* ID_AA64AFR0_EL1 */ - ARM64_SYS_REG(3, 0, 0, 5, 5), /* ID_AA64AFR1_EL1 */ + KVM_ARM64_SYS_REG(SYS_ID_AA64AFR0_EL1), + KVM_ARM64_SYS_REG(SYS_ID_AA64AFR1_EL1), ARM64_SYS_REG(3, 0, 0, 5, 6), ARM64_SYS_REG(3, 0, 0, 5, 7), - ARM64_SYS_REG(3, 0, 0, 6, 0), /* ID_AA64ISAR0_EL1 */ - ARM64_SYS_REG(3, 0, 0, 6, 1), /* ID_AA64ISAR1_EL1 */ - ARM64_SYS_REG(3, 0, 0, 6, 2), /* ID_AA64ISAR2_EL1 */ + KVM_ARM64_SYS_REG(SYS_ID_AA64ISAR0_EL1), + KVM_ARM64_SYS_REG(SYS_ID_AA64ISAR1_EL1), + KVM_ARM64_SYS_REG(SYS_ID_AA64ISAR2_EL1), ARM64_SYS_REG(3, 0, 0, 6, 3), ARM64_SYS_REG(3, 0, 0, 6, 4), ARM64_SYS_REG(3, 0, 0, 6, 5), ARM64_SYS_REG(3, 0, 0, 6, 6), ARM64_SYS_REG(3, 0, 0, 6, 7), - ARM64_SYS_REG(3, 0, 0, 7, 0), /* ID_AA64MMFR0_EL1 */ - ARM64_SYS_REG(3, 0, 0, 7, 1), /* ID_AA64MMFR1_EL1 */ - ARM64_SYS_REG(3, 0, 0, 7, 2), /* ID_AA64MMFR2_EL1 */ - ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */ - ARM64_SYS_REG(3, 0, 0, 7, 4), /* ID_AA64MMFR4_EL1 */ + KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR0_EL1), + KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR1_EL1), + KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR2_EL1), + KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR3_EL1), + KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR4_EL1), ARM64_SYS_REG(3, 0, 0, 7, 5), ARM64_SYS_REG(3, 0, 0, 7, 6), ARM64_SYS_REG(3, 0, 0, 7, 7), - ARM64_SYS_REG(3, 0, 1, 0, 0), /* SCTLR_EL1 */ - ARM64_SYS_REG(3, 0, 1, 0, 1), /* ACTLR_EL1 */ - ARM64_SYS_REG(3, 0, 1, 0, 2), /* CPACR_EL1 */ - ARM64_SYS_REG(3, 0, 2, 0, 0), /* TTBR0_EL1 */ - ARM64_SYS_REG(3, 0, 2, 0, 1), /* TTBR1_EL1 */ - ARM64_SYS_REG(3, 0, 2, 0, 2), /* TCR_EL1 */ - ARM64_SYS_REG(3, 0, 2, 0, 3), /* TCR2_EL1 */ - ARM64_SYS_REG(3, 0, 5, 1, 0), /* AFSR0_EL1 */ - ARM64_SYS_REG(3, 0, 5, 1, 1), /* AFSR1_EL1 */ - ARM64_SYS_REG(3, 0, 5, 2, 0), /* ESR_EL1 */ - ARM64_SYS_REG(3, 0, 6, 0, 0), /* FAR_EL1 */ - ARM64_SYS_REG(3, 0, 7, 4, 0), /* PAR_EL1 */ - ARM64_SYS_REG(3, 0, 10, 2, 0), /* MAIR_EL1 */ - ARM64_SYS_REG(3, 0, 10, 2, 2), /* PIRE0_EL1 */ - ARM64_SYS_REG(3, 0, 10, 2, 3), /* PIR_EL1 */ - ARM64_SYS_REG(3, 0, 10, 3, 0), /* AMAIR_EL1 */ - ARM64_SYS_REG(3, 0, 12, 0, 0), /* VBAR_EL1 */ - ARM64_SYS_REG(3, 0, 12, 1, 1), /* DISR_EL1 */ - ARM64_SYS_REG(3, 0, 13, 0, 1), /* CONTEXTIDR_EL1 */ - ARM64_SYS_REG(3, 0, 13, 0, 4), /* TPIDR_EL1 */ - ARM64_SYS_REG(3, 0, 14, 1, 0), /* CNTKCTL_EL1 */ - ARM64_SYS_REG(3, 2, 0, 0, 0), /* CSSELR_EL1 */ - ARM64_SYS_REG(3, 3, 13, 0, 2), /* TPIDR_EL0 */ - ARM64_SYS_REG(3, 3, 13, 0, 3), /* TPIDRRO_EL0 */ - ARM64_SYS_REG(3, 3, 14, 0, 1), /* CNTPCT_EL0 */ - ARM64_SYS_REG(3, 3, 14, 2, 1), /* CNTP_CTL_EL0 */ - ARM64_SYS_REG(3, 3, 14, 2, 2), /* CNTP_CVAL_EL0 */ - ARM64_SYS_REG(3, 4, 3, 0, 0), /* DACR32_EL2 */ - ARM64_SYS_REG(3, 4, 5, 0, 1), /* IFSR32_EL2 */ - ARM64_SYS_REG(3, 4, 5, 3, 0), /* FPEXC32_EL2 */ + KVM_ARM64_SYS_REG(SYS_SCTLR_EL1), + KVM_ARM64_SYS_REG(SYS_ACTLR_EL1), + KVM_ARM64_SYS_REG(SYS_CPACR_EL1), + KVM_ARM64_SYS_REG(SYS_TTBR0_EL1), + KVM_ARM64_SYS_REG(SYS_TTBR1_EL1), + KVM_ARM64_SYS_REG(SYS_TCR_EL1), + KVM_ARM64_SYS_REG(SYS_TCR2_EL1), + KVM_ARM64_SYS_REG(SYS_AFSR0_EL1), + KVM_ARM64_SYS_REG(SYS_AFSR1_EL1), + KVM_ARM64_SYS_REG(SYS_ESR_EL1), + KVM_ARM64_SYS_REG(SYS_FAR_EL1), + KVM_ARM64_SYS_REG(SYS_PAR_EL1), + KVM_ARM64_SYS_REG(SYS_MAIR_EL1), + KVM_ARM64_SYS_REG(SYS_PIRE0_EL1), + KVM_ARM64_SYS_REG(SYS_PIR_EL1), + KVM_ARM64_SYS_REG(SYS_AMAIR_EL1), + KVM_ARM64_SYS_REG(SYS_VBAR_EL1), + KVM_ARM64_SYS_REG(SYS_DISR_EL1), + KVM_ARM64_SYS_REG(SYS_CONTEXTIDR_EL1), + KVM_ARM64_SYS_REG(SYS_TPIDR_EL1), + KVM_ARM64_SYS_REG(SYS_CNTKCTL_EL1), + KVM_ARM64_SYS_REG(SYS_CSSELR_EL1), + KVM_ARM64_SYS_REG(SYS_TPIDR_EL0), + KVM_ARM64_SYS_REG(SYS_TPIDRRO_EL0), + KVM_ARM64_SYS_REG(SYS_CNTPCT_EL0), + KVM_ARM64_SYS_REG(SYS_CNTP_CTL_EL0), + KVM_ARM64_SYS_REG(SYS_CNTP_CVAL_EL0), + KVM_ARM64_SYS_REG(SYS_DACR32_EL2), + KVM_ARM64_SYS_REG(SYS_IFSR32_EL2), + KVM_ARM64_SYS_REG(SYS_FPEXC32_EL2), };
static __u64 pmu_regs[] = { - ARM64_SYS_REG(3, 0, 9, 14, 1), /* PMINTENSET_EL1 */ - ARM64_SYS_REG(3, 0, 9, 14, 2), /* PMINTENCLR_EL1 */ - ARM64_SYS_REG(3, 3, 9, 12, 0), /* PMCR_EL0 */ - ARM64_SYS_REG(3, 3, 9, 12, 1), /* PMCNTENSET_EL0 */ - ARM64_SYS_REG(3, 3, 9, 12, 2), /* PMCNTENCLR_EL0 */ - ARM64_SYS_REG(3, 3, 9, 12, 3), /* PMOVSCLR_EL0 */ - ARM64_SYS_REG(3, 3, 9, 12, 4), /* PMSWINC_EL0 */ - ARM64_SYS_REG(3, 3, 9, 12, 5), /* PMSELR_EL0 */ - ARM64_SYS_REG(3, 3, 9, 13, 0), /* PMCCNTR_EL0 */ - ARM64_SYS_REG(3, 3, 9, 14, 0), /* PMUSERENR_EL0 */ - ARM64_SYS_REG(3, 3, 9, 14, 3), /* PMOVSSET_EL0 */ + KVM_ARM64_SYS_REG(SYS_PMINTENSET_EL1), + KVM_ARM64_SYS_REG(SYS_PMINTENCLR_EL1), + KVM_ARM64_SYS_REG(SYS_PMCR_EL0), + KVM_ARM64_SYS_REG(SYS_PMCNTENSET_EL0), + KVM_ARM64_SYS_REG(SYS_PMCNTENCLR_EL0), + KVM_ARM64_SYS_REG(SYS_PMOVSCLR_EL0), + KVM_ARM64_SYS_REG(SYS_PMSWINC_EL0), + KVM_ARM64_SYS_REG(SYS_PMSELR_EL0), + KVM_ARM64_SYS_REG(SYS_PMCCNTR_EL0), + KVM_ARM64_SYS_REG(SYS_PMUSERENR_EL0), + KVM_ARM64_SYS_REG(SYS_PMOVSSET_EL0), ARM64_SYS_REG(3, 3, 14, 8, 0), ARM64_SYS_REG(3, 3, 14, 8, 1), ARM64_SYS_REG(3, 3, 14, 8, 2), @@ -552,7 +552,7 @@ static __u64 pmu_regs[] = { ARM64_SYS_REG(3, 3, 14, 15, 4), ARM64_SYS_REG(3, 3, 14, 15, 5), ARM64_SYS_REG(3, 3, 14, 15, 6), - ARM64_SYS_REG(3, 3, 14, 15, 7), /* PMCCFILTR_EL0 */ + KVM_ARM64_SYS_REG(SYS_PMCCFILTR_EL0), };
static __u64 vregs[] = { @@ -641,7 +641,7 @@ static __u64 sve_regs[] = { KVM_REG_ARM64_SVE_PREG(14, 0), KVM_REG_ARM64_SVE_PREG(15, 0), KVM_REG_ARM64_SVE_FFR(0), - ARM64_SYS_REG(3, 0, 1, 2, 0), /* ZCR_EL1 */ + KVM_ARM64_SYS_REG(SYS_ZCR_EL1), };
static __u64 sve_rejects_set[] = { @@ -649,19 +649,19 @@ static __u64 sve_rejects_set[] = { };
static __u64 pauth_addr_regs[] = { - ARM64_SYS_REG(3, 0, 2, 1, 0), /* APIAKEYLO_EL1 */ - ARM64_SYS_REG(3, 0, 2, 1, 1), /* APIAKEYHI_EL1 */ - ARM64_SYS_REG(3, 0, 2, 1, 2), /* APIBKEYLO_EL1 */ - ARM64_SYS_REG(3, 0, 2, 1, 3), /* APIBKEYHI_EL1 */ - ARM64_SYS_REG(3, 0, 2, 2, 0), /* APDAKEYLO_EL1 */ - ARM64_SYS_REG(3, 0, 2, 2, 1), /* APDAKEYHI_EL1 */ - ARM64_SYS_REG(3, 0, 2, 2, 2), /* APDBKEYLO_EL1 */ - ARM64_SYS_REG(3, 0, 2, 2, 3) /* APDBKEYHI_EL1 */ + KVM_ARM64_SYS_REG(SYS_APIAKEYLO_EL1), + KVM_ARM64_SYS_REG(SYS_APIAKEYHI_EL1), + KVM_ARM64_SYS_REG(SYS_APIBKEYLO_EL1), + KVM_ARM64_SYS_REG(SYS_APIBKEYHI_EL1), + KVM_ARM64_SYS_REG(SYS_APDAKEYLO_EL1), + KVM_ARM64_SYS_REG(SYS_APDAKEYHI_EL1), + KVM_ARM64_SYS_REG(SYS_APDBKEYLO_EL1), + KVM_ARM64_SYS_REG(SYS_APDBKEYHI_EL1), };
static __u64 pauth_generic_regs[] = { - ARM64_SYS_REG(3, 0, 2, 3, 0), /* APGAKEYLO_EL1 */ - ARM64_SYS_REG(3, 0, 2, 3, 1), /* APGAKEYHI_EL1 */ + KVM_ARM64_SYS_REG(SYS_APGAKEYLO_EL1), + KVM_ARM64_SYS_REG(SYS_APGAKEYHI_EL1), };
#define BASE_SUBLIST \
On Fri, 02 Aug 2024 22:57:54 +0100, Mark Brown broonie@kernel.org wrote:
Currently the get-reg-list test uses directly specified numeric values to define system registers to validate. Since we already have a macro which allows us to use the generated system register definitions from the main kernel easily let's update all the registers where we have specified the name in a comment to just use that macro. This reduces the number of places where we need to validate the name to number mapping.
This conversion was done with the sed command:
sed -i -E 's-ARM64_SYS_REG.*/* (.*) */-KVM_ARM64_SYS_REG(SYS_\1),-' tools/testing/selftests/kvm/aarch64/get-reg-list.c
[Eyes rolling]
What I asked about scripting the whole thing, it never occurred to me that you would use the *comments* as a reliable source of information. Do we have anything less reliable than comments in the kernel?
The matching must be done from the arch/arm64/tools/sysreg file, because that's the (admittedly dubious) source of truth. We actually trust the encodings because they are reported by the kernel itself. The comment is hand-written, and likely wrong.
Also, this hides the horrible truth about existing ABI bugs, see below.
We still have a number of numerically specified registers, some of these are reserved registers without defined names (eg, unallocated ID registers) and others don't have kernel macro definitions yet.
No change in the generated output.
Suggested-by: Marc Zyngier maz@kernel.org Signed-off-by: Mark Brown broonie@kernel.org
tools/testing/selftests/kvm/aarch64/get-reg-list.c | 208 ++++++++++----------- 1 file changed, 104 insertions(+), 104 deletions(-)
diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c index a00322970578..4d786c4ab28a 100644 --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c @@ -313,14 +313,14 @@ static __u64 base_regs[] = { KVM_REG_ARM_FW_FEAT_BMAP_REG(0), /* KVM_REG_ARM_STD_BMAP */ KVM_REG_ARM_FW_FEAT_BMAP_REG(1), /* KVM_REG_ARM_STD_HYP_BMAP */ KVM_REG_ARM_FW_FEAT_BMAP_REG(2), /* KVM_REG_ARM_VENDOR_HYP_BMAP */
- ARM64_SYS_REG(3, 3, 14, 3, 1), /* CNTV_CTL_EL0 */
- ARM64_SYS_REG(3, 3, 14, 3, 2), /* CNTV_CVAL_EL0 */
- KVM_ARM64_SYS_REG(SYS_CNTV_CTL_EL0),
- KVM_ARM64_SYS_REG(SYS_CNTV_CVAL_EL0), ARM64_SYS_REG(3, 3, 14, 0, 2),
Great. So not only you fail convert a register, but you also ignore the nugget described in arch/arm64/invlude/uapi/asm/kvm.h:267.
Sure, having both described hides the crap, as we don't attach any significance to the registers themselves. But that shows how untrustworthy the comments are.
- ARM64_SYS_REG(3, 0, 0, 0, 0), /* MIDR_EL1 */
- ARM64_SYS_REG(3, 0, 0, 0, 6), /* REVIDR_EL1 */
- ARM64_SYS_REG(3, 1, 0, 0, 1), /* CLIDR_EL1 */
- ARM64_SYS_REG(3, 1, 0, 0, 7), /* AIDR_EL1 */
- ARM64_SYS_REG(3, 3, 0, 0, 1), /* CTR_EL0 */
- KVM_ARM64_SYS_REG(SYS_MIDR_EL1),
- KVM_ARM64_SYS_REG(SYS_REVIDR_EL1),
- KVM_ARM64_SYS_REG(SYS_CLIDR_EL1),
- KVM_ARM64_SYS_REG(SYS_AIDR_EL1),
- KVM_ARM64_SYS_REG(SYS_CTR_EL0), ARM64_SYS_REG(2, 0, 0, 0, 4), ARM64_SYS_REG(2, 0, 0, 0, 5), ARM64_SYS_REG(2, 0, 0, 0, 6),
As far as I can tell, these registers are not unallocated, and they should be named.
Thanks,
M.
On Sat, Aug 03, 2024 at 10:35:54AM +0100, Marc Zyngier wrote:
Mark Brown broonie@kernel.org wrote:
This conversion was done with the sed command:
sed -i -E 's-ARM64_SYS_REG.*/* (.*) */-KVM_ARM64_SYS_REG(SYS_\1),-' tools/testing/selftests/kvm/aarch64/get-reg-list.c
[Eyes rolling]
What I asked about scripting the whole thing, it never occurred to me that you would use the *comments* as a reliable source of information. Do we have anything less reliable than comments in the kernel?
I think we should ultimately be using both the comments and the encodings - the comments indicate what people thought was being tested and it's useful to make sure we have that coverage even if the implementation were to have been wrong.
Doing this step is also going to have picked up registers which we don't yet have in the sysreg file, some of which are going to be painful to add there (things like ESR for example) so aren't likely to get done in a hurry due to complexity in their definitions.
This was quick to do, represents progress, and offers a hint to anyone adding new registers that they should use the symbolic definitions.
The matching must be done from the arch/arm64/tools/sysreg file, because that's the (admittedly dubious) source of truth. We actually trust the encodings because they are reported by the kernel itself. The comment is hand-written, and likely wrong.
Sure, there's a reason I compared the resulting binaries rather than just trusting that the conversion gave the same result.
- ARM64_SYS_REG(3, 3, 14, 3, 1), /* CNTV_CTL_EL0 */
- ARM64_SYS_REG(3, 3, 14, 3, 2), /* CNTV_CVAL_EL0 */
- KVM_ARM64_SYS_REG(SYS_CNTV_CTL_EL0),
- KVM_ARM64_SYS_REG(SYS_CNTV_CVAL_EL0), ARM64_SYS_REG(3, 3, 14, 0, 2),
Great. So not only you fail convert a register, but you also ignore the nugget described in arch/arm64/invlude/uapi/asm/kvm.h:267.
That's that CNTV_CTL_EL0 and CNTV_CVAL_EL0 have their encodings reversed in the ABI.
Sure, having both described hides the crap, as we don't attach any significance to the registers themselves. But that shows how untrustworthy the comments are.
I'm afraid that any automated conversion is likely to trip over an ABI issue like that - the obvious thing to do when looking up by encoding would be to just emit a KVM_ARM64_SYS_REG() if we find the encoding which would give the same end result. I'll add a separate manual update of these registers.
Are there any other similar issues? I didn't spot anything in kvm.h.
ARM64_SYS_REG(2, 0, 0, 0, 4), ARM64_SYS_REG(2, 0, 0, 0, 5), ARM64_SYS_REG(2, 0, 0, 0, 6),
As far as I can tell, these registers are not unallocated, and they should be named.
I agree that we should do all named registers eventually, the above are numbered debug registers (DBGBVR0_EL1, DBGBCR0_EL1 and DBGWVR0_EL1) which aren't in the sysreg file yet so wouldn't currently be covered by a conversion based on pulling encodings from there. They could also be done immediately with a generator script as there are DBGBVRn_EL1 style macros there.
Like I say this is a quick first step and does improve things, there's still more to do but I do think this moves us forward. We can and should come back later and build on things as people have time.
On Fri, Aug 02, 2024 at 10:57:54PM GMT, Mark Brown wrote:
Currently the get-reg-list test uses directly specified numeric values to define system registers to validate. Since we already have a macro which allows us to use the generated system register definitions from the main kernel easily let's update all the registers where we have specified the name in a comment to just use that macro. This reduces the number of places where we need to validate the name to number mapping.
This conversion was done with the sed command:
sed -i -E 's-ARM64_SYS_REG.*/* (.*) */-KVM_ARM64_SYS_REG(SYS_\1),-' tools/testing/selftests/kvm/aarch64/get-reg-list.c
We still have a number of numerically specified registers, some of these are reserved registers without defined names (eg, unallocated ID registers) and others don't have kernel macro definitions yet.
FWIW, the "ARM64_SYS_REG(...encoding...), /* NAME */" format was intentional. The idea was that when get-reg-list outputs new or missing registers it discovers, or the user lists registers with --list, the best it can do is output "ARM64_SYS_REG(...encoding...)". Putting that format directly into the test enabled copy+paste of the list output into a test case. However, the lack of names did lead to scripting the generation of the name comments, which means it wasn't a direct copy+paste anyway. The other benefit of the format was being able to directly grep the test for the 'missing' registers. Anyway, maybe always going through asm/sysreg.h with greps of '...encoding...' is the better approach.
Thanks, drew
linux-kselftest-mirror@lists.linaro.org
-
Andrew Jones -
Marc Zyngier -
Mark Brown