| Message ID | 20240109165622.4104387-1-jingzhangos@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] KVM: arm64: selftests: Handle feature fields with nonzero minimum value correctly | expand |
On Tue, Jan 09, 2024 at 08:56:21AM -0800, 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@huawei.com> > Reported-by: Itaru Kitayama <itaru.kitayama@linux.dev> > Signed-off-by: Jing Zhang <jingzhangos@google.com> > --- > .../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; > + > + 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--; > 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); > This fixes the issue seen on an AEM RevC FVP launched via the shrinkwrap ns-edk2.yaml config. [...] # ok 79 ID_AA64ZFR0_EL1_SVEver # # Totals: pass:79 fail:0 xfail:0 xpass:0 skip:0 error:0 ok 1 selftests: kvm: set_id_regs Tested-by: Itaru Kitayama <itaru.kitayama@fujitsu.com> Thanks, Itaru. > base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a > -- > 2.43.0.472.g3155946c3a-goog >
Hi Jing, On 2024/1/10 0: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@huawei.com> > Reported-by: Itaru Kitayama <itaru.kitayama@linux.dev> > Signed-off-by: Jing Zhang <jingzhangos@google.com> > --- > .../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; > + > + if (ftr > min_safe) As I mentioned in my previous reply, there is a compilation error with gcc-10.3.1. | aarch64/set_id_regs.c: In function 'get_safe_value': | aarch64/set_id_regs.c:233:4: error: a label can only be part of a statement and a declaration is not a statement | 233 | uint64_t min_safe = 0; | | ^~~~~~~~ | aarch64/set_id_regs.c:262:4: error: a label can only be part of a statement and a declaration is not a statement | 262 | uint64_t min_safe = 0; | | ^~~~~~~~ Please fix it. Zenghui
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@huawei.com> > Reported-by: Itaru Kitayama <itaru.kitayama@linux.dev> > Signed-off-by: Jing Zhang <jingzhangos@google.com> > --- > .../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
Hi Zenghui, On Sun, Jan 14, 2024 at 11:41 PM Zenghui Yu <yuzenghui@huawei.com> wrote: > > Hi Jing, > > On 2024/1/10 0: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@huawei.com> > > Reported-by: Itaru Kitayama <itaru.kitayama@linux.dev> > > Signed-off-by: Jing Zhang <jingzhangos@google.com> > > --- > > .../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; > > + > > + if (ftr > min_safe) > > As I mentioned in my previous reply, there is a compilation error with > gcc-10.3.1. > > | aarch64/set_id_regs.c: In function 'get_safe_value': > | aarch64/set_id_regs.c:233:4: error: a label can only be part of a > statement and a declaration is not a statement > | 233 | uint64_t min_safe = 0; > | | ^~~~~~~~ > | aarch64/set_id_regs.c:262:4: error: a label can only be part of a > statement and a declaration is not a statement > | 262 | uint64_t min_safe = 0; > | | ^~~~~~~~ > > Please fix it. Will fix it. > > Zenghui Thanks, Jing
Hi Suzuki, On Mon, Jan 15, 2024 at 1:34 AM Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > > 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@huawei.com> > > Reported-by: Itaru Kitayama <itaru.kitayama@linux.dev> > > Signed-off-by: Jing Zhang <jingzhangos@google.com> > > --- > > .../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 ? > SGTM. Will do. > > + > > + 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 ? They are all used as unsigned on purpose. That's why the handling for signed features are handled in different cases. > > 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 > Thanks, Jing
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; + + 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--; 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);
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@huawei.com> Reported-by: Itaru Kitayama <itaru.kitayama@linux.dev> Signed-off-by: Jing Zhang <jingzhangos@google.com> --- .../selftests/kvm/aarch64/set_id_regs.c | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a