From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 80421C02199 for ; Thu, 6 Feb 2025 17:35:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Type:MIME-Version: References:In-Reply-To:Subject:Cc:To:From:Message-ID:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=h3+o6j1h8yfZZvlMm0xz4Wn2mMnH1wEvcnKhuOcTzzw=; b=YpFi0UoPqKh3m0YpdV8OCDt2Po luDsV0hnWcvPCJCWK+ZIxSbYnUgMx716SsQdJeYfe7SqBqly7v2jRsm8H6lG3CkDl2CIsSG4cyN9d HRQb3U1xkPiUe1rgag3OqIdA/ZcQV3Yagn45vRlmRm6Kxv93jOP+4eREriIifNZKusFTUlGXxn/rm ZE9QeLO7cTgGvkmuUM6lwn9CLieoUhlDcZgqceyLmMcqKvg5mtVHD5s3aZ9m0m2hhHG2i2X0hFwaS FXHTUayKjRgD2ZlRKQrT1FO8/lmQCwuYVFnnBHhpURcz0H6HfeOjLAuJvST7ZxMhl0IaQz2K2Ogqm g5RK961Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1tg5mW-000000070S1-1SoO; Thu, 06 Feb 2025 17:35:16 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1tg5iB-00000006ze7-196H for linux-arm-kernel@lists.infradead.org; Thu, 06 Feb 2025 17:30:48 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id ED1D25C5C26; Thu, 6 Feb 2025 17:30:06 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4AEB6C4CEDD; Thu, 6 Feb 2025 17:30:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1738863046; bh=dEzyg2r4tZKWMxK6fgAdi9bkgZ+Gg97tUdv2KrGqngY=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=kmJhN3pEUEX7HmbyYzousuk6RwyqiD+uQ0neynfv0Zzru2qTIJfYAYAQ9M15TgToN B8oYRMKQdkCh5HoMPT831dhlNYGZPK9b/aeb5P8Ts1mISgehqbU/yejenz2dRDFjS6 SRlqjkQuUPrT4guXBqtotmRYS96p+1LCjZZaBDeI4v/YMhVZXj9MrfDuJeNmp3YINj 4Wi7TyYvrfNr3ZXbxSp6U53QCPiLJAJfbG0F/R3ON0KY8lxSTOCk+MlJfvHU/cPcM0 0x1Zee1z3BDwOh/2k1s43SwA0zFNFuxAfSvX0xwcinHTD1NBjz1GmXLv2ZUrLNshGz alEImmKigZ4ug== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1tg5i7-001DLm-VD; Thu, 06 Feb 2025 17:30:44 +0000 Date: Thu, 06 Feb 2025 17:30:43 +0000 Message-ID: <86pljvt1z0.wl-maz@kernel.org> From: Marc Zyngier To: Ganapatrao Kulkarni Cc: kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org, oliver.upton@linux.dev, joey.gouly@arm.com, suzuki.poulose@arm.com, darren@os.amperecomputing.com, scott@os.amperecomputing.com Subject: Re: [RFC PATCH 2/2] KVM: arm64: nv: selftests: Access VNCR mapped registers In-Reply-To: <20250206164120.4045569-3-gankulkarni@os.amperecomputing.com> References: <20250206164120.4045569-1-gankulkarni@os.amperecomputing.com> <20250206164120.4045569-3-gankulkarni@os.amperecomputing.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/29.4 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: gankulkarni@os.amperecomputing.com, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org, oliver.upton@linux.dev, joey.gouly@arm.com, suzuki.poulose@arm.com, darren@os.amperecomputing.com, scott@os.amperecomputing.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250206_093047_403709_02DC708D X-CRM114-Status: GOOD ( 34.29 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, 06 Feb 2025 16:41:20 +0000, Ganapatrao Kulkarni 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 > --- > 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 > +#include > +#include > +#include > + > +#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.