From: Reiji Watanabe <[email protected]>
The number of PMU event counters is indicated in PMCR_EL0.N.
For a vCPU with PMUv3 configured, the value is set to the same
value as the current PE on every vCPU reset. Unless the vCPU is
pinned to PEs that has the PMU associated to the guest from the
initial vCPU reset, the value might be different from the PMU's
PMCR_EL0.N on heterogeneous PMU systems.
Fix this by setting the vCPU's PMCR_EL0.N to the PMU's PMCR_EL0.N
value. Track the PMCR_EL0.N per guest, as only one PMU can be set
for the guest (PMCR_EL0.N must be the same for all vCPUs of the
guest), and it is convenient for updating the value.
KVM does not yet support userspace modifying PMCR_EL0.N.
The following patch will add support for that.
Signed-off-by: Reiji Watanabe <[email protected]>
Signed-off-by: Raghavendra Rao Ananta <[email protected]>
---
arch/arm64/include/asm/kvm_host.h | 3 +++
arch/arm64/kvm/pmu-emul.c | 14 +++++++++++++-
arch/arm64/kvm/sys_regs.c | 15 +++++++++------
3 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f7e5132c0a23..a7f326a85077 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -283,6 +283,9 @@ struct kvm_arch {
cpumask_var_t supported_cpus;
+ /* PMCR_EL0.N value for the guest */
+ u8 pmcr_n;
+
/* Hypercall features firmware registers' descriptor */
struct kvm_smccc_features smccc_feat;
struct maple_tree smccc_filter;
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 84aa8efd9163..4daa9f6b170a 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -690,6 +690,9 @@ void kvm_host_pmu_init(struct arm_pmu *pmu)
if (!entry)
goto out_unlock;
+ WARN_ON((pmu->num_events <= 0) ||
+ (pmu->num_events > ARMV8_PMU_MAX_COUNTERS));
+
entry->arm_pmu = pmu;
list_add_tail(&entry->entry, &arm_pmus);
@@ -895,6 +898,7 @@ static void kvm_arm_set_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
lockdep_assert_held(&kvm->arch.config_lock);
kvm->arch.arm_pmu = arm_pmu;
+ kvm->arch.pmcr_n = kvm_arm_get_num_counters(kvm);
}
/**
@@ -1105,8 +1109,16 @@ u8 kvm_arm_pmu_get_pmuver_limit(void)
/**
* kvm_vcpu_read_pmcr - Read PMCR_EL0 register for the vCPU
* @vcpu: The vcpu pointer
+ *
+ * The function returns the value of PMCR.N based on the per-VM tracked
+ * value (kvm->arch.pmcr_n). This is to ensure that the register field
+ * remains consistent for the VM, even on heterogeneous systems where
+ * the value may vary when read from different CPUs (during vCPU reset).
*/
u64 kvm_vcpu_read_pmcr(struct kvm_vcpu *vcpu)
{
- return __vcpu_sys_reg(vcpu, PMCR_EL0);
+ u64 pmcr = __vcpu_sys_reg(vcpu, PMCR_EL0) &
+ ~(ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
+
+ return pmcr | ((u64)vcpu->kvm->arch.pmcr_n << ARMV8_PMU_PMCR_N_SHIFT);
}
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index ff0f7095eaca..c750722fbe4a 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -745,12 +745,8 @@ static u64 reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
{
u64 pmcr;
- /* No PMU available, PMCR_EL0 may UNDEF... */
- if (!kvm_arm_support_pmu_v3())
- return 0;
-
/* Only preserve PMCR_EL0.N, and reset the rest to 0 */
- pmcr = read_sysreg(pmcr_el0) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
+ pmcr = kvm_vcpu_read_pmcr(vcpu) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
if (!kvm_supports_32bit_el0())
pmcr |= ARMV8_PMU_PMCR_LC;
@@ -1084,6 +1080,13 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
return true;
}
+static int get_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
+ u64 *val)
+{
+ *val = kvm_vcpu_read_pmcr(vcpu);
+ return 0;
+}
+
/* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
#define DBG_BCR_BVR_WCR_WVR_EL1(n) \
{ SYS_DESC(SYS_DBGBVRn_EL1(n)), \
@@ -2148,7 +2151,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
{ SYS_DESC(SYS_SVCR), undef_access },
{ PMU_SYS_REG(PMCR_EL0), .access = access_pmcr,
- .reset = reset_pmcr, .reg = PMCR_EL0 },
+ .reset = reset_pmcr, .reg = PMCR_EL0, .get_user = get_pmcr },
{ PMU_SYS_REG(PMCNTENSET_EL0),
.access = access_pmcnten, .reg = PMCNTENSET_EL0 },
{ PMU_SYS_REG(PMCNTENCLR_EL0),
--
2.42.0.609.gbb76f46606-goog
On Mon, 9 Oct 2023, Raghavendra Rao Ananta wrote:
> u64 kvm_vcpu_read_pmcr(struct kvm_vcpu *vcpu)
> {
> - return __vcpu_sys_reg(vcpu, PMCR_EL0);
> + u64 pmcr = __vcpu_sys_reg(vcpu, PMCR_EL0) &
> + ~(ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
> +
> + return pmcr | ((u64)vcpu->kvm->arch.pmcr_n << ARMV8_PMU_PMCR_N_SHIFT);
> }
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index ff0f7095eaca..c750722fbe4a 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -745,12 +745,8 @@ static u64 reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> {
> u64 pmcr;
>
> - /* No PMU available, PMCR_EL0 may UNDEF... */
> - if (!kvm_arm_support_pmu_v3())
> - return 0;
> -
> /* Only preserve PMCR_EL0.N, and reset the rest to 0 */
> - pmcr = read_sysreg(pmcr_el0) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
> + pmcr = kvm_vcpu_read_pmcr(vcpu) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
pmcr = ((u64)vcpu->kvm->arch.pmcr_n << ARMV8_PMU_PMCR_N_SHIFT);
Would that maybe make it more clear what is done here?
On Mon, Oct 16, 2023 at 6:35 AM Sebastian Ott <[email protected]> wrote:
>
> On Mon, 9 Oct 2023, Raghavendra Rao Ananta wrote:
> > u64 kvm_vcpu_read_pmcr(struct kvm_vcpu *vcpu)
> > {
> > - return __vcpu_sys_reg(vcpu, PMCR_EL0);
> > + u64 pmcr = __vcpu_sys_reg(vcpu, PMCR_EL0) &
> > + ~(ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
> > +
> > + return pmcr | ((u64)vcpu->kvm->arch.pmcr_n << ARMV8_PMU_PMCR_N_SHIFT);
> > }
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index ff0f7095eaca..c750722fbe4a 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -745,12 +745,8 @@ static u64 reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > {
> > u64 pmcr;
> >
> > - /* No PMU available, PMCR_EL0 may UNDEF... */
> > - if (!kvm_arm_support_pmu_v3())
> > - return 0;
> > -
> > /* Only preserve PMCR_EL0.N, and reset the rest to 0 */
> > - pmcr = read_sysreg(pmcr_el0) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
> > + pmcr = kvm_vcpu_read_pmcr(vcpu) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
>
> pmcr = ((u64)vcpu->kvm->arch.pmcr_n << ARMV8_PMU_PMCR_N_SHIFT);
> Would that maybe make it more clear what is done here?
>
Since we require the entire PMCR register, and not just the PMCR.N
field, I think using kvm_vcpu_read_pmcr() would be technically
correct, don't you think?
Thank you.
Raghavendra
On Mon, Oct 16, 2023 at 12:02:27PM -0700, Raghavendra Rao Ananta wrote:
> On Mon, Oct 16, 2023 at 6:35 AM Sebastian Ott <[email protected]> wrote:
> >
> > On Mon, 9 Oct 2023, Raghavendra Rao Ananta wrote:
> > > u64 kvm_vcpu_read_pmcr(struct kvm_vcpu *vcpu)
> > > {
> > > - return __vcpu_sys_reg(vcpu, PMCR_EL0);
> > > + u64 pmcr = __vcpu_sys_reg(vcpu, PMCR_EL0) &
> > > + ~(ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
> > > +
> > > + return pmcr | ((u64)vcpu->kvm->arch.pmcr_n << ARMV8_PMU_PMCR_N_SHIFT);
> > > }
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index ff0f7095eaca..c750722fbe4a 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -745,12 +745,8 @@ static u64 reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > > {
> > > u64 pmcr;
> > >
> > > - /* No PMU available, PMCR_EL0 may UNDEF... */
> > > - if (!kvm_arm_support_pmu_v3())
> > > - return 0;
> > > -
> > > /* Only preserve PMCR_EL0.N, and reset the rest to 0 */
> > > - pmcr = read_sysreg(pmcr_el0) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
> > > + pmcr = kvm_vcpu_read_pmcr(vcpu) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
> >
> > pmcr = ((u64)vcpu->kvm->arch.pmcr_n << ARMV8_PMU_PMCR_N_SHIFT);
> > Would that maybe make it more clear what is done here?
> >
> Since we require the entire PMCR register, and not just the PMCR.N
> field, I think using kvm_vcpu_read_pmcr() would be technically
> correct, don't you think?
No, this isn't using the entire PMCR value, it is just grabbing
PMCR_EL0.N.
What's the point of doing this in the first place? The implementation of
kvm_vcpu_read_pmcr() is populating PMCR_EL0.N using the VM-scoped value.
--
Thanks,
Oliver
On Mon, Oct 16, 2023 at 12:16 PM Oliver Upton <[email protected]> wrote:
>
> On Mon, Oct 16, 2023 at 12:02:27PM -0700, Raghavendra Rao Ananta wrote:
> > On Mon, Oct 16, 2023 at 6:35 AM Sebastian Ott <[email protected]> wrote:
> > >
> > > On Mon, 9 Oct 2023, Raghavendra Rao Ananta wrote:
> > > > u64 kvm_vcpu_read_pmcr(struct kvm_vcpu *vcpu)
> > > > {
> > > > - return __vcpu_sys_reg(vcpu, PMCR_EL0);
> > > > + u64 pmcr = __vcpu_sys_reg(vcpu, PMCR_EL0) &
> > > > + ~(ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
> > > > +
> > > > + return pmcr | ((u64)vcpu->kvm->arch.pmcr_n << ARMV8_PMU_PMCR_N_SHIFT);
> > > > }
> > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > > index ff0f7095eaca..c750722fbe4a 100644
> > > > --- a/arch/arm64/kvm/sys_regs.c
> > > > +++ b/arch/arm64/kvm/sys_regs.c
> > > > @@ -745,12 +745,8 @@ static u64 reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > > > {
> > > > u64 pmcr;
> > > >
> > > > - /* No PMU available, PMCR_EL0 may UNDEF... */
> > > > - if (!kvm_arm_support_pmu_v3())
> > > > - return 0;
> > > > -
> > > > /* Only preserve PMCR_EL0.N, and reset the rest to 0 */
> > > > - pmcr = read_sysreg(pmcr_el0) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
> > > > + pmcr = kvm_vcpu_read_pmcr(vcpu) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
> > >
> > > pmcr = ((u64)vcpu->kvm->arch.pmcr_n << ARMV8_PMU_PMCR_N_SHIFT);
> > > Would that maybe make it more clear what is done here?
> > >
> > Since we require the entire PMCR register, and not just the PMCR.N
> > field, I think using kvm_vcpu_read_pmcr() would be technically
> > correct, don't you think?
>
> No, this isn't using the entire PMCR value, it is just grabbing
> PMCR_EL0.N.
>
Oh sorry, my bad.
> What's the point of doing this in the first place? The implementation of
> kvm_vcpu_read_pmcr() is populating PMCR_EL0.N using the VM-scoped value.
>
I guess originally the change replaced read_sysreg(pmcr_el0) with
kvm_vcpu_read_pmcr(vcpu) to maintain consistency with others.
But if you and Sebastian feel that it's an overkill and directly
getting the value via vcpu->kvm->arch.pmcr_n is more readable, I'm
happy to make the change.
Thank you.
Raghavendra
> --
> Thanks,
> Oliver
On Mon, Oct 16, 2023 at 02:35:52PM -0700, Raghavendra Rao Ananta wrote:
[...]
> > What's the point of doing this in the first place? The implementation of
> > kvm_vcpu_read_pmcr() is populating PMCR_EL0.N using the VM-scoped value.
> >
> I guess originally the change replaced read_sysreg(pmcr_el0) with
> kvm_vcpu_read_pmcr(vcpu) to maintain consistency with others.
> But if you and Sebastian feel that it's an overkill and directly
> getting the value via vcpu->kvm->arch.pmcr_n is more readable, I'm
> happy to make the change.
No, I'd rather you delete the line where PMCR_EL0.N altogether.
reset_pmcr() tries to initialize the field, but your
kvm_vcpu_read_pmcr() winds up replacing it with pmcr_n.
> @@ -1105,8 +1109,16 @@ u8 kvm_arm_pmu_get_pmuver_limit(void)
> /**
> * kvm_vcpu_read_pmcr - Read PMCR_EL0 register for the vCPU
> * @vcpu: The vcpu pointer
> + *
> + * The function returns the value of PMCR.N based on the per-VM tracked
> + * value (kvm->arch.pmcr_n). This is to ensure that the register field
> + * remains consistent for the VM, even on heterogeneous systems where
> + * the value may vary when read from different CPUs (during vCPU reset).
Since I'm looking at this again, I don't believe the comment is adding
much. KVM doesn't read pmcr_el0 directly anymore, and kvm_arch is
clearly VM-scoped context.
> */
> u64 kvm_vcpu_read_pmcr(struct kvm_vcpu *vcpu)
> {
> - return __vcpu_sys_reg(vcpu, PMCR_EL0);
> + u64 pmcr = __vcpu_sys_reg(vcpu, PMCR_EL0) &
> + ~(ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
> +
> + return pmcr | ((u64)vcpu->kvm->arch.pmcr_n << ARMV8_PMU_PMCR_N_SHIFT);
> }
--
Thanks,
Oliver
On Tue, Oct 17, 2023 at 05:52:24AM +0000, Oliver Upton wrote:
> On Mon, Oct 16, 2023 at 02:35:52PM -0700, Raghavendra Rao Ananta wrote:
>
> [...]
>
> > > What's the point of doing this in the first place? The implementation of
> > > kvm_vcpu_read_pmcr() is populating PMCR_EL0.N using the VM-scoped value.
> > >
> > I guess originally the change replaced read_sysreg(pmcr_el0) with
> > kvm_vcpu_read_pmcr(vcpu) to maintain consistency with others.
> > But if you and Sebastian feel that it's an overkill and directly
> > getting the value via vcpu->kvm->arch.pmcr_n is more readable, I'm
> > happy to make the change.
>
> No, I'd rather you delete the line where PMCR_EL0.N altogether.
... where we set up ...
> reset_pmcr() tries to initialize the field, but your
> kvm_vcpu_read_pmcr() winds up replacing it with pmcr_n.
--
Thanks,
Oliver
On Mon, Oct 16, 2023 at 10:52 PM Oliver Upton <[email protected]> wrote:
>
> On Mon, Oct 16, 2023 at 02:35:52PM -0700, Raghavendra Rao Ananta wrote:
>
> [...]
>
> > > What's the point of doing this in the first place? The implementation of
> > > kvm_vcpu_read_pmcr() is populating PMCR_EL0.N using the VM-scoped value.
> > >
> > I guess originally the change replaced read_sysreg(pmcr_el0) with
> > kvm_vcpu_read_pmcr(vcpu) to maintain consistency with others.
> > But if you and Sebastian feel that it's an overkill and directly
> > getting the value via vcpu->kvm->arch.pmcr_n is more readable, I'm
> > happy to make the change.
>
> No, I'd rather you delete the line where PMCR_EL0.N altogether.
> reset_pmcr() tries to initialize the field, but your
> kvm_vcpu_read_pmcr() winds up replacing it with pmcr_n.
>
I didn't get this comment. We still do initialize pmcr, but using the
pmcr.n read via kvm_vcpu_read_pmcr() instead of the actual system
register.
Thank you.
Raghavendra
> > @@ -1105,8 +1109,16 @@ u8 kvm_arm_pmu_get_pmuver_limit(void)
> > /**
> > * kvm_vcpu_read_pmcr - Read PMCR_EL0 register for the vCPU
> > * @vcpu: The vcpu pointer
> > + *
> > + * The function returns the value of PMCR.N based on the per-VM tracked
> > + * value (kvm->arch.pmcr_n). This is to ensure that the register field
> > + * remains consistent for the VM, even on heterogeneous systems where
> > + * the value may vary when read from different CPUs (during vCPU reset).
>
> Since I'm looking at this again, I don't believe the comment is adding
> much. KVM doesn't read pmcr_el0 directly anymore, and kvm_arch is
> clearly VM-scoped context.
>
> > */
> > u64 kvm_vcpu_read_pmcr(struct kvm_vcpu *vcpu)
> > {
> > - return __vcpu_sys_reg(vcpu, PMCR_EL0);
> > + u64 pmcr = __vcpu_sys_reg(vcpu, PMCR_EL0) &
> > + ~(ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
> > +
> > + return pmcr | ((u64)vcpu->kvm->arch.pmcr_n << ARMV8_PMU_PMCR_N_SHIFT);
> > }
>
>
> --
> Thanks,
> Oliver
On Tue, Oct 17, 2023 at 09:58:08AM -0700, Raghavendra Rao Ananta wrote:
> On Mon, Oct 16, 2023 at 10:52 PM Oliver Upton <[email protected]> wrote:
> >
> > On Mon, Oct 16, 2023 at 02:35:52PM -0700, Raghavendra Rao Ananta wrote:
> >
> > [...]
> >
> > > > What's the point of doing this in the first place? The implementation of
> > > > kvm_vcpu_read_pmcr() is populating PMCR_EL0.N using the VM-scoped value.
> > > >
> > > I guess originally the change replaced read_sysreg(pmcr_el0) with
> > > kvm_vcpu_read_pmcr(vcpu) to maintain consistency with others.
> > > But if you and Sebastian feel that it's an overkill and directly
> > > getting the value via vcpu->kvm->arch.pmcr_n is more readable, I'm
> > > happy to make the change.
> >
> > No, I'd rather you delete the line where PMCR_EL0.N altogether.
> > reset_pmcr() tries to initialize the field, but your
> > kvm_vcpu_read_pmcr() winds up replacing it with pmcr_n.
> >
> I didn't get this comment. We still do initialize pmcr, but using the
> pmcr.n read via kvm_vcpu_read_pmcr() instead of the actual system
> register.
You have two bits of code trying to do the exact same thing:
1) reset_pmcr() initializes __vcpu_sys_reg(vcpu, PMCR_EL0) with the N
field set up.
2) kvm_vcpu_read_pmcr() takes whatever is in __vcpu_sys_reg(vcpu, PMCR_EL0),
*masks out* the N field and re-initializes it with vcpu->kvm->arch.pmcr_n
Why do you need (1) if you do (2)?
--
Thanks,
Oliver
On Tue, Oct 17, 2023 at 10:09 AM Oliver Upton <[email protected]> wrote:
>
> On Tue, Oct 17, 2023 at 09:58:08AM -0700, Raghavendra Rao Ananta wrote:
> > On Mon, Oct 16, 2023 at 10:52 PM Oliver Upton <[email protected]> wrote:
> > >
> > > On Mon, Oct 16, 2023 at 02:35:52PM -0700, Raghavendra Rao Ananta wrote:
> > >
> > > [...]
> > >
> > > > > What's the point of doing this in the first place? The implementation of
> > > > > kvm_vcpu_read_pmcr() is populating PMCR_EL0.N using the VM-scoped value.
> > > > >
> > > > I guess originally the change replaced read_sysreg(pmcr_el0) with
> > > > kvm_vcpu_read_pmcr(vcpu) to maintain consistency with others.
> > > > But if you and Sebastian feel that it's an overkill and directly
> > > > getting the value via vcpu->kvm->arch.pmcr_n is more readable, I'm
> > > > happy to make the change.
> > >
> > > No, I'd rather you delete the line where PMCR_EL0.N altogether.
> > > reset_pmcr() tries to initialize the field, but your
> > > kvm_vcpu_read_pmcr() winds up replacing it with pmcr_n.
> > >
> > I didn't get this comment. We still do initialize pmcr, but using the
> > pmcr.n read via kvm_vcpu_read_pmcr() instead of the actual system
> > register.
>
> You have two bits of code trying to do the exact same thing:
>
> 1) reset_pmcr() initializes __vcpu_sys_reg(vcpu, PMCR_EL0) with the N
> field set up.
>
> 2) kvm_vcpu_read_pmcr() takes whatever is in __vcpu_sys_reg(vcpu, PMCR_EL0),
> *masks out* the N field and re-initializes it with vcpu->kvm->arch.pmcr_n
>
> Why do you need (1) if you do (2)?
>
Okay, I see what you mean now. In that case, let reset_pmcr():
- Initialize 'pmcr' using vcpu->kvm->arch.pmcr_n
- Set ARMV8_PMU_PMCR_LC as appropriate in 'pmcr'
- Write 'pmcr' to the vcpu reg
From here on out, kvm_vcpu_read_pmcr() would read off of this
initialized value, unless of course, userspace updates the pmcr.n.
Is this the flow that you were suggesting?
Thank you.
Raghavendra
> --
> Thanks,
> Oliver
On Tue, Oct 17, 2023 at 10:25:50AM -0700, Raghavendra Rao Ananta wrote:
> On Tue, Oct 17, 2023 at 10:09 AM Oliver Upton <[email protected]> wrote:
> >
> > On Tue, Oct 17, 2023 at 09:58:08AM -0700, Raghavendra Rao Ananta wrote:
> > > On Mon, Oct 16, 2023 at 10:52 PM Oliver Upton <[email protected]> wrote:
> > > >
> > > > On Mon, Oct 16, 2023 at 02:35:52PM -0700, Raghavendra Rao Ananta wrote:
> > > >
> > > > [...]
> > > >
> > > > > > What's the point of doing this in the first place? The implementation of
> > > > > > kvm_vcpu_read_pmcr() is populating PMCR_EL0.N using the VM-scoped value.
> > > > > >
> > > > > I guess originally the change replaced read_sysreg(pmcr_el0) with
> > > > > kvm_vcpu_read_pmcr(vcpu) to maintain consistency with others.
> > > > > But if you and Sebastian feel that it's an overkill and directly
> > > > > getting the value via vcpu->kvm->arch.pmcr_n is more readable, I'm
> > > > > happy to make the change.
> > > >
> > > > No, I'd rather you delete the line where PMCR_EL0.N altogether.
> > > > reset_pmcr() tries to initialize the field, but your
> > > > kvm_vcpu_read_pmcr() winds up replacing it with pmcr_n.
> > > >
> > > I didn't get this comment. We still do initialize pmcr, but using the
> > > pmcr.n read via kvm_vcpu_read_pmcr() instead of the actual system
> > > register.
> >
> > You have two bits of code trying to do the exact same thing:
> >
> > 1) reset_pmcr() initializes __vcpu_sys_reg(vcpu, PMCR_EL0) with the N
> > field set up.
> >
> > 2) kvm_vcpu_read_pmcr() takes whatever is in __vcpu_sys_reg(vcpu, PMCR_EL0),
> > *masks out* the N field and re-initializes it with vcpu->kvm->arch.pmcr_n
> >
> > Why do you need (1) if you do (2)?
> >
> Okay, I see what you mean now. In that case, let reset_pmcr():
> - Initialize 'pmcr' using vcpu->kvm->arch.pmcr_n
> - Set ARMV8_PMU_PMCR_LC as appropriate in 'pmcr'
> - Write 'pmcr' to the vcpu reg
>
> From here on out, kvm_vcpu_read_pmcr() would read off of this
> initialized value, unless of course, userspace updates the pmcr.n.
> Is this the flow that you were suggesting?
Just squash this in:
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index d1db1f292645..7b54c7843bef 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -743,10 +743,8 @@ static u64 reset_pmselr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
static u64 reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
{
- u64 pmcr;
+ u64 pmcr = 0;
- /* Only preserve PMCR_EL0.N, and reset the rest to 0 */
- pmcr = kvm_vcpu_read_pmcr(vcpu) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
if (!kvm_supports_32bit_el0())
pmcr |= ARMV8_PMU_PMCR_LC;
--
Thanks,
Oliver
On Tue, Oct 17, 2023 at 11:11 AM Oliver Upton <[email protected]> wrote:
>
> On Tue, Oct 17, 2023 at 10:25:50AM -0700, Raghavendra Rao Ananta wrote:
> > On Tue, Oct 17, 2023 at 10:09 AM Oliver Upton <[email protected]> wrote:
> > >
> > > On Tue, Oct 17, 2023 at 09:58:08AM -0700, Raghavendra Rao Ananta wrote:
> > > > On Mon, Oct 16, 2023 at 10:52 PM Oliver Upton <[email protected]> wrote:
> > > > >
> > > > > On Mon, Oct 16, 2023 at 02:35:52PM -0700, Raghavendra Rao Ananta wrote:
> > > > >
> > > > > [...]
> > > > >
> > > > > > > What's the point of doing this in the first place? The implementation of
> > > > > > > kvm_vcpu_read_pmcr() is populating PMCR_EL0.N using the VM-scoped value.
> > > > > > >
> > > > > > I guess originally the change replaced read_sysreg(pmcr_el0) with
> > > > > > kvm_vcpu_read_pmcr(vcpu) to maintain consistency with others.
> > > > > > But if you and Sebastian feel that it's an overkill and directly
> > > > > > getting the value via vcpu->kvm->arch.pmcr_n is more readable, I'm
> > > > > > happy to make the change.
> > > > >
> > > > > No, I'd rather you delete the line where PMCR_EL0.N altogether.
> > > > > reset_pmcr() tries to initialize the field, but your
> > > > > kvm_vcpu_read_pmcr() winds up replacing it with pmcr_n.
> > > > >
> > > > I didn't get this comment. We still do initialize pmcr, but using the
> > > > pmcr.n read via kvm_vcpu_read_pmcr() instead of the actual system
> > > > register.
> > >
> > > You have two bits of code trying to do the exact same thing:
> > >
> > > 1) reset_pmcr() initializes __vcpu_sys_reg(vcpu, PMCR_EL0) with the N
> > > field set up.
> > >
> > > 2) kvm_vcpu_read_pmcr() takes whatever is in __vcpu_sys_reg(vcpu, PMCR_EL0),
> > > *masks out* the N field and re-initializes it with vcpu->kvm->arch.pmcr_n
> > >
> > > Why do you need (1) if you do (2)?
> > >
> > Okay, I see what you mean now. In that case, let reset_pmcr():
> > - Initialize 'pmcr' using vcpu->kvm->arch.pmcr_n
> > - Set ARMV8_PMU_PMCR_LC as appropriate in 'pmcr'
> > - Write 'pmcr' to the vcpu reg
> >
> > From here on out, kvm_vcpu_read_pmcr() would read off of this
> > initialized value, unless of course, userspace updates the pmcr.n.
> > Is this the flow that you were suggesting?
>
> Just squash this in:
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index d1db1f292645..7b54c7843bef 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -743,10 +743,8 @@ static u64 reset_pmselr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>
> static u64 reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> {
> - u64 pmcr;
> + u64 pmcr = 0;
>
> - /* Only preserve PMCR_EL0.N, and reset the rest to 0 */
> - pmcr = kvm_vcpu_read_pmcr(vcpu) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
> if (!kvm_supports_32bit_el0())
> pmcr |= ARMV8_PMU_PMCR_LC;
>
>
Oh, I get the redundancy that you were suggesting to get rid of!
Thanks for the diff. It helped.
- Raghavendra
> --
> Thanks,
> Oliver