Re: [PATCH 2/2] KVM: selftests: arm64: Use generated defines for named system registers

[Marc Zyngier]

On Fri, 02 Aug 2024 22:57:54 +0100,
Mark Brown <broonie@xxxxxxxxxx> wrote:
> 
> Currently the get-reg-list test uses directly specified numeric values to
> define system registers to validate. Since we already have a macro which
> allows us to use the generated system register definitions from the main
> kernel easily let's update all the registers where we have specified the
> name in a comment to just use that macro. This reduces the number of
> places where we need to validate the name to number mapping.
> 
> This conversion was done with the sed command:
> 
>   sed -i -E 's-ARM64_SYS_REG.*/\* (.*) \*/-KVM_ARM64_SYS_REG(SYS_\1),-' tools/testing/selftests/kvm/aarch64/get-reg-list.c
>

[Eyes rolling]

What I asked about scripting the whole thing, it never occurred to me
that you would use the *comments* as a reliable source of information.
Do we have anything less reliable than comments in the kernel?

The matching must be done from the arch/arm64/tools/sysreg file,
because that's the (admittedly dubious) source of truth. We actually
trust the encodings because they are reported by the kernel itself.
The comment is hand-written, and likely wrong.

Also, this hides the horrible truth about existing ABI bugs, see
below.

> We still have a number of numerically specified registers, some of these
> are reserved registers without defined names (eg, unallocated ID registers)
> and others don't have kernel macro definitions yet.
> 
> No change in the generated output.
> 
> Suggested-by: Marc Zyngier <maz@xxxxxxxxxx>
> Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>
> ---
>  tools/testing/selftests/kvm/aarch64/get-reg-list.c | 208 ++++++++++-----------
>  1 file changed, 104 insertions(+), 104 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> index a00322970578..4d786c4ab28a 100644
> --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
> @@ -313,14 +313,14 @@ static __u64 base_regs[] = {
>  	KVM_REG_ARM_FW_FEAT_BMAP_REG(0),	/* KVM_REG_ARM_STD_BMAP */
>  	KVM_REG_ARM_FW_FEAT_BMAP_REG(1),	/* KVM_REG_ARM_STD_HYP_BMAP */
>  	KVM_REG_ARM_FW_FEAT_BMAP_REG(2),	/* KVM_REG_ARM_VENDOR_HYP_BMAP */
> -	ARM64_SYS_REG(3, 3, 14, 3, 1),	/* CNTV_CTL_EL0 */
> -	ARM64_SYS_REG(3, 3, 14, 3, 2),	/* CNTV_CVAL_EL0 */
> +	KVM_ARM64_SYS_REG(SYS_CNTV_CTL_EL0),
> +	KVM_ARM64_SYS_REG(SYS_CNTV_CVAL_EL0),
>  	ARM64_SYS_REG(3, 3, 14, 0, 2),

Great. So not only you fail convert a register, but you also ignore
the nugget described in arch/arm64/invlude/uapi/asm/kvm.h:267.

Sure, having both described hides the crap, as we don't attach any
significance to the registers themselves. But that shows how
untrustworthy the comments are.

> -	ARM64_SYS_REG(3, 0, 0, 0, 0),	/* MIDR_EL1 */
> -	ARM64_SYS_REG(3, 0, 0, 0, 6),	/* REVIDR_EL1 */
> -	ARM64_SYS_REG(3, 1, 0, 0, 1),	/* CLIDR_EL1 */
> -	ARM64_SYS_REG(3, 1, 0, 0, 7),	/* AIDR_EL1 */
> -	ARM64_SYS_REG(3, 3, 0, 0, 1),	/* CTR_EL0 */
> +	KVM_ARM64_SYS_REG(SYS_MIDR_EL1),
> +	KVM_ARM64_SYS_REG(SYS_REVIDR_EL1),
> +	KVM_ARM64_SYS_REG(SYS_CLIDR_EL1),
> +	KVM_ARM64_SYS_REG(SYS_AIDR_EL1),
> +	KVM_ARM64_SYS_REG(SYS_CTR_EL0),
>  	ARM64_SYS_REG(2, 0, 0, 0, 4),
>  	ARM64_SYS_REG(2, 0, 0, 0, 5),
>  	ARM64_SYS_REG(2, 0, 0, 0, 6),

As far as I can tell, these registers are not unallocated, and they
should be named.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.