Hi Reiji,
On 2022-10-27 15:33, Reiji Watanabe wrote:
Hi Marc,
> > +static void kvm_pmu_counter_increment(struct kvm_vcpu *vcpu,
> > + unsigned long mask, u32 event)
> > +{
> > + int i;
> > +
> > + if (!kvm_vcpu_has_pmu(vcpu))
> > + return;
> > +
> > + if (!(__vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_E))
> > + return;
> > +
> > + /* Weed out disabled counters */
> > + mask &= __vcpu_sys_reg(vcpu, PMCNTENSET_EL0);
> > +
> > + for_each_set_bit(i, &mask, ARMV8_PMU_CYCLE_IDX) {
> > + u64 type, reg;
> > +
> > + /* Filter on event type */
> > + type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i);
> > + type &= kvm_pmu_event_mask(vcpu->kvm);
> > + if (type != event)
> > + continue;
> > +
> > + /* Increment this counter */
> > + reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1;
> > + reg = lower_32_bits(reg);
> > + __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = reg;
> > +
> > + if (reg) /* No overflow? move on */
> > + continue;
> > +
> > + /* Mark overflow */
> > + __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i);
>
> Perhaps it might be useful to create another helper that takes
> care of just one counter (it would essentially do the code above
> in the loop). The helper could be used (in addition to the above
> loop) from the code below for the CHAIN event case and from
> kvm_pmu_perf_overflow(). Then unnecessary execution of
> for_each_set_bit() could be avoided for these two cases.
I'm not sure it really helps. We would still need to check whether the
counter is enabled, and we'd need to bring that into the helper
instead of keeping it outside of the loop.
That's true. It seems that I overlooked that.
Although it appears checking with kvm_vcpu_has_pmu() is unnecessary
(redundant), the check with PMCR_EL0.E is necessary.
Ah indeed, I'll drop the kvm_vcpu_has_pmu() check.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
