From: Reiji Watanabe <reijiw@google.com> To: Oliver Upton <oliver.upton@linux.dev> Cc: Marc Zyngier <maz@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Will Deacon <will@kernel.org>, Catalin Marinas <catalin.marinas@arm.com>, kvmarm@lists.linux.dev, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, James Morse <james.morse@arm.com>, Alexandru Elisei <alexandru.elisei@arm.com>, Zenghui Yu <yuzenghui@huawei.com>, Suzuki K Poulose <suzuki.poulose@arm.com>, Paolo Bonzini <pbonzini@redhat.com>, Ricardo Koller <ricarkol@google.com>, Jing Zhang <jingzhangos@google.com>, Raghavendra Rao Anata <rananta@google.com>, Shaoqin Huang <shahuang@redhat.com>, Rob Herring <robh@kernel.org> Subject: Re: [PATCH v3 2/2] KVM: arm64: PMU: Don't overwrite PMUSERENR with vcpu loaded Date: Fri, 26 May 2023 21:16:58 -0700 [thread overview] Message-ID: <20230527041658.zgftvtaskylzmr6l@google.com> (raw) In-Reply-To: <ZG/w95pYjWnMJB62@linux.dev> Hi Oliver, On Thu, May 25, 2023 at 11:36:23PM +0000, Oliver Upton wrote: > Hi Reiji, > > Apologies, this fell off my list of reviews. > > On Sat, Apr 15, 2023 at 09:40:29AM -0700, Reiji Watanabe wrote: > > [...] > > > static void armv8pmu_enable_event(struct perf_event *event) > > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > > index 6718731729fd..7e73be12cfaf 100644 > > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > > @@ -82,12 +82,24 @@ 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; > > + > > + /* > > + * Disable IRQs to prevent a race condition between the > > + * following code and IPIs that attempts to update > > + * PMUSERENR_EL0. See also kvm_set_pmuserenr(). > > + */ > > + 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); > > Can the IRQ save/restore be moved to {activate,deactivate}_traps_vhe_{load,put}()? > > That'd eliminate the dance to avoid using kernel-only symbols in nVHE > and would be consistent with the existing usage of > __{activate,deactivate}_traps_common() from nVHE (IRQs already > disabled). > > IMO, the less nVHE knows about the kernel the better. Thank you for the comments. Sure, I will move them to {activate,deactivate}_traps_vhe_{load,put}(). > > > } > > > > vcpu->arch.mdcr_el2_host = read_sysreg(mdcr_el2); > > @@ -112,9 +124,21 @@ 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; > > + > > + /* > > + * Disable IRQs to prevent a race condition between the > > + * following code and IPIs that attempts to update > > + * PMUSERENR_EL0. See also kvm_set_pmuserenr(). > > + */ > > + 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/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile > > index 530347cdebe3..2c08a54ca7d9 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/Makefile > > +++ b/arch/arm64/kvm/hyp/nvhe/Makefile > > @@ -10,7 +10,7 @@ asflags-y := -D__KVM_NVHE_HYPERVISOR__ -D__DISABLE_EXPORTS > > # will explode instantly (Words of Marc Zyngier). So introduce a generic flag > > # __DISABLE_TRACE_MMIO__ to disable MMIO tracing for nVHE KVM. > > ccflags-y := -D__KVM_NVHE_HYPERVISOR__ -D__DISABLE_EXPORTS -D__DISABLE_TRACE_MMIO__ > > -ccflags-y += -fno-stack-protector \ > > +ccflags-y += -fno-stack-protector -DNO_TRACE_IRQFLAGS \ > > -DDISABLE_BRANCH_PROFILING \ > > $(DISABLE_STACKLEAK_PLUGIN) > > > > diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c > > index 7887133d15f0..d6a863853bfe 100644 > > --- a/arch/arm64/kvm/pmu.c > > +++ b/arch/arm64/kvm/pmu.c > > @@ -209,3 +209,28 @@ 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 PMUSERENR_EL0 for the guest 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. > > wording: s/the EL0/EL0. Or, alternatively, to avoid repeating yourself > you can just say "returning to userspace". > > You may also want to mention in passing why this isn't necessary for > nVHE, as the register is context switched for every guest enter/exit. Thank you for the suggestions. I will fix those. Thank you, Reiji > > > + * > > + * Return true if KVM takes care of the register. Otherwise return false. > > + */ > > +bool kvm_set_pmuserenr(u64 val) > > +{ > > + struct kvm_cpu_context *hctxt; > > + struct kvm_vcpu *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; > > + ctxt_sys_reg(hctxt, PMUSERENR_EL0) = val; > > + return true; > > +} > > -- > Thanks, > Oliver
WARNING: multiple messages have this Message-ID (diff)
From: Reiji Watanabe <reijiw@google.com> To: Oliver Upton <oliver.upton@linux.dev> Cc: Marc Zyngier <maz@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Will Deacon <will@kernel.org>, Catalin Marinas <catalin.marinas@arm.com>, kvmarm@lists.linux.dev, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, James Morse <james.morse@arm.com>, Alexandru Elisei <alexandru.elisei@arm.com>, Zenghui Yu <yuzenghui@huawei.com>, Suzuki K Poulose <suzuki.poulose@arm.com>, Paolo Bonzini <pbonzini@redhat.com>, Ricardo Koller <ricarkol@google.com>, Jing Zhang <jingzhangos@google.com>, Raghavendra Rao Anata <rananta@google.com>, Shaoqin Huang <shahuang@redhat.com>, Rob Herring <robh@kernel.org> Subject: Re: [PATCH v3 2/2] KVM: arm64: PMU: Don't overwrite PMUSERENR with vcpu loaded Date: Fri, 26 May 2023 21:16:58 -0700 [thread overview] Message-ID: <20230527041658.zgftvtaskylzmr6l@google.com> (raw) In-Reply-To: <ZG/w95pYjWnMJB62@linux.dev> Hi Oliver, On Thu, May 25, 2023 at 11:36:23PM +0000, Oliver Upton wrote: > Hi Reiji, > > Apologies, this fell off my list of reviews. > > On Sat, Apr 15, 2023 at 09:40:29AM -0700, Reiji Watanabe wrote: > > [...] > > > static void armv8pmu_enable_event(struct perf_event *event) > > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > > index 6718731729fd..7e73be12cfaf 100644 > > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > > @@ -82,12 +82,24 @@ 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; > > + > > + /* > > + * Disable IRQs to prevent a race condition between the > > + * following code and IPIs that attempts to update > > + * PMUSERENR_EL0. See also kvm_set_pmuserenr(). > > + */ > > + 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); > > Can the IRQ save/restore be moved to {activate,deactivate}_traps_vhe_{load,put}()? > > That'd eliminate the dance to avoid using kernel-only symbols in nVHE > and would be consistent with the existing usage of > __{activate,deactivate}_traps_common() from nVHE (IRQs already > disabled). > > IMO, the less nVHE knows about the kernel the better. Thank you for the comments. Sure, I will move them to {activate,deactivate}_traps_vhe_{load,put}(). > > > } > > > > vcpu->arch.mdcr_el2_host = read_sysreg(mdcr_el2); > > @@ -112,9 +124,21 @@ 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; > > + > > + /* > > + * Disable IRQs to prevent a race condition between the > > + * following code and IPIs that attempts to update > > + * PMUSERENR_EL0. See also kvm_set_pmuserenr(). > > + */ > > + 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/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile > > index 530347cdebe3..2c08a54ca7d9 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/Makefile > > +++ b/arch/arm64/kvm/hyp/nvhe/Makefile > > @@ -10,7 +10,7 @@ asflags-y := -D__KVM_NVHE_HYPERVISOR__ -D__DISABLE_EXPORTS > > # will explode instantly (Words of Marc Zyngier). So introduce a generic flag > > # __DISABLE_TRACE_MMIO__ to disable MMIO tracing for nVHE KVM. > > ccflags-y := -D__KVM_NVHE_HYPERVISOR__ -D__DISABLE_EXPORTS -D__DISABLE_TRACE_MMIO__ > > -ccflags-y += -fno-stack-protector \ > > +ccflags-y += -fno-stack-protector -DNO_TRACE_IRQFLAGS \ > > -DDISABLE_BRANCH_PROFILING \ > > $(DISABLE_STACKLEAK_PLUGIN) > > > > diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c > > index 7887133d15f0..d6a863853bfe 100644 > > --- a/arch/arm64/kvm/pmu.c > > +++ b/arch/arm64/kvm/pmu.c > > @@ -209,3 +209,28 @@ 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 PMUSERENR_EL0 for the guest 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. > > wording: s/the EL0/EL0. Or, alternatively, to avoid repeating yourself > you can just say "returning to userspace". > > You may also want to mention in passing why this isn't necessary for > nVHE, as the register is context switched for every guest enter/exit. Thank you for the suggestions. I will fix those. Thank you, Reiji > > > + * > > + * Return true if KVM takes care of the register. Otherwise return false. > > + */ > > +bool kvm_set_pmuserenr(u64 val) > > +{ > > + struct kvm_cpu_context *hctxt; > > + struct kvm_vcpu *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; > > + ctxt_sys_reg(hctxt, PMUSERENR_EL0) = val; > > + return true; > > +} > > -- > Thanks, > Oliver _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-05-27 4:17 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-04-15 16:40 [PATCH v3 0/2] KVM: arm64: PMU: Correct the handling of PMUSERENR_EL0 Reiji Watanabe 2023-04-15 16:40 ` Reiji Watanabe 2023-04-15 16:40 ` [PATCH v3 1/2] KVM: arm64: PMU: Restore the host's PMUSERENR_EL0 Reiji Watanabe 2023-04-15 16:40 ` Reiji Watanabe 2023-04-15 16:40 ` [PATCH v3 2/2] KVM: arm64: PMU: Don't overwrite PMUSERENR with vcpu loaded Reiji Watanabe 2023-04-15 16:40 ` Reiji Watanabe 2023-04-15 22:09 ` kernel test robot 2023-04-15 22:09 ` kernel test robot 2023-04-16 3:31 ` Reiji Watanabe 2023-04-16 3:31 ` Reiji Watanabe 2023-05-25 23:36 ` Oliver Upton 2023-05-25 23:36 ` Oliver Upton 2023-05-27 4:16 ` Reiji Watanabe [this message] 2023-05-27 4:16 ` Reiji Watanabe
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20230527041658.zgftvtaskylzmr6l@google.com \ --to=reijiw@google.com \ --cc=alexandru.elisei@arm.com \ --cc=catalin.marinas@arm.com \ --cc=james.morse@arm.com \ --cc=jingzhangos@google.com \ --cc=kvm@vger.kernel.org \ --cc=kvmarm@lists.linux.dev \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=mark.rutland@arm.com \ --cc=maz@kernel.org \ --cc=oliver.upton@linux.dev \ --cc=pbonzini@redhat.com \ --cc=rananta@google.com \ --cc=ricarkol@google.com \ --cc=robh@kernel.org \ --cc=shahuang@redhat.com \ --cc=suzuki.poulose@arm.com \ --cc=will@kernel.org \ --cc=yuzenghui@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.