Re: [PATCH v3 2/4] KVM: arm64: Keep a list of probed PMUs

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Marc,

Sorry for the long silence, I didn't manage to get to your comments before
going on holiday.

On Tue, Dec 14, 2021 at 12:30:30PM +0000, Marc Zyngier wrote:
> On Mon, 13 Dec 2021 15:23:07 +0000,
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> > 
> > The ARM PMU driver calls kvm_host_pmu_init() after probing to tell KVM that
> > a hardware PMU is available for guest emulation. Heterogeneous systems can
> > have more than one PMU present, and the callback gets called multiple
> > times, once for each of them. Keep track of all the PMUs available to KVM,
> > as they're going to be needed later.
> > 
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> >  arch/arm64/kvm/pmu-emul.c | 25 +++++++++++++++++++++++--
> >  include/kvm/arm_pmu.h     |  5 +++++
> >  2 files changed, 28 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > index a5e4bbf5e68f..eb4be96f144d 100644
> > --- a/arch/arm64/kvm/pmu-emul.c
> > +++ b/arch/arm64/kvm/pmu-emul.c
> > @@ -7,6 +7,7 @@
> >  #include <linux/cpu.h>
> >  #include <linux/kvm.h>
> >  #include <linux/kvm_host.h>
> > +#include <linux/list.h>
> >  #include <linux/perf_event.h>
> >  #include <linux/perf/arm_pmu.h>
> >  #include <linux/uaccess.h>
> > @@ -14,6 +15,9 @@
> >  #include <kvm/arm_pmu.h>
> >  #include <kvm/arm_vgic.h>
> >  
> > +static LIST_HEAD(arm_pmus);
> > +static DEFINE_MUTEX(arm_pmus_lock);
> > +
> >  static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx);
> >  static void kvm_pmu_update_pmc_chained(struct kvm_vcpu *vcpu, u64 select_idx);
> >  static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc);
> > @@ -742,9 +746,26 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
> >  
> >  void kvm_host_pmu_init(struct arm_pmu *pmu)
> >  {
> > -	if (pmu->pmuver != 0 && pmu->pmuver != ID_AA64DFR0_PMUVER_IMP_DEF &&
> > -	    !kvm_arm_support_pmu_v3() && !is_protected_kvm_enabled())
> > +	struct arm_pmu_entry *entry;
> > +
> > +	if (pmu->pmuver == 0 || pmu->pmuver == ID_AA64DFR0_PMUVER_IMP_DEF ||
> > +	    is_protected_kvm_enabled())
> > +		return;
> > +
> > +	mutex_lock(&arm_pmus_lock);
> > +
> > +	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> > +	if (!entry)
> > +		goto out_unlock;
> > +
> > +	if (list_empty(&arm_pmus))
> >  		static_branch_enable(&kvm_arm_pmu_available);
> 
> I find it slightly dodgy that you switch the static key before
> actually populating the entry. I'd suggest moving it after the
> list_add_tail(), and check on list_is_singular() instead.

That's better, will do.

Thanks,
Alex

> 
> > +
> > +	entry->arm_pmu = pmu;
> > +	list_add_tail(&entry->entry, &arm_pmus);
> > +
> > +out_unlock:
> > +	mutex_unlock(&arm_pmus_lock);
> >  }
> >  
> >  static int kvm_pmu_probe_pmuver(void)
> > diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> > index 90f21898aad8..e249c5f172aa 100644
> > --- a/include/kvm/arm_pmu.h
> > +++ b/include/kvm/arm_pmu.h
> > @@ -36,6 +36,11 @@ struct kvm_pmu {
> >  	struct irq_work overflow_work;
> >  };
> >  
> > +struct arm_pmu_entry {
> > +	struct list_head entry;
> > +	struct arm_pmu *arm_pmu;
> > +};
> > +
> >  #define kvm_arm_pmu_irq_initialized(v)	((v)->arch.pmu.irq_num >= VGIC_NR_SGIS)
> >  u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx);
> >  void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val);
> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm