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 C02DAC48BF6 for ; Mon, 26 Feb 2024 13:35:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=p1kc3K8bZT4CpGSpyDgdl2uue1+O04F9fvG7gvE8m64=; b=rqyTwBIPN62ab3 d5jw7fBfp3wpJZAAL1vTdKK3ZELYlRhbfHjlEhPSQsKzqzlek+uO0eSiys3Lfqzw30r62Xcg7//Hx xNmbo32Lru1KLRIaUSGBX2+ZsASPRjA4Xz5AWuZCLtp7LbbDJ03B+MFjYMFnm+8MzwzTBjTMMX8lR IZR+ZiX9X0ptxF8IEo+ZtOzuWQnzBN3cn0btB3YPlZlnD484KZdWFA9BAOyG/n5QuhQeJDal+gfri IcvSFcZcG9JkE6NVIe5RjZ0DZ3HHMWHAFETaU9CLIr+5B9hbrUQ/VaehzGjm/RVwFuba4OLSL3LTT IMuRHWid3kEW8xiOjuPQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1reb8z-00000000r9z-0Z3M; Mon, 26 Feb 2024 13:35:45 +0000 Received: from sin.source.kernel.org ([2604:1380:40e1:4800::1]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1reb8v-00000000r7d-1GF3 for linux-arm-kernel@lists.infradead.org; Mon, 26 Feb 2024 13:35:43 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id 28C3DCE1808; Mon, 26 Feb 2024 13:35:33 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 40F62C433C7; Mon, 26 Feb 2024 13:35:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1708954532; bh=XS1neCkWPoPljIEQbwRt980RilF+BhhhXaZaIXAH2fA=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=VVeP1i/l/3ad5qyNtST/friOyK4IhqPqSnJUeVqVY51RimcikAIC0+2syENeKG3g8 ihYyvoHKQCh2JvT1o4WhL8eBsQYqK6OdgKvWpjMiM80saLjtVlrc91oezM8q+T95TR 8BHgVS2IYkA1IuuV0+QlIA4jwyFr5z1T1v3P2A5/vSwuz973NHPnJSO/R6E+UEQibG Ho4RmhbEQ1bn6LGDRzptQ+iQhn4xhW+Fi0uS4dhg2tFv9cVD+wjaqYpV0pTa3WKgLL WMRSIPATUD6E+IDCdLpXCEVD50XMZ5whrryDreyyil7w22YOSAuAjFGnm9nUJ0BjCN ZnF5RdRPDwbNA== 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 1reb8j-006sTv-BW; Mon, 26 Feb 2024 13:35:29 +0000 Date: Mon, 26 Feb 2024 13:35:28 +0000 Message-ID: <86edcz2vrz.wl-maz@kernel.org> From: Marc Zyngier To: James Clark Cc: coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, suzuki.poulose@arm.com, acme@kernel.org, oliver.upton@linux.dev, broonie@kernel.org, James Morse , Zenghui Yu , Catalin Marinas , Will Deacon , Mike Leach , Alexander Shishkin , Anshuman Khandual , Miguel Luis , Joey Gouly , Ard Biesheuvel , Quentin Perret , Javier Martinez Canillas , Mark Rutland , Arnd Bergmann , Vincent Donnefort , Ryan Roberts , Fuad Tabba , Jing Zhang , linux-kernel@vger.kernel.org Subject: Re: [PATCH v6 5/8] arm64: KVM: Add iflag for FEAT_TRF In-Reply-To: <20240226113044.228403-6-james.clark@arm.com> References: <20240226113044.228403-1-james.clark@arm.com> <20240226113044.228403-6-james.clark@arm.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.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: james.clark@arm.com, coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, suzuki.poulose@arm.com, acme@kernel.org, oliver.upton@linux.dev, broonie@kernel.org, james.morse@arm.com, yuzenghui@huawei.com, catalin.marinas@arm.com, will@kernel.org, mike.leach@linaro.org, alexander.shishkin@linux.intel.com, anshuman.khandual@arm.com, miguel.luis@oracle.com, joey.gouly@arm.com, ardb@kernel.org, qperret@google.com, javierm@redhat.com, mark.rutland@arm.com, arnd@arndb.de, vdonnefort@google.com, ryan.roberts@arm.com, tabba@google.com, jingzhangos@google.com, linux-kernel@vger.kernel.org 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-20240226_053541_758360_D6547ACB X-CRM114-Status: GOOD ( 36.11 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, 26 Feb 2024 11:30:33 +0000, James Clark wrote: > > Add an extra iflag to signify if the TRFCR register is accessible. That's not what this flag means: it indicates whether TRFCR needs to be saved. At lease that's what the name suggests. > Because TRBE requires FEAT_TRF, DEBUG_STATE_SAVE_TRBE still has the same > behavior even though it's only set when FEAT_TRF is present. This sentence seems completely out of context, because you didn't explain that you were making TRBE *conditional* on TRF being implemented, as per the architecture requirements. > > The following holes are left in struct kvm_vcpu_arch, but there aren't > enough other 8 bit fields to rearrange it to leave any hole smaller than > 7 bytes: > > u8 cflags; /* 2292 1 */ > /* XXX 1 byte hole, try to pack */ > u16 iflags; /* 2294 2 */ > u8 sflags; /* 2296 1 */ > bool pause; /* 2297 1 */ > /* XXX 6 bytes hole, try to pack */ I don't think that's particularly useful in a commit message, but more relevant to the cover letter. However, see below. > > Reviewed-by: Suzuki K Poulose > Signed-off-by: James Clark > --- > arch/arm64/include/asm/kvm_host.h | 4 +++- > arch/arm64/kvm/debug.c | 24 ++++++++++++++++++++---- > 2 files changed, 23 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 21c57b812569..85b5477bd1b4 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -569,7 +569,7 @@ struct kvm_vcpu_arch { > u8 cflags; > > /* Input flags to the hypervisor code, potentially cleared after use */ > - u8 iflags; > + u16 iflags; > > /* State flags for kernel bookkeeping, unused by the hypervisor code */ > u8 sflags; > @@ -779,6 +779,8 @@ struct kvm_vcpu_arch { > #define DEBUG_STATE_SAVE_TRBE __vcpu_single_flag(iflags, BIT(6)) > /* vcpu running in HYP context */ > #define VCPU_HYP_CONTEXT __vcpu_single_flag(iflags, BIT(7)) > +/* Save trace filter controls */ > +#define DEBUG_STATE_SAVE_TRFCR __vcpu_single_flag(iflags, BIT(8)) I'd rather you cherry-pick [1] and avoid expanding the iflags. [1] https://lore.kernel.org/r/20240226100601.2379693-4-maz@kernel.org Now, I think the whole SPE/TRBE/TRCR flag management should be improved, see below. > > /* SVE enabled for host EL0 */ > #define HOST_SVE_ENABLED __vcpu_single_flag(sflags, BIT(0)) > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > index ce8886122ed3..49a13e72ddd2 100644 > --- a/arch/arm64/kvm/debug.c > +++ b/arch/arm64/kvm/debug.c > @@ -332,14 +332,30 @@ void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu) > !(read_sysreg_s(SYS_PMBIDR_EL1) & BIT(PMBIDR_EL1_P_SHIFT))) > vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_SPE); > > - /* Check if we have TRBE implemented and available at the host */ > - if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_TraceBuffer_SHIFT) && > - !(read_sysreg_s(SYS_TRBIDR_EL1) & TRBIDR_EL1_P)) > - vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRBE); > + /* > + * Set SAVE_TRFCR flag if FEAT_TRF (TraceFilt) exists. This flag > + * signifies that the exclude_host/exclude_guest settings of any active > + * host Perf session on a core running a VCPU can be written into > + * TRFCR_EL1 on guest switch. > + */ > + if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_TraceFilt_SHIFT)) { > + vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRFCR); Can we avoid doing this unconditionally? It only makes sense to save the trace crud if it is going to be changed, right? > + /* > + * Check if we have TRBE implemented and available at the host. > + * If it's in use at the time of guest switch then trace will > + * need to be completely disabled. The architecture mandates > + * FEAT_TRF with TRBE, so we only need to check for TRBE after > + * TRF. > + */ > + if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_TraceBuffer_SHIFT) && > + !(read_sysreg_s(SYS_TRBIDR_EL1) & TRBIDR_EL1_P)) > + vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRBE); > + } > } > > void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu) > { > vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE); > vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE); > + vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRFCR); > } Dealing with flags that are strongly coupled in a disjoined way a pretty bad idea. Look at the generated code, and realise we flip the preempt flag on each access. Can we do better? You bet. The vcpu_{set,clear}_flags infrastructure is capable of dealing with multiple flags at once, as demonstrated by the way we deal with exception encoding. Something like: diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index addf79ba8fa0..3e50e535fdd4 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -885,6 +885,10 @@ struct kvm_vcpu_arch { #define DEBUG_STATE_SAVE_SPE __vcpu_single_flag(iflags, BIT(5)) /* Save TRBE context if active */ #define DEBUG_STATE_SAVE_TRBE __vcpu_single_flag(iflags, BIT(6)) +/* Save Trace Filter Controls */ +#define DEBUG_STATE_SAVE_TRFCR __vcpu_single_flag(iflags, BIT(7)) +/* Global debug mask */ +#define DEBUG_STATE_SAVE_MASK __vcpu_single_flag(iflags, GENMASK(7, 5)) /* SVE enabled for host EL0 */ #define HOST_SVE_ENABLED __vcpu_single_flag(sflags, BIT(0)) diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c index 8725291cb00a..f9b197a00582 100644 --- a/arch/arm64/kvm/debug.c +++ b/arch/arm64/kvm/debug.c @@ -339,6 +339,6 @@ void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu) void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu) { - vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE); - vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE); + if (!has_vhe()) + vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_MASK); } Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel