On Mon, Jun 10, 2019 at 01:54:35PM +0100, Suzuki K Poulose wrote:
>
>
> On 22/05/2019 17:26, Andrew Murray wrote:
> > On Wed, May 22, 2019 at 05:07:31PM +0100, Marc Zyngier wrote:
> > > On 22/05/2019 16:30, Andrew Murray wrote:
> > > > We currently use pmc->bitmask to determine the width of the pmc - however
> > > > it's superfluous as the pmc index already describes if the pmc is a cycle
> > > > counter or event counter. The architecture clearly describes the widths of
> > > > these counters.
> > > >
> > > > Let's remove the bitmask to simplify the code.
> > > >
> > > > Signed-off-by: Andrew Murray <andrew.murray@xxxxxxx>
> > > > ---
> > > > include/kvm/arm_pmu.h | 1 -
> > > > virt/kvm/arm/pmu.c | 15 +++++----------
> > > > 2 files changed, 5 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> > > > index b73f31baca52..2f0e28dc5a9e 100644
> > > > --- a/include/kvm/arm_pmu.h
> > > > +++ b/include/kvm/arm_pmu.h
> > > > @@ -28,7 +28,6 @@
> > > > struct kvm_pmc {
> > > > u8 idx; /* index into the pmu->pmc array */
> > > > struct perf_event *perf_event;
> > > > - u64 bitmask;
> > > > };
>
>
> > > > -
> > > > - if (val & ARMV8_PMU_PMCR_LC) {
> > > > - pmc = &pmu->pmc[ARMV8_PMU_CYCLE_IDX];
> > > > - pmc->bitmask = 0xffffffffffffffffUL;
> > > > - }
> > > > }
>
> ...
>
> > > > static bool kvm_pmu_counter_is_enabled(struct kvm_vcpu *vcpu, u64 select_idx)
> > > > @@ -420,7 +415,7 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
> > > > counter = kvm_pmu_get_counter_value(vcpu, select_idx);
> > > > /* The initial sample period (overflow count) of an event. */
> > > > - attr.sample_period = (-counter) & pmc->bitmask;
> > > > + attr.sample_period = (-counter) & GENMASK(31, 0);
> > >
> > > Isn't this the one case where the bitmask actually matters? If we're
> > > dealing with the cycle counter, it shouldn't be truncated, right?
> >
> > Ah yes, that should be conditional on idx as well.
>
> The mask for Cycle counter also depends on the PMCR.LC field set by the
> guest, isn't it ? So unless we correlate that with the idx, we could be
> passing in wrong results ?
Yes that's right, I misread the description of LC - it's RES1 for AArch64
but of course this doesn't stop a 32 bit host kernel. I'll update this.
Thanks,
Andrew Murray
>
> Suzuki
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
