Re: [PATCH 1/2] KVM: selftests: arm64: Introduce and use hardware-definition macros

[Oliver Upton]

Hi Raghu,

On Fri, Apr 04, 2025 at 10:06:58PM +0000, Raghavendra Rao Ananta wrote:
> The kvm selftest library for arm64 currently configures the hardware
> fields, such as shift and mask in the page-table entries and registers,
> directly with numbers. While it add comments at places, it's better to
> rewrite them with appropriate macros to improve the readability and
> reduce the risk of errors. Hence, introduce macros to define the
> hardware fields and use them in the arm64 processor library.
> 
> Most of the definitions are primary copied from the Linux's header,
> arch/arm64/include/asm/pgtable-hwdef.h.

Thank you for doing this. Having magic numbers all around the shop was a
complete mess. Just a single comment:

> No functional change intended.
> 
> Suggested-by: Oliver Upton <oupton@xxxxxxxxxx>
> Signed-off-by: Raghavendra Rao Ananta <rananta@xxxxxxxxxx>
> ---
>  tools/arch/arm64/include/asm/sysreg.h         | 38 +++++++++++++
>  .../selftests/kvm/arm64/page_fault_test.c     |  2 +-
>  .../selftests/kvm/include/arm64/processor.h   | 28 +++++++--
>  .../selftests/kvm/lib/arm64/processor.c       | 57 ++++++++++---------
>  4 files changed, 92 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/arch/arm64/include/asm/sysreg.h b/tools/arch/arm64/include/asm/sysreg.h
> index 150416682e2c..6fcde168f3a6 100644
> --- a/tools/arch/arm64/include/asm/sysreg.h
> +++ b/tools/arch/arm64/include/asm/sysreg.h
> @@ -884,6 +884,44 @@
>  	 SCTLR_EL1_LSMAOE | SCTLR_EL1_nTLSMD | SCTLR_EL1_EIS   | \
>  	 SCTLR_EL1_TSCXT  | SCTLR_EL1_EOS)
>  
> +/* TCR_EL1 specific flags */
> +#define TCR_T0SZ_OFFSET	0
> +#define TCR_T0SZ(x)		((UL(64) - (x)) << TCR_T0SZ_OFFSET)
> +
> +#define TCR_IRGN0_SHIFT	8
> +#define TCR_IRGN0_MASK		(UL(3) << TCR_IRGN0_SHIFT)
> +#define TCR_IRGN0_NC		(UL(0) << TCR_IRGN0_SHIFT)
> +#define TCR_IRGN0_WBWA		(UL(1) << TCR_IRGN0_SHIFT)
> +#define TCR_IRGN0_WT		(UL(2) << TCR_IRGN0_SHIFT)
> +#define TCR_IRGN0_WBnWA	(UL(3) << TCR_IRGN0_SHIFT)
> +
> +#define TCR_ORGN0_SHIFT	10
> +#define TCR_ORGN0_MASK		(UL(3) << TCR_ORGN0_SHIFT)
> +#define TCR_ORGN0_NC		(UL(0) << TCR_ORGN0_SHIFT)
> +#define TCR_ORGN0_WBWA		(UL(1) << TCR_ORGN0_SHIFT)
> +#define TCR_ORGN0_WT		(UL(2) << TCR_ORGN0_SHIFT)
> +#define TCR_ORGN0_WBnWA	(UL(3) << TCR_ORGN0_SHIFT)
> +
> +#define TCR_SH0_SHIFT		12
> +#define TCR_SH0_MASK		(UL(3) << TCR_SH0_SHIFT)
> +#define TCR_SH0_INNER		(UL(3) << TCR_SH0_SHIFT)
> +
> +#define TCR_TG0_SHIFT		14
> +#define TCR_TG0_MASK		(UL(3) << TCR_TG0_SHIFT)
> +#define TCR_TG0_4K		(UL(0) << TCR_TG0_SHIFT)
> +#define TCR_TG0_64K		(UL(1) << TCR_TG0_SHIFT)
> +#define TCR_TG0_16K		(UL(2) << TCR_TG0_SHIFT)
> +
> +#define TCR_IPS_SHIFT		32
> +#define TCR_IPS_MASK		(UL(7) << TCR_IPS_SHIFT)
> +#define TCR_IPS_52_BITS	(UL(6) << TCR_IPS_SHIFT)
> +#define TCR_IPS_48_BITS	(UL(5) << TCR_IPS_SHIFT)
> +#define TCR_IPS_40_BITS	(UL(2) << TCR_IPS_SHIFT)
> +#define TCR_IPS_36_BITS	(UL(1) << TCR_IPS_SHIFT)
> +
> +#define TCR_HA			(UL(1) << 39)
> +#define TCR_DS			(UL(1) << 59)
> +

sysreg.h isn't the right home for these definitions since it is meant to
be a copy of the corresponding kernel header.

Since KVM selftests are likely the only thing in tools to care about
setting up page tables, adding this to processor.h seems like a better
place.

Thanks,
Oliver