Hi Mark,
On 3/6/26 17:01, 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 <[email protected]>
> ---
> tools/testing/selftests/kvm/arm64/set_id_regs.c | 63
> +++++++++++++++++++++----
> 1 file changed, 53 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/arm64/set_id_regs.c
> b/tools/testing/selftests/kvm/arm64/set_id_regs.c
> index bfca7be3e766..928e7d9e5ab7 100644
> --- a/tools/testing/selftests/kvm/arm64/set_id_regs.c
> +++ b/tools/testing/selftests/kvm/arm64/set_id_regs.c
> @@ -317,11 +317,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) {
> @@ -329,42 +330,81 @@ 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);
> 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;
> 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++;
> break;
> case FTR_HIGHER_SAFE:
> - ftr--;
> + /* FIXME: "need to check for the actual highest." */
> + if (ftr == ftr_max)
> + *skip = true;
> + else
> + ftr--;
> break;
> case FTR_HIGHER_OR_ZERO_SAFE:
> - if (ftr == 0)
> - ftr = ftr_max - 1;
> - else
> + 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;
> + switch (ftr_bits->type) {
> + case FTR_LOWER_SAFE:
> + if (ftr == 0)
> + *skip = true;
> + else
> + ftr = 0;
> + break;
> + default:
> + *skip = true;
> + break;
> + }
> }
I hacked up a quick loop to check what this function is doing.
With a mask=0x1 I see some value returned that have bits set
outside of the mask.
safe_val ftr out
UNSIGNED
FTR_EXACT
0x0 0x0 0x1
0x0 0x1 0x2 # out of range
0x1 0x0 0x2 # out of range
0x1 0x1 0x2 # out of range
FTR_LOWER_SAFE
0x0 0x0 0x1
0x0 0x1 SKIP
0x1 0x0 0x1
0x1 0x1 SKIP
FTR_HIGHER_SAFE
0x0 0x0 SKIP
0x0 0x1 0x0
0x1 0x0 SKIP
0x1 0x1 0x0
FTR_HIGHER_OR_ZERO_SAFE
0x0 0x0 0x1
0x0 0x1 SKIP
0x1 0x0 0x1
0x1 0x1 SKIP
SIGNED
FTR_EXACT
0x0 0x0 SKIP
0x0 0x1 SKIP
0x1 0x0 SKIP
0x1 0x1 SKIP
FTR_LOWER_SAFE
0x0 0x0 0x1
0x0 0x1 0x0
0x1 0x0 0x1
0x1 0x1 0x0
FTR_HIGHER_SAFE
0x0 0x0 0xffffffffffffffff # out of range
0x0 0x1 SKIP
0x1 0x0 0xffffffffffffffff # out of range
0x1 0x1 SKIP
FTR_HIGHER_OR_ZERO_SAFE
0x0 0x0 SKIP
0x0 0x1 SKIP
0x1 0x0 SKIP
0x1 0x1 SKIP
Thanks,
Ben
>
> return ftr;
> @@ -399,12 +439,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;
>
