From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f42.google.com (mail-wm1-f42.google.com [209.85.128.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A369C199FBF for ; Mon, 23 Dec 2024 11:36:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.42 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734953807; cv=none; b=K4I1ImUwvZ9SExxNPmxBByNubEqOTtB2BgJGTSf0ARjognLOLJubrL8cRTdMN22b6OPCJFbZkhH/g8SvbvYTH1UrOvsQO5AA/oYUtigptvdeBrFyPi1tmvkB0u1vJKu+jsqg2PUQ5wrKIMch96fIaFJek47LpaET7uMtIVYLrOs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734953807; c=relaxed/simple; bh=P5x4oAHeG7HJEd/0t9TBmtHI+gPp2r6ifa994LIjj8E=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=C7UCfzuYWvpv0oTafLJCd30WpCrniLpXyvZOaIdeTFRcJdna3BZvmQBqgmMXe2kyl8tyuF4JGCKoUvmWqJNUNKarTjPdEy0Fgh1dHrXJgWn/cUmd+utjXUG3Ggu0kv6ZtlLLQ+KFdWl53Hs1UB9yXAkeJDyy5rM9FUQwjkoegBk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=byszp0fk; arc=none smtp.client-ip=209.85.128.42 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="byszp0fk" Received: by mail-wm1-f42.google.com with SMTP id 5b1f17b1804b1-43622354a3eso27784115e9.1 for ; Mon, 23 Dec 2024 03:36:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1734953804; x=1735558604; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=pfAEnN47k2yUPyXkehPRQB5Var7XNNy5tr8DW7zJULQ=; b=byszp0fkImHSeizSZwSsnovvIuekVDUL2ArVfCCvyi3yaMI1Z7cIem3SviGb0GqFe/ zRbznm7cFiphwbZO5JPpYkzdbD/jfXWJbuiv+/cmwUagl1WdG3kF60WSbmrtDeS/Fh3t chxP45y+se0G5m81j8vWAMV5Zfw2fCQnWELSjm/fbTt+2TXqS2+MagAXtPK5xic4C2JB 86OtX9tY9+KOTxTecz2kBU80aeFmPh0KWDRmvJLFEwcLcwR5DS8ljVwFZJjOhBBaBV/T +if3Ze6BXCJCagpyr7+h7Jg96km1kvURGbgxIrUnDzu6qxKwD37LhlHXGU3PBmDS7wP6 BcTg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1734953804; x=1735558604; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=pfAEnN47k2yUPyXkehPRQB5Var7XNNy5tr8DW7zJULQ=; b=aZSpTJDUkDEvnk3/Lh3/7ss5oCJ8L5c5bG4a+BDUSPgQyZqIFFXuaKxTLUnRRydFbt NbELe4LqVhTlw8An82vh4Q6VEO1PvvgdzO3x0AR4X5gV3LoTidA9Uaw/lepaA2u1FHjV JBSoWMKB52+IRWuiAMVOK7aGrRYNG0LtC2t12xeMKMS3yx08y6imQhen5D8rXFDGANxu iGbgokpXp0QRSHRgU0ruBO9tgjZzi6O+dsZ53cPKUNMfa96fAS2Zg/84L7etujX8N5u0 IhMkYHmiyyFDuA6yvG48UMi02YLuCxZhtwl8EPYwA4mwaj8mnFiiIJUexnhpExuwVwiR HnTw== X-Forwarded-Encrypted: i=1; AJvYcCXRVCRoHJz+PRRa25xytTLGAV1/xGY6uf7x+ilm8WITW0UjizKPgOV+pVdyylYp0pWICf0rhw6COquBs5M=@vger.kernel.org X-Gm-Message-State: AOJu0Ywc1tySlNqw5Ul2fZ/O8Ucprkd6Z/HCVt5d1dY/QbnGUWUaZ1zX esYA/QXi7FLHR6twhq1RXp/OBpH77POi1xW/JYHMX2Ya7wy3K5ILHr+85fnJ3R0= X-Gm-Gg: ASbGncvbss1liHlc6YUkIFvfxIN++nGfcwE7VLfM9HbwBfZYqOLbW1d+qKlitd0oSz2 aBJ+eBaVVHtpV0CNI914TTDYNnCRxsxYXimsI93LQgpsgM4ZxexfWtTFX0ZsaIQvm6ZsTU1dfPS CVBEtLEXmJIPkZjOUSolfdQWZtwre042omNJHpQFm44BVZsAeczBrroDA3a4hjFr6p3Z2drG24Y GTs3lf4z2N8HA+wj2SMgTPTStGDE5eP0+/6S9UTghIQ074H9LLZIYdQenDFOBoyxw== X-Google-Smtp-Source: AGHT+IH1xux9sPKNWQ/qyA/QpYj6eYBI9DgnVKcK7PP07rrtLIZQhCaDtbBqQzfpAVwh4XJhK9q1vw== X-Received: by 2002:a5d:5f4a:0:b0:385:f19f:5a8f with SMTP id ffacd0b85a97d-38a221f2d90mr10269118f8f.4.1734953803802; Mon, 23 Dec 2024 03:36:43 -0800 (PST) Received: from [192.168.68.163] ([145.224.66.70]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-38a1c8ace0esm10999383f8f.106.2024.12.23.03.36.42 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 23 Dec 2024 03:36:43 -0800 (PST) Message-ID: Date: Mon, 23 Dec 2024 11:36:41 +0000 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v8 6/8] KVM: arm64: coresight: Give TRBE enabled state to KVM To: Marc Zyngier Cc: kvmarm@lists.linux.dev, oliver.upton@linux.dev, suzuki.poulose@arm.com, coresight@lists.linaro.org, Joey Gouly , Zenghui Yu , Catalin Marinas , Will Deacon , Mike Leach , Alexander Shishkin , Mark Brown , Anshuman Khandual , "Rob Herring (Arm)" , Shiqi Liu , Fuad Tabba , James Morse , Raghavendra Rao Ananta , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org References: <20241127100130.1162639-1-james.clark@linaro.org> <20241127100130.1162639-7-james.clark@linaro.org> <86ttayp9n9.wl-maz@kernel.org> <86seqhp7yg.wl-maz@kernel.org> Content-Language: en-US From: James Clark In-Reply-To: <86seqhp7yg.wl-maz@kernel.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 21/12/2024 11:54 am, Marc Zyngier wrote: > On Fri, 20 Dec 2024 17:32:17 +0000, > James Clark wrote: >> >> >> >> On 20/12/2024 5:05 pm, Marc Zyngier wrote: >>> On Wed, 27 Nov 2024 10:01:23 +0000, >>> James Clark wrote: >>>> >>>> Currently in nVHE, KVM has to check if TRBE is enabled on every guest >>>> switch even if it was never used. Because it's a debug feature and is >>>> more likely to not be used than used, give KVM the TRBE buffer status to >>>> allow a much simpler and faster do-nothing path in the hyp. >>>> >>>> This is always called with preemption disabled except for probe/hotplug >>>> which gets wrapped with preempt_disable(). >>>> >>>> Protected mode disables trace regardless of TRBE (because >>>> guest_trfcr_el1 is always 0), which was not previously done. HAS_TRBE >>>> becomes redundant, but HAS_TRF is now required for this. >>>> >>>> Signed-off-by: James Clark >>>> --- >>>> arch/arm64/include/asm/kvm_host.h | 10 +++- >>>> arch/arm64/kvm/debug.c | 25 ++++++++-- >>>> arch/arm64/kvm/hyp/nvhe/debug-sr.c | 51 +++++++++++--------- >>>> drivers/hwtracing/coresight/coresight-trbe.c | 5 ++ >>>> 4 files changed, 65 insertions(+), 26 deletions(-) >>>> >>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>>> index 7e3478386351..ba251caa593b 100644 >>>> --- a/arch/arm64/include/asm/kvm_host.h >>>> +++ b/arch/arm64/include/asm/kvm_host.h >>>> @@ -611,7 +611,8 @@ struct cpu_sve_state { >>>> */ >>>> struct kvm_host_data { >>>> #define KVM_HOST_DATA_FLAG_HAS_SPE 0 >>>> -#define KVM_HOST_DATA_FLAG_HAS_TRBE 1 >>>> +#define KVM_HOST_DATA_FLAG_HAS_TRF 1 >>>> +#define KVM_HOST_DATA_FLAG_TRBE_ENABLED 2 >>>> unsigned long flags; >>>> struct kvm_cpu_context host_ctxt; >>>> @@ -657,6 +658,9 @@ struct kvm_host_data { >>>> u64 mdcr_el2; >>>> } host_debug_state; >>>> + /* Guest trace filter value */ >>>> + u64 guest_trfcr_el1; >>> >>> Guest value? Or host state while running the guest? If the former, >>> then this has nothing to do here. If the latter, this should be >>> spelled out (trfcr_in_guest?), and the comment amended. >>> >>>> + >>>> /* Number of programmable event counters (PMCR_EL0.N) for this CPU */ >>>> unsigned int nr_event_counters; >>>> }; >>>> @@ -1381,6 +1385,8 @@ static inline bool kvm_pmu_counter_deferred(struct perf_event_attr *attr) >>>> void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr); >>>> void kvm_clr_pmu_events(u64 clr); >>>> bool kvm_set_pmuserenr(u64 val); >>>> +void kvm_enable_trbe(void); >>>> +void kvm_disable_trbe(void); >>>> #else >>>> static inline void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr) {} >>>> static inline void kvm_clr_pmu_events(u64 clr) {} >>>> @@ -1388,6 +1394,8 @@ static inline bool kvm_set_pmuserenr(u64 val) >>>> { >>>> return false; >>>> } >>>> +static inline void kvm_enable_trbe(void) {} >>>> +static inline void kvm_disable_trbe(void) {} >>>> #endif >>>> void kvm_vcpu_load_vhe(struct kvm_vcpu *vcpu); >>>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c >>>> index dd9e139dfd13..0c340ae7b5d1 100644 >>>> --- a/arch/arm64/kvm/debug.c >>>> +++ b/arch/arm64/kvm/debug.c >>>> @@ -314,7 +314,26 @@ void kvm_init_host_debug_data(void) >>>> !(read_sysreg_s(SYS_PMBIDR_EL1) & PMBIDR_EL1_P)) >>>> host_data_set_flag(HAS_SPE); >>>> - if (cpuid_feature_extract_unsigned_field(dfr0, >>>> ID_AA64DFR0_EL1_TraceBuffer_SHIFT) && >>>> - !(read_sysreg_s(SYS_TRBIDR_EL1) & TRBIDR_EL1_P)) >>>> - host_data_set_flag(HAS_TRBE); >>>> + if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_TraceFilt_SHIFT)) >>>> + host_data_set_flag(HAS_TRF); >>>> } >>>> + >>>> +void kvm_enable_trbe(void) >>>> +{ >>>> + if (has_vhe() || is_protected_kvm_enabled() || >>>> + WARN_ON_ONCE(preemptible())) >>>> + return; >>>> + >>>> + host_data_set_flag(TRBE_ENABLED); >>>> +} >>>> +EXPORT_SYMBOL_GPL(kvm_enable_trbe); >>>> + >>>> +void kvm_disable_trbe(void) >>>> +{ >>>> + if (has_vhe() || is_protected_kvm_enabled() || >>>> + WARN_ON_ONCE(preemptible())) >>>> + return; >>>> + >>>> + host_data_clear_flag(TRBE_ENABLED); >>>> +} >>>> +EXPORT_SYMBOL_GPL(kvm_disable_trbe); >>>> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c >>>> index 858bb38e273f..9479bee41801 100644 >>>> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c >>>> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c >>>> @@ -51,32 +51,39 @@ static void __debug_restore_spe(u64 pmscr_el1) >>>> write_sysreg_el1(pmscr_el1, SYS_PMSCR); >>>> } >>>> -static void __debug_save_trace(u64 *trfcr_el1) >>>> +static void __trace_do_switch(u64 *saved_trfcr, u64 new_trfcr) >>>> { >>>> - *trfcr_el1 = 0; >>>> + *saved_trfcr = read_sysreg_el1(SYS_TRFCR); >>>> + write_sysreg_el1(new_trfcr, SYS_TRFCR); >>>> - /* Check if the TRBE is enabled */ >>>> - if (!(read_sysreg_s(SYS_TRBLIMITR_EL1) & TRBLIMITR_EL1_E)) >>>> + /* No need to drain if going to an enabled state or from disabled state */ >>>> + if (new_trfcr || !*saved_trfcr) >>> >>> What if TRFCR_EL1.TS is set to something non-zero? I'd rather you >>> check for the E*TRE bits instead of assuming things. >>> >> >> Yeah it's probably better that way. TS is actually always set when any >> tracing session starts and then never cleared, so doing it the simpler >> way made it always flush even after tracing finished, which probably >> wasn't great. > > Quite. Can you please *test* these things? > > [...] > Sorry to confuse things I wasn't 100% accurate here, yes it's tested and working. It works because of the split set/clear_trfcr() API. The Coresight driver specifically calls clear at the end of the session rather than a set of 0. That signals this function not to be called so there's no excessive swapping. Secondly, the buffer flushing case is triggered by TRBE_ENABLED, which forces TRFCR to 0, so "if (new_trfcr)" is an OK way to gate the flush. >>>> @@ -253,8 +256,10 @@ static void trbe_drain_and_disable_local(struct trbe_cpudata *cpudata) >>>> static void trbe_reset_local(struct trbe_cpudata *cpudata) >>>> { >>>> + preempt_disable(); >>>> trbe_drain_and_disable_local(cpudata); >>>> write_sysreg_s(0, SYS_TRBLIMITR_EL1); >>>> + preempt_enable(); >>> >>> This looks terribly wrong. If you need to disable preemption here, why >>> doesn't the critical section cover all register accesses? Surely you >>> don't want to nuke another CPU's context? >>> >>> But looking at the calling sites, this makes even less sense. The two >>> callers of this thing mess with *per-CPU* interrupts. Dealing with >>> per-CPU interrupts in preemptible context is a big no-no (hint: they >>> start with a call to smp_processor_id()). >>> >>> So what is this supposed to ensure? >>> >>> M. >>> >> >> These ones are only intended to silence the >> WARN_ON_ONCE(preemptible()) in kvm_enable_trbe() when this is called >> from boot/hotplug (arm_trbe_enable_cpu()). Preemption isn't disabled, >> but a guest can't run at that point either. >> >> The "real" calls to kvm_enable_trbe() _are_ called from an atomic >> context. I think there was a previous review comment about when it was >> safe to call the KVM parts of this change, which is why I added the >> warning making sure it was always called with preemption disabled. But >> actually I could remove the warning and these preempt_disables() and >> replace them with a comment. > > You should keep the WARN_ON(), and either *never* end-up calling this > stuff during a CPUHP event, or handle the fact that preemption isn't > initialised yet. For example by checking whether the current CPU is > online. > > But this sort of random spreading of preemption disabling is not an > acceptable outcome. > > M. > I'll look into this again. This was my initial attempt but couldn't find any easily accessible state that allowed to to be done this way. Maybe I missed something, but the obvious cpu_online() etc were already true at this point. Thanks James