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.3 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 967BAC433E0 for ; Tue, 26 Jan 2021 14:38:17 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 5A50C2251F for ; Tue, 26 Jan 2021 14:38:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5A50C2251F 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=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Message-ID:References:In-Reply-To:Subject:To:From: Date:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=LFpYLJyygDs0TkRemGfsi1yt7b0gS52Aw8FW5Z+CK/M=; b=GKPG+vbgdrmMGf9bFgosJjChG Ow8CrGQABukeexzyRsom5G0Gpeqhv2NWWU0q2wQ4xH4wkixd+0g/K0iDNP7Jc9sSmWmm44u+eMRUM TJDcQPtUoI7nd3cK5/b4SKDVfjVSptKkKfqVxWAiycccR3IQD5J+Bb+rAWdKNIwL0XWPvTlFQy5Sd IBH5T/9Q75jRxmhY3C1NCwDa2I3CYqT6g/a9jgiZCLlY8o4BLlrgpY790baEv98tjsnP4nFxdg0ww M7YIUhanMfdVuTrIfJrvUs4Uh2X1FPr1ov40JL01trg6IoNWbxVTabspqQ2z/mNtfKJlvalsL9xir kfLGZsaZQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l4PSJ-0003wx-ND; Tue, 26 Jan 2021 14:36:31 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1l4PSG-0003wS-Ov for linux-arm-kernel@lists.infradead.org; Tue, 26 Jan 2021 14:36:30 +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 9B5F222D58; Tue, 26 Jan 2021 14:36:27 +0000 (UTC) Received: from disco-boy.misterjones.org ([51.254.78.96] helo=www.loen.fr) by disco-boy.misterjones.org with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94) (envelope-from ) id 1l4PSD-00A9KQ-Hs; Tue, 26 Jan 2021 14:36:25 +0000 MIME-Version: 1.0 Date: Tue, 26 Jan 2021 14:36:25 +0000 From: Marc Zyngier To: Andre Przywara Subject: Re: [RFC PATCH] KVM: arm64: Avoid unconditional PMU register access In-Reply-To: <20210126140653.051ab5b8@slackpad.fritz.box> References: <20210118173054.188160-1-andre.przywara@arm.com> <20210126140653.051ab5b8@slackpad.fritz.box> User-Agent: Roundcube Webmail/1.4.10 Message-ID: <0299261d1ecb13da8ca86dcdc50854ad@kernel.org> X-Sender: maz@kernel.org X-SA-Exim-Connect-IP: 51.254.78.96 X-SA-Exim-Rcpt-To: andre.przywara@arm.com, catalin.marinas@arm.com, will@kernel.org, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.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-20210126_093628_964743_1200DB6D X-CRM114-Status: GOOD ( 39.51 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Catalin Marinas , Will Deacon , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 2021-01-26 14:06, Andre Przywara wrote: > On Mon, 25 Jan 2021 18:40:40 +0000 > Marc Zyngier wrote: > > Hi Marc, > > thanks for having a look! > >> On 2021-01-18 17:30, Andre Przywara wrote: >> > The ARM PMU is an optional CPU feature, so we should consult the CPUID >> > registers before accessing any PMU related registers. >> > However the KVM code accesses some PMU registers (PMUSERENR_EL0 and >> > PMSEL_EL0) unconditionally, when activating the traps. >> > This wasn't a problem so far, because every(?) silicon implements the >> > PMU, with KVM guests being the lone exception, and those never ran >> > KVM host code. >> > >> > As this is about to change with nested virt, we need to guard PMU >> > register accesses with a proper CPU feature check. >> > >> > Add a new CPU capability, which marks whether we have at least the >> > basic >> > PMUv3 registers available. Use that in the KVM VHE code to check before >> > accessing the PMU registers. >> > >> > Signed-off-by: Andre Przywara >> > --- >> > Hi, >> > >> > not sure a new CPU capability isn't a bit over the top here, and we >> > should >> > use a simple static key instead? >> >> Yes, I think this is a bit excessive, specially as we already have >> a predicate for the HW having a PMU (or the PMU being able on the >> host, which amounts to the same thing). I'm definitely not keen >> on adding more another one that has slightly different semantics. >> >> How about this instead, completely untested? > > Thanks, I was hoping for something like this, just didn't have any clue > where in the tree to put the pieces into. > > I gave that a spin, and that fixes the issue as well. One comments > below: > >> diff --git a/arch/arm64/kernel/image-vars.h >> b/arch/arm64/kernel/image-vars.h >> index 23f1a557bd9f..5aa9ed1e9ec6 100644 >> --- a/arch/arm64/kernel/image-vars.h >> +++ b/arch/arm64/kernel/image-vars.h >> @@ -101,6 +101,9 @@ KVM_NVHE_ALIAS(__stop___kvm_ex_table); >> /* Array containing bases of nVHE per-CPU memory regions. */ >> KVM_NVHE_ALIAS(kvm_arm_hyp_percpu_base); >> >> +/* PMU available static key */ >> +KVM_NVHE_ALIAS(kvm_arm_pmu_available); >> + >> #endif /* CONFIG_KVM */ >> >> #endif /* __ARM64_KERNEL_IMAGE_VARS_H */ >> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h >> b/arch/arm64/kvm/hyp/include/hyp/switch.h >> index 54f4860cd87c..6c1f51f25eb3 100644 >> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h >> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h >> @@ -90,15 +90,18 @@ static inline void __activate_traps_common(struct >> kvm_vcpu *vcpu) >> * counter, which could make a PMXEVCNTR_EL0 access UNDEF at >> * EL1 instead of being trapped to EL2. >> */ >> - write_sysreg(0, pmselr_el0); >> - write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); >> + if (kvm_arm_support_pmu_v3()) { >> + write_sysreg(0, pmselr_el0); >> + write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); >> + } >> write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2); >> } >> >> static inline void __deactivate_traps_common(void) >> { >> write_sysreg(0, hstr_el2); >> - write_sysreg(0, pmuserenr_el0); >> + if (kvm_arm_support_pmu_v3()) >> + write_sysreg(0, pmuserenr_el0); >> } >> >> static inline void ___activate_traps(struct kvm_vcpu *vcpu) >> diff --git a/arch/arm64/kvm/perf.c b/arch/arm64/kvm/perf.c >> index d45b8b9a4415..198fa4266b2d 100644 >> --- a/arch/arm64/kvm/perf.c >> +++ b/arch/arm64/kvm/perf.c >> @@ -11,6 +11,8 @@ >> >> #include >> >> +DEFINE_STATIC_KEY_FALSE(kvm_arm_pmu_available); >> + >> static int kvm_is_in_guest(void) >> { >> return kvm_get_running_vcpu() != NULL; >> @@ -48,6 +50,14 @@ static struct perf_guest_info_callbacks >> kvm_guest_cbs >> = { >> >> int kvm_perf_init(void) >> { >> + /* >> + * Check if HW_PERF_EVENTS are supported by checking the number of >> + * hardware performance counters. This could ensure the presence of >> + * a physical PMU and CONFIG_PERF_EVENT is selected. >> + */ >> + if (perf_num_counters() > 0) >> + static_branch_enable(&kvm_arm_pmu_available); >> + >> return perf_register_guest_info_callbacks(&kvm_guest_cbs); >> } >> >> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c >> index 247422ac78a9..8d5ff7f3d416 100644 >> --- a/arch/arm64/kvm/pmu-emul.c >> +++ b/arch/arm64/kvm/pmu-emul.c >> @@ -817,16 +817,6 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, >> bool >> pmceid1) >> return val & mask; >> } >> >> -bool kvm_arm_support_pmu_v3(void) >> -{ >> - /* >> - * Check if HW_PERF_EVENTS are supported by checking the number of >> - * hardware performance counters. This could ensure the presence of >> - * a physical PMU and CONFIG_PERF_EVENT is selected. >> - */ >> - return (perf_num_counters() > 0); >> -} >> - >> int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu) >> { >> if (!kvm_vcpu_has_pmu(vcpu)) >> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h >> index 8dcb3e1477bc..45631af820cd 100644 >> --- a/include/kvm/arm_pmu.h >> +++ b/include/kvm/arm_pmu.h >> @@ -13,6 +13,13 @@ >> #define ARMV8_PMU_CYCLE_IDX (ARMV8_PMU_MAX_COUNTERS - 1) >> #define ARMV8_PMU_MAX_COUNTER_PAIRS ((ARMV8_PMU_MAX_COUNTERS + 1) >> >> 1) >> >> +DECLARE_STATIC_KEY_FALSE(kvm_arm_pmu_available); >> + >> +static __always_inline bool kvm_arm_support_pmu_v3(void) >> +{ >> + return static_branch_likely(&kvm_arm_pmu_available); >> +} >> + > > Doesn't that definition belong into the part below, guarded by > CONFIG_HW_PERF_EVENTS? Otherwise disabling CONFIG_ARM_PMU will break > compilation, as kvm_arm_support_pmu_v3() is defined twice. Yes, I fixed that already (after the kbuild robot shouted at me...). > And speaking of which, the actual code in question here, above in > hyp/switch.h, sounds like taking care of an architectural trait, that > is independent from the host using the performance counters? So it > should be executed regardless of the host kernel supporting Linux' > ARM_PMU feature? At the moment disabling ARM_PMU skips this system > register setup, is that intended? > Or do I understand the comment in __activate_traps_common() wrongly, > and > this is only an issue if the host kernel might be using performance > counters itself? There are multiple things at play here: - If there is no HW PMU, we shouldn't touch these registers. This is what this patch fixes - If there is a HW PMU, but that the host kernel doesn't have any PMU support compiled in, it is the same as not having a PMU at all as far as the guest is concerned (because we handle the PMU as an in-kernel perf client). The PMU accesses will trap, and we will inject UNDEF exceptions. The actual state of the HW PMU is pretty much irrelevant in this case. - If there is a PMU handled by the host, but that the guest isn't configured for one, that's where things become a bit muddy. We really should check for the PMU being enabled for the guest, but that's a couple of pointers away, probably slower than just writing two sysregs without synchronisation. So we just do that. - If there is a PMU in both host and guest, then these writes are really mandatory (see 21cbe3cc8a for the gory details of the PMSELR_EL0 handling). Anyway, I'll write some commit messages and post the patches. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel