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;if (ftr == 0)*skip = true; ftr--; break;
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;if (ftr > ftr_max)*skip = true; break;
ftr++;
if (ftr == ftr_max)*skip = true;
This is the opposite condition of the enclosing else if.
elseftr++; break;
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;}*skip = true; break;
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