Re: [PATCH 4/6] kvm powerpc/book3s-apiv2: Introduce kvm-hv specific PMU

[Vaibhav Jain <vaibhav@linux.ibm.com>]

Hi Gautam,

Thanks for reviewing this patch. My responses to your review comments
inline below:

Gautam Menghani <gautam@linux.ibm.com> writes:

> On Sun, Dec 22, 2024 at 07:32:32PM +0530, Vaibhav Jain wrote:
>> Introduce a new PMU named 'kvm-hv' to report Book3s kvm-hv specific
>> performance counters. This will expose KVM-HV specific performance
>> attributes to user-space via kernel's PMU infrastructure and would enable
>> users to monitor active kvm-hv based guests.
>> 
>> The patch creates necessary scaffolding to for the new PMU callbacks and
>> introduces two new exports kvmppc_{,un}register_pmu() that are called from
>> kvm-hv init and exit function to perform initialize and cleanup for the
>> 'kvm-hv' PMU. The patch doesn't introduce any perf-events yet, which will
>> be introduced in later patches
>> 
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>>  arch/powerpc/include/asm/kvm_book3s.h |  12 +++
>>  arch/powerpc/kvm/Makefile             |   6 ++
>>  arch/powerpc/kvm/book3s_hv.c          |   7 ++
>>  arch/powerpc/kvm/book3s_hv_pmu.c      | 133 ++++++++++++++++++++++++++
>>  4 files changed, 158 insertions(+)
>>  create mode 100644 arch/powerpc/kvm/book3s_hv_pmu.c
>> 
>> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
>> index e1ff291ba891..cf91a1493159 100644
>> --- a/arch/powerpc/include/asm/kvm_book3s.h
>> +++ b/arch/powerpc/include/asm/kvm_book3s.h
>> @@ -334,6 +334,9 @@ static inline bool kvmhv_is_nestedv1(void)
>>  	return !static_branch_likely(&__kvmhv_is_nestedv2);
>>  }
>>  
>> +int kvmppc_register_pmu(void);
>> +void kvmppc_unregister_pmu(void);
>> +
>>  #else
>>  
>>  static inline bool kvmhv_is_nestedv2(void)
>> @@ -346,6 +349,15 @@ static inline bool kvmhv_is_nestedv1(void)
>>  	return false;
>>  }
>>  
>> +static int kvmppc_register_pmu(void)
>> +{
>> +	return 0;
>> +}
>> +
>> +static void kvmppc_unregister_pmu(void)
>> +{
>> +}
>> +
>>  #endif
>>  
>>  int __kvmhv_nestedv2_reload_ptregs(struct kvm_vcpu *vcpu, struct pt_regs *regs);
>> diff --git a/arch/powerpc/kvm/Makefile b/arch/powerpc/kvm/Makefile
>> index 4bd9d1230869..094c3916d9d0 100644
>> --- a/arch/powerpc/kvm/Makefile
>> +++ b/arch/powerpc/kvm/Makefile
>> @@ -92,6 +92,12 @@ kvm-book3s_64-builtin-objs-$(CONFIG_KVM_BOOK3S_64_HANDLER) += \
>>  	$(kvm-book3s_64-builtin-tm-objs-y) \
>>  	$(kvm-book3s_64-builtin-xics-objs-y)
>>  
>> +# enable kvm_hv perf events
>> +ifdef CONFIG_HAVE_PERF_EVENTS
>> +kvm-book3s_64-builtin-objs-$(CONFIG_KVM_BOOK3S_64_HANDLER) += \
>> +	book3s_hv_pmu.o
>> +endif
>> +
>>  obj-$(CONFIG_GUEST_STATE_BUFFER_TEST) += test-guest-state-buffer.o
>>  endif
>>  
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index 25429905ae90..83bcce2fb557 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -6662,6 +6662,12 @@ static int kvmppc_book3s_init_hv(void)
>>  		return r;
>>  	}
>>  
>> +	r = kvmppc_register_pmu();
>> +	if (r) {
>> +		pr_err("KVM-HV: Unable to register PMUs %d\n", r);
>> +		goto err;
>> +	}
>> +
>>  	kvm_ops_hv.owner = THIS_MODULE;
>>  	kvmppc_hv_ops = &kvm_ops_hv;
>>  
>> @@ -6676,6 +6682,7 @@ static int kvmppc_book3s_init_hv(void)
>>  
>>  static void kvmppc_book3s_exit_hv(void)
>>  {
>> +	kvmppc_unregister_pmu();
>>  	kvmppc_uvmem_free();
>>  	kvmppc_free_host_rm_ops();
>>  	if (kvmppc_radix_possible())
>> diff --git a/arch/powerpc/kvm/book3s_hv_pmu.c b/arch/powerpc/kvm/book3s_hv_pmu.c
>> new file mode 100644
>> index 000000000000..e72542d5e750
>> --- /dev/null
>> +++ b/arch/powerpc/kvm/book3s_hv_pmu.c
>> @@ -0,0 +1,133 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Description: PMUs specific to running nested KVM-HV guests
>> + * on Book3S processors (specifically POWER9 and later).
>> + */
>> +
>> +#define pr_fmt(fmt)  "kvmppc-pmu: " fmt
>> +
>> +#include "asm-generic/local64.h"
>> +#include <linux/kernel.h>
>> +#include <linux/errno.h>
>> +#include <linux/ratelimit.h>
>> +#include <linux/kvm_host.h>
>> +#include <linux/gfp_types.h>
>> +#include <linux/pgtable.h>
>> +#include <linux/perf_event.h>
>> +#include <linux/spinlock_types.h>
>> +#include <linux/spinlock.h>
>> +
>> +#include <asm/types.h>
>> +#include <asm/kvm_ppc.h>
>> +#include <asm/kvm_book3s.h>
>> +#include <asm/mmu.h>
>> +#include <asm/pgalloc.h>
>> +#include <asm/pte-walk.h>
>> +#include <asm/reg.h>
>> +#include <asm/plpar_wrappers.h>
>> +#include <asm/firmware.h>
>> +
>> +enum kvmppc_pmu_eventid {
>> +	KVMPPC_EVENT_MAX,
>> +};
>> +
>> +static struct attribute *kvmppc_pmu_events_attr[] = {
>> +	NULL,
>> +};
>> +
>> +static const struct attribute_group kvmppc_pmu_events_group = {
>> +	.name = "events",
>> +	.attrs = kvmppc_pmu_events_attr,
>> +};
>> +
>> +PMU_FORMAT_ATTR(event, "config:0");
>> +static struct attribute *kvmppc_pmu_format_attr[] = {
>> +	&format_attr_event.attr,
>> +	NULL,
>> +};
>> +
>> +static struct attribute_group kvmppc_pmu_format_group = {
>> +	.name = "format",
>> +	.attrs = kvmppc_pmu_format_attr,
>> +};
>> +
>> +static const struct attribute_group *kvmppc_pmu_attr_groups[] = {
>> +	&kvmppc_pmu_events_group,
>> +	&kvmppc_pmu_format_group,
>> +	NULL,
>> +};
>> +
>> +static int kvmppc_pmu_event_init(struct perf_event *event)
>> +{
>> +	unsigned int config = event->attr.config;
>> +
>> +	pr_debug("%s: Event(%p) id=%llu cpu=%x on_cpu=%x config=%u",
>> +		 __func__, event, event->id, event->cpu,
>> +		 event->oncpu, config);
>> +
>> +	if (event->attr.type != event->pmu->type)
>> +		return -ENOENT;
>> +
>> +	if (config >= KVMPPC_EVENT_MAX)
>> +		return -EINVAL;
>> +
>> +	local64_set(&event->hw.prev_count, 0);
>> +	local64_set(&event->count, 0);
>> +
>> +	return 0;
>> +}
>> +
>> +static void kvmppc_pmu_del(struct perf_event *event, int flags)
>> +{
>> +}
>> +
>> +static int kvmppc_pmu_add(struct perf_event *event, int flags)
>> +{
>> +	return 0;
>> +}
>> +
>> +static void kvmppc_pmu_read(struct perf_event *event)
>> +{
>> +}
>> +
>> +/* L1 wide counters PMU */
>> +static struct pmu kvmppc_pmu = {
>> +	.task_ctx_nr = perf_sw_context,
>> +	.name = "kvm-hv",
>> +	.event_init = kvmppc_pmu_event_init,
>> +	.add = kvmppc_pmu_add,
>> +	.del = kvmppc_pmu_del,
>> +	.read = kvmppc_pmu_read,
>> +	.attr_groups = kvmppc_pmu_attr_groups,
>> +	.type = -1,
>> +};
>> +
>> +int kvmppc_register_pmu(void)
>> +{
>> +	int rc = -EOPNOTSUPP;
>> +
>> +	/* only support events for nestedv2 right now */
>> +	if (kvmhv_is_nestedv2()) {
>> +		/* Setup done now register the PMU */
>> +		pr_info("Registering kvm-hv pmu");
>> +
>> +		/* Register only if we arent already registyered */
>> +		rc = (kvmppc_pmu.type == -1) ?
>> +			     perf_pmu_register(&kvmppc_pmu, kvmppc_pmu.name,
>> +					       -1) : 0;
>> +	}
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_GPL(kvmppc_register_pmu);
>
> This logic breaks on bare metal. kvmppc_register_pmu() returns
> -EOPNOTSUPP because of which kvm_hv cannot be loaded.
>
Good catch. Fixing this in v2

>> +
>> +void kvmppc_unregister_pmu(void)
>> +{
>> +	if (kvmhv_is_nestedv2()) {
>> +		if (kvmppc_pmu.type != -1)
>> +			perf_pmu_unregister(&kvmppc_pmu);
>> +
>> +		pr_info("kvmhv_pmu unregistered.\n");
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(kvmppc_unregister_pmu);
>> -- 
>> 2.47.1
>> 

-- 
Cheers
~ Vaibhav