Message-ID: <86pljvt1z0.wl-maz@kernel.org>
Date: Thu, 06 Feb 2025 17:30:43 +0000
From: Marc Zyngier <maz@...nel.org>
To: Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com>
Cc: kvm@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	kvmarm@...ts.linux.dev,
	linux-kernel@...r.kernel.org,
	linux-kselftest@...r.kernel.org,
	oliver.upton@...ux.dev,
	joey.gouly@....com,
	suzuki.poulose@....com,
	darren@...amperecomputing.com,
	scott@...amperecomputing.com
Subject: Re: [RFC PATCH 2/2] KVM: arm64: nv: selftests: Access VNCR mapped registers

On Thu, 06 Feb 2025 16:41:20 +0000,
Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com> wrote:
> 
> With NV2 enabled, some of the EL1/EL2/EL12 register accesses are
> transformed to memory accesses. This test code accesses all those
> registers in guest code to validate that they are not trapped to L0.
>

Traps to L0 are invisible to the guest -- by definition. What the
guest can observe is an exception, be it injected by L0 or directly
delivered by the HW.

But the *absence* of an exception doesn't mean things are fine. Quite
the opposite.

> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com>
> ---
>  tools/testing/selftests/kvm/Makefile.kvm      |   1 +
>  .../selftests/kvm/arm64/nv_vncr_regs_test.c   | 255 ++++++++++++++++++
>  2 files changed, 256 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/arm64/nv_vncr_regs_test.c
> 
> diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
> index a85d3bec9fb1..7790e4021013 100644
> --- a/tools/testing/selftests/kvm/Makefile.kvm
> +++ b/tools/testing/selftests/kvm/Makefile.kvm
> @@ -155,6 +155,7 @@ TEST_GEN_PROGS_arm64 += arm64/vgic_lpi_stress
>  TEST_GEN_PROGS_arm64 += arm64/vpmu_counter_access
>  TEST_GEN_PROGS_arm64 += arm64/no-vgic-v3
>  TEST_GEN_PROGS_arm64 += arm64/nv_guest_hypervisor
> +TEST_GEN_PROGS_arm64 += arm64/nv_vncr_regs_test
>  TEST_GEN_PROGS_arm64 += access_tracking_perf_test
>  TEST_GEN_PROGS_arm64 += arch_timer
>  TEST_GEN_PROGS_arm64 += coalesced_io_test
> diff --git a/tools/testing/selftests/kvm/arm64/nv_vncr_regs_test.c b/tools/testing/selftests/kvm/arm64/nv_vncr_regs_test.c
> new file mode 100644
> index 000000000000..d05b20b828ff
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/arm64/nv_vncr_regs_test.c
> @@ -0,0 +1,255 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2025 Ampere Computing LLC
> + *
> + * This is a test to validate Nested Virtualization.
> + */
> +#include <kvm_util.h>
> +#include <nv_util.h>
> +#include <processor.h>
> +#include <vgic.h>
> +
> +#define __check_sr_read(r)					\
> +	({							\
> +		uint64_t val;					\
> +								\
> +		handled = false;				\
> +		dsb(sy);					\
> +		val = read_sysreg_s(SYS_ ## r);			\
> +		val;						\
> +	})
> +
> +#define __check_sr_write(r)					\
> +	do {							\
> +		handled = false;				\
> +		dsb(sy);					\
> +		write_sysreg_s(0, SYS_ ## r);			\
> +		isb();						\
> +	} while (0)
> +
> +
> +#define check_sr_read(r)					  \
> +	do {							  \
> +		__check_sr_read(r);				  \
> +		__GUEST_ASSERT(!handled, #r "Read Test Failed");  \
> +	} while (0)
> +
> +#define check_sr_write(r)					  \
> +	do {							  \
> +		__check_sr_write(r);				  \
> +		__GUEST_ASSERT(!handled, #r "Write Test Failed"); \
> +	} while (0)
> +
> +#define check_sr_rw(r)				\
> +	do {					\
> +		GUEST_PRINTF("%s\n", #r);	\
> +		check_sr_write(r);		\
> +		check_sr_read(r);		\
> +	} while (0)

Instead of lifting things from existing tests, you could move these
things to an include file for everybody's benefit.

> +
> +static void test_vncr_mapped_regs(void);
> +static void regs_test_ich_lr(void);
> +
> +static volatile bool handled;
> +
> +static void regs_test_ich_lr(void)
> +{
> +	int nr_lr, lr;
> +
> +	nr_lr  = (read_sysreg_s(SYS_ICH_VTR_EL2) & 0xf);
> +
> +	for (lr = 0; lr <= nr_lr;  lr++) {
> +		switch (lr) {
> +		case 0:
> +			check_sr_rw(ICH_LR0_EL2);
> +			break;
> +		case 1:
> +			check_sr_rw(ICH_LR1_EL2);
> +			break;
> +		case 2:
> +			check_sr_rw(ICH_LR2_EL2);
> +			break;
> +		case 3:
> +			check_sr_rw(ICH_LR3_EL2);
> +			break;
> +		case 4:
> +			check_sr_rw(ICH_LR4_EL2);
> +			break;
> +		case 5:
> +			check_sr_rw(ICH_LR5_EL2);
> +			break;
> +		case 6:
> +			check_sr_rw(ICH_LR6_EL2);
> +			break;
> +		case 7:
> +			check_sr_rw(ICH_LR7_EL2);
> +			break;
> +		case 8:
> +			check_sr_rw(ICH_LR8_EL2);
> +			break;
> +		case 9:
> +			check_sr_rw(ICH_LR9_EL2);
> +			break;
> +		case 10:
> +			check_sr_rw(ICH_LR10_EL2);
> +			break;
> +		case 11:
> +			check_sr_rw(ICH_LR11_EL2);
> +			break;
> +		case 12:
> +			check_sr_rw(ICH_LR12_EL2);
> +			break;
> +		case 13:
> +			check_sr_rw(ICH_LR13_EL2);
> +			break;
> +		case 14:
> +			check_sr_rw(ICH_LR14_EL2);
> +			break;
> +		case 15:
> +			check_sr_rw(ICH_LR15_EL2);
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +}
> +
> +/*
> + * Validate READ/WRITE to VNCR Mapped registers for NV1=0
> + */
> +
> +static void test_vncr_mapped_regs(void)
> +{
> +	/*
> +	 * Access all VNCR Mapped registers, and fail if we get an UNDEF.
> +	 */

No. You are accessing a lot of random registers, irrespective of the
configuration exposed to the guest. Being able to access it is not an
indication of correctness. Seeing it UNDEF is not an indication of a
bug.

Also, this is supposed to be an EL2 test that doesn't deal with NV
*itself*. There is no VNCR page in this test. The fact that the host
uses VNCR as a way to virtualise EL2 has nothing to do with the test
at all.

> +
> +	GUEST_PRINTF("VNCR Mapped registers access test:\n");
> +	check_sr_rw(VTTBR_EL2);
> +	check_sr_rw(VTCR_EL2);
> +	check_sr_rw(VMPIDR_EL2);
> +	check_sr_rw(CNTVOFF_EL2);
> +	check_sr_rw(HCR_EL2);
> +	check_sr_rw(HSTR_EL2);
> +	check_sr_rw(VPIDR_EL2);
> +	check_sr_rw(TPIDR_EL2);
> +	check_sr_rw(VNCR_EL2);
> +	check_sr_rw(CPACR_EL12);
> +	check_sr_rw(CONTEXTIDR_EL12);
> +	check_sr_rw(SCTLR_EL12);
> +	check_sr_rw(ACTLR_EL1);
> +	check_sr_rw(TCR_EL12);
> +	check_sr_rw(AFSR0_EL12);
> +	check_sr_rw(AFSR1_EL12);
> +	check_sr_rw(ESR_EL12);
> +	check_sr_rw(MAIR_EL12);
> +	check_sr_rw(AMAIR_EL12);
> +	check_sr_rw(MDSCR_EL1);
> +	check_sr_rw(SPSR_EL12);
> +	check_sr_rw(CNTV_CVAL_EL02);
> +	check_sr_rw(CNTV_CTL_EL02);
> +	check_sr_rw(CNTP_CVAL_EL02);
> +	check_sr_rw(CNTP_CTL_EL02);
> +	check_sr_rw(HAFGRTR_EL2);
> +	check_sr_rw(TTBR0_EL12);
> +	check_sr_rw(TTBR1_EL12);
> +	check_sr_rw(FAR_EL12);
> +	check_sr_rw(ELR_EL12);
> +	check_sr_rw(SP_EL1);
> +	check_sr_rw(VBAR_EL12);
> +
> +	regs_test_ich_lr();
> +
> +	check_sr_rw(ICH_AP0R0_EL2);
> +	check_sr_rw(ICH_AP1R0_EL2);
> +	check_sr_rw(ICH_HCR_EL2);
> +	check_sr_rw(ICH_VMCR_EL2);
> +	check_sr_rw(VDISR_EL2);

This should absolutely UNDEF in the absence of FEAT_RAS exposed to the
guest. And yet it won't. Why? That's because the architecture doesn't
allow this to be trapped. Does it mean being able to access it is
right? Absolutely not. This is an *architecture* bug.

> +	check_sr_rw(MPAM1_EL12);

This should UNDEF in the test if the configuration doesn't expose MPAM
to the guest. And I really hope it explodes or this is a glaring bug,
because MPAM should never be exposed to a guest.

The conclusion is that blindly testing that you can R/W registers buys
us nothing if you are not checking the validity of the access against
the architecture rules.

Any test should take as input:

- the configuration of the guest

- the expected outcome of the access for that particular configuration
  and trap configuration, as described in the ARM ARM pseudocode (or,
  even better, in the BSD-licensed JSON file that has all that
  information)

The result of the access must then be matched against these inputs and
any discrepancy reported, either as fatal (because the outcome of the
access wasn't the expected one) or as an expected, non-fatal failure
(because we know that the architecture is not self-consistent).

Yes, this looks a lot like a full CPU validation suite. What a
surprise.

	M.

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

Powered by blists - more mailing lists