On 09/01/2024 16:56, Jing Zhang wrote:
There are some feature fields with nonzero minimum valid value. Make sure get_safe_value() won't return invalid field values for them. Also fix a bug that wrongly uses the feature bits type as the feature bits sign causing all fields as signed in the get_safe_value() and get_invalid_value(). Fixes: 54a9ea73527d ("KVM: arm64: selftests: Test for setting ID register from usersapce") Reported-by: Zenghui Yu <yuzenghui@xxxxxxxxxx> Reported-by: Itaru Kitayama <itaru.kitayama@xxxxxxxxx> Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx> --- .../selftests/kvm/aarch64/set_id_regs.c | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/tools/testing/selftests/kvm/aarch64/set_id_regs.c b/tools/testing/selftests/kvm/aarch64/set_id_regs.c index bac05210b539..f17454dc6d9e 100644 --- a/tools/testing/selftests/kvm/aarch64/set_id_regs.c +++ b/tools/testing/selftests/kvm/aarch64/set_id_regs.c @@ -224,13 +224,20 @@ uint64_t get_safe_value(const struct reg_ftr_bits *ftr_bits, uint64_t ftr) { uint64_t ftr_max = GENMASK_ULL(ARM64_FEATURE_FIELD_BITS - 1, 0);- if (ftr_bits->type == FTR_UNSIGNED) {+ if (ftr_bits->sign == FTR_UNSIGNED) { switch (ftr_bits->type) { case FTR_EXACT: ftr = ftr_bits->safe_val; break; case FTR_LOWER_SAFE: - if (ftr > 0) + uint64_t min_safe = 0; + + if (!strcmp(ftr_bits->name, "ID_AA64DFR0_EL1_DebugVer")) + min_safe = ID_AA64DFR0_EL1_DebugVer_IMP; + else if (!strcmp(ftr_bits->name, "ID_DFR0_EL1_CopDbg")) + min_safe = ID_DFR0_EL1_CopDbg_Armv8;
Instead of hardcoding the safe value here in the code, why not "fix" the safe value in the ftr_id table and use ftr_bits->safe_val for both the
above cases ?
+ + if (ftr > min_safe) ftr--; break; case FTR_HIGHER_SAFE: @@ -252,7 +259,12 @@ uint64_t get_safe_value(const struct reg_ftr_bits *ftr_bits, uint64_t ftr) ftr = ftr_bits->safe_val; break; case FTR_LOWER_SAFE: - if (ftr > 0) + uint64_t min_safe = 0; + + if (!strcmp(ftr_bits->name, "ID_DFR0_EL1_PerfMon")) + min_safe = ID_DFR0_EL1_PerfMon_PMUv3; + + if (ftr > min_safe) ftr--;
Also, here, don't we need to type case both "ftr" and min_safe to int64_t for signed features ?
Suzuki
break; case FTR_HIGHER_SAFE: @@ -276,7 +288,7 @@ uint64_t get_invalid_value(const struct reg_ftr_bits *ftr_bits, uint64_t ftr) { uint64_t ftr_max = GENMASK_ULL(ARM64_FEATURE_FIELD_BITS - 1, 0);- if (ftr_bits->type == FTR_UNSIGNED) {+ if (ftr_bits->sign == FTR_UNSIGNED) { switch (ftr_bits->type) { case FTR_EXACT: ftr = max((uint64_t)ftr_bits->safe_val + 1, ftr + 1); base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a
