From: Marc Zyngier <maz@kernel.org> To: Reiji Watanabe <reijiw@google.com> Cc: 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>, Oliver Upton <oliver.upton@linux.dev>, Jing Zhang <jingzhangos@google.com>, Raghavendra Rao Anata <rananta@google.com> Subject: Re: [PATCH v2 1/8] KVM: arm64: PMU: Have reset_pmu_reg() to clear a register Date: Fri, 20 Jan 2023 14:11:25 +0000 [thread overview] Message-ID: <86mt6dmh2q.wl-maz@kernel.org> (raw) In-Reply-To: <86o7qtmher.wl-maz@kernel.org> On Fri, 20 Jan 2023 14:04:12 +0000, Marc Zyngier <maz@kernel.org> wrote: > > On Tue, 17 Jan 2023 01:35:35 +0000, > Reiji Watanabe <reijiw@google.com> wrote: > > > > On vCPU reset, PMCNTEN{SET,CLR}_EL0, PMINTEN{SET,CLR}_EL1, and > > PMOVS{SET,CLR}_EL1 for a vCPU are reset by reset_pmu_reg(). > > This function clears RAZ bits of those registers corresponding > > to unimplemented event counters on the vCPU, and sets bits > > corresponding to implemented event counters to a predefined > > pseudo UNKNOWN value (some bits are set to 1). > > > > The function identifies (un)implemented event counters on the > > vCPU based on the PMCR_EL1.N value on the host. Using the host > > value for this would be problematic when KVM supports letting > > userspace set PMCR_EL1.N to a value different from the host value > > (some of the RAZ bits of those registers could end up being set to 1). > > > > Fix reset_pmu_reg() to clear the registers so that it can ensure > > that all the RAZ bits are cleared even when the PMCR_EL1.N value > > for the vCPU is different from the host value. > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > --- > > arch/arm64/kvm/sys_regs.c | 10 +--------- > > 1 file changed, 1 insertion(+), 9 deletions(-) > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index c6cbfe6b854b..ec4bdaf71a15 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -604,19 +604,11 @@ static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu, > > > > static void reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > > { > > - u64 n, mask = BIT(ARMV8_PMU_CYCLE_IDX); > > - > > /* No PMU available, any PMU reg may UNDEF... */ > > if (!kvm_arm_support_pmu_v3()) > > return; > > Is this still true? We remove the PMCR_EL0 access just below. > > > > > - n = read_sysreg(pmcr_el0) >> ARMV8_PMU_PMCR_N_SHIFT; > > - n &= ARMV8_PMU_PMCR_N_MASK; > > - if (n) > > - mask |= GENMASK(n - 1, 0); > > - > > - reset_unknown(vcpu, r); > > - __vcpu_sys_reg(vcpu, r->reg) &= mask; > > + __vcpu_sys_reg(vcpu, r->reg) = 0; > > } > > At the end of the day, this function has no dependency on the host at > all, and only writes 0 to the per-vcpu register. > > So why not get rid of it altogether and have: > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index c6cbfe6b854b..1d1514b89d75 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -976,7 +976,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > trap_wcr, reset_wcr, 0, 0, get_wcr, set_wcr } > > #define PMU_SYS_REG(r) \ > - SYS_DESC(r), .reset = reset_pmu_reg, .visibility = pmu_visibility > + SYS_DESC(r), .visibility = pmu_visibility > > /* Macro to expand the PMEVCNTRn_EL0 register */ > #define PMU_PMEVCNTR_EL0(n) \ > > which would fall-back the specified reset value (zero by default)? Scratch that, we need: diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index c6cbfe6b854b..6f6a928c92ec 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -976,7 +976,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, trap_wcr, reset_wcr, 0, 0, get_wcr, set_wcr } #define PMU_SYS_REG(r) \ - SYS_DESC(r), .reset = reset_pmu_reg, .visibility = pmu_visibility + SYS_DESC(r), .reset = reset_val, .visibility = pmu_visibility /* Macro to expand the PMEVCNTRn_EL0 register */ #define PMU_PMEVCNTR_EL0(n) \ But otherwise, this should be enough. M. -- Without deviation from the norm, progress is not possible.
WARNING: multiple messages have this Message-ID (diff)
From: Marc Zyngier <maz@kernel.org> To: Reiji Watanabe <reijiw@google.com> Cc: 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>, Oliver Upton <oliver.upton@linux.dev>, Jing Zhang <jingzhangos@google.com>, Raghavendra Rao Anata <rananta@google.com> Subject: Re: [PATCH v2 1/8] KVM: arm64: PMU: Have reset_pmu_reg() to clear a register Date: Fri, 20 Jan 2023 14:11:25 +0000 [thread overview] Message-ID: <86mt6dmh2q.wl-maz@kernel.org> (raw) In-Reply-To: <86o7qtmher.wl-maz@kernel.org> On Fri, 20 Jan 2023 14:04:12 +0000, Marc Zyngier <maz@kernel.org> wrote: > > On Tue, 17 Jan 2023 01:35:35 +0000, > Reiji Watanabe <reijiw@google.com> wrote: > > > > On vCPU reset, PMCNTEN{SET,CLR}_EL0, PMINTEN{SET,CLR}_EL1, and > > PMOVS{SET,CLR}_EL1 for a vCPU are reset by reset_pmu_reg(). > > This function clears RAZ bits of those registers corresponding > > to unimplemented event counters on the vCPU, and sets bits > > corresponding to implemented event counters to a predefined > > pseudo UNKNOWN value (some bits are set to 1). > > > > The function identifies (un)implemented event counters on the > > vCPU based on the PMCR_EL1.N value on the host. Using the host > > value for this would be problematic when KVM supports letting > > userspace set PMCR_EL1.N to a value different from the host value > > (some of the RAZ bits of those registers could end up being set to 1). > > > > Fix reset_pmu_reg() to clear the registers so that it can ensure > > that all the RAZ bits are cleared even when the PMCR_EL1.N value > > for the vCPU is different from the host value. > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > --- > > arch/arm64/kvm/sys_regs.c | 10 +--------- > > 1 file changed, 1 insertion(+), 9 deletions(-) > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index c6cbfe6b854b..ec4bdaf71a15 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -604,19 +604,11 @@ static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu, > > > > static void reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) > > { > > - u64 n, mask = BIT(ARMV8_PMU_CYCLE_IDX); > > - > > /* No PMU available, any PMU reg may UNDEF... */ > > if (!kvm_arm_support_pmu_v3()) > > return; > > Is this still true? We remove the PMCR_EL0 access just below. > > > > > - n = read_sysreg(pmcr_el0) >> ARMV8_PMU_PMCR_N_SHIFT; > > - n &= ARMV8_PMU_PMCR_N_MASK; > > - if (n) > > - mask |= GENMASK(n - 1, 0); > > - > > - reset_unknown(vcpu, r); > > - __vcpu_sys_reg(vcpu, r->reg) &= mask; > > + __vcpu_sys_reg(vcpu, r->reg) = 0; > > } > > At the end of the day, this function has no dependency on the host at > all, and only writes 0 to the per-vcpu register. > > So why not get rid of it altogether and have: > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index c6cbfe6b854b..1d1514b89d75 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -976,7 +976,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > trap_wcr, reset_wcr, 0, 0, get_wcr, set_wcr } > > #define PMU_SYS_REG(r) \ > - SYS_DESC(r), .reset = reset_pmu_reg, .visibility = pmu_visibility > + SYS_DESC(r), .visibility = pmu_visibility > > /* Macro to expand the PMEVCNTRn_EL0 register */ > #define PMU_PMEVCNTR_EL0(n) \ > > which would fall-back the specified reset value (zero by default)? Scratch that, we need: diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index c6cbfe6b854b..6f6a928c92ec 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -976,7 +976,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, trap_wcr, reset_wcr, 0, 0, get_wcr, set_wcr } #define PMU_SYS_REG(r) \ - SYS_DESC(r), .reset = reset_pmu_reg, .visibility = pmu_visibility + SYS_DESC(r), .reset = reset_val, .visibility = pmu_visibility /* Macro to expand the PMEVCNTRn_EL0 register */ #define PMU_PMEVCNTR_EL0(n) \ But otherwise, this should be enough. 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
next prev parent reply other threads:[~2023-01-20 14:13 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-01-17 1:35 [PATCH v2 0/8] KVM: arm64: PMU: Allow userspace to limit the number of PMCs on vCPU Reiji Watanabe 2023-01-17 1:35 ` Reiji Watanabe 2023-01-17 1:35 ` [PATCH v2 1/8] KVM: arm64: PMU: Have reset_pmu_reg() to clear a register Reiji Watanabe 2023-01-17 1:35 ` Reiji Watanabe 2023-01-20 14:04 ` Marc Zyngier 2023-01-20 14:04 ` Marc Zyngier 2023-01-20 14:11 ` Marc Zyngier [this message] 2023-01-20 14:11 ` Marc Zyngier 2023-01-21 5:18 ` Reiji Watanabe 2023-01-21 5:18 ` Reiji Watanabe 2023-01-17 1:35 ` [PATCH v2 2/8] KVM: arm64: PMU: Use reset_pmu_reg() for PMUSERENR_EL0 and PMCCFILTR_EL0 Reiji Watanabe 2023-01-17 1:35 ` Reiji Watanabe 2023-01-17 1:35 ` [PATCH v2 3/8] KVM: arm64: PMU: Preserve vCPU's PMCR_EL0.N value on vCPU reset Reiji Watanabe 2023-01-17 1:35 ` Reiji Watanabe 2023-01-20 0:30 ` Oliver Upton 2023-01-20 0:30 ` Oliver Upton 2023-01-20 12:12 ` Marc Zyngier 2023-01-20 12:12 ` Marc Zyngier 2023-01-20 18:04 ` Oliver Upton 2023-01-20 18:04 ` Oliver Upton 2023-01-20 18:53 ` Reiji Watanabe 2023-01-20 18:53 ` Reiji Watanabe 2023-01-17 1:35 ` [PATCH v2 4/8] KVM: arm64: PMU: Disallow userspace to set PMCR.N greater than the host value Reiji Watanabe 2023-01-17 1:35 ` Reiji Watanabe 2023-01-20 14:18 ` Marc Zyngier 2023-01-20 14:18 ` Marc Zyngier 2023-01-17 1:35 ` [PATCH v2 5/8] tools: arm64: Import perf_event.h Reiji Watanabe 2023-01-17 1:35 ` Reiji Watanabe 2023-01-17 1:35 ` [PATCH v2 6/8] KVM: selftests: aarch64: Introduce vpmu_counter_access test Reiji Watanabe 2023-01-17 1:35 ` Reiji Watanabe 2023-01-17 1:35 ` [PATCH v2 7/8] KVM: selftests: aarch64: vPMU register test for implemented counters Reiji Watanabe 2023-01-17 1:35 ` Reiji Watanabe 2023-01-18 7:47 ` Shaoqin Huang 2023-01-18 7:47 ` Shaoqin Huang 2023-01-19 3:02 ` Reiji Watanabe 2023-01-19 3:02 ` Reiji Watanabe 2023-01-17 1:35 ` [PATCH v2 8/8] KVM: selftests: aarch64: vPMU register test for unimplemented counters Reiji Watanabe 2023-01-17 1:35 ` Reiji Watanabe 2023-01-18 7:49 ` Shaoqin Huang 2023-01-18 7:49 ` Shaoqin Huang 2023-01-19 3:04 ` Reiji Watanabe 2023-01-19 3:04 ` Reiji Watanabe 2023-01-17 7:25 ` [PATCH v2 0/8] KVM: arm64: PMU: Allow userspace to limit the number of PMCs on vCPU Shaoqin Huang 2023-01-17 7:25 ` Shaoqin Huang 2023-01-18 5:53 ` Reiji Watanabe 2023-01-18 5:53 ` 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=86mt6dmh2q.wl-maz@kernel.org \ --to=maz@kernel.org \ --cc=alexandru.elisei@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=oliver.upton@linux.dev \ --cc=pbonzini@redhat.com \ --cc=rananta@google.com \ --cc=reijiw@google.com \ --cc=ricarkol@google.com \ --cc=suzuki.poulose@arm.com \ --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.