Hi Mark,
On 12/23/25 01:21, Mark Brown wrote:
> The set_id_regs test currently assumes that there will always be invalid
> values available in bitfields for it to generate but this may not be the
> case if the architecture has defined meanings for every possible value for
> the bitfield. An assert added in commit bf09ee918053e ("KVM: arm64:
> selftests: Remove ARM64_FEATURE_FIELD_BITS and its last user") refuses to
> run for single bit fields which will show the issue most readily but there
> is no reason wider ones can't show the same issue.
>
> Rework the tests for invalid value to check if an invalid value can be
> generated and skip the test if not, removing the assert.
>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
> tools/testing/selftests/kvm/arm64/set_id_regs.c | 58 ++++++++++++++++++++-----
> 1 file changed, 46 insertions(+), 12 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/arm64/set_id_regs.c b/tools/testing/selftests/kvm/arm64/set_id_regs.c
> index 322cd13b9352..641194c5005a 100644
> --- a/tools/testing/selftests/kvm/arm64/set_id_regs.c
> +++ b/tools/testing/selftests/kvm/arm64/set_id_regs.c
> @@ -318,11 +318,12 @@ uint64_t get_safe_value(const struct reg_ftr_bits *ftr_bits, uint64_t ftr)
> }
>
> /* Return an invalid value to a given ftr_bits an ftr value */
> -uint64_t get_invalid_value(const struct reg_ftr_bits *ftr_bits, uint64_t ftr)
> +uint64_t get_invalid_value(const struct reg_ftr_bits *ftr_bits, uint64_t ftr,
> + bool *skip)
> {
> uint64_t ftr_max = ftr_bits->mask >> ftr_bits->shift;
>
> - TEST_ASSERT(ftr_max > 1, "This test doesn't support single bit features");
> + *skip = false;
>
> if (ftr_bits->sign == FTR_UNSIGNED) {
> switch (ftr_bits->type) {
> @@ -330,42 +331,72 @@ uint64_t get_invalid_value(const struct reg_ftr_bits *ftr_bits, uint64_t ftr)
> ftr = max((uint64_t)ftr_bits->safe_val + 1, ftr + 1);
Don't you want the check you've got in the signed case here to deal with
safe_val or ftr being equal to ftr_max. In the 1 bit case we there won't
be any invalid values if the safe_val and ftr are different.
> break;
> case FTR_LOWER_SAFE:
> + if (ftr == ftr_max)
> + *skip = true;
> ftr++;
> break;
> case FTR_HIGHER_SAFE:
> + if (ftr == 0)
> + *skip = true;
> ftr--;
> break;
> case FTR_HIGHER_OR_ZERO_SAFE:
> - if (ftr == 0)
> + switch (ftr) {
> + case 0:
> ftr = ftr_max;
> - else
> + break;
> + case 1:
> + *skip = true;
> + break;
> + default:
> ftr--;
> - break;
> + break;
> + }
Missing break for the outer switch statement.
> default:
> + *skip = true;
> break;
> }
> } else if (ftr != ftr_max) {
> switch (ftr_bits->type) {
> case FTR_EXACT:
> ftr = max((uint64_t)ftr_bits->safe_val + 1, ftr + 1);
> + if (ftr > ftr_max)
> + *skip = true;
> break;
> case FTR_LOWER_SAFE:
> - ftr++;
> + if (ftr == ftr_max)
> + *skip = true;
This is the opposite condition of the enclosing else if.
> + else
> + ftr++;
> break;
> case FTR_HIGHER_SAFE:
> - ftr--;
> - break;
> - case FTR_HIGHER_OR_ZERO_SAFE:
> if (ftr == 0)
> - ftr = ftr_max - 1;
> + *skip = true;
Isn't ftr_max, -1, invalid in FTR_HIGHER_SAFE case when ftr is 0. Also,
need to check for the actual highest.
> else
> ftr--;
> break;
> + case FTR_HIGHER_OR_ZERO_SAFE:
> + switch (ftr) {
> + case 0:
> + if (ftr_max > 1)
> + ftr = ftr_max - 1;
> + else
> + *skip = true;
> + break;
> + case 1:
> + *skip = true;
> + break;
> + default:
> + ftr--;
> + break;
> + break;
> + }
> default:
> + *skip = true;
> break;
> }
> } else {
> - ftr = 0;
> + *skip = true;
Why do we always skip when signed and ftr is -1? Wouldn't 0 be an
invalid in the FTR_LOWER_SAFE case.
> }
>
> return ftr;
> @@ -400,12 +431,15 @@ static void test_reg_set_fail(struct kvm_vcpu *vcpu, uint64_t reg,
> uint8_t shift = ftr_bits->shift;
> uint64_t mask = ftr_bits->mask;
> uint64_t val, old_val, ftr;
> + bool skip;
> int r;
>
> val = vcpu_get_reg(vcpu, reg);
> ftr = (val & mask) >> shift;
>
> - ftr = get_invalid_value(ftr_bits, ftr);
> + ftr = get_invalid_value(ftr_bits, ftr, &skip);
> + if (skip)
> + return;
>
> old_val = val;
> ftr <<= shift;
>
Thanks,
Ben