On Thu, Feb 14, 2019 at 11:42:15AM +0000, Suzuki K Poulose wrote:
>
>
> On 05/02/2019 14:33, Julien Thierry wrote:
> > Hi Andrew,
> >
> > On 04/02/2019 16:53, Andrew Murray wrote:
> > > Emulate chained PMU counters by creating a single 64 bit event counter
> > > for a pair of chained KVM counters.
> > >
> > > Signed-off-by: Andrew Murray <andrew.murray at arm.com>
> > > ---
> > > include/kvm/arm_pmu.h | 1 +
> > > virt/kvm/arm/pmu.c | 321 +++++++++++++++++++++++++++++++++++++++++---------
> > > 2 files changed, 269 insertions(+), 53 deletions(-)
> > >
> > > diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> > > index b73f31b..8e691ee 100644
> > > --- a/include/kvm/arm_pmu.h
> > > +++ b/include/kvm/arm_pmu.h
> > > @@ -29,6 +29,7 @@ struct kvm_pmc {
> > > u8 idx; /* index into the pmu->pmc array */
> > > struct perf_event *perf_event;
> > > u64 bitmask;
> > > + u64 overflow_count;
> > > };
> > > struct kvm_pmu {
> > > diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> > > index a64aeb2..9318130 100644
> > > --- a/virt/kvm/arm/pmu.c
> > > +++ b/virt/kvm/arm/pmu.c
> > > @@ -24,9 +24,25 @@
> > > #include <kvm/arm_pmu.h>
> > > #include <kvm/arm_vgic.h>
> > > +#define ARMV8_PMUV3_PERFCTR_CHAIN 0x1E
> >
> > I find it a bit awkward to have this redefined here.
> >
> > Maybe we could define a helper in kvm_host.h:
> > bool kvm_pmu_typer_is_chain(u64 typer);
> >
> > That would always return false for arm32?
>
> We don't support ARMv7 host, so that doesn't matter. But
> it is a good idea to wrap it in a function here.
I'm not sure kvm_host.h is the right place for this as this really relates
to the ARM PMU drivers. Seeing that armv8pmu_filter_match in
arch/arm64/kernel/perf_event.c would also benefit from this new function I'll
add it to arch/arm64/include/asm/perf_event.h and stub it out in arm_pmu.c
for the ARM32 case.
>
> > > kvm_vcpu_kick(vcpu);
> > > @@ -440,6 +632,10 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
> > > select_idx != ARMV8_PMU_CYCLE_IDX)
> > > return;
> > > + /* Handled by even event */
> > > + if (eventsel == ARMV8_PMUV3_PERFCTR_CHAIN)
> > > + return;
> > > +
> > > memset(&attr, 0, sizeof(struct perf_event_attr));
> > > attr.type = PERF_TYPE_RAW;
> > > attr.size = sizeof(attr);
> > > @@ -452,6 +648,9 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
> > > attr.config = (select_idx == ARMV8_PMU_CYCLE_IDX) ?
> > > ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
> > > + if (kvm_pmu_event_is_chained(vcpu, select_idx))
> > > + attr.config1 |= 0x1;
> >
> > I'm not very familiar with the usage of perf attributes configs, but is
> > there any chance we could name this flag? Even if only for the local
> > file? Something like PERF_ATTR_CFG1_KVM_PMU_CHAINED (unless there is an
> > existing naming convention for event attributes).
>
> There is ARPMU_EVT_64BIT in "linux/perf/arm_pmu.h". But in general we don't
> specify where the Bit goes in (i.e, CFG1 etc).
I'll add PERF_ATTR_CFG1_KVM_PMU_CHAINED to this file as you suggest.
Thanks,
Andrew Murray
>
> Cheers
> Suzuki
