Hi Gautam,
Thanks for reviewing this patch. My responses to your review comments
inline below:
Gautam Menghani <gautam@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> ---
>> 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
