Re: [PATCH v10 28/30] KVM: arm64: selftests: Skip impossible invalid value tests

[Ben Horgan <ben.horgan@arm.com>]

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 <broonie@kernel.org>
> ---
>  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;
>