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 X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D4F45C433C1 for ; Tue, 30 Mar 2021 12:16:58 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 500266023F for ; Tue, 30 Mar 2021 12:16:58 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 500266023F Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=desiato.20200630; 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=5WbwERYPvW0UhwsjYRxLVrID5GotTIn9K2y+b83zMpE=; b=RhKpcl97Z5MmvDfaKDkPMqYNd /2EM3NrDYtNzjsJhXi5Dglgc5OQBfz3g/cbsqSzXbng3xtBP5XxXRiXOE7fkApJ3Jv4enrB2iZGe/ KKt1PqwfumPThYzUK3JH8NGV6stToKarkruNZmqzqvdQlagbnwGB0A79WmkEeLfjqaZ3Szrubcimy rK5xl2VTdIj4enO113Gj/TD+xlDe4B/fpd7yvVO0ldeuZ2lJfkb+QAuOCTH0axp05j/6CkwIy/Adg z4iGI4YLd3/P1oQDQim1p7+V1YdP1wIgtnAbpS+a6KY8K5RlH7AuOFtVY89kGs5IbM1+wIYm9iFEM u1e7fHUpw==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lRDHG-003fRn-KL; Tue, 30 Mar 2021 12:15:22 +0000 Received: from mail.kernel.org ([198.145.29.99]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lRDH9-003fQx-H0 for linux-arm-kernel@lists.infradead.org; Tue, 30 Mar 2021 12:15:19 +0000 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id F03C761983; Tue, 30 Mar 2021 12:15:13 +0000 (UTC) Received: from 78.163-31-62.static.virginmediabusiness.co.uk ([62.31.163.78] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94) (envelope-from ) id 1lRDH5-004fnl-RM; Tue, 30 Mar 2021 13:15:12 +0100 Date: Tue, 30 Mar 2021 13:15:11 +0100 Message-ID: <87mtukrg6o.wl-maz@kernel.org> From: Marc Zyngier To: Suzuki K Poulose Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, coresight@lists.linaro.org, mathieu.poirier@linaro.org, mike.leach@linaro.org, leo.yan@linaro.org, anshuman.khandual@arm.com, catalin.marinas@arm.com, Will Deacon , Mark Rutland , Alexandru Elisei Subject: Re: [PATCH v5 07/19] arm64: kvm: Enable access to TRBE support for host In-Reply-To: References: <20210323120647.454211-1-suzuki.poulose@arm.com> <20210323120647.454211-8-suzuki.poulose@arm.com> <87r1jxq7ax.wl-maz@kernel.org> 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/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 62.31.163.78 X-SA-Exim-Rcpt-To: suzuki.poulose@arm.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, coresight@lists.linaro.org, mathieu.poirier@linaro.org, mike.leach@linaro.org, leo.yan@linaro.org, anshuman.khandual@arm.com, catalin.marinas@arm.com, will@kernel.org, mark.rutland@arm.com, Alexandru.Elisei@arm.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-20210330_131517_638033_59411B1B X-CRM114-Status: GOOD ( 54.17 ) 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 Tue, 30 Mar 2021 12:12:49 +0100, Suzuki K Poulose wrote: > > Hi Marc > > On 30/03/2021 11:12, Marc Zyngier wrote: > > Hi Suzuki, > > > > [+ Alex] > > > > On Tue, 23 Mar 2021 12:06:35 +0000, > > Suzuki K Poulose wrote: > >> > >> For a nvhe host, the EL2 must allow the EL1&0 translation > >> regime for TraceBuffer (MDCR_EL2.E2TB == 0b11). This must > >> be saved/restored over a trip to the guest. Also, before > >> entering the guest, we must flush any trace data if the > >> TRBE was enabled. And we must prohibit the generation > >> of trace while we are in EL1 by clearing the TRFCR_EL1. > >> > >> For vhe, the EL2 must prevent the EL1 access to the Trace > >> Buffer. > >> > >> Cc: Will Deacon > >> Cc: Catalin Marinas > >> Cc: Marc Zyngier > >> Cc: Mark Rutland > >> Cc: Anshuman Khandual > >> Acked-by: Mathieu Poirier > >> Signed-off-by: Suzuki K Poulose > >> --- > >> arch/arm64/include/asm/el2_setup.h | 13 +++++++++ > >> arch/arm64/include/asm/kvm_arm.h | 2 ++ > >> arch/arm64/include/asm/kvm_host.h | 2 ++ > >> arch/arm64/kernel/hyp-stub.S | 3 ++- > >> arch/arm64/kvm/debug.c | 6 ++--- > >> arch/arm64/kvm/hyp/nvhe/debug-sr.c | 42 ++++++++++++++++++++++++++++++ > >> arch/arm64/kvm/hyp/nvhe/switch.c | 1 + > >> 7 files changed, 65 insertions(+), 4 deletions(-) > >> > >> diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h > >> index d77d358f9395..bda918948471 100644 > >> --- a/arch/arm64/include/asm/el2_setup.h > >> +++ b/arch/arm64/include/asm/el2_setup.h > >> @@ -65,6 +65,19 @@ > >> // use EL1&0 translation. > >> .Lskip_spe_\@: > >> + /* Trace buffer */ > >> + ubfx x0, x1, #ID_AA64DFR0_TRBE_SHIFT, #4 > >> + cbz x0, .Lskip_trace_\@ // Skip if TraceBuffer is not present > >> + > >> + mrs_s x0, SYS_TRBIDR_EL1 > >> + and x0, x0, TRBIDR_PROG > >> + cbnz x0, .Lskip_trace_\@ // If TRBE is available at EL2 > >> + > >> + mov x0, #(MDCR_EL2_E2TB_MASK << MDCR_EL2_E2TB_SHIFT) > >> + orr x2, x2, x0 // allow the EL1&0 translation > >> + // to own it. > >> + > >> +.Lskip_trace_\@: > >> msr mdcr_el2, x2 // Configure debug traps > >> .endm > >> diff --git a/arch/arm64/include/asm/kvm_arm.h > >> b/arch/arm64/include/asm/kvm_arm.h > >> index 94d4025acc0b..692c9049befa 100644 > >> --- a/arch/arm64/include/asm/kvm_arm.h > >> +++ b/arch/arm64/include/asm/kvm_arm.h > >> @@ -278,6 +278,8 @@ > >> #define CPTR_EL2_DEFAULT CPTR_EL2_RES1 > >> /* Hyp Debug Configuration Register bits */ > >> +#define MDCR_EL2_E2TB_MASK (UL(0x3)) > >> +#define MDCR_EL2_E2TB_SHIFT (UL(24)) > > > > Where are these bits defined? DDI0487G_a has them as RES0. > > They are part of the Future architecture technology and a register > definition XML is available here : > > https://developer.arm.com/documentation/ddi0601/2020-12/AArch64-Registers/MDCR-EL2--Monitor-Debug-Configuration-Register--EL2-?lang=en#fieldset_0-25_24-1 > It be worth adding a pointer to that documentation until this is part of a released ARM ARM. > > > >> #define MDCR_EL2_TTRF (1 << 19) > >> #define MDCR_EL2_TPMS (1 << 14) > >> #define MDCR_EL2_E2PB_MASK (UL(0x3)) > >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > >> index 3d10e6527f7d..80d0a1a82a4c 100644 > >> --- a/arch/arm64/include/asm/kvm_host.h > >> +++ b/arch/arm64/include/asm/kvm_host.h > >> @@ -315,6 +315,8 @@ struct kvm_vcpu_arch { > >> struct kvm_guest_debug_arch regs; > >> /* Statistical profiling extension */ > >> u64 pmscr_el1; > >> + /* Self-hosted trace */ > >> + u64 trfcr_el1; > >> } host_debug_state; > >> /* VGIC state */ > >> diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S > >> index 5eccbd62fec8..05d25e645b46 100644 > >> --- a/arch/arm64/kernel/hyp-stub.S > >> +++ b/arch/arm64/kernel/hyp-stub.S > >> @@ -115,9 +115,10 @@ SYM_CODE_START_LOCAL(mutate_to_vhe) > >> mrs_s x0, SYS_VBAR_EL12 > >> msr vbar_el1, x0 > >> - // Use EL2 translations for SPE and disable access from EL1 > >> + // Use EL2 translations for SPE & TRBE and disable access from EL1 > >> mrs x0, mdcr_el2 > >> bic x0, x0, #(MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT) > >> + bic x0, x0, #(MDCR_EL2_E2TB_MASK << MDCR_EL2_E2TB_SHIFT) > >> msr mdcr_el2, x0 > >> // Transfer the MM state from EL1 to EL2 > >> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > >> index dbc890511631..7b16f42d39f4 100644 > >> --- a/arch/arm64/kvm/debug.c > >> +++ b/arch/arm64/kvm/debug.c > >> @@ -89,7 +89,7 @@ void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) > >> * - Debug ROM Address (MDCR_EL2_TDRA) > >> * - OS related registers (MDCR_EL2_TDOSA) > >> * - Statistical profiler (MDCR_EL2_TPMS/MDCR_EL2_E2PB) > >> - * - Self-hosted Trace Filter controls (MDCR_EL2_TTRF) > >> + * - Self-hosted Trace (MDCR_EL2_TTRF/MDCR_EL2_E2TB) > > > > For the record, this is likely to conflict with [1], although that > > patch still has some issues. > > Thanks for the heads up. I think that patch will also conflict with my > fixes that is queued in kvmarm/fixes. Most probably. This is a popular landing spot, these days... > > > > > >> * > >> * Additionally, KVM only traps guest accesses to the debug registers if > >> * the guest is not actively using them (see the KVM_ARM64_DEBUG_DIRTY > >> @@ -107,8 +107,8 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) > >> trace_kvm_arm_setup_debug(vcpu, vcpu->guest_debug); > >> /* > >> - * This also clears MDCR_EL2_E2PB_MASK to disable guest access > >> - * to the profiling buffer. > >> + * This also clears MDCR_EL2_E2PB_MASK and MDCR_EL2_E2TB_MASK > >> + * to disable guest access to the profiling and trace buffers > >> */ > >> vcpu->arch.mdcr_el2 = __this_cpu_read(mdcr_el2) & MDCR_EL2_HPMN_MASK; > >> vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM | > >> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c > >> index f401724f12ef..9499e18dd28f 100644 > >> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c > >> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c > >> @@ -58,10 +58,51 @@ static void __debug_restore_spe(u64 pmscr_el1) > >> write_sysreg_s(pmscr_el1, SYS_PMSCR_EL1); > >> } > >> +static void __debug_save_trace(u64 *trfcr_el1) > >> +{ > >> + > > > > Spurious blank line? > > > > Sure, will fix it > > >> + *trfcr_el1 = 0; > >> + > >> + /* Check if we have TRBE */ > >> + if (!cpuid_feature_extract_unsigned_field(read_sysreg(id_aa64dfr0_el1), > >> + ID_AA64DFR0_TRBE_SHIFT)) > >> + return; > > > > Do we have a way to track this that doesn't involve reading an ID > > register? This is on the hot path, and is going to really suck badly > > with NV (which traps all ID regs for obvious reasons). I would have > > hoped that one way or another, we'd have a static key for this. > > TRBE, like SPE can be optionally enabled on a subset of the CPUs. We > could have a per-CPU static key in the worst case. I guess this would > apply to SPE as well. Ah, so you want to support asymmetric tracing... fair enough. But I don't think you need a per-CPU static key (and I'm not sure how that'd work either). You could have a static key indicating if *any* CPU implements tracing, in which case the check only happens when at least one CPU is capable of tracing. You would only need a new capability. > May be we could do this check at kvm_arch_vcpu_load()/put() ? That would extend the tracing blackout period enormously, wouldn't it? I'm not sure that's the best thing to do... > > > >> + > >> + /* Check we can access the TRBE */ > >> + if ((read_sysreg_s(SYS_TRBIDR_EL1) & TRBIDR_PROG)) > >> + return; > >> + > >> + /* Check if the TRBE is enabled */ > >> + if (!(read_sysreg_s(SYS_TRBLIMITR_EL1) & TRBLIMITR_ENABLE)) > >> + return; > >> + /* > >> + * Prohibit trace generation while we are in guest. > >> + * Since access to TRFCR_EL1 is trapped, the guest can't > >> + * modify the filtering set by the host. > > > > If TRFCR_EL1 is trapped, where is the trap handling? This series > > doesn't touch sys_regs.c, so I assume you rely on the "inject an UNDEF > > for anything unknown" default behaviour? > > Yes. > > > > > If that's the case, I'd rather you add an explicit handler. > > > > I could add one. 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