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 B6847C77B6E for ; Wed, 12 Apr 2023 05:15:42 +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:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=TWIKpLPplsUjp4ValsshD/Wksx8n51XLdvJPXZQDea4=; b=3fQxfTVQ8bz70c /KEdjJi7OkxLjGY4xvv+vBjkfHSbjLxmxIdsnl9g2NCP5lLNp/Lxcsm7c0awNfDUaubO7+Dn1raqO m16q7yBfh2o5sT/tjUP2Jm90qVvnrmCYueHNfhtiFOvbQD3Bjxq72WxcekXEyfvwCvE9Bz2jDjvSH apF+Hrr5G3FhVyKSQTLoLhx18ZZwCc+hXYwNYvMr48iJGoGDsSbJtBCIAfRYKAXO/sphKz2H+2Fw0 h8pFulVFysBShdFMUIJmCgegMPYvlYBiPVNQPq4YDb0vaKRxoWgN5ciYWgy883NUcewtmNa8rLFGA 96fNWEmuOZ/m/GLmYdaA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pmSoR-001oyp-3B; Wed, 12 Apr 2023 05:14:31 +0000 Received: from mail-pl1-x635.google.com ([2607:f8b0:4864:20::635]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pmSoP-001oxA-0G for linux-arm-kernel@lists.infradead.org; Wed, 12 Apr 2023 05:14:31 +0000 Received: by mail-pl1-x635.google.com with SMTP id d9443c01a7336-1a652700c36so92575ad.0 for ; Tue, 11 Apr 2023 22:14:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1681276455; x=1683868455; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=/EHKnc9bkAkiSZQscMfllEWVOu0svCFLlXoed3Jbc5I=; b=MUpdUBHIyYkZKEC2wLsztPIxWvgaCjCYi4KUm0hRqW3G6QdZnHu9vxDZ5rC7gRuCtE 984JnshQ7RLMq4xISrahByP4oKEBhRZx59pk2KOD/lCVLY9cWqo2QSOZ7cusVW2UsT9E 2xUG8uye6MRuT3rwZoGIgyL9KBaslC1MSIAS/x+O2ligidYobmWasVqDmykKzTm4WKmh ZBq7dNMReiuY5L+IJjhuOzbKeXVD4HhKmlMFiS3gb3cajzzyFAXGukReFDYyB2F9s7Pn /HC9lsQ86vzuc/mLEYO8IRswaNXccYv+dZNc9I2AXYxbDXirP9ph0MIUcuMwQjkDauvF azjQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1681276455; x=1683868455; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=/EHKnc9bkAkiSZQscMfllEWVOu0svCFLlXoed3Jbc5I=; b=L5Rx7eVmI1Wc8td3jniovHGZr6qUDUgkicY3LKQOhgO/fXPJRKhQ3mPMj+AXnAIJj0 LZ7smJ2/qaPVq7FiGir1tnTtTCa0quN7vulfOUsq37Njdh/nP2hncbFQ7NWiFdRkMLNQ bGL2ha/2Shuh6ulLg1UIYFET9ePeOErp+k/mDSABl1OwWzGFPDOngfBGQ3rpFEiR1Jh/ s707Hpk3Q8YpZjzvBz4Z+xJXeDxJ9Hyxb8InZTHVG2RlnUH6e7Hzu4wUAIjiebS8nvr2 W+slutHrnkHY189HWPpfHzzEnWZ3VzFn1Uivz/c5ahtkAdA2PWBWY7bB0kFtO4J2bore t5TA== X-Gm-Message-State: AAQBX9erZZqH9jyPfIj4Re7qY2dkKPnqWZJLlfNzCks0372lYa22vds8 bhiLR6MRXgZ8m22G31bjS+wkGw== X-Google-Smtp-Source: AKy350btnbFhxfyQrF5h829VIoeiqZNniw5B7SyT6ffETVvFkjkNoWqA9b703VB2xav3VABt5DlHSA== X-Received: by 2002:a17:902:9793:b0:1a1:854b:99d3 with SMTP id q19-20020a170902979300b001a1854b99d3mr434904plp.19.1681276454667; Tue, 11 Apr 2023 22:14:14 -0700 (PDT) Received: from google.com (223.103.125.34.bc.googleusercontent.com. [34.125.103.223]) by smtp.gmail.com with ESMTPSA id i13-20020aa787cd000000b00636c4bd7c8bsm5385764pfo.43.2023.04.11.22.14.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 11 Apr 2023 22:14:14 -0700 (PDT) Date: Tue, 11 Apr 2023 22:14:10 -0700 From: Reiji Watanabe To: Mark Rutland Cc: Marc Zyngier , Oliver Upton , Will Deacon , Catalin Marinas , kvmarm@lists.linux.dev, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, James Morse , Alexandru Elisei , Zenghui Yu , Suzuki K Poulose , Paolo Bonzini , Ricardo Koller , Jing Zhang , Raghavendra Rao Anata , Shaoqin Huang , Rob Herring Subject: Re: [PATCH v2 2/2] KVM: arm64: PMU: Don't overwrite PMUSERENR with vcpu loaded Message-ID: <20230412051410.emaip77vyak624pu@google.com> References: <20230408034759.2369068-1-reijiw@google.com> <20230408034759.2369068-3-reijiw@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230411_221429_147861_7EFD0855 X-CRM114-Status: GOOD ( 51.99 ) 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, Apr 11, 2023 at 10:33:50AM +0100, Mark Rutland wrote: > On Fri, Apr 07, 2023 at 08:47:59PM -0700, Reiji Watanabe wrote: > > Currently, with VHE, KVM sets ER, CR, SW and EN bits of > > PMUSERENR_EL0 to 1 on vcpu_load(), and saves and restores > > the register value for the host on vcpu_load() and vcpu_put(). > > If the value of those bits are cleared on a pCPU with a vCPU > > loaded (armv8pmu_start() would do that when PMU counters are > > programmed for the guest), PMU access from the guest EL0 might > > be trapped to the guest EL1 directly regardless of the current > > PMUSERENR_EL0 value of the vCPU. > > > > Fix this by not letting armv8pmu_start() overwrite PMUSERENR on > > the pCPU on which a vCPU is loaded, and instead updating the > > saved shadow register value for the host, so that the value can > > be restored on vcpu_put() later. > > I'm happy with the hook in the PMU code, but I think there's still a race > between an IPI and vcpu_{load,put}() where we can lose an update to > PMUSERERNR_EL0. I tried to point that out in my final question in: > > https://lore.kernel.org/all/ZCwzV7ACl21VbLru@FVFF77S0Q05N.cambridge.arm.com/ > > ... but I looks like that wasn't all that clear. > > Consider vcpu_load(): > > void vcpu_load(struct kvm_vcpu *vcpu) > { > int cpu = get_cpu(); > > __this_cpu_write(kvm_running_vcpu, vcpu); > preempt_notifier_register(&vcpu->preempt_notifier); > kvm_arch_vcpu_load(vcpu, cpu); > put_cpu(); > } > > AFAICT that's called with IRQs enabled, and the {get,put}_cpu() calls will only > disable migration/preemption. After the write to kvm_running_vcpu, the code in > kvm_set_pmuserenr() will see that there is a running vcpu, and write to the > host context without updating the real PMUSERENR_EL0 register. > > If we take an IPI and call kvm_set_pmuserenr() after the write to > kvm_running_vcpu but before kvm_running_vcpu() completes, the call to > kvm_set_pmuserenr() could update the host context (without updating the real > PMUSERENR_EL0 value) before __activate_traps_common() saves the host value > with: > > ctxt_sys_reg(hctxt, PMUSERENR_EL0) = read_sysreg(pmuserenr_el0); > > ... which would discard the write made by kvm_set_pmuserenr(). > > Similar can happen in vcpu_put() where an IPI after __deactivate_traps_common() > but before kvm_running_vcpu is cleared would result in kvm_set_pmuserenr() > writing to the host context, but this value would never be written into HW. > > Unless I'm missing something (e.g. if interrupts are actually masked during > those windows), I don't think this is a complete fix as-is. > > I'm not sure if there is a smart fix for that. Thank you for the comment. Uh, right, interrupts are not masked during those windows... What I am currently considering on this would be disabling IRQs while manipulating the register, and introducing a new flag to indicate whether the PMUSERENR for the guest EL0 is loaded, and having kvm_set_pmuserenr() check the new flag. The code would be something like below (local_irq_save/local_irq_restore needs to be excluded for NVHE though). What do you think ? --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -668,6 +668,8 @@ struct kvm_vcpu_arch { /* Software step state is Active-pending */ #define DBG_SS_ACTIVE_PENDING __vcpu_single_flag(sflags, BIT(5)) +/* PMUSERENR for the guest EL0 is on physical CPU */ +#define PMUSERENR_ON_CPU __vcpu_single_flag(sflags, BIT(6)) /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */ #define vcpu_sve_pffr(vcpu) (kern_hyp_va((vcpu)->arch.sve_state) + \ diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h index 6718731729fd..57e4f480874a 100644 --- a/arch/arm64/kvm/hyp/include/hyp/switch.h +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h @@ -82,12 +82,19 @@ static inline void __activate_traps_common(struct kvm_vcpu *vcpu) */ if (kvm_arm_support_pmu_v3()) { struct kvm_cpu_context *hctxt; + unsigned long flags; write_sysreg(0, pmselr_el0); hctxt = &this_cpu_ptr(&kvm_host_data)->host_ctxt; + + local_irq_save(flags); + ctxt_sys_reg(hctxt, PMUSERENR_EL0) = read_sysreg(pmuserenr_el0); write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); + vcpu_set_flag(vcpu, PMUSERENR_ON_CPU); + + local_irq_restore(flags); } vcpu->arch.mdcr_el2_host = read_sysreg(mdcr_el2); @@ -112,9 +119,16 @@ static inline void __deactivate_traps_common(struct kvm_vcpu *vcpu) write_sysreg(0, hstr_el2); if (kvm_arm_support_pmu_v3()) { struct kvm_cpu_context *hctxt; + unsigned long flags; hctxt = &this_cpu_ptr(&kvm_host_data)->host_ctxt; + + local_irq_save(flags); + write_sysreg(ctxt_sys_reg(hctxt, PMUSERENR_EL0), pmuserenr_el0); + vcpu_clear_flag(vcpu, PMUSERENR_ON_CPU); + + local_irq_restore(flags); } if (cpus_have_final_cap(ARM64_SME)) { diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c index 40bb2cb13317..33cd8e1ecbd6 100644 --- a/arch/arm64/kvm/pmu.c +++ b/arch/arm64/kvm/pmu.c @@ -221,8 +221,13 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu) bool kvm_set_pmuserenr(u64 val) { struct kvm_cpu_context *hctxt; + struct kvm_vcpu *vcpu; - if (!kvm_arm_support_pmu_v3() || !has_vhe() || !kvm_get_running_vcpu()) + if (!kvm_arm_support_pmu_v3() || !has_vhe()) + return false; + + vcpu = kvm_get_running_vcpu(); + if (!vcpu || !vcpu_get_flag(vcpu, PMUSERENR_ON_CPU)) return false; hctxt = &this_cpu_ptr(&kvm_host_data)->host_ctxt; -- Thank you, Reiji > > Thanks, > Mark. > > > Suggested-by: Mark Rutland > > Suggested-by: Marc Zyngier > > Fixes: 83a7a4d643d3 ("arm64: perf: Enable PMU counter userspace access for perf event") > > Signed-off-by: Reiji Watanabe > > --- > > arch/arm64/include/asm/kvm_host.h | 5 +++++ > > arch/arm64/kernel/perf_event.c | 21 ++++++++++++++++++--- > > arch/arm64/kvm/pmu.c | 20 ++++++++++++++++++++ > > 3 files changed, 43 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index bcd774d74f34..22db2f885c17 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -1028,9 +1028,14 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu); > > #ifdef CONFIG_KVM > > void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr); > > void kvm_clr_pmu_events(u32 clr); > > +bool kvm_set_pmuserenr(u64 val); > > #else > > static inline void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr) {} > > static inline void kvm_clr_pmu_events(u32 clr) {} > > +static inline bool kvm_set_pmuserenr(u64 val) > > +{ > > + return false; > > +} > > #endif > > > > void kvm_vcpu_load_sysregs_vhe(struct kvm_vcpu *vcpu); > > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c > > index dde06c0f97f3..0fffe4c56c28 100644 > > --- a/arch/arm64/kernel/perf_event.c > > +++ b/arch/arm64/kernel/perf_event.c > > @@ -741,9 +741,25 @@ static inline u32 armv8pmu_getreset_flags(void) > > return value; > > } > > > > +static void update_pmuserenr(u64 val) > > +{ > > + lockdep_assert_irqs_disabled(); > > + > > + /* > > + * The current pmuserenr value might be the value for the guest. > > + * If that's the case, have KVM keep tracking of the register value > > + * for the host EL0 so that KVM can restore it before returning to > > + * the host EL0. Otherwise, update the register now. > > + */ > > + if (kvm_set_pmuserenr(val)) > > + return; > > + > > + write_sysreg(val, pmuserenr_el0); > > +} > > + > > static void armv8pmu_disable_user_access(void) > > { > > - write_sysreg(0, pmuserenr_el0); > > + update_pmuserenr(0); > > } > > > > static void armv8pmu_enable_user_access(struct arm_pmu *cpu_pmu) > > @@ -759,8 +775,7 @@ static void armv8pmu_enable_user_access(struct arm_pmu *cpu_pmu) > > armv8pmu_write_evcntr(i, 0); > > } > > > > - write_sysreg(0, pmuserenr_el0); > > - write_sysreg(ARMV8_PMU_USERENR_ER | ARMV8_PMU_USERENR_CR, pmuserenr_el0); > > + update_pmuserenr(ARMV8_PMU_USERENR_ER | ARMV8_PMU_USERENR_CR); > > } > > > > static void armv8pmu_enable_event(struct perf_event *event) > > diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c > > index 7887133d15f0..40bb2cb13317 100644 > > --- a/arch/arm64/kvm/pmu.c > > +++ b/arch/arm64/kvm/pmu.c > > @@ -209,3 +209,23 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu) > > kvm_vcpu_pmu_enable_el0(events_host); > > kvm_vcpu_pmu_disable_el0(events_guest); > > } > > + > > +/* > > + * With VHE, keep track of the PMUSERENR_EL0 value for the host EL0 on > > + * the pCPU where vCPU is loaded, since PMUSERENR_EL0 is switched to > > + * the value for the guest on vcpu_load(). The value for the host EL0 > > + * will be restored on vcpu_put(), before returning to the EL0. > > + * > > + * Return true if KVM takes care of the register. Otherwise return false. > > + */ > > +bool kvm_set_pmuserenr(u64 val) > > +{ > > + struct kvm_cpu_context *hctxt; > > + > > + if (!kvm_arm_support_pmu_v3() || !has_vhe() || !kvm_get_running_vcpu()) > > + return false; > > + > > + hctxt = &this_cpu_ptr(&kvm_host_data)->host_ctxt; > > + ctxt_sys_reg(hctxt, PMUSERENR_EL0) = val; > > + return true; > > +} > > -- > > 2.40.0.577.gac1e443424-goog > > > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel