qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [RFC v2 0/5] accel/kvm: Support KVM PMU filter
@ 2025-01-22  9:05 Zhao Liu
  2025-01-22  9:05 ` [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object Zhao Liu
                   ` (6 more replies)
  0 siblings, 7 replies; 27+ messages in thread
From: Zhao Liu @ 2025-01-22  9:05 UTC (permalink / raw)
  To: Paolo Bonzini, Eric Blake, Markus Armbruster, Michael Roth,
	Daniel P . Berrangé, Eduardo Habkost, Marcelo Tosatti,
	Shaoqin Huang, Eric Auger, Peter Maydell, Laurent Vivier,
	Thomas Huth, Sebastian Ott, Gavin Shan
  Cc: qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai, Zhao Liu

Hi folks,

Sorry for the long wait, but RFC v2 is here at last.

Compared with v1 [1], v2 mianly makes `action` as a global parameter,
and all events (and fixed counters) are based on a unified action.

Learned from the discussion with Shaoqin in v1, current pmu-filter QOM
design could meet the requirements from the ARM KVM side.


Background
==========

I picked up Shaoqing's previous work [2] on the KVM PMU filter for arm,
and now is trying to support this feature for x86 with a JSON-compatible
API.

While arm and x86 use different KVM ioctls to configure the PMU filter,
considering they all have similar inputs (PMU event + action), it is
still possible to abstract a generic, cross-architecture kvm-pmu-filter
object and provide users with a sufficiently generic or near-consistent
QAPI interface.

That's what I did in this series, a new kvm-pmu-filter object, with the
API like:

-object '{"qom-type":"kvm-pmu-filter","id":"f0","action":"allow","events":[{"format":"raw","code":"0xc4"}]}'

For i386, this object is inserted into kvm accelerator and is extended
to support fixed-counter and more formats ("x86-default" and
"x86-masked-entry"):

-accel kvm,pmu-filter=f0 \
-object pmu='{"qom-type":"kvm-pmu-filter","id":"f0","action":"allow","x86-fixed-counter":{"bitmap":"0x0"},"events":[{"format":"x86-masked-entry","select":"0xc4","mask":"0xff","match":"0","exclude":true},{"format":"x86-masked-entry","select":"0xc5","mask":"0xff","match":"0","exclude":true}]}'

This object can still be added as the property to the arch CPU if it is
desired as a per CPU feature (as Shaoqin did for arm before).


Introduction
============


Formats supported in kvm-pmu-filter
-----------------------------------

This series supports 3 formats:

* raw format (general format).

  This format indicates the code that has been encoded to be able to
  index the PMU events, and which can be delivered directly to the KVM
  ioctl. For arm, this means the event code, and for i386, this means
  the raw event with the layout like:

      select high bit | umask | select low bits

* x86-default format (i386 specific)

  x86 commonly uses select&umask to identify PMU events, and this format
  is used to support the select&umask. Then QEMU will encode select and
  umask into a raw format code.

* x86-masked-entry (i386 specific)

  This is a special format that x86's KVM_SET_PMU_EVENT_FILTER supports.


Hexadecimal value string
------------------------

In practice, the values associated with PMU events (code for arm, select&
umask for x86) are often expressed in hexadecimal. Further, from linux
perf related information (tools/perf/pmu-events/arch/*/*/*.json), x86/
arm64/riscv/nds32/powerpc all prefer the hexadecimal numbers and only
s390 uses decimal value.

Therefore, it is necessary to support hexadecimal in order to honor PMU
conventions.

However, unfortunately, standard JSON (RFC 8259) does not support
hexadecimal numbers. So I can only consider using the numeric string in
the QAPI and then parsing it to a number.

To achieve this, I defined two versions of PMU-related structures in
kvm.json:
 * a native version that accepts numeric values, which is used for
   QEMU's internal code processing,

 * and a variant version that accepts numeric string, which is used to
   receive user input.

kvm-pmu-filter object will take care of converting the string version
of the event/counter information into the numeric version.

The related implementation can be found in patch 1.


CPU property v.s. KVM property
------------------------------

In Shaoqin's previous implementation [2], KVM PMU filter is made as a
arm CPU property. This is because arm uses a per CPU ioctl
(KVM_SET_DEVICE_ATTR) to configure KVM PMU filter.

However, for x86, the dependent ioctl (KVM_SET_PMU_EVENT_FILTER) is per
VM. In the meantime, considering that for hybrid architecture, maybe in
the future there will be a new per vCPU ioctl, or there will be
practices to support filter fixed counter by configuring CPUIDs.

Based on the above thoughts, for x86, it is not appropriate to make the
current per-VM ioctl-based PMU filter a CPU property. Instead, I make it
a kvm property and configure it via "-accel kvm,pmu-filter=obj_id".

So in summary, it is feasible to use the KVM PMU filter as either a CPU
or a KVM property, depending on whether it is used as a CPU feature or a
VM feature.

The kvm-pmu-filter object, as an abstraction, is general enough to
support filter configurations for different scopes (per-CPU or per-VM).

[1]: https://lore.kernel.org/qemu-devel/20240710045117.3164577-1-zhao1.liu@intel.com/
[2]: https://lore.kernel.org/qemu-devel/20240409024940.180107-1-shahuang@redhat.com/

Thanks and Best Regards,
Zhao
---
Zhao Liu (5):
  qapi/qom: Introduce kvm-pmu-filter object
  i386/kvm: Support basic KVM PMU filter
  i386/kvm: Support event with select & umask format in KVM PMU filter
  i386/kvm: Support event with masked entry format in KVM PMU filter
  i386/kvm: Support fixed counter in KVM PMU filter

 MAINTAINERS              |   1 +
 accel/kvm/kvm-pmu.c      | 386 +++++++++++++++++++++++++++++++++++++++
 accel/kvm/meson.build    |   1 +
 include/system/kvm-pmu.h |  44 +++++
 include/system/kvm_int.h |   2 +
 qapi/kvm.json            | 246 +++++++++++++++++++++++++
 qapi/meson.build         |   1 +
 qapi/qapi-schema.json    |   1 +
 qapi/qom.json            |   3 +
 target/i386/kvm/kvm.c    | 176 ++++++++++++++++++
 10 files changed, 861 insertions(+)
 create mode 100644 accel/kvm/kvm-pmu.c
 create mode 100644 include/system/kvm-pmu.h
 create mode 100644 qapi/kvm.json

-- 
2.34.1



^ permalink raw reply	[flat|nested] 27+ messages in thread

* [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object
  2025-01-22  9:05 [RFC v2 0/5] accel/kvm: Support KVM PMU filter Zhao Liu
@ 2025-01-22  9:05 ` Zhao Liu
  2025-02-05 10:03   ` Markus Armbruster
  2025-01-22  9:05 ` [RFC v2 2/5] i386/kvm: Support basic KVM PMU filter Zhao Liu
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 27+ messages in thread
From: Zhao Liu @ 2025-01-22  9:05 UTC (permalink / raw)
  To: Paolo Bonzini, Eric Blake, Markus Armbruster, Michael Roth,
	Daniel P . Berrangé, Eduardo Habkost, Marcelo Tosatti,
	Shaoqin Huang, Eric Auger, Peter Maydell, Laurent Vivier,
	Thomas Huth, Sebastian Ott, Gavin Shan
  Cc: qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai, Zhao Liu

Introduce the kvm-pmu-filter object and support the PMU event with raw
format.

The raw format, as a native PMU event code representation, can be used
for several architectures.

Considering that PMU event related fields are commonly used in
hexadecimal, define KVMPMURawEventVariant, KVMPMUFilterEventVariant, and
KVMPMUFilterPropertyVariant in kvm.json to support hexadecimal number
strings in JSON.

Additionally, define the corresponding numeric versions of
KVMPMURawEvent, KVMPMUFilterEvent, and KVMPMUFilterProperty in kvm.json.
This allows to handle numeric values more effectively and take advantage
of the qapi helpers.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC v1:
 * Make "action" as a global (per filter object) item, not a per-event
   parameter. (Dapeng)
 * Bump up the supported QAPI version to 10.0.
---
 MAINTAINERS              |   1 +
 accel/kvm/kvm-pmu.c      | 164 +++++++++++++++++++++++++++++++++++++++
 accel/kvm/meson.build    |   1 +
 include/system/kvm-pmu.h |  30 +++++++
 qapi/kvm.json            | 116 +++++++++++++++++++++++++++
 qapi/meson.build         |   1 +
 qapi/qapi-schema.json    |   1 +
 qapi/qom.json            |   3 +
 8 files changed, 317 insertions(+)
 create mode 100644 accel/kvm/kvm-pmu.c
 create mode 100644 include/system/kvm-pmu.h
 create mode 100644 qapi/kvm.json

diff --git a/MAINTAINERS b/MAINTAINERS
index 846b81e3ec03..21adc1c10607 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -440,6 +440,7 @@ F: accel/kvm/
 F: accel/stubs/kvm-stub.c
 F: include/hw/kvm/
 F: include/system/kvm*.h
+F: qapi/kvm.json
 F: scripts/kvm/kvm_flightrecorder
 
 ARM KVM CPUs
diff --git a/accel/kvm/kvm-pmu.c b/accel/kvm/kvm-pmu.c
new file mode 100644
index 000000000000..4d0d4e7a452b
--- /dev/null
+++ b/accel/kvm/kvm-pmu.c
@@ -0,0 +1,164 @@
+/*
+ * QEMU KVM PMU Abstractions
+ *
+ * Copyright (C) 2024 Intel Corporation.
+ *
+ * Author: Zhao Liu <zhao1.liu@intel.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later.  See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include "qapi/error.h"
+#include "qapi/qapi-visit-kvm.h"
+#include "qemu/cutils.h"
+#include "qom/object_interfaces.h"
+#include "system/kvm-pmu.h"
+
+static void kvm_pmu_filter_set_action(Object *obj, int value,
+                                      Error **errp G_GNUC_UNUSED)
+{
+    KVMPMUFilter *filter = KVM_PMU_FILTER(obj);
+
+    filter->action = value;
+}
+
+
+static int kvm_pmu_filter_get_action(Object *obj,
+                                     Error **errp G_GNUC_UNUSED)
+{
+    KVMPMUFilter *filter = KVM_PMU_FILTER(obj);
+
+    return filter->action;
+}
+
+static void kvm_pmu_filter_get_event(Object *obj, Visitor *v, const char *name,
+                                     void *opaque, Error **errp)
+{
+    KVMPMUFilter *filter = KVM_PMU_FILTER(obj);
+    KVMPMUFilterEventList *node;
+    KVMPMUFilterEventVariantList *head = NULL;
+    KVMPMUFilterEventVariantList **tail = &head;
+
+    for (node = filter->events; node; node = node->next) {
+        KVMPMUFilterEventVariant *str_event;
+        KVMPMUFilterEvent *event = node->value;
+
+        str_event = g_new(KVMPMUFilterEventVariant, 1);
+        str_event->format = event->format;
+
+        switch (event->format) {
+        case KVM_PMU_EVENT_FMT_RAW:
+            str_event->u.raw.code = g_strdup_printf("0x%lx",
+                                                    event->u.raw.code);
+            break;
+        default:
+            g_assert_not_reached();
+        }
+
+        QAPI_LIST_APPEND(tail, str_event);
+    }
+
+    visit_type_KVMPMUFilterEventVariantList(v, name, &head, errp);
+    qapi_free_KVMPMUFilterEventVariantList(head);
+}
+
+static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
+                                     void *opaque, Error **errp)
+{
+    KVMPMUFilter *filter = KVM_PMU_FILTER(obj);
+    KVMPMUFilterEventVariantList *list, *node;
+    KVMPMUFilterEventList *head = NULL, *old_head;
+    KVMPMUFilterEventList **tail = &head;
+    int ret, nevents = 0;
+
+    if (!visit_type_KVMPMUFilterEventVariantList(v, name, &list, errp)) {
+        return;
+    }
+
+    for (node = list; node; node = node->next) {
+        KVMPMUFilterEvent *event = g_new(KVMPMUFilterEvent, 1);
+        KVMPMUFilterEventVariant *str_event = node->value;
+
+        event->format = str_event->format;
+
+        switch (str_event->format) {
+        case KVM_PMU_EVENT_FMT_RAW:
+            ret = qemu_strtou64(str_event->u.raw.code, NULL,
+                                0, &event->u.raw.code);
+            if (ret < 0) {
+                error_setg(errp,
+                           "Invalid %s PMU event (code: %s): %s. "
+                           "The code must be a uint64 string.",
+                           KVMPMUEventEncodeFmt_str(str_event->format),
+                           str_event->u.raw.code, strerror(-ret));
+                g_free(event);
+                goto fail;
+            }
+            break;
+        default:
+            g_assert_not_reached();
+        }
+
+        nevents++;
+        QAPI_LIST_APPEND(tail, event);
+    }
+
+    old_head = filter->events;
+    filter->events = head;
+    filter->nevents = nevents;
+
+    qapi_free_KVMPMUFilterEventVariantList(list);
+    qapi_free_KVMPMUFilterEventList(old_head);
+    return;
+
+fail:
+    qapi_free_KVMPMUFilterEventList(head);
+}
+
+static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
+{
+    object_class_property_add_enum(oc, "action", "KVMPMUFilterAction",
+                                   &KVMPMUFilterAction_lookup,
+                                   kvm_pmu_filter_get_action,
+                                   kvm_pmu_filter_set_action);
+    object_class_property_set_description(oc, "action",
+                                          "KVM PMU event action");
+
+    object_class_property_add(oc, "events", "KVMPMUFilterEvent",
+                              kvm_pmu_filter_get_event,
+                              kvm_pmu_filter_set_event,
+                              NULL, NULL);
+    object_class_property_set_description(oc, "events",
+                                          "KVM PMU event list");
+}
+
+static void kvm_pmu_filter_instance_init(Object *obj)
+{
+    KVMPMUFilter *filter = KVM_PMU_FILTER(obj);
+
+    /* Initial state, 0 events allowed. */
+    filter->action = KVM_PMU_FILTER_ACTION_ALLOW;
+    filter->nevents = 0;
+}
+
+static const TypeInfo kvm_pmu_filter_info = {
+    .parent = TYPE_OBJECT,
+    .name = TYPE_KVM_PMU_FILTER,
+    .class_init = kvm_pmu_filter_class_init,
+    .instance_size = sizeof(KVMPMUFilter),
+    .instance_init = kvm_pmu_filter_instance_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_USER_CREATABLE },
+        { }
+    }
+};
+
+static void kvm_pmu_event_register_type(void)
+{
+    type_register_static(&kvm_pmu_filter_info);
+}
+
+type_init(kvm_pmu_event_register_type);
diff --git a/accel/kvm/meson.build b/accel/kvm/meson.build
index 397a1fe1fd1e..dfab2854f3a8 100644
--- a/accel/kvm/meson.build
+++ b/accel/kvm/meson.build
@@ -2,6 +2,7 @@ kvm_ss = ss.source_set()
 kvm_ss.add(files(
   'kvm-all.c',
   'kvm-accel-ops.c',
+  'kvm-pmu.c',
 ))
 
 specific_ss.add_all(when: 'CONFIG_KVM', if_true: kvm_ss)
diff --git a/include/system/kvm-pmu.h b/include/system/kvm-pmu.h
new file mode 100644
index 000000000000..b09f70d3a370
--- /dev/null
+++ b/include/system/kvm-pmu.h
@@ -0,0 +1,30 @@
+/*
+ * QEMU KVM PMU Abstraction Header
+ *
+ * Copyright (C) 2024 Intel Corporation.
+ *
+ * Authors:
+ *  Zhao Liu <zhao1.liu@intel.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later.  See the COPYING file in the top-level directory.
+ */
+
+#ifndef KVM_PMU_H
+#define KVM_PMU_H
+
+#include "qapi/qapi-types-kvm.h"
+#include "qom/object.h"
+
+#define TYPE_KVM_PMU_FILTER "kvm-pmu-filter"
+OBJECT_DECLARE_SIMPLE_TYPE(KVMPMUFilter, KVM_PMU_FILTER)
+
+struct KVMPMUFilter {
+    Object parent_obj;
+
+    KVMPMUFilterAction action;
+    uint32_t nevents;
+    KVMPMUFilterEventList *events;
+};
+
+#endif /* KVM_PMU_H */
diff --git a/qapi/kvm.json b/qapi/kvm.json
new file mode 100644
index 000000000000..d51aeeba7cd8
--- /dev/null
+++ b/qapi/kvm.json
@@ -0,0 +1,116 @@
+# -*- Mode: Python -*-
+# vim: filetype=python
+
+##
+# = KVM based feature API
+##
+
+##
+# @KVMPMUFilterAction:
+#
+# Actions that KVM PMU filter supports.
+#
+# @deny: disable the PMU event/counter in KVM PMU filter.
+#
+# @allow: enable the PMU event/counter in KVM PMU filter.
+#
+# Since 10.0
+##
+{ 'enum': 'KVMPMUFilterAction',
+  'prefix': 'KVM_PMU_FILTER_ACTION',
+  'data': ['allow', 'deny'] }
+
+##
+# @KVMPMUEventEncodeFmt:
+#
+# Encoding formats of PMU event that QEMU/KVM supports.
+#
+# @raw: the encoded event code that KVM can directly consume.
+#
+# Since 10.0
+##
+{ 'enum': 'KVMPMUEventEncodeFmt',
+  'prefix': 'KVM_PMU_EVENT_FMT',
+  'data': ['raw'] }
+
+##
+# @KVMPMURawEvent:
+#
+# Raw PMU event code.
+#
+# @code: the raw value that has been encoded, and QEMU could deliver
+#     to KVM directly.
+#
+# Since 10.0
+##
+{ 'struct': 'KVMPMURawEvent',
+  'data': { 'code': 'uint64' } }
+
+##
+# @KVMPMUFilterEvent:
+#
+# PMU event filtered by KVM.
+#
+# @format: PMU event format.
+#
+# Since 10.0
+##
+{ 'union': 'KVMPMUFilterEvent',
+  'base': { 'format': 'KVMPMUEventEncodeFmt' },
+  'discriminator': 'format',
+  'data': { 'raw': 'KVMPMURawEvent' } }
+
+##
+# @KVMPMUFilterProperty:
+#
+# Property of KVM PMU Filter.
+#
+# @events: the KVMPMUFilterEvent list.
+#
+# Since 10.0
+##
+{ 'struct': 'KVMPMUFilterProperty',
+  'data': { '*events': ['KVMPMUFilterEvent'] } }
+
+##
+# @KVMPMURawEventVariant:
+#
+# The variant of KVMPMURawEvent with the string, rather than the
+# numeric value.
+#
+# @code: the raw value that has been encoded, and QEMU could deliver
+#     to KVM directly.  This field is a uint64 string.
+#
+# Since 10.0
+##
+{ 'struct': 'KVMPMURawEventVariant',
+  'data': { 'code': 'str' } }
+
+##
+# @KVMPMUFilterEventVariant:
+#
+# The variant of KVMPMUFilterEvent.
+#
+# @format: PMU event format.
+#
+# Since 10.0
+##
+{ 'union': 'KVMPMUFilterEventVariant',
+  'base': { 'format': 'KVMPMUEventEncodeFmt' },
+  'discriminator': 'format',
+  'data': { 'raw': 'KVMPMURawEventVariant' } }
+
+##
+# @KVMPMUFilterPropertyVariant:
+#
+# The variant of KVMPMUFilterProperty.
+#
+# @action: action that KVM PMU filter will take.
+#
+# @events: the KVMPMUFilterEventVariant list.
+#
+# Since 10.0
+##
+{ 'struct': 'KVMPMUFilterPropertyVariant',
+  'data': { 'action': 'KVMPMUFilterAction',
+            '*events': ['KVMPMUFilterEventVariant'] } }
diff --git a/qapi/meson.build b/qapi/meson.build
index e7bc54e5d047..856439c76b67 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -37,6 +37,7 @@ qapi_all_modules = [
   'error',
   'introspect',
   'job',
+  'kvm',
   'machine-common',
   'machine',
   'machine-target',
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
index b1581988e4eb..742818d16e45 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -64,6 +64,7 @@
 { 'include': 'compat.json' }
 { 'include': 'control.json' }
 { 'include': 'introspect.json' }
+{ 'include': 'kvm.json' }
 { 'include': 'qom.json' }
 { 'include': 'qdev.json' }
 { 'include': 'machine-common.json' }
diff --git a/qapi/qom.json b/qapi/qom.json
index 28ce24cd8d08..c75ec4b21e95 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -8,6 +8,7 @@
 { 'include': 'block-core.json' }
 { 'include': 'common.json' }
 { 'include': 'crypto.json' }
+{ 'include': 'kvm.json' }
 
 ##
 # = QEMU Object Model (QOM)
@@ -1108,6 +1109,7 @@
       'if': 'CONFIG_LINUX' },
     'iommufd',
     'iothread',
+    'kvm-pmu-filter',
     'main-loop',
     { 'name': 'memory-backend-epc',
       'if': 'CONFIG_LINUX' },
@@ -1183,6 +1185,7 @@
                                       'if': 'CONFIG_LINUX' },
       'iommufd':                    'IOMMUFDProperties',
       'iothread':                   'IothreadProperties',
+      'kvm-pmu-filter':             'KVMPMUFilterPropertyVariant',
       'main-loop':                  'MainLoopProperties',
       'memory-backend-epc':         { 'type': 'MemoryBackendEpcProperties',
                                       'if': 'CONFIG_LINUX' },
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [RFC v2 2/5] i386/kvm: Support basic KVM PMU filter
  2025-01-22  9:05 [RFC v2 0/5] accel/kvm: Support KVM PMU filter Zhao Liu
  2025-01-22  9:05 ` [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object Zhao Liu
@ 2025-01-22  9:05 ` Zhao Liu
  2025-01-22  9:05 ` [RFC v2 3/5] i386/kvm: Support event with select & umask format in " Zhao Liu
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 27+ messages in thread
From: Zhao Liu @ 2025-01-22  9:05 UTC (permalink / raw)
  To: Paolo Bonzini, Eric Blake, Markus Armbruster, Michael Roth,
	Daniel P . Berrangé, Eduardo Habkost, Marcelo Tosatti,
	Shaoqin Huang, Eric Auger, Peter Maydell, Laurent Vivier,
	Thomas Huth, Sebastian Ott, Gavin Shan
  Cc: qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai, Zhao Liu

Filter PMU events with raw format in i386 code.

For i386, raw format indicates that the PMU event code is already
encoded according to the KVM ioctl requirements, and can be delivered
directly to KVM without additional encoding work.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since v1:
 * Stop check whether per-event actions are the same, as "action" has
   been a global parameter. (Dapeng)
 * Make pmu filter related functions return int in
   target/i386/kvm/kvm.c.
---
 include/system/kvm_int.h |   2 +
 target/i386/kvm/kvm.c    | 123 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 125 insertions(+)

diff --git a/include/system/kvm_int.h b/include/system/kvm_int.h
index 4de6106869b0..743fed29b17b 100644
--- a/include/system/kvm_int.h
+++ b/include/system/kvm_int.h
@@ -17,6 +17,7 @@
 #include "hw/boards.h"
 #include "hw/i386/topology.h"
 #include "io/channel-socket.h"
+#include "system/kvm-pmu.h"
 
 typedef struct KVMSlot
 {
@@ -166,6 +167,7 @@ struct KVMState
     uint16_t xen_gnttab_max_frames;
     uint16_t xen_evtchn_max_pirq;
     char *device;
+    KVMPMUFilter *pmu_filter;
 };
 
 void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 6c749d4ee812..b82adbed50f4 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -34,6 +34,7 @@
 #include "system/system.h"
 #include "system/hw_accel.h"
 #include "system/kvm_int.h"
+#include "system/kvm-pmu.h"
 #include "system/runstate.h"
 #include "kvm_i386.h"
 #include "../confidential-guest.h"
@@ -110,6 +111,7 @@ typedef struct {
 static void kvm_init_msrs(X86CPU *cpu);
 static int kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr,
                           QEMUWRMSRHandler *wrmsr);
+static int kvm_filter_pmu_event(KVMState *s);
 
 const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
     KVM_CAP_INFO(SET_TSS_ADDR),
@@ -3346,6 +3348,14 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
         }
     }
 
+    if (s->pmu_filter) {
+        ret = kvm_filter_pmu_event(s);
+        if (ret < 0) {
+            error_report("Could not set KVM PMU filter");
+            return ret;
+        }
+    }
+
     return 0;
 }
 
@@ -5942,6 +5952,82 @@ static int kvm_handle_wrmsr(X86CPU *cpu, struct kvm_run *run)
     g_assert_not_reached();
 }
 
+static bool kvm_config_pmu_event(KVMPMUFilter *filter,
+                                 struct kvm_pmu_event_filter *kvm_filter)
+{
+    KVMPMUFilterEventList *events;
+    KVMPMUFilterEvent *event;
+    uint64_t code;
+    int idx = 0;
+
+    kvm_filter->nevents = filter->nevents;
+    events = filter->events;
+    while (events) {
+        assert(idx < kvm_filter->nevents);
+
+        event = events->value;
+        switch (event->format) {
+        case KVM_PMU_EVENT_FMT_RAW:
+            code = event->u.raw.code;
+            break;
+        default:
+            g_assert_not_reached();
+        }
+
+        kvm_filter->events[idx++] = code;
+        events = events->next;
+    }
+
+    return true;
+}
+
+static int kvm_install_pmu_event_filter(KVMState *s)
+{
+    struct kvm_pmu_event_filter *kvm_filter;
+    KVMPMUFilter *filter = s->pmu_filter;
+    int ret;
+
+    kvm_filter = g_malloc0(sizeof(struct kvm_pmu_event_filter) +
+                           filter->nevents * sizeof(uint64_t));
+
+    switch (filter->action) {
+    case KVM_PMU_FILTER_ACTION_ALLOW:
+        kvm_filter->action = KVM_PMU_EVENT_ALLOW;
+        break;
+    case KVM_PMU_FILTER_ACTION_DENY:
+        kvm_filter->action = KVM_PMU_EVENT_DENY;
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    if (!kvm_config_pmu_event(filter, kvm_filter)) {
+        goto fail;
+    }
+
+    ret = kvm_vm_ioctl(s, KVM_SET_PMU_EVENT_FILTER, kvm_filter);
+    if (ret) {
+        error_report("KVM_SET_PMU_EVENT_FILTER fails (%s)", strerror(-ret));
+        goto fail;
+    }
+
+    g_free(kvm_filter);
+    return 0;
+fail:
+    g_free(kvm_filter);
+    return -EINVAL;
+}
+
+static int kvm_filter_pmu_event(KVMState *s)
+{
+    if (!kvm_vm_check_extension(s, KVM_CAP_PMU_EVENT_FILTER)) {
+        error_report("KVM PMU filter is not supported by Host.");
+        return -1;
+    }
+
+    return kvm_install_pmu_event_filter(s);
+}
+
 static bool has_sgx_provisioning;
 
 static bool __kvm_enable_sgx_provisioning(KVMState *s)
@@ -6537,6 +6623,35 @@ static void kvm_arch_set_xen_evtchn_max_pirq(Object *obj, Visitor *v,
     s->xen_evtchn_max_pirq = value;
 }
 
+static void kvm_arch_check_pmu_filter(const Object *obj, const char *name,
+                                      Object *child, Error **errp)
+{
+    KVMPMUFilter *filter = KVM_PMU_FILTER(child);
+    KVMPMUFilterEventList *events = filter->events;
+
+    if (!filter->nevents) {
+        error_setg(errp,
+                   "Empty KVM PMU filter.");
+        return;
+    }
+
+    while (events) {
+        KVMPMUFilterEvent *event = events->value;
+
+        switch (event->format) {
+        case KVM_PMU_EVENT_FMT_RAW:
+            break;
+        default:
+            error_setg(errp,
+                       "Unsupported PMU event format %s.",
+                       KVMPMUEventEncodeFmt_str(events->value->format));
+            return;
+        }
+
+        events = events->next;
+    }
+}
+
 void kvm_arch_accel_class_init(ObjectClass *oc)
 {
     object_class_property_add_enum(oc, "notify-vmexit", "NotifyVMexitOption",
@@ -6576,6 +6691,14 @@ void kvm_arch_accel_class_init(ObjectClass *oc)
                               NULL, NULL);
     object_class_property_set_description(oc, "xen-evtchn-max-pirq",
                                           "Maximum number of Xen PIRQs");
+
+    object_class_property_add_link(oc, "pmu-filter",
+                                   TYPE_KVM_PMU_FILTER,
+                                   offsetof(KVMState, pmu_filter),
+                                   kvm_arch_check_pmu_filter,
+                                   OBJ_PROP_LINK_STRONG);
+    object_class_property_set_description(oc, "pmu-filter",
+                                          "Set the KVM PMU filter");
 }
 
 void kvm_set_max_apic_id(uint32_t max_apic_id)
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [RFC v2 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter
  2025-01-22  9:05 [RFC v2 0/5] accel/kvm: Support KVM PMU filter Zhao Liu
  2025-01-22  9:05 ` [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object Zhao Liu
  2025-01-22  9:05 ` [RFC v2 2/5] i386/kvm: Support basic KVM PMU filter Zhao Liu
@ 2025-01-22  9:05 ` Zhao Liu
  2025-02-05 10:07   ` Markus Armbruster
  2025-01-22  9:05 ` [RFC v2 4/5] i386/kvm: Support event with masked entry " Zhao Liu
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 27+ messages in thread
From: Zhao Liu @ 2025-01-22  9:05 UTC (permalink / raw)
  To: Paolo Bonzini, Eric Blake, Markus Armbruster, Michael Roth,
	Daniel P . Berrangé, Eduardo Habkost, Marcelo Tosatti,
	Shaoqin Huang, Eric Auger, Peter Maydell, Laurent Vivier,
	Thomas Huth, Sebastian Ott, Gavin Shan
  Cc: qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai, Zhao Liu

The select&umask is the common way for x86 to identify the PMU event,
so support this way as the "x86-default" format in kvm-pmu-filter
object.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC v1:
 * Bump up the supported QAPI version to 10.0.
---
 accel/kvm/kvm-pmu.c      | 62 ++++++++++++++++++++++++++++++++++++++++
 include/system/kvm-pmu.h | 13 +++++++++
 qapi/kvm.json            | 46 +++++++++++++++++++++++++++--
 target/i386/kvm/kvm.c    |  5 ++++
 4 files changed, 123 insertions(+), 3 deletions(-)

diff --git a/accel/kvm/kvm-pmu.c b/accel/kvm/kvm-pmu.c
index 4d0d4e7a452b..cbd32e8e21f8 100644
--- a/accel/kvm/kvm-pmu.c
+++ b/accel/kvm/kvm-pmu.c
@@ -17,6 +17,8 @@
 #include "qom/object_interfaces.h"
 #include "system/kvm-pmu.h"
 
+#define UINT12_MAX (4095)
+
 static void kvm_pmu_filter_set_action(Object *obj, int value,
                                       Error **errp G_GNUC_UNUSED)
 {
@@ -54,6 +56,12 @@ static void kvm_pmu_filter_get_event(Object *obj, Visitor *v, const char *name,
             str_event->u.raw.code = g_strdup_printf("0x%lx",
                                                     event->u.raw.code);
             break;
+        case KVM_PMU_EVENT_FMT_X86_DEFAULT:
+            str_event->u.x86_default.select =
+                g_strdup_printf("0x%x", event->u.x86_default.select);
+            str_event->u.x86_default.umask =
+                g_strdup_printf("0x%x", event->u.x86_default.umask);
+            break;
         default:
             g_assert_not_reached();
         }
@@ -98,6 +106,60 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
                 goto fail;
             }
             break;
+        case KVM_PMU_EVENT_FMT_X86_DEFAULT: {
+            uint64_t select, umask;
+
+            ret = qemu_strtou64(str_event->u.x86_default.select, NULL,
+                                0, &select);
+            if (ret < 0) {
+                error_setg(errp,
+                           "Invalid %s PMU event (select: %s): %s. "
+                           "The select must be a "
+                           "12-bit unsigned number string.",
+                           KVMPMUEventEncodeFmt_str(str_event->format),
+                           str_event->u.x86_default.select,
+                           strerror(-ret));
+                g_free(event);
+                goto fail;
+            }
+            if (select > UINT12_MAX) {
+                error_setg(errp,
+                           "Invalid %s PMU event (select: %s): "
+                           "Numerical result out of range. "
+                           "The select must be a "
+                           "12-bit unsigned number string.",
+                           KVMPMUEventEncodeFmt_str(str_event->format),
+                           str_event->u.x86_default.select);
+                g_free(event);
+                goto fail;
+            }
+            event->u.x86_default.select = select;
+
+            ret = qemu_strtou64(str_event->u.x86_default.umask, NULL,
+                                0, &umask);
+            if (ret < 0) {
+                error_setg(errp,
+                           "Invalid %s PMU event (umask: %s): %s. "
+                           "The umask must be a uint8 string.",
+                           KVMPMUEventEncodeFmt_str(str_event->format),
+                           str_event->u.x86_default.umask,
+                           strerror(-ret));
+                g_free(event);
+                goto fail;
+            }
+            if (umask > UINT8_MAX) {
+                error_setg(errp,
+                           "Invalid %s PMU event (umask: %s): "
+                           "Numerical result out of range. "
+                           "The umask must be a uint8 string.",
+                           KVMPMUEventEncodeFmt_str(str_event->format),
+                           str_event->u.x86_default.umask);
+                g_free(event);
+                goto fail;
+            }
+            event->u.x86_default.umask = umask;
+            break;
+        }
         default:
             g_assert_not_reached();
         }
diff --git a/include/system/kvm-pmu.h b/include/system/kvm-pmu.h
index b09f70d3a370..63402f75cfdc 100644
--- a/include/system/kvm-pmu.h
+++ b/include/system/kvm-pmu.h
@@ -27,4 +27,17 @@ struct KVMPMUFilter {
     KVMPMUFilterEventList *events;
 };
 
+/*
+ * Stolen from Linux kernel (RAW_EVENT at tools/testing/selftests/kvm/include/
+ * x86_64/pmu.h).
+ *
+ * Encode an eventsel+umask pair into event-select MSR format.  Note, this is
+ * technically AMD's format, as Intel's format only supports 8 bits for the
+ * event selector, i.e. doesn't use bits 24:16 for the selector.  But, OR-ing
+ * in '0' is a nop and won't clobber the CMASK.
+ */
+#define X86_PMU_RAW_EVENT(eventsel, umask) (((eventsel & 0xf00UL) << 24) | \
+                                            ((eventsel) & 0xff) | \
+                                            ((umask) & 0xff) << 8)
+
 #endif /* KVM_PMU_H */
diff --git a/qapi/kvm.json b/qapi/kvm.json
index d51aeeba7cd8..93b869e3f90c 100644
--- a/qapi/kvm.json
+++ b/qapi/kvm.json
@@ -27,11 +27,13 @@
 #
 # @raw: the encoded event code that KVM can directly consume.
 #
+# @x86-default: standard x86 encoding format with select and umask.
+#
 # Since 10.0
 ##
 { 'enum': 'KVMPMUEventEncodeFmt',
   'prefix': 'KVM_PMU_EVENT_FMT',
-  'data': ['raw'] }
+  'data': ['raw', 'x86-default'] }
 
 ##
 # @KVMPMURawEvent:
@@ -46,6 +48,25 @@
 { 'struct': 'KVMPMURawEvent',
   'data': { 'code': 'uint64' } }
 
+##
+# @KVMPMUX86DefalutEvent:
+#
+# x86 PMU event encoding with select and umask.
+# raw_event = ((select & 0xf00UL) << 24) | \
+#              (select) & 0xff) | \
+#              ((umask) & 0xff) << 8)
+#
+# @select: x86 PMU event select field, which is a 12-bit unsigned
+#     number.
+#
+# @umask: x86 PMU event umask field.
+#
+# Since 10.0
+##
+{ 'struct': 'KVMPMUX86DefalutEvent',
+  'data': { 'select': 'uint16',
+            'umask': 'uint8' } }
+
 ##
 # @KVMPMUFilterEvent:
 #
@@ -58,7 +79,8 @@
 { 'union': 'KVMPMUFilterEvent',
   'base': { 'format': 'KVMPMUEventEncodeFmt' },
   'discriminator': 'format',
-  'data': { 'raw': 'KVMPMURawEvent' } }
+  'data': { 'raw': 'KVMPMURawEvent',
+            'x86-default': 'KVMPMUX86DefalutEvent' } }
 
 ##
 # @KVMPMUFilterProperty:
@@ -86,6 +108,23 @@
 { 'struct': 'KVMPMURawEventVariant',
   'data': { 'code': 'str' } }
 
+##
+# @KVMPMUX86DefalutEventVariant:
+#
+# The variant of KVMPMUX86DefalutEvent with the string, rather than
+# the numeric value.
+#
+# @select: x86 PMU event select field.  This field is a 12-bit
+#     unsigned number string.
+#
+# @umask: x86 PMU event umask field. This field is a uint8 string.
+#
+# Since 10.0
+##
+{ 'struct': 'KVMPMUX86DefalutEventVariant',
+  'data': { 'select': 'str',
+            'umask': 'str' } }
+
 ##
 # @KVMPMUFilterEventVariant:
 #
@@ -98,7 +137,8 @@
 { 'union': 'KVMPMUFilterEventVariant',
   'base': { 'format': 'KVMPMUEventEncodeFmt' },
   'discriminator': 'format',
-  'data': { 'raw': 'KVMPMURawEventVariant' } }
+  'data': { 'raw': 'KVMPMURawEventVariant',
+            'x86-default': 'KVMPMUX86DefalutEventVariant' } }
 
 ##
 # @KVMPMUFilterPropertyVariant:
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index b82adbed50f4..bab58761417a 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -5970,6 +5970,10 @@ static bool kvm_config_pmu_event(KVMPMUFilter *filter,
         case KVM_PMU_EVENT_FMT_RAW:
             code = event->u.raw.code;
             break;
+        case KVM_PMU_EVENT_FMT_X86_DEFAULT:
+            code = X86_PMU_RAW_EVENT(event->u.x86_default.select,
+                                     event->u.x86_default.umask);
+            break;
         default:
             g_assert_not_reached();
         }
@@ -6640,6 +6644,7 @@ static void kvm_arch_check_pmu_filter(const Object *obj, const char *name,
 
         switch (event->format) {
         case KVM_PMU_EVENT_FMT_RAW:
+        case KVM_PMU_EVENT_FMT_X86_DEFAULT:
             break;
         default:
             error_setg(errp,
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [RFC v2 4/5] i386/kvm: Support event with masked entry format in KVM PMU filter
  2025-01-22  9:05 [RFC v2 0/5] accel/kvm: Support KVM PMU filter Zhao Liu
                   ` (2 preceding siblings ...)
  2025-01-22  9:05 ` [RFC v2 3/5] i386/kvm: Support event with select & umask format in " Zhao Liu
@ 2025-01-22  9:05 ` Zhao Liu
  2025-01-22  9:05 ` [RFC v2 5/5] i386/kvm: Support fixed counter " Zhao Liu
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 27+ messages in thread
From: Zhao Liu @ 2025-01-22  9:05 UTC (permalink / raw)
  To: Paolo Bonzini, Eric Blake, Markus Armbruster, Michael Roth,
	Daniel P . Berrangé, Eduardo Habkost, Marcelo Tosatti,
	Shaoqin Huang, Eric Auger, Peter Maydell, Laurent Vivier,
	Thomas Huth, Sebastian Ott, Gavin Shan
  Cc: qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai, Zhao Liu

KVM_SET_PMU_EVENT_FILTER of x86 KVM supports masked events mode, which
accepts masked entry format event to flexibly represent a group of PMU
events.

Support masked entry format in kvm-pmu-filter object and handle this in
i386 kvm codes.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC v1:
 * Bump up the supported QAPI version to 10.0.
---
 accel/kvm/kvm-pmu.c   | 91 +++++++++++++++++++++++++++++++++++++++++++
 qapi/kvm.json         | 68 ++++++++++++++++++++++++++++++--
 target/i386/kvm/kvm.c | 39 +++++++++++++++++++
 3 files changed, 195 insertions(+), 3 deletions(-)

diff --git a/accel/kvm/kvm-pmu.c b/accel/kvm/kvm-pmu.c
index cbd32e8e21f8..9d68cd15e477 100644
--- a/accel/kvm/kvm-pmu.c
+++ b/accel/kvm/kvm-pmu.c
@@ -62,6 +62,16 @@ static void kvm_pmu_filter_get_event(Object *obj, Visitor *v, const char *name,
             str_event->u.x86_default.umask =
                 g_strdup_printf("0x%x", event->u.x86_default.umask);
             break;
+        case KVM_PMU_EVENT_FMT_X86_MASKED_ENTRY:
+            str_event->u.x86_masked_entry.select =
+                g_strdup_printf("0x%x", event->u.x86_masked_entry.select);
+            str_event->u.x86_masked_entry.match =
+                g_strdup_printf("0x%x", event->u.x86_masked_entry.match);
+            str_event->u.x86_masked_entry.mask =
+                g_strdup_printf("0x%x", event->u.x86_masked_entry.mask);
+            str_event->u.x86_masked_entry.exclude =
+                event->u.x86_masked_entry.exclude;
+            break;
         default:
             g_assert_not_reached();
         }
@@ -160,6 +170,87 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
             event->u.x86_default.umask = umask;
             break;
         }
+        case KVM_PMU_EVENT_FMT_X86_MASKED_ENTRY: {
+            uint64_t select, match, mask;
+
+            ret = qemu_strtou64(str_event->u.x86_masked_entry.select,
+                                NULL, 0, &select);
+            if (ret < 0) {
+                error_setg(errp,
+                           "Invalid %s PMU event (select: %s): %s. "
+                           "The select must be a "
+                           "12-bit unsigned number string.",
+                           KVMPMUEventEncodeFmt_str(str_event->format),
+                           str_event->u.x86_masked_entry.select,
+                           strerror(-ret));
+                g_free(event);
+                goto fail;
+            }
+            if (select > UINT12_MAX) {
+                error_setg(errp,
+                           "Invalid %s PMU event (select: %s): "
+                           "Numerical result out of range. "
+                           "The select must be a "
+                           "12-bit unsigned number string.",
+                           KVMPMUEventEncodeFmt_str(str_event->format),
+                           str_event->u.x86_masked_entry.select);
+                g_free(event);
+                goto fail;
+            }
+            event->u.x86_masked_entry.select = select;
+
+            ret = qemu_strtou64(str_event->u.x86_masked_entry.match,
+                                NULL, 0, &match);
+            if (ret < 0) {
+                error_setg(errp,
+                           "Invalid %s PMU event (match: %s): %s. "
+                           "The match must be a uint8 string.",
+                           KVMPMUEventEncodeFmt_str(str_event->format),
+                           str_event->u.x86_masked_entry.match,
+                           strerror(-ret));
+                g_free(event);
+                goto fail;
+            }
+            if (match > UINT8_MAX) {
+                error_setg(errp,
+                           "Invalid %s PMU event (match: %s): "
+                           "Numerical result out of range. "
+                           "The match must be a uint8 string.",
+                           KVMPMUEventEncodeFmt_str(str_event->format),
+                           str_event->u.x86_masked_entry.match);
+                g_free(event);
+                goto fail;
+            }
+            event->u.x86_masked_entry.match = match;
+
+            ret = qemu_strtou64(str_event->u.x86_masked_entry.mask,
+                                NULL, 0, &mask);
+            if (ret < 0) {
+                error_setg(errp,
+                           "Invalid %s PMU event (mask: %s): %s. "
+                           "The mask must be a uint8 string.",
+                           KVMPMUEventEncodeFmt_str(str_event->format),
+                           str_event->u.x86_masked_entry.mask,
+                           strerror(-ret));
+                g_free(event);
+                goto fail;
+            }
+            if (mask > UINT8_MAX) {
+                error_setg(errp,
+                           "Invalid %s PMU event (mask: %s): "
+                           "Numerical result out of range. "
+                           "The mask must be a uint8 string.",
+                           KVMPMUEventEncodeFmt_str(str_event->format),
+                           str_event->u.x86_masked_entry.mask);
+                g_free(event);
+                goto fail;
+            }
+            event->u.x86_masked_entry.mask = mask;
+
+            event->u.x86_masked_entry.exclude =
+                str_event->u.x86_masked_entry.exclude;
+            break;
+        }
         default:
             g_assert_not_reached();
         }
diff --git a/qapi/kvm.json b/qapi/kvm.json
index 93b869e3f90c..a673e499e7ea 100644
--- a/qapi/kvm.json
+++ b/qapi/kvm.json
@@ -29,11 +29,14 @@
 #
 # @x86-default: standard x86 encoding format with select and umask.
 #
+# @x86-masked-entry: KVM's masked entry format for x86, which could
+#                    mask bunch of events.
+#
 # Since 10.0
 ##
 { 'enum': 'KVMPMUEventEncodeFmt',
   'prefix': 'KVM_PMU_EVENT_FMT',
-  'data': ['raw', 'x86-default'] }
+  'data': ['raw', 'x86-default', 'x86-masked-entry'] }
 
 ##
 # @KVMPMURawEvent:
@@ -67,6 +70,40 @@
   'data': { 'select': 'uint16',
             'umask': 'uint8' } }
 
+##
+# @KVMPMUX86MaskedEntry:
+#
+# x86 PMU events encoding in KVM masked entry format.
+#
+# Encoding layout:
+# Bits   Description
+# ----   -----------
+# 7:0    event select (low bits)
+# 15:8   (umask) match
+# 31:16  unused
+# 35:32  event select (high bits)
+# 36:54  unused
+# 55     exclude bit
+# 63:56  (umask) mask
+#
+# Events are selected by (umask & mask == match)
+#
+# @select: x86 PMU event select, which is a 12-bit unsigned number.
+#
+# @match: umask match.
+#
+# @mask: umask mask.
+#
+# @exclude: Whether the matched events are excluded.
+#
+# Since 10.0
+##
+{ 'struct': 'KVMPMUX86MaskedEntry',
+  'data': { 'select': 'uint16',
+            'match': 'uint8',
+            'mask': 'uint8',
+            'exclude': 'bool' } }
+
 ##
 # @KVMPMUFilterEvent:
 #
@@ -80,7 +117,8 @@
   'base': { 'format': 'KVMPMUEventEncodeFmt' },
   'discriminator': 'format',
   'data': { 'raw': 'KVMPMURawEvent',
-            'x86-default': 'KVMPMUX86DefalutEvent' } }
+            'x86-default': 'KVMPMUX86DefalutEvent',
+            'x86-masked-entry': 'KVMPMUX86MaskedEntry' } }
 
 ##
 # @KVMPMUFilterProperty:
@@ -125,6 +163,29 @@
   'data': { 'select': 'str',
             'umask': 'str' } }
 
+##
+# @KVMPMUX86MaskedEntryVariant:
+#
+# The variant of KVMPMUX86MaskedEntry with the string, rather than
+# the numeric value.
+#
+# @select: x86 PMU event select.  This field is a 12-bit unsigned
+#     number string.
+#
+# @match: umask match.  This field is a uint8 string.
+#
+# @mask: umask mask.  This field is a uint8 string.
+#
+# @exclude: Whether the matched events are excluded.
+#
+# Since 10.0
+##
+{ 'struct': 'KVMPMUX86MaskedEntryVariant',
+  'data': { 'select': 'str',
+            'match': 'str',
+            'mask': 'str',
+            'exclude': 'bool' } }
+
 ##
 # @KVMPMUFilterEventVariant:
 #
@@ -138,7 +199,8 @@
   'base': { 'format': 'KVMPMUEventEncodeFmt' },
   'discriminator': 'format',
   'data': { 'raw': 'KVMPMURawEventVariant',
-            'x86-default': 'KVMPMUX86DefalutEventVariant' } }
+            'x86-default': 'KVMPMUX86DefalutEventVariant',
+            'x86-masked-entry': 'KVMPMUX86MaskedEntryVariant' } }
 
 ##
 # @KVMPMUFilterPropertyVariant:
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index bab58761417a..97b94c331271 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -5974,6 +5974,13 @@ static bool kvm_config_pmu_event(KVMPMUFilter *filter,
             code = X86_PMU_RAW_EVENT(event->u.x86_default.select,
                                      event->u.x86_default.umask);
             break;
+        case KVM_PMU_EVENT_FMT_X86_MASKED_ENTRY:
+            code = KVM_PMU_ENCODE_MASKED_ENTRY(
+                       event->u.x86_masked_entry.select,
+                       event->u.x86_masked_entry.mask,
+                       event->u.x86_masked_entry.match,
+                       event->u.x86_masked_entry.exclude ? 1 : 0);
+            break;
         default:
             g_assert_not_reached();
         }
@@ -6005,6 +6012,21 @@ static int kvm_install_pmu_event_filter(KVMState *s)
         g_assert_not_reached();
     }
 
+    /*
+     * The check in kvm_arch_check_pmu_filter() ensures masked entry
+     * format won't be mixed with other formats.
+     */
+    kvm_filter->flags = filter->events->value->format ==
+                        KVM_PMU_EVENT_FMT_X86_MASKED_ENTRY ?
+                        KVM_PMU_EVENT_FLAG_MASKED_EVENTS : 0;
+
+    if (kvm_filter->flags == KVM_PMU_EVENT_FLAG_MASKED_EVENTS &&
+        !kvm_vm_check_extension(s, KVM_CAP_PMU_EVENT_MASKED_EVENTS)) {
+        error_report("Masked entry format of PMU event "
+                     "is not supported by Host.");
+        goto fail;
+    }
+
     if (!kvm_config_pmu_event(filter, kvm_filter)) {
         goto fail;
     }
@@ -6632,6 +6654,7 @@ static void kvm_arch_check_pmu_filter(const Object *obj, const char *name,
 {
     KVMPMUFilter *filter = KVM_PMU_FILTER(child);
     KVMPMUFilterEventList *events = filter->events;
+    uint32_t base_flag;
 
     if (!filter->nevents) {
         error_setg(errp,
@@ -6639,12 +6662,21 @@ static void kvm_arch_check_pmu_filter(const Object *obj, const char *name,
         return;
     }
 
+    /* Pick the first event's flag as the base one. */
+    base_flag = events->value->format ==
+                KVM_PMU_EVENT_FMT_X86_MASKED_ENTRY ?
+                KVM_PMU_EVENT_FLAG_MASKED_EVENTS : 0;
     while (events) {
         KVMPMUFilterEvent *event = events->value;
+        uint32_t flag;
 
         switch (event->format) {
         case KVM_PMU_EVENT_FMT_RAW:
         case KVM_PMU_EVENT_FMT_X86_DEFAULT:
+            flag = 0;
+            break;
+        case KVM_PMU_EVENT_FMT_X86_MASKED_ENTRY:
+            flag = KVM_PMU_EVENT_FLAG_MASKED_EVENTS;
             break;
         default:
             error_setg(errp,
@@ -6653,6 +6685,13 @@ static void kvm_arch_check_pmu_filter(const Object *obj, const char *name,
             return;
         }
 
+        if (flag != base_flag) {
+            error_setg(errp,
+                       "Masked entry format cannot be mixed with "
+                       "other formats.");
+            return;
+        }
+
         events = events->next;
     }
 }
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [RFC v2 5/5] i386/kvm: Support fixed counter in KVM PMU filter
  2025-01-22  9:05 [RFC v2 0/5] accel/kvm: Support KVM PMU filter Zhao Liu
                   ` (3 preceding siblings ...)
  2025-01-22  9:05 ` [RFC v2 4/5] i386/kvm: Support event with masked entry " Zhao Liu
@ 2025-01-22  9:05 ` Zhao Liu
  2025-01-24  8:00 ` [RFC v2 0/5] accel/kvm: Support " Lai, Yi
  2025-03-18  7:35 ` Shaoqin Huang
  6 siblings, 0 replies; 27+ messages in thread
From: Zhao Liu @ 2025-01-22  9:05 UTC (permalink / raw)
  To: Paolo Bonzini, Eric Blake, Markus Armbruster, Michael Roth,
	Daniel P . Berrangé, Eduardo Habkost, Marcelo Tosatti,
	Shaoqin Huang, Eric Auger, Peter Maydell, Laurent Vivier,
	Thomas Huth, Sebastian Ott, Gavin Shan
  Cc: qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai, Zhao Liu

KVM_SET_PMU_EVENT_FILTER of x86 KVM allows user to configure x86 fixed
function counters by a bitmap.

Add the support of x86-fixed-counter in kvm-pmu-filter object and handle
this in i386 kvm codes.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC v1:
 * Make "action" as a global (per filter object) item, not a per-counter
   parameter. (Dapeng)
 * Bump up the supported QAPI version to 10.0.
---
 accel/kvm/kvm-pmu.c      | 69 ++++++++++++++++++++++++++++++++++++++++
 include/system/kvm-pmu.h |  1 +
 qapi/kvm.json            | 30 ++++++++++++++++-
 target/i386/kvm/kvm.c    | 47 ++++++++++++++++-----------
 4 files changed, 127 insertions(+), 20 deletions(-)

diff --git a/accel/kvm/kvm-pmu.c b/accel/kvm/kvm-pmu.c
index 9d68cd15e477..b5e6f430632f 100644
--- a/accel/kvm/kvm-pmu.c
+++ b/accel/kvm/kvm-pmu.c
@@ -271,6 +271,66 @@ fail:
     qapi_free_KVMPMUFilterEventList(head);
 }
 
+static void kvm_pmu_filter_get_fixed_counter(Object *obj, Visitor *v,
+                                             const char *name, void *opaque,
+                                             Error **errp)
+{
+    KVMPMUFilter *filter = KVM_PMU_FILTER(obj);
+    KVMPMUX86FixedCounterVariant *str_counter;
+
+    str_counter = g_new(KVMPMUX86FixedCounterVariant, 1);
+    str_counter->bitmap = g_strdup_printf("0x%x",
+                                          filter->x86_fixed_counter->bitmap);
+
+    visit_type_KVMPMUX86FixedCounterVariant(v, name, &str_counter, errp);
+    qapi_free_KVMPMUX86FixedCounterVariant(str_counter);
+}
+
+static void kvm_pmu_filter_set_fixed_counter(Object *obj, Visitor *v,
+                                             const char *name, void *opaque,
+                                             Error **errp)
+{
+    KVMPMUFilter *filter = KVM_PMU_FILTER(obj);
+    KVMPMUX86FixedCounterVariant *str_counter;
+    KVMPMUX86FixedCounter *new_counter, *old_counter;
+    uint64_t bitmap;
+    int ret;
+
+    old_counter = filter->x86_fixed_counter;
+    if (!visit_type_KVMPMUX86FixedCounterVariant(v, name,
+                                                 &str_counter, errp)) {
+        return;
+    }
+
+    new_counter  = g_new(KVMPMUX86FixedCounter, 1);
+
+    ret = qemu_strtou64(str_counter->bitmap, NULL,
+                        0, &bitmap);
+    if (ret < 0) {
+        error_setg(errp,
+                   "Invalid x86 fixed counter (bitmap: %s): %s. "
+                   "The bitmap must be a uint32 string.",
+                   str_counter->bitmap, strerror(-ret));
+        g_free(new_counter);
+        goto fail;
+    }
+    if (bitmap > UINT32_MAX) {
+        error_setg(errp,
+                   "Invalid x86 fixed counter (bitmap: %s): "
+                   "Numerical result out of range. "
+                   "The bitmap must be a uint32 string.",
+                   str_counter->bitmap);
+        g_free(new_counter);
+        goto fail;
+    }
+    new_counter->bitmap = bitmap;
+    filter->x86_fixed_counter = new_counter;
+    qapi_free_KVMPMUX86FixedCounter(old_counter);
+
+fail:
+    qapi_free_KVMPMUX86FixedCounterVariant(str_counter);
+}
+
 static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
 {
     object_class_property_add_enum(oc, "action", "KVMPMUFilterAction",
@@ -286,6 +346,15 @@ static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
                               NULL, NULL);
     object_class_property_set_description(oc, "events",
                                           "KVM PMU event list");
+
+    object_class_property_add(oc, "x86-fixed-counter",
+                              "KVMPMUX86FixedCounter",
+                              kvm_pmu_filter_get_fixed_counter,
+                              kvm_pmu_filter_set_fixed_counter,
+                              NULL, NULL);
+    object_class_property_set_description(oc, "x86-fixed-counter",
+                                          "Enablement bitmap of "
+                                          "x86 PMU fixed counter");
 }
 
 static void kvm_pmu_filter_instance_init(Object *obj)
diff --git a/include/system/kvm-pmu.h b/include/system/kvm-pmu.h
index 63402f75cfdc..1a1da37b6304 100644
--- a/include/system/kvm-pmu.h
+++ b/include/system/kvm-pmu.h
@@ -25,6 +25,7 @@ struct KVMPMUFilter {
     KVMPMUFilterAction action;
     uint32_t nevents;
     KVMPMUFilterEventList *events;
+    KVMPMUX86FixedCounter *x86_fixed_counter;
 };
 
 /*
diff --git a/qapi/kvm.json b/qapi/kvm.json
index a673e499e7ea..31447dfeffb0 100644
--- a/qapi/kvm.json
+++ b/qapi/kvm.json
@@ -120,6 +120,18 @@
             'x86-default': 'KVMPMUX86DefalutEvent',
             'x86-masked-entry': 'KVMPMUX86MaskedEntry' } }
 
+##
+# @KVMPMUX86FixedCounter:
+#
+# x86 fixed counter that KVM PMU filter supports.
+#
+# @bitmap: x86 fixed counter bitmap.
+#
+# Since 10.0
+##
+{ 'struct': 'KVMPMUX86FixedCounter',
+  'data': { 'bitmap': 'uint32' } }
+
 ##
 # @KVMPMUFilterProperty:
 #
@@ -202,6 +214,19 @@
             'x86-default': 'KVMPMUX86DefalutEventVariant',
             'x86-masked-entry': 'KVMPMUX86MaskedEntryVariant' } }
 
+##
+# @KVMPMUX86FixedCounterVariant:
+#
+# The variant of KVMPMUX86FixedCounter with the string, rather than
+# the numeric value.
+#
+# @bitmap: x86 fixed counter bitmap.  This field is a uint32 string.
+#
+# Since 10.0
+##
+{ 'struct': 'KVMPMUX86FixedCounterVariant',
+  'data': { 'bitmap': 'str' } }
+
 ##
 # @KVMPMUFilterPropertyVariant:
 #
@@ -211,8 +236,11 @@
 #
 # @events: the KVMPMUFilterEventVariant list.
 #
+# @x86-fixed-counter: enablement bitmap of x86 fixed counters.
+#
 # Since 10.0
 ##
 { 'struct': 'KVMPMUFilterPropertyVariant',
   'data': { 'action': 'KVMPMUFilterAction',
-            '*events': ['KVMPMUFilterEventVariant'] } }
+            '*events': ['KVMPMUFilterEventVariant'],
+            '*x86-fixed-counter': 'KVMPMUX86FixedCounterVariant' } }
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 97b94c331271..56094361cb8f 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -6012,23 +6012,29 @@ static int kvm_install_pmu_event_filter(KVMState *s)
         g_assert_not_reached();
     }
 
-    /*
-     * The check in kvm_arch_check_pmu_filter() ensures masked entry
-     * format won't be mixed with other formats.
-     */
-    kvm_filter->flags = filter->events->value->format ==
-                        KVM_PMU_EVENT_FMT_X86_MASKED_ENTRY ?
-                        KVM_PMU_EVENT_FLAG_MASKED_EVENTS : 0;
-
-    if (kvm_filter->flags == KVM_PMU_EVENT_FLAG_MASKED_EVENTS &&
-        !kvm_vm_check_extension(s, KVM_CAP_PMU_EVENT_MASKED_EVENTS)) {
-        error_report("Masked entry format of PMU event "
-                     "is not supported by Host.");
-        goto fail;
+    if (filter->x86_fixed_counter) {
+        kvm_filter->fixed_counter_bitmap = filter->x86_fixed_counter->bitmap;
     }
 
-    if (!kvm_config_pmu_event(filter, kvm_filter)) {
-        goto fail;
+    if (filter->nevents) {
+        /*
+         * The check in kvm_arch_check_pmu_filter() ensures masked entry
+         * format won't be mixed with other formats.
+         */
+        kvm_filter->flags = filter->events->value->format ==
+                            KVM_PMU_EVENT_FMT_X86_MASKED_ENTRY ?
+                            KVM_PMU_EVENT_FLAG_MASKED_EVENTS : 0;
+
+        if (kvm_filter->flags == KVM_PMU_EVENT_FLAG_MASKED_EVENTS &&
+            !kvm_vm_check_extension(s, KVM_CAP_PMU_EVENT_MASKED_EVENTS)) {
+            error_report("Masked entry format of PMU event "
+                         "is not supported by Host.");
+            goto fail;
+        }
+
+        if (!kvm_config_pmu_event(filter, kvm_filter)) {
+            goto fail;
+        }
     }
 
     ret = kvm_vm_ioctl(s, KVM_SET_PMU_EVENT_FILTER, kvm_filter);
@@ -6656,16 +6662,19 @@ static void kvm_arch_check_pmu_filter(const Object *obj, const char *name,
     KVMPMUFilterEventList *events = filter->events;
     uint32_t base_flag;
 
-    if (!filter->nevents) {
+    if (!filter->x86_fixed_counter && !filter->nevents) {
         error_setg(errp,
                    "Empty KVM PMU filter.");
         return;
     }
 
     /* Pick the first event's flag as the base one. */
-    base_flag = events->value->format ==
-                KVM_PMU_EVENT_FMT_X86_MASKED_ENTRY ?
-                KVM_PMU_EVENT_FLAG_MASKED_EVENTS : 0;
+    base_flag = 0;
+    if (filter->nevents &&
+        events->value->format == KVM_PMU_EVENT_FMT_X86_MASKED_ENTRY) {
+        base_flag = KVM_PMU_EVENT_FLAG_MASKED_EVENTS;
+    }
+
     while (events) {
         KVMPMUFilterEvent *event = events->value;
         uint32_t flag;
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [RFC v2 0/5] accel/kvm: Support KVM PMU filter
  2025-01-22  9:05 [RFC v2 0/5] accel/kvm: Support KVM PMU filter Zhao Liu
                   ` (4 preceding siblings ...)
  2025-01-22  9:05 ` [RFC v2 5/5] i386/kvm: Support fixed counter " Zhao Liu
@ 2025-01-24  8:00 ` Lai, Yi
  2025-03-18  7:35 ` Shaoqin Huang
  6 siblings, 0 replies; 27+ messages in thread
From: Lai, Yi @ 2025-01-24  8:00 UTC (permalink / raw)
  To: Zhao Liu
  Cc: Paolo Bonzini, Eric Blake, Markus Armbruster, Michael Roth,
	Daniel P . Berrangé, Eduardo Habkost, Marcelo Tosatti,
	Shaoqin Huang, Eric Auger, Peter Maydell, Laurent Vivier,
	Thomas Huth, Sebastian Ott, Gavin Shan, qemu-devel, kvm, qemu-arm,
	Dapeng Mi, Yi Lai

On Wed, Jan 22, 2025 at 05:05:12PM +0800, Zhao Liu wrote:
> Hi folks,
> 
> Sorry for the long wait, but RFC v2 is here at last.
> 
> Compared with v1 [1], v2 mianly makes `action` as a global parameter,
> and all events (and fixed counters) are based on a unified action.
> 
> Learned from the discussion with Shaoqin in v1, current pmu-filter QOM
> design could meet the requirements from the ARM KVM side.
> 
>

Tested the v2 patches series with v6.13 kernel.

The 3 supported event formats work correctly with both action "allow" and
"deny". The x86-fixed-counter support also works as expected.

Regards,
Yi Lai

> Background
> ==========
> 
> I picked up Shaoqing's previous work [2] on the KVM PMU filter for arm,
> and now is trying to support this feature for x86 with a JSON-compatible
> API.
> 
> While arm and x86 use different KVM ioctls to configure the PMU filter,
> considering they all have similar inputs (PMU event + action), it is
> still possible to abstract a generic, cross-architecture kvm-pmu-filter
> object and provide users with a sufficiently generic or near-consistent
> QAPI interface.
> 
> That's what I did in this series, a new kvm-pmu-filter object, with the
> API like:
> 
> -object '{"qom-type":"kvm-pmu-filter","id":"f0","action":"allow","events":[{"format":"raw","code":"0xc4"}]}'
> 
> For i386, this object is inserted into kvm accelerator and is extended
> to support fixed-counter and more formats ("x86-default" and
> "x86-masked-entry"):
> 
> -accel kvm,pmu-filter=f0 \
> -object pmu='{"qom-type":"kvm-pmu-filter","id":"f0","action":"allow","x86-fixed-counter":{"bitmap":"0x0"},"events":[{"format":"x86-masked-entry","select":"0xc4","mask":"0xff","match":"0","exclude":true},{"format":"x86-masked-entry","select":"0xc5","mask":"0xff","match":"0","exclude":true}]}'
> 
> This object can still be added as the property to the arch CPU if it is
> desired as a per CPU feature (as Shaoqin did for arm before).
> 
> 
> Introduction
> ============
> 
> 
> Formats supported in kvm-pmu-filter
> -----------------------------------
> 
> This series supports 3 formats:
> 
> * raw format (general format).
> 
>   This format indicates the code that has been encoded to be able to
>   index the PMU events, and which can be delivered directly to the KVM
>   ioctl. For arm, this means the event code, and for i386, this means
>   the raw event with the layout like:
> 
>       select high bit | umask | select low bits
> 
> * x86-default format (i386 specific)
> 
>   x86 commonly uses select&umask to identify PMU events, and this format
>   is used to support the select&umask. Then QEMU will encode select and
>   umask into a raw format code.
> 
> * x86-masked-entry (i386 specific)
> 
>   This is a special format that x86's KVM_SET_PMU_EVENT_FILTER supports.
> 
> 
> Hexadecimal value string
> ------------------------
> 
> In practice, the values associated with PMU events (code for arm, select&
> umask for x86) are often expressed in hexadecimal. Further, from linux
> perf related information (tools/perf/pmu-events/arch/*/*/*.json), x86/
> arm64/riscv/nds32/powerpc all prefer the hexadecimal numbers and only
> s390 uses decimal value.
> 
> Therefore, it is necessary to support hexadecimal in order to honor PMU
> conventions.
> 
> However, unfortunately, standard JSON (RFC 8259) does not support
> hexadecimal numbers. So I can only consider using the numeric string in
> the QAPI and then parsing it to a number.
> 
> To achieve this, I defined two versions of PMU-related structures in
> kvm.json:
>  * a native version that accepts numeric values, which is used for
>    QEMU's internal code processing,
> 
>  * and a variant version that accepts numeric string, which is used to
>    receive user input.
> 
> kvm-pmu-filter object will take care of converting the string version
> of the event/counter information into the numeric version.
> 
> The related implementation can be found in patch 1.
> 
> 
> CPU property v.s. KVM property
> ------------------------------
> 
> In Shaoqin's previous implementation [2], KVM PMU filter is made as a
> arm CPU property. This is because arm uses a per CPU ioctl
> (KVM_SET_DEVICE_ATTR) to configure KVM PMU filter.
> 
> However, for x86, the dependent ioctl (KVM_SET_PMU_EVENT_FILTER) is per
> VM. In the meantime, considering that for hybrid architecture, maybe in
> the future there will be a new per vCPU ioctl, or there will be
> practices to support filter fixed counter by configuring CPUIDs.
> 
> Based on the above thoughts, for x86, it is not appropriate to make the
> current per-VM ioctl-based PMU filter a CPU property. Instead, I make it
> a kvm property and configure it via "-accel kvm,pmu-filter=obj_id".
> 
> So in summary, it is feasible to use the KVM PMU filter as either a CPU
> or a KVM property, depending on whether it is used as a CPU feature or a
> VM feature.
> 
> The kvm-pmu-filter object, as an abstraction, is general enough to
> support filter configurations for different scopes (per-CPU or per-VM).
> 
> [1]: https://lore.kernel.org/qemu-devel/20240710045117.3164577-1-zhao1.liu@intel.com/
> [2]: https://lore.kernel.org/qemu-devel/20240409024940.180107-1-shahuang@redhat.com/
> 
> Thanks and Best Regards,
> Zhao
> ---
> Zhao Liu (5):
>   qapi/qom: Introduce kvm-pmu-filter object
>   i386/kvm: Support basic KVM PMU filter
>   i386/kvm: Support event with select & umask format in KVM PMU filter
>   i386/kvm: Support event with masked entry format in KVM PMU filter
>   i386/kvm: Support fixed counter in KVM PMU filter
> 
>  MAINTAINERS              |   1 +
>  accel/kvm/kvm-pmu.c      | 386 +++++++++++++++++++++++++++++++++++++++
>  accel/kvm/meson.build    |   1 +
>  include/system/kvm-pmu.h |  44 +++++
>  include/system/kvm_int.h |   2 +
>  qapi/kvm.json            | 246 +++++++++++++++++++++++++
>  qapi/meson.build         |   1 +
>  qapi/qapi-schema.json    |   1 +
>  qapi/qom.json            |   3 +
>  target/i386/kvm/kvm.c    | 176 ++++++++++++++++++
>  10 files changed, 861 insertions(+)
>  create mode 100644 accel/kvm/kvm-pmu.c
>  create mode 100644 include/system/kvm-pmu.h
>  create mode 100644 qapi/kvm.json
> 
> -- 
> 2.34.1
> 


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object
  2025-01-22  9:05 ` [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object Zhao Liu
@ 2025-02-05 10:03   ` Markus Armbruster
  2025-02-06 10:19     ` Zhao Liu
  0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2025-02-05 10:03 UTC (permalink / raw)
  To: Zhao Liu
  Cc: Paolo Bonzini, Eric Blake, Markus Armbruster, Michael Roth,
	Daniel P . Berrangé, Eduardo Habkost, Marcelo Tosatti,
	Shaoqin Huang, Eric Auger, Peter Maydell, Laurent Vivier,
	Thomas Huth, Sebastian Ott, Gavin Shan, qemu-devel, kvm, qemu-arm,
	Dapeng Mi, Yi Lai

Quick & superficial review for now.

Zhao Liu <zhao1.liu@intel.com> writes:

> Introduce the kvm-pmu-filter object and support the PMU event with raw
> format.
>
> The raw format, as a native PMU event code representation, can be used
> for several architectures.
>
> Considering that PMU event related fields are commonly used in
> hexadecimal, define KVMPMURawEventVariant, KVMPMUFilterEventVariant, and
> KVMPMUFilterPropertyVariant in kvm.json to support hexadecimal number
> strings in JSON.
>
> Additionally, define the corresponding numeric versions of
> KVMPMURawEvent, KVMPMUFilterEvent, and KVMPMUFilterProperty in kvm.json.
> This allows to handle numeric values more effectively and take advantage
> of the qapi helpers.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

[...]

> diff --git a/qapi/kvm.json b/qapi/kvm.json
> new file mode 100644
> index 000000000000..d51aeeba7cd8
> --- /dev/null
> +++ b/qapi/kvm.json
> @@ -0,0 +1,116 @@
> +# -*- Mode: Python -*-
> +# vim: filetype=python
> +
> +##
> +# = KVM based feature API

This is a top-level section.  It ends up between sections "QMP
introspection" and "QEMU Object Model (QOM)".  Is this what we want?  Or
should it be a sub-section of something?  Or next to something else?

> +##
> +
> +##
> +# @KVMPMUFilterAction:
> +#
> +# Actions that KVM PMU filter supports.
> +#
> +# @deny: disable the PMU event/counter in KVM PMU filter.
> +#
> +# @allow: enable the PMU event/counter in KVM PMU filter.
> +#
> +# Since 10.0
> +##
> +{ 'enum': 'KVMPMUFilterAction',
> +  'prefix': 'KVM_PMU_FILTER_ACTION',
> +  'data': ['allow', 'deny'] }
> +
> +##
> +# @KVMPMUEventEncodeFmt:

Please don't abbreviate Format to Fmt.  We use Format elsewhere, and
consistency is desirable.

> +#
> +# Encoding formats of PMU event that QEMU/KVM supports.
> +#
> +# @raw: the encoded event code that KVM can directly consume.
> +#
> +# Since 10.0
> +##
> +{ 'enum': 'KVMPMUEventEncodeFmt',
> +  'prefix': 'KVM_PMU_EVENT_FMT',
> +  'data': ['raw'] }
> +
> +##
> +# @KVMPMURawEvent:
> +#
> +# Raw PMU event code.
> +#
> +# @code: the raw value that has been encoded, and QEMU could deliver
> +#     to KVM directly.
> +#
> +# Since 10.0
> +##
> +{ 'struct': 'KVMPMURawEvent',
> +  'data': { 'code': 'uint64' } }
> +
> +##
> +# @KVMPMUFilterEvent:
> +#
> +# PMU event filtered by KVM.
> +#
> +# @format: PMU event format.
> +#
> +# Since 10.0
> +##
> +{ 'union': 'KVMPMUFilterEvent',
> +  'base': { 'format': 'KVMPMUEventEncodeFmt' },
> +  'discriminator': 'format',
> +  'data': { 'raw': 'KVMPMURawEvent' } }
> +
> +##
> +# @KVMPMUFilterProperty:
> +#
> +# Property of KVM PMU Filter.
> +#
> +# @events: the KVMPMUFilterEvent list.
> +#
> +# Since 10.0
> +##
> +{ 'struct': 'KVMPMUFilterProperty',
> +  'data': { '*events': ['KVMPMUFilterEvent'] } }
> +
> +##
> +# @KVMPMURawEventVariant:
> +#
> +# The variant of KVMPMURawEvent with the string, rather than the
> +# numeric value.
> +#
> +# @code: the raw value that has been encoded, and QEMU could deliver
> +#     to KVM directly.  This field is a uint64 string.
> +#
> +# Since 10.0
> +##
> +{ 'struct': 'KVMPMURawEventVariant',
> +  'data': { 'code': 'str' } }
> +
> +##
> +# @KVMPMUFilterEventVariant:
> +#
> +# The variant of KVMPMUFilterEvent.
> +#
> +# @format: PMU event format.
> +#
> +# Since 10.0
> +##
> +{ 'union': 'KVMPMUFilterEventVariant',
> +  'base': { 'format': 'KVMPMUEventEncodeFmt' },
> +  'discriminator': 'format',
> +  'data': { 'raw': 'KVMPMURawEventVariant' } }
> +
> +##
> +# @KVMPMUFilterPropertyVariant:
> +#
> +# The variant of KVMPMUFilterProperty.
> +#
> +# @action: action that KVM PMU filter will take.
> +#
> +# @events: the KVMPMUFilterEventVariant list.
> +#
> +# Since 10.0
> +##
> +{ 'struct': 'KVMPMUFilterPropertyVariant',
> +  'data': { 'action': 'KVMPMUFilterAction',
> +            '*events': ['KVMPMUFilterEventVariant'] } }
> diff --git a/qapi/meson.build b/qapi/meson.build
> index e7bc54e5d047..856439c76b67 100644
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -37,6 +37,7 @@ qapi_all_modules = [
>    'error',
>    'introspect',
>    'job',
> +  'kvm',
>    'machine-common',
>    'machine',
>    'machine-target',
> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> index b1581988e4eb..742818d16e45 100644
> --- a/qapi/qapi-schema.json
> +++ b/qapi/qapi-schema.json
> @@ -64,6 +64,7 @@
>  { 'include': 'compat.json' }
>  { 'include': 'control.json' }
>  { 'include': 'introspect.json' }
> +{ 'include': 'kvm.json' }
>  { 'include': 'qom.json' }
>  { 'include': 'qdev.json' }
>  { 'include': 'machine-common.json' }
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 28ce24cd8d08..c75ec4b21e95 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -8,6 +8,7 @@
>  { 'include': 'block-core.json' }
>  { 'include': 'common.json' }
>  { 'include': 'crypto.json' }
> +{ 'include': 'kvm.json' }
>  
>  ##
>  # = QEMU Object Model (QOM)
> @@ -1108,6 +1109,7 @@
>        'if': 'CONFIG_LINUX' },
>      'iommufd',
>      'iothread',
> +    'kvm-pmu-filter',
>      'main-loop',
>      { 'name': 'memory-backend-epc',
>        'if': 'CONFIG_LINUX' },
> @@ -1183,6 +1185,7 @@
>                                        'if': 'CONFIG_LINUX' },
>        'iommufd':                    'IOMMUFDProperties',
>        'iothread':                   'IothreadProperties',
> +      'kvm-pmu-filter':             'KVMPMUFilterPropertyVariant',

The others are like

         'mumble': 'MumbleProperties'

Let's stick to that, and also avoid running together multiple
capitalized acronyms: KvmPmuFilterProperties.

>        'main-loop':                  'MainLoopProperties',
>        'memory-backend-epc':         { 'type': 'MemoryBackendEpcProperties',
>                                        'if': 'CONFIG_LINUX' },



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter
  2025-01-22  9:05 ` [RFC v2 3/5] i386/kvm: Support event with select & umask format in " Zhao Liu
@ 2025-02-05 10:07   ` Markus Armbruster
  2025-02-06  9:54     ` Zhao Liu
  0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2025-02-05 10:07 UTC (permalink / raw)
  To: Zhao Liu
  Cc: Paolo Bonzini, Eric Blake, Michael Roth, Daniel P . Berrangé,
	Eduardo Habkost, Marcelo Tosatti, Shaoqin Huang, Eric Auger,
	Peter Maydell, Laurent Vivier, Thomas Huth, Sebastian Ott,
	Gavin Shan, qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai

Zhao Liu <zhao1.liu@intel.com> writes:

> The select&umask is the common way for x86 to identify the PMU event,
> so support this way as the "x86-default" format in kvm-pmu-filter
> object.

So, format 'raw' lets you specify the PMU event code as a number, wheras
'x86-default' lets you specify it as select and umask, correct?

Why do we want both?

> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

[...]

> diff --git a/qapi/kvm.json b/qapi/kvm.json
> index d51aeeba7cd8..93b869e3f90c 100644
> --- a/qapi/kvm.json
> +++ b/qapi/kvm.json
> @@ -27,11 +27,13 @@
>  #
>  # @raw: the encoded event code that KVM can directly consume.
>  #
> +# @x86-default: standard x86 encoding format with select and umask.

Why is this named -default?

> +#
>  # Since 10.0
>  ##
>  { 'enum': 'KVMPMUEventEncodeFmt',
>    'prefix': 'KVM_PMU_EVENT_FMT',
> -  'data': ['raw'] }
> +  'data': ['raw', 'x86-default'] }
>  
>  ##
>  # @KVMPMURawEvent:
> @@ -46,6 +48,25 @@
>  { 'struct': 'KVMPMURawEvent',
>    'data': { 'code': 'uint64' } }
>  
> +##
> +# @KVMPMUX86DefalutEvent:

Default, I suppose.

> +#
> +# x86 PMU event encoding with select and umask.
> +# raw_event = ((select & 0xf00UL) << 24) | \
> +#              (select) & 0xff) | \
> +#              ((umask) & 0xff) << 8)

Sphinx rejects this with "Unexpected indentation."

Is the formula needed here?

> +#
> +# @select: x86 PMU event select field, which is a 12-bit unsigned
> +#     number.
> +#
> +# @umask: x86 PMU event umask field.
> +#
> +# Since 10.0
> +##
> +{ 'struct': 'KVMPMUX86DefalutEvent',
> +  'data': { 'select': 'uint16',
> +            'umask': 'uint8' } }
> +
>  ##
>  # @KVMPMUFilterEvent:
>  #
> @@ -58,7 +79,8 @@
>  { 'union': 'KVMPMUFilterEvent',
>    'base': { 'format': 'KVMPMUEventEncodeFmt' },
>    'discriminator': 'format',
> -  'data': { 'raw': 'KVMPMURawEvent' } }
> +  'data': { 'raw': 'KVMPMURawEvent',
> +            'x86-default': 'KVMPMUX86DefalutEvent' } }
>  
>  ##
>  # @KVMPMUFilterProperty:
> @@ -86,6 +108,23 @@
>  { 'struct': 'KVMPMURawEventVariant',
>    'data': { 'code': 'str' } }
>  
> +##
> +# @KVMPMUX86DefalutEventVariant:
> +#
> +# The variant of KVMPMUX86DefalutEvent with the string, rather than
> +# the numeric value.
> +#
> +# @select: x86 PMU event select field.  This field is a 12-bit
> +#     unsigned number string.
> +#
> +# @umask: x86 PMU event umask field. This field is a uint8 string.

Why are these strings?  How are they parsed into numbers?

> +#
> +# Since 10.0
> +##
> +{ 'struct': 'KVMPMUX86DefalutEventVariant',
> +  'data': { 'select': 'str',
> +            'umask': 'str' } }
> +
>  ##
>  # @KVMPMUFilterEventVariant:
>  #
> @@ -98,7 +137,8 @@
>  { 'union': 'KVMPMUFilterEventVariant',
>    'base': { 'format': 'KVMPMUEventEncodeFmt' },
>    'discriminator': 'format',
> -  'data': { 'raw': 'KVMPMURawEventVariant' } }
> +  'data': { 'raw': 'KVMPMURawEventVariant',
> +            'x86-default': 'KVMPMUX86DefalutEventVariant' } }
>  
>  ##
>  # @KVMPMUFilterPropertyVariant:

[...]



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter
  2025-02-06  9:54     ` Zhao Liu
@ 2025-02-06  9:42       ` Daniel P. Berrangé
  2025-02-06 10:23         ` Zhao Liu
  2025-02-06 10:24         ` Markus Armbruster
  0 siblings, 2 replies; 27+ messages in thread
From: Daniel P. Berrangé @ 2025-02-06  9:42 UTC (permalink / raw)
  To: Zhao Liu
  Cc: Markus Armbruster, Paolo Bonzini, Eric Blake, Michael Roth,
	Eduardo Habkost, Marcelo Tosatti, Shaoqin Huang, Eric Auger,
	Peter Maydell, Laurent Vivier, Thomas Huth, Sebastian Ott,
	Gavin Shan, qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai

On Thu, Feb 06, 2025 at 05:54:32PM +0800, Zhao Liu wrote:
> On Wed, Feb 05, 2025 at 11:07:10AM +0100, Markus Armbruster wrote:
> > Date: Wed, 05 Feb 2025 11:07:10 +0100
> > From: Markus Armbruster <armbru@redhat.com>
> > Subject: Re: [RFC v2 3/5] i386/kvm: Support event with select & umask
> >  format in KVM PMU filter
> > 
> > Zhao Liu <zhao1.liu@intel.com> writes:
> > 
> > > The select&umask is the common way for x86 to identify the PMU event,
> > > so support this way as the "x86-default" format in kvm-pmu-filter
> > > object.
> > 
> > So, format 'raw' lets you specify the PMU event code as a number, wheras
> > 'x86-default' lets you specify it as select and umask, correct?
> 
> Yes!
> 
> > Why do we want both?
> 
> This 2 formats are both wildly used in x86(for example, in perf tool).
> 
> x86 documents usually specify the umask and select fields.
> 
> But raw format could also be applied for ARM since ARM just uses a number
> to encode event.
> 
> > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > 
> > [...]
> > 
> > > diff --git a/qapi/kvm.json b/qapi/kvm.json
> > > index d51aeeba7cd8..93b869e3f90c 100644
> > > --- a/qapi/kvm.json
> > > +++ b/qapi/kvm.json
> > > @@ -27,11 +27,13 @@
> > >  #
> > >  # @raw: the encoded event code that KVM can directly consume.
> > >  #
> > > +# @x86-default: standard x86 encoding format with select and umask.
> > 
> > Why is this named -default?
> 
> Intel and AMD both use umask+select to encode events, but this format
> doesn't have a name... so I call it `default`, or what about
> "x86-umask-select"?
> 
> > > +#
> > >  # Since 10.0
> > >  ##
> > >  { 'enum': 'KVMPMUEventEncodeFmt',
> > >    'prefix': 'KVM_PMU_EVENT_FMT',
> > > -  'data': ['raw'] }
> > > +  'data': ['raw', 'x86-default'] }
> > >  
> > >  ##
> > >  # @KVMPMURawEvent:
> > > @@ -46,6 +48,25 @@
> > >  { 'struct': 'KVMPMURawEvent',
> > >    'data': { 'code': 'uint64' } }
> > >  
> > > +##
> > > +# @KVMPMUX86DefalutEvent:
> > 
> > Default, I suppose.
> 
> Thanks!
> 
> > > +#
> > > +# x86 PMU event encoding with select and umask.
> > > +# raw_event = ((select & 0xf00UL) << 24) | \
> > > +#              (select) & 0xff) | \
> > > +#              ((umask) & 0xff) << 8)
> > 
> > Sphinx rejects this with "Unexpected indentation."
> > 
> > Is the formula needed here?
> 
> I tried to explain the relationship between raw format and umask+select.
> 
> Emm, where do you think is the right place to put the document like
> this?
> 
> ...
> 
> > > +##
> > > +# @KVMPMUX86DefalutEventVariant:

Typo   s/Defalut/Default/ - repeated many times in this patch.

> > > +#
> > > +# The variant of KVMPMUX86DefalutEvent with the string, rather than
> > > +# the numeric value.
> > > +#
> > > +# @select: x86 PMU event select field.  This field is a 12-bit
> > > +#     unsigned number string.
> > > +#
> > > +# @umask: x86 PMU event umask field. This field is a uint8 string.
> > 
> > Why are these strings?  How are they parsed into numbers?
> 
> In practice, the values associated with PMU events (code for arm, select&
> umask for x86) are often expressed in hexadecimal. Further, from linux
> perf related information (tools/perf/pmu-events/arch/*/*/*.json), x86/
> arm64/riscv/nds32/powerpc all prefer the hexadecimal numbers and only
> s390 uses decimal value.
> 
> Therefore, it is necessary to support hexadecimal in order to honor PMU
> conventions.

IMHO having a data format that matches an arbitrary external tool is not
a goal for QMP. It should be neutral and exclusively use the normal JSON
encoding, ie base-10 decimal. Yes, this means a user/client may have to
convert from hex to dec before sending data over QMP. This is true of
many areas of QMP/QEMU config though and thus normal/expected behaviour.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter
  2025-02-05 10:07   ` Markus Armbruster
@ 2025-02-06  9:54     ` Zhao Liu
  2025-02-06  9:42       ` Daniel P. Berrangé
  0 siblings, 1 reply; 27+ messages in thread
From: Zhao Liu @ 2025-02-06  9:54 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Paolo Bonzini, Eric Blake, Michael Roth, Daniel P . Berrangé,
	Eduardo Habkost, Marcelo Tosatti, Shaoqin Huang, Eric Auger,
	Peter Maydell, Laurent Vivier, Thomas Huth, Sebastian Ott,
	Gavin Shan, qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai

On Wed, Feb 05, 2025 at 11:07:10AM +0100, Markus Armbruster wrote:
> Date: Wed, 05 Feb 2025 11:07:10 +0100
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [RFC v2 3/5] i386/kvm: Support event with select & umask
>  format in KVM PMU filter
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > The select&umask is the common way for x86 to identify the PMU event,
> > so support this way as the "x86-default" format in kvm-pmu-filter
> > object.
> 
> So, format 'raw' lets you specify the PMU event code as a number, wheras
> 'x86-default' lets you specify it as select and umask, correct?

Yes!

> Why do we want both?

This 2 formats are both wildly used in x86(for example, in perf tool).

x86 documents usually specify the umask and select fields.

But raw format could also be applied for ARM since ARM just uses a number
to encode event.

> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> 
> [...]
> 
> > diff --git a/qapi/kvm.json b/qapi/kvm.json
> > index d51aeeba7cd8..93b869e3f90c 100644
> > --- a/qapi/kvm.json
> > +++ b/qapi/kvm.json
> > @@ -27,11 +27,13 @@
> >  #
> >  # @raw: the encoded event code that KVM can directly consume.
> >  #
> > +# @x86-default: standard x86 encoding format with select and umask.
> 
> Why is this named -default?

Intel and AMD both use umask+select to encode events, but this format
doesn't have a name... so I call it `default`, or what about
"x86-umask-select"?

> > +#
> >  # Since 10.0
> >  ##
> >  { 'enum': 'KVMPMUEventEncodeFmt',
> >    'prefix': 'KVM_PMU_EVENT_FMT',
> > -  'data': ['raw'] }
> > +  'data': ['raw', 'x86-default'] }
> >  
> >  ##
> >  # @KVMPMURawEvent:
> > @@ -46,6 +48,25 @@
> >  { 'struct': 'KVMPMURawEvent',
> >    'data': { 'code': 'uint64' } }
> >  
> > +##
> > +# @KVMPMUX86DefalutEvent:
> 
> Default, I suppose.

Thanks!

> > +#
> > +# x86 PMU event encoding with select and umask.
> > +# raw_event = ((select & 0xf00UL) << 24) | \
> > +#              (select) & 0xff) | \
> > +#              ((umask) & 0xff) << 8)
> 
> Sphinx rejects this with "Unexpected indentation."
> 
> Is the formula needed here?

I tried to explain the relationship between raw format and umask+select.

Emm, where do you think is the right place to put the document like
this?

...

> > +##
> > +# @KVMPMUX86DefalutEventVariant:
> > +#
> > +# The variant of KVMPMUX86DefalutEvent with the string, rather than
> > +# the numeric value.
> > +#
> > +# @select: x86 PMU event select field.  This field is a 12-bit
> > +#     unsigned number string.
> > +#
> > +# @umask: x86 PMU event umask field. This field is a uint8 string.
> 
> Why are these strings?  How are they parsed into numbers?

In practice, the values associated with PMU events (code for arm, select&
umask for x86) are often expressed in hexadecimal. Further, from linux
perf related information (tools/perf/pmu-events/arch/*/*/*.json), x86/
arm64/riscv/nds32/powerpc all prefer the hexadecimal numbers and only
s390 uses decimal value.

Therefore, it is necessary to support hexadecimal in order to honor PMU
conventions.

However, unfortunately, standard JSON (RFC 8259) does not support
hexadecimal numbers. So I can only consider using the numeric string in
the QAPI and then parsing it to a number.

I parse this string into number by qemu_strtou64().



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object
  2025-02-05 10:03   ` Markus Armbruster
@ 2025-02-06 10:19     ` Zhao Liu
  2025-02-06 10:27       ` Zhao Liu
  2025-02-06 12:13       ` Markus Armbruster
  0 siblings, 2 replies; 27+ messages in thread
From: Zhao Liu @ 2025-02-06 10:19 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Paolo Bonzini, Eric Blake, Michael Roth, Daniel P . Berrangé,
	Eduardo Habkost, Marcelo Tosatti, Shaoqin Huang, Eric Auger,
	Peter Maydell, Laurent Vivier, Thomas Huth, Sebastian Ott,
	Gavin Shan, qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai

On Wed, Feb 05, 2025 at 11:03:51AM +0100, Markus Armbruster wrote:
> Date: Wed, 05 Feb 2025 11:03:51 +0100
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object
> 
> Quick & superficial review for now.

Thanks!

> > diff --git a/qapi/kvm.json b/qapi/kvm.json
> > new file mode 100644
> > index 000000000000..d51aeeba7cd8
> > --- /dev/null
> > +++ b/qapi/kvm.json
> > @@ -0,0 +1,116 @@
> > +# -*- Mode: Python -*-
> > +# vim: filetype=python
> > +
> > +##
> > +# = KVM based feature API
> 
> This is a top-level section.  It ends up between sections "QMP
> introspection" and "QEMU Object Model (QOM)".  Is this what we want?  Or
> should it be a sub-section of something?  Or next to something else?

Do you mean it's not in the right place in the qapi-schema.json?

diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
index b1581988e4eb..742818d16e45 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -64,6 +64,7 @@
 { 'include': 'compat.json' }
 { 'include': 'control.json' }
 { 'include': 'introspect.json' }
+{ 'include': 'kvm.json' }
 { 'include': 'qom.json' }
 { 'include': 'qdev.json' }
 { 'include': 'machine-common.json' }

Because qom.json includes kvm.json, so I have to place it before
qom.json.

It doesn't have any dependencies itself, so placing it in the previous
position should be fine, where do you prefer?

> > +##
> > +
> > +##
> > +# @KVMPMUFilterAction:
> > +#
> > +# Actions that KVM PMU filter supports.
> > +#
> > +# @deny: disable the PMU event/counter in KVM PMU filter.
> > +#
> > +# @allow: enable the PMU event/counter in KVM PMU filter.
> > +#
> > +# Since 10.0
> > +##
> > +{ 'enum': 'KVMPMUFilterAction',
> > +  'prefix': 'KVM_PMU_FILTER_ACTION',
> > +  'data': ['allow', 'deny'] }
> > +
> > +##
> > +# @KVMPMUEventEncodeFmt:
> 
> Please don't abbreviate Format to Fmt.  We use Format elsewhere, and
> consistency is desirable.

OK, will fix.

> >  ##
> >  # = QEMU Object Model (QOM)
> > @@ -1108,6 +1109,7 @@
> >        'if': 'CONFIG_LINUX' },
> >      'iommufd',
> >      'iothread',
> > +    'kvm-pmu-filter',
> >      'main-loop',
> >      { 'name': 'memory-backend-epc',
> >        'if': 'CONFIG_LINUX' },
> > @@ -1183,6 +1185,7 @@
> >                                        'if': 'CONFIG_LINUX' },
> >        'iommufd':                    'IOMMUFDProperties',
> >        'iothread':                   'IothreadProperties',
> > +      'kvm-pmu-filter':             'KVMPMUFilterPropertyVariant',
> 
> The others are like
> 
>          'mumble': 'MumbleProperties'
> 
> Let's stick to that, and also avoid running together multiple
> capitalized acronyms: KvmPmuFilterProperties.

IIUC, then I should use the name "KvmPmuFilterProperties" (string version
for QAPI), and the name "KvmPmuFilterPropertiesVariant" (numeric version
in codes), do you agree?
 
> >        'main-loop':                  'MainLoopProperties',
> >        'memory-backend-epc':         { 'type': 'MemoryBackendEpcProperties',
> >                                        'if': 'CONFIG_LINUX' },
> 


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [RFC v2 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter
  2025-02-06  9:42       ` Daniel P. Berrangé
@ 2025-02-06 10:23         ` Zhao Liu
  2025-02-06 10:24         ` Markus Armbruster
  1 sibling, 0 replies; 27+ messages in thread
From: Zhao Liu @ 2025-02-06 10:23 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: Markus Armbruster, Paolo Bonzini, Eric Blake, Michael Roth,
	Eduardo Habkost, Marcelo Tosatti, Shaoqin Huang, Eric Auger,
	Peter Maydell, Laurent Vivier, Thomas Huth, Sebastian Ott,
	Gavin Shan, qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai

> > > > +##
> > > > +# @KVMPMUX86DefalutEventVariant:
> 
> Typo   s/Defalut/Default/ - repeated many times in this patch.

My bad! Will fix!

> > > > +#
> > > > +# The variant of KVMPMUX86DefalutEvent with the string, rather than
> > > > +# the numeric value.
> > > > +#
> > > > +# @select: x86 PMU event select field.  This field is a 12-bit
> > > > +#     unsigned number string.
> > > > +#
> > > > +# @umask: x86 PMU event umask field. This field is a uint8 string.
> > > 
> > > Why are these strings?  How are they parsed into numbers?
> > 
> > In practice, the values associated with PMU events (code for arm, select&
> > umask for x86) are often expressed in hexadecimal. Further, from linux
> > perf related information (tools/perf/pmu-events/arch/*/*/*.json), x86/
> > arm64/riscv/nds32/powerpc all prefer the hexadecimal numbers and only
> > s390 uses decimal value.
> > 
> > Therefore, it is necessary to support hexadecimal in order to honor PMU
> > conventions.
> 
> IMHO having a data format that matches an arbitrary external tool is not
> a goal for QMP. It should be neutral and exclusively use the normal JSON
> encoding, ie base-10 decimal. Yes, this means a user/client may have to
> convert from hex to dec before sending data over QMP. This is true of
> many areas of QMP/QEMU config though and thus normal/expected behaviour.
>

Thanks! This will simplify the code a lot.



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter
  2025-02-06  9:42       ` Daniel P. Berrangé
  2025-02-06 10:23         ` Zhao Liu
@ 2025-02-06 10:24         ` Markus Armbruster
  2025-02-06 14:22           ` Zhao Liu
  1 sibling, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2025-02-06 10:24 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: Zhao Liu, Paolo Bonzini, Eric Blake, Michael Roth,
	Eduardo Habkost, Marcelo Tosatti, Shaoqin Huang, Eric Auger,
	Peter Maydell, Laurent Vivier, Thomas Huth, Sebastian Ott,
	Gavin Shan, qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Feb 06, 2025 at 05:54:32PM +0800, Zhao Liu wrote:
>> On Wed, Feb 05, 2025 at 11:07:10AM +0100, Markus Armbruster wrote:
>> > Date: Wed, 05 Feb 2025 11:07:10 +0100
>> > From: Markus Armbruster <armbru@redhat.com>
>> > Subject: Re: [RFC v2 3/5] i386/kvm: Support event with select & umask
>> >  format in KVM PMU filter
>> > 
>> > Zhao Liu <zhao1.liu@intel.com> writes:
>> > 
>> > > The select&umask is the common way for x86 to identify the PMU event,
>> > > so support this way as the "x86-default" format in kvm-pmu-filter
>> > > object.
>> > 
>> > So, format 'raw' lets you specify the PMU event code as a number, wheras
>> > 'x86-default' lets you specify it as select and umask, correct?
>> 
>> Yes!
>> 
>> > Why do we want both?
>> 
>> This 2 formats are both wildly used in x86(for example, in perf tool).
>> 
>> x86 documents usually specify the umask and select fields.
>> 
>> But raw format could also be applied for ARM since ARM just uses a number
>> to encode event.

Is it really too much to ask of the client to compute a raw value from
umask and select values?

>> > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>> > 
>> > [...]
>> > 
>> > > diff --git a/qapi/kvm.json b/qapi/kvm.json
>> > > index d51aeeba7cd8..93b869e3f90c 100644
>> > > --- a/qapi/kvm.json
>> > > +++ b/qapi/kvm.json
>> > > @@ -27,11 +27,13 @@
>> > >  #
>> > >  # @raw: the encoded event code that KVM can directly consume.
>> > >  #
>> > > +# @x86-default: standard x86 encoding format with select and umask.
>> > 
>> > Why is this named -default?
>> 
>> Intel and AMD both use umask+select to encode events, but this format
>> doesn't have a name... so I call it `default`, or what about
>> "x86-umask-select"?

Works for me.

>> > > +#
>> > >  # Since 10.0
>> > >  ##
>> > >  { 'enum': 'KVMPMUEventEncodeFmt',
>> > >    'prefix': 'KVM_PMU_EVENT_FMT',
>> > > -  'data': ['raw'] }
>> > > +  'data': ['raw', 'x86-default'] }
>> > >  
>> > >  ##
>> > >  # @KVMPMURawEvent:
>> > > @@ -46,6 +48,25 @@
>> > >  { 'struct': 'KVMPMURawEvent',
>> > >    'data': { 'code': 'uint64' } }
>> > >  
>> > > +##
>> > > +# @KVMPMUX86DefalutEvent:
>> > 
>> > Default, I suppose.
>> 
>> Thanks!
>> 
>> > > +#
>> > > +# x86 PMU event encoding with select and umask.
>> > > +# raw_event = ((select & 0xf00UL) << 24) | \
>> > > +#              (select) & 0xff) | \
>> > > +#              ((umask) & 0xff) << 8)
>> > 
>> > Sphinx rejects this with "Unexpected indentation."
>> > 
>> > Is the formula needed here?
>> 
>> I tried to explain the relationship between raw format and umask+select.
>> 
>> Emm, where do you think is the right place to put the document like
>> this?

Do users need to know how to compute the raw event value from @select
and @umask?

If yes, is C code the best way?

Here's another way:

    bits  0..7 : bits 0..7 of @select
    bits  8..15: @umask
    bits 24..27: bits 8..11 of @select
    all other bits: zero

>> ...
>> 
>> > > +##
>> > > +# @KVMPMUX86DefalutEventVariant:
>
> Typo   s/Defalut/Default/ - repeated many times in this patch.
>
>> > > +#
>> > > +# The variant of KVMPMUX86DefalutEvent with the string, rather than
>> > > +# the numeric value.
>> > > +#
>> > > +# @select: x86 PMU event select field.  This field is a 12-bit
>> > > +#     unsigned number string.
>> > > +#
>> > > +# @umask: x86 PMU event umask field. This field is a uint8 string.
>> > 
>> > Why are these strings?  How are they parsed into numbers?
>> 
>> In practice, the values associated with PMU events (code for arm, select&
>> umask for x86) are often expressed in hexadecimal. Further, from linux
>> perf related information (tools/perf/pmu-events/arch/*/*/*.json), x86/
>> arm64/riscv/nds32/powerpc all prefer the hexadecimal numbers and only
>> s390 uses decimal value.
>> 
>> Therefore, it is necessary to support hexadecimal in order to honor PMU
>> conventions.
>
> IMHO having a data format that matches an arbitrary external tool is not
> a goal for QMP. It should be neutral and exclusively use the normal JSON
> encoding, ie base-10 decimal. Yes, this means a user/client may have to
> convert from hex to dec before sending data over QMP. This is true of
> many areas of QMP/QEMU config though and thus normal/expected behaviour.

Concur.



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object
  2025-02-06 10:19     ` Zhao Liu
@ 2025-02-06 10:27       ` Zhao Liu
  2025-02-06 12:13       ` Markus Armbruster
  1 sibling, 0 replies; 27+ messages in thread
From: Zhao Liu @ 2025-02-06 10:27 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Paolo Bonzini, Eric Blake, Michael Roth,
	Daniel P . Berrang�, Eduardo Habkost, Marcelo Tosatti,
	Shaoqin Huang, Eric Auger, Peter Maydell, Laurent Vivier,
	Thomas Huth, Sebastian Ott, Gavin Shan, qemu-devel, kvm, qemu-arm,
	Dapeng Mi, Yi Lai

> > > @@ -1183,6 +1185,7 @@
> > >                                        'if': 'CONFIG_LINUX' },
> > >        'iommufd':                    'IOMMUFDProperties',
> > >        'iothread':                   'IothreadProperties',
> > > +      'kvm-pmu-filter':             'KVMPMUFilterPropertyVariant',
> > 
> > The others are like
> > 
> >          'mumble': 'MumbleProperties'
> > 
> > Let's stick to that, and also avoid running together multiple
> > capitalized acronyms: KvmPmuFilterProperties.
> 
> IIUC, then I should use the name "KvmPmuFilterProperties" (string version
> for QAPI), and the name "KvmPmuFilterPropertiesVariant" (numeric version
> in codes), do you agree?
>  

Thanks to Daniel's feedback, pmu filter doesn't need the string version
anymore. So there's only 1 "KvmPmuFilterProperties" structure in QAPI.



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object
  2025-02-06 10:19     ` Zhao Liu
  2025-02-06 10:27       ` Zhao Liu
@ 2025-02-06 12:13       ` Markus Armbruster
  2025-02-06 14:32         ` Zhao Liu
  1 sibling, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2025-02-06 12:13 UTC (permalink / raw)
  To: Zhao Liu
  Cc: Paolo Bonzini, Eric Blake, Michael Roth, Daniel P . Berrangé,
	Eduardo Habkost, Marcelo Tosatti, Shaoqin Huang, Eric Auger,
	Peter Maydell, Laurent Vivier, Thomas Huth, Sebastian Ott,
	Gavin Shan, qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai

Zhao Liu <zhao1.liu@intel.com> writes:

> On Wed, Feb 05, 2025 at 11:03:51AM +0100, Markus Armbruster wrote:
>> Date: Wed, 05 Feb 2025 11:03:51 +0100
>> From: Markus Armbruster <armbru@redhat.com>
>> Subject: Re: [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object
>> 
>> Quick & superficial review for now.
>
> Thanks!
>
>> > diff --git a/qapi/kvm.json b/qapi/kvm.json
>> > new file mode 100644
>> > index 000000000000..d51aeeba7cd8
>> > --- /dev/null
>> > +++ b/qapi/kvm.json
>> > @@ -0,0 +1,116 @@
>> > +# -*- Mode: Python -*-
>> > +# vim: filetype=python
>> > +
>> > +##
>> > +# = KVM based feature API
>> 
>> This is a top-level section.  It ends up between sections "QMP
>> introspection" and "QEMU Object Model (QOM)".  Is this what we want?  Or
>> should it be a sub-section of something?  Or next to something else?
>
> Do you mean it's not in the right place in the qapi-schema.json?
>
> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> index b1581988e4eb..742818d16e45 100644
> --- a/qapi/qapi-schema.json
> +++ b/qapi/qapi-schema.json
> @@ -64,6 +64,7 @@
>  { 'include': 'compat.json' }
>  { 'include': 'control.json' }
>  { 'include': 'introspect.json' }
> +{ 'include': 'kvm.json' }
>  { 'include': 'qom.json' }
>  { 'include': 'qdev.json' }
>  { 'include': 'machine-common.json' }
>
> Because qom.json includes kvm.json, so I have to place it before
> qom.json.
>
> It doesn't have any dependencies itself, so placing it in the previous
> position should be fine, where do you prefer?

Let's ignore how to place it for now, and focus on where we would *like*
to place it.

Is it related to anything other than ObjectType / ObjectOptions in the
QMP reference manual?

I guess qapi/kvm.json is for KVM-specific stuff in general, not just the
KVM PMU filter.  Should we have a section for accelerator-specific
stuff, with subsections for the various accelerators?

[...]



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter
  2025-02-06 10:24         ` Markus Armbruster
@ 2025-02-06 14:22           ` Zhao Liu
  2025-02-06 14:54             ` Zhao Liu
  0 siblings, 1 reply; 27+ messages in thread
From: Zhao Liu @ 2025-02-06 14:22 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Daniel P. Berrangé, Paolo Bonzini, Eric Blake, Michael Roth,
	Eduardo Habkost, Marcelo Tosatti, Shaoqin Huang, Eric Auger,
	Peter Maydell, Laurent Vivier, Thomas Huth, Sebastian Ott,
	Gavin Shan, qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai

> >> > > The select&umask is the common way for x86 to identify the PMU event,
> >> > > so support this way as the "x86-default" format in kvm-pmu-filter
> >> > > object.
> >> > 
> >> > So, format 'raw' lets you specify the PMU event code as a number, wheras
> >> > 'x86-default' lets you specify it as select and umask, correct?
> >> 
> >> Yes!
> >> 
> >> > Why do we want both?
> >> 
> >> This 2 formats are both wildly used in x86(for example, in perf tool).
> >> 
> >> x86 documents usually specify the umask and select fields.
> >> 
> >> But raw format could also be applied for ARM since ARM just uses a number
> >> to encode event.
> 
> Is it really too much to ask of the client to compute a raw value from
> umask and select values?

I understand you're wondering if we can omit the select+umask format...

In principle, having either one should work correctly... The additional
support for the umask+select format is more user-friendly and doesn't
introduce much complexity in the code.

My own observation is that Intel's doc rarely uses the raw format
directly, whereas AMD uses the raw format relatively more often.
Personally, when using the perf tool, I prefer the umask+select format.

Even if only the raw format is supported, users still need to be aware
of the umask+select format because there is the third format - "masked
entry" (which tries to mask a group of events with same select, patch 4).

So I would like to keep both raw and umask+select formats if possible.

...

> >> > > +#
> >> > > +# x86 PMU event encoding with select and umask.
> >> > > +# raw_event = ((select & 0xf00UL) << 24) | \
> >> > > +#              (select) & 0xff) | \
> >> > > +#              ((umask) & 0xff) << 8)
> >> > 
> >> > Sphinx rejects this with "Unexpected indentation."
> >> > 
> >> > Is the formula needed here?
> >> 
> >> I tried to explain the relationship between raw format and umask+select.
> >> 
> >> Emm, where do you think is the right place to put the document like
> >> this?
> 
> Do users need to know how to compute the raw event value from @select
> and @umask?

Yes, because it's also a unified calculation. AMD and Intel have
differences in bits for supported select field, but this calculation
(which follows from the KVM code) makes both compatible.

> If yes, is C code the best way?
> 
> Here's another way:
> 
>     bits  0..7 : bits 0..7 of @select
>     bits  8..15: @umask
>     bits 24..27: bits 8..11 of @select
>     all other bits: zero
>

Thank you! This is what I want.




^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object
  2025-02-06 12:13       ` Markus Armbruster
@ 2025-02-06 14:32         ` Zhao Liu
  2025-02-07 13:02           ` Markus Armbruster
  0 siblings, 1 reply; 27+ messages in thread
From: Zhao Liu @ 2025-02-06 14:32 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Paolo Bonzini, Eric Blake, Michael Roth, Daniel P . Berrangé,
	Eduardo Habkost, Marcelo Tosatti, Shaoqin Huang, Eric Auger,
	Peter Maydell, Laurent Vivier, Thomas Huth, Sebastian Ott,
	Gavin Shan, qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai

> Let's ignore how to place it for now, and focus on where we would *like*
> to place it.
> 
> Is it related to anything other than ObjectType / ObjectOptions in the
> QMP reference manual?

Yes!

> I guess qapi/kvm.json is for KVM-specific stuff in general, not just the
> KVM PMU filter.  Should we have a section for accelerator-specific
> stuff, with subsections for the various accelerators?
> 
> [...]

If we consider the accelerator from a top-down perspective, I understand
that we need to add accelerator.json, kvm.json, and kvm-pmu-filter.json.

The first two files are just to include subsections without any additional
content. Is this overkill? Could we just add a single kvm-pmu-filter.json
(I also considered this name, thinking that kvm might need to add more
things in the future)?

Of course, I lack experience with the file organization here. If you think
the three-level sections (accelerator.json, kvm.json, and kvm-pmu-filter.json)
is necessary, I am happy to try this way. :-)




^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter
  2025-02-06 14:22           ` Zhao Liu
@ 2025-02-06 14:54             ` Zhao Liu
  2025-02-07 13:06               ` Markus Armbruster
  0 siblings, 1 reply; 27+ messages in thread
From: Zhao Liu @ 2025-02-06 14:54 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Daniel P. Berrang�, Paolo Bonzini, Eric Blake,
	Michael Roth, Eduardo Habkost, Marcelo Tosatti, Shaoqin Huang,
	Eric Auger, Peter Maydell, Laurent Vivier, Thomas Huth,
	Sebastian Ott, Gavin Shan, qemu-devel, kvm, qemu-arm, Dapeng Mi,
	Yi Lai

> > Do users need to know how to compute the raw event value from @select
> > and @umask?
> 
> Yes, because it's also a unified calculation. AMD and Intel have
> differences in bits for supported select field, but this calculation
> (which follows from the KVM code) makes both compatible.
> 
> > If yes, is C code the best way?

Sorry, I missed this line. In this patch, there's macro:

+#define X86_PMU_RAW_EVENT(eventsel, umask) (((eventsel & 0xf00UL) << 24) | \
+                                            ((eventsel) & 0xff) | \
+                                            ((umask) & 0xff) << 8)

So could I said something like the following?

+##
+# @KVMPMUX86SelectUmaskEvent:
+#
+# x86 PMU event encoding with select and umask.  Using the X86_PMU_RAW_EVENT
+# macro, the select and umask fields will be encoded into raw foramt and
+# delivered to KVM.
+#
+# @select: x86 PMU event select field, which is a 12-bit unsigned
+#     number.
+#
+# @umask: x86 PMU event umask field.
+#
+# Since 10.0
+##
+{ 'struct': 'KVMPMUX86DefalutEvent',
+  'data': { 'select': 'uint16',
+            'umask': 'uint8' } }
+

Thanks very much!

> > Here's another way:
> > 
> >     bits  0..7 : bits 0..7 of @select
> >     bits  8..15: @umask
> >     bits 24..27: bits 8..11 of @select
> >     all other bits: zero
> >
> 
> Thank you! This is what I want.
> 
> 
> 


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object
  2025-02-06 14:32         ` Zhao Liu
@ 2025-02-07 13:02           ` Markus Armbruster
  2025-04-01  7:47             ` Zhao Liu
  0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2025-02-07 13:02 UTC (permalink / raw)
  To: Zhao Liu
  Cc: Paolo Bonzini, Eric Blake, Michael Roth, Daniel P . Berrangé,
	Eduardo Habkost, Marcelo Tosatti, Shaoqin Huang, Eric Auger,
	Peter Maydell, Laurent Vivier, Thomas Huth, Sebastian Ott,
	Gavin Shan, qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai

Zhao Liu <zhao1.liu@intel.com> writes:

>> Let's ignore how to place it for now, and focus on where we would *like*
>> to place it.
>> 
>> Is it related to anything other than ObjectType / ObjectOptions in the
>> QMP reference manual?
>
> Yes!

Now I'm confused :)

It is related to ObjectType / ObjectType.

Is it related to anything else in the QMP reference manual, and if yes,
to what exactly is it related?

>> I guess qapi/kvm.json is for KVM-specific stuff in general, not just the
>> KVM PMU filter.  Should we have a section for accelerator-specific
>> stuff, with subsections for the various accelerators?
>> 
>> [...]
>
> If we consider the accelerator from a top-down perspective, I understand
> that we need to add accelerator.json, kvm.json, and kvm-pmu-filter.json.
>
> The first two files are just to include subsections without any additional
> content. Is this overkill? Could we just add a single kvm-pmu-filter.json
> (I also considered this name, thinking that kvm might need to add more
> things in the future)?
>
> Of course, I lack experience with the file organization here. If you think
> the three-level sections (accelerator.json, kvm.json, and kvm-pmu-filter.json)
> is necessary, I am happy to try this way. :-)

We don't have to create files just to get a desired section structure.

I'll show you how in a jiffie, but before I do that, let me stress: we
should figure out what we want *first*, and only then how to get it.
So, what section structure would make the most sense for the QMP
reference manual?

A few hints on how...

Consider how qapi/block.json includes qapi/block-core.json:

    ##
    # = Block devices
    ##

    { 'include': 'block-core.json' }

    ##
    # == Additional block stuff (VM related)
    ##

block-core.json starts with

    ##
    # == Block core (VM unrelated)
    ##

Together, this produces this section structure

    = Block devices
    == 
    ##

Together, this produces this section structure

    = Block devices
    == Block core (VM unrelated)
    == Additional block stuff (VM related)

Note that qapi/block-core.json isn't included anywhere else.
qapi/qapi-schema.json advises:

    # Documentation generated with qapi-gen.py is in source order, with
    # included sub-schemas inserted at the first include directive
    # (subsequent include directives have no effect).  To get a sane and
    # stable order, it's best to include each sub-schema just once, or
    # include it first right here.



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter
  2025-02-06 14:54             ` Zhao Liu
@ 2025-02-07 13:06               ` Markus Armbruster
  2025-04-01  7:53                 ` Zhao Liu
  0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2025-02-07 13:06 UTC (permalink / raw)
  To: Zhao Liu
  Cc: Daniel P. Berrang�, Paolo Bonzini, Eric Blake,
	Michael Roth, Eduardo Habkost, Marcelo Tosatti, Shaoqin Huang,
	Eric Auger, Peter Maydell, Laurent Vivier, Thomas Huth,
	Sebastian Ott, Gavin Shan, qemu-devel, kvm, qemu-arm, Dapeng Mi,
	Yi Lai

Zhao Liu <zhao1.liu@intel.com> writes:

>> > Do users need to know how to compute the raw event value from @select
>> > and @umask?
>> 
>> Yes, because it's also a unified calculation. AMD and Intel have
>> differences in bits for supported select field, but this calculation
>> (which follows from the KVM code) makes both compatible.
>> 
>> > If yes, is C code the best way?
>
> Sorry, I missed this line. In this patch, there's macro:
>
> +#define X86_PMU_RAW_EVENT(eventsel, umask) (((eventsel & 0xf00UL) << 24) | \
> +                                            ((eventsel) & 0xff) | \
> +                                            ((umask) & 0xff) << 8)
>
> So could I said something like the following?
>
> +##
> +# @KVMPMUX86SelectUmaskEvent:
> +#
> +# x86 PMU event encoding with select and umask.  Using the X86_PMU_RAW_EVENT
> +# macro, the select and umask fields will be encoded into raw foramt and
> +# delivered to KVM.

Doc comments are for the QMP reference manual, i.e. for users of QMP.
Explaining the QMP interface in terms of its implementation in QEMU is
not nice.

> +#
> +# @select: x86 PMU event select field, which is a 12-bit unsigned
> +#     number.
> +#
> +# @umask: x86 PMU event umask field.
> +#
> +# Since 10.0
> +##
> +{ 'struct': 'KVMPMUX86DefalutEvent',
> +  'data': { 'select': 'uint16',
> +            'umask': 'uint8' } }
> +
>
> Thanks very much!
>
>> > Here's another way:
>> > 
>> >     bits  0..7 : bits 0..7 of @select
>> >     bits  8..15: @umask
>> >     bits 24..27: bits 8..11 of @select
>> >     all other bits: zero
>> >
>> 
>> Thank you! This is what I want.
>> 
>> 
>> 



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 0/5] accel/kvm: Support KVM PMU filter
  2025-01-22  9:05 [RFC v2 0/5] accel/kvm: Support KVM PMU filter Zhao Liu
                   ` (5 preceding siblings ...)
  2025-01-24  8:00 ` [RFC v2 0/5] accel/kvm: Support " Lai, Yi
@ 2025-03-18  7:35 ` Shaoqin Huang
  2025-03-21  3:43   ` Zhao Liu
  6 siblings, 1 reply; 27+ messages in thread
From: Shaoqin Huang @ 2025-03-18  7:35 UTC (permalink / raw)
  To: Zhao Liu, Paolo Bonzini, Eric Blake, Markus Armbruster,
	Michael Roth, Daniel P . Berrangé, Eduardo Habkost,
	Marcelo Tosatti, Eric Auger, Peter Maydell, Laurent Vivier,
	Thomas Huth, Sebastian Ott, Gavin Shan
  Cc: qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai

Hi Zhao,

Thanks for your effort to respin the PMU Filter series.

I tried your series on ARM64, but it reports error at compile time, here 
is the error output:

qapi/kvm.json:59:Unexpected indentation.

While I compiled it on x86, everything is ok. Could you please check why 
it failed on ARM64?

By the mean time, I will review and test this series.

Thanks,
Shaoqin

On 1/22/25 5:05 PM, Zhao Liu wrote:
> Hi folks,
> 
> Sorry for the long wait, but RFC v2 is here at last.
> 
> Compared with v1 [1], v2 mianly makes `action` as a global parameter,
> and all events (and fixed counters) are based on a unified action.
> 
> Learned from the discussion with Shaoqin in v1, current pmu-filter QOM
> design could meet the requirements from the ARM KVM side.
> 
> 
> Background
> ==========
> 
> I picked up Shaoqing's previous work [2] on the KVM PMU filter for arm,
> and now is trying to support this feature for x86 with a JSON-compatible
> API.
> 
> While arm and x86 use different KVM ioctls to configure the PMU filter,
> considering they all have similar inputs (PMU event + action), it is
> still possible to abstract a generic, cross-architecture kvm-pmu-filter
> object and provide users with a sufficiently generic or near-consistent
> QAPI interface.
> 
> That's what I did in this series, a new kvm-pmu-filter object, with the
> API like:
> 
> -object '{"qom-type":"kvm-pmu-filter","id":"f0","action":"allow","events":[{"format":"raw","code":"0xc4"}]}'
> 
> For i386, this object is inserted into kvm accelerator and is extended
> to support fixed-counter and more formats ("x86-default" and
> "x86-masked-entry"):
> 
> -accel kvm,pmu-filter=f0 \
> -object pmu='{"qom-type":"kvm-pmu-filter","id":"f0","action":"allow","x86-fixed-counter":{"bitmap":"0x0"},"events":[{"format":"x86-masked-entry","select":"0xc4","mask":"0xff","match":"0","exclude":true},{"format":"x86-masked-entry","select":"0xc5","mask":"0xff","match":"0","exclude":true}]}'
> 
> This object can still be added as the property to the arch CPU if it is
> desired as a per CPU feature (as Shaoqin did for arm before).
> 
> 
> Introduction
> ============
> 
> 
> Formats supported in kvm-pmu-filter
> -----------------------------------
> 
> This series supports 3 formats:
> 
> * raw format (general format).
> 
>    This format indicates the code that has been encoded to be able to
>    index the PMU events, and which can be delivered directly to the KVM
>    ioctl. For arm, this means the event code, and for i386, this means
>    the raw event with the layout like:
> 
>        select high bit | umask | select low bits
> 
> * x86-default format (i386 specific)
> 
>    x86 commonly uses select&umask to identify PMU events, and this format
>    is used to support the select&umask. Then QEMU will encode select and
>    umask into a raw format code.
> 
> * x86-masked-entry (i386 specific)
> 
>    This is a special format that x86's KVM_SET_PMU_EVENT_FILTER supports.
> 
> 
> Hexadecimal value string
> ------------------------
> 
> In practice, the values associated with PMU events (code for arm, select&
> umask for x86) are often expressed in hexadecimal. Further, from linux
> perf related information (tools/perf/pmu-events/arch/*/*/*.json), x86/
> arm64/riscv/nds32/powerpc all prefer the hexadecimal numbers and only
> s390 uses decimal value.
> 
> Therefore, it is necessary to support hexadecimal in order to honor PMU
> conventions.
> 
> However, unfortunately, standard JSON (RFC 8259) does not support
> hexadecimal numbers. So I can only consider using the numeric string in
> the QAPI and then parsing it to a number.
> 
> To achieve this, I defined two versions of PMU-related structures in
> kvm.json:
>   * a native version that accepts numeric values, which is used for
>     QEMU's internal code processing,
> 
>   * and a variant version that accepts numeric string, which is used to
>     receive user input.
> 
> kvm-pmu-filter object will take care of converting the string version
> of the event/counter information into the numeric version.
> 
> The related implementation can be found in patch 1.
> 
> 
> CPU property v.s. KVM property
> ------------------------------
> 
> In Shaoqin's previous implementation [2], KVM PMU filter is made as a
> arm CPU property. This is because arm uses a per CPU ioctl
> (KVM_SET_DEVICE_ATTR) to configure KVM PMU filter.
> 
> However, for x86, the dependent ioctl (KVM_SET_PMU_EVENT_FILTER) is per
> VM. In the meantime, considering that for hybrid architecture, maybe in
> the future there will be a new per vCPU ioctl, or there will be
> practices to support filter fixed counter by configuring CPUIDs.
> 
> Based on the above thoughts, for x86, it is not appropriate to make the
> current per-VM ioctl-based PMU filter a CPU property. Instead, I make it
> a kvm property and configure it via "-accel kvm,pmu-filter=obj_id".
> 
> So in summary, it is feasible to use the KVM PMU filter as either a CPU
> or a KVM property, depending on whether it is used as a CPU feature or a
> VM feature.
> 
> The kvm-pmu-filter object, as an abstraction, is general enough to
> support filter configurations for different scopes (per-CPU or per-VM).
> 
> [1]: https://lore.kernel.org/qemu-devel/20240710045117.3164577-1-zhao1.liu@intel.com/
> [2]: https://lore.kernel.org/qemu-devel/20240409024940.180107-1-shahuang@redhat.com/
> 
> Thanks and Best Regards,
> Zhao
> ---
> Zhao Liu (5):
>    qapi/qom: Introduce kvm-pmu-filter object
>    i386/kvm: Support basic KVM PMU filter
>    i386/kvm: Support event with select & umask format in KVM PMU filter
>    i386/kvm: Support event with masked entry format in KVM PMU filter
>    i386/kvm: Support fixed counter in KVM PMU filter
> 
>   MAINTAINERS              |   1 +
>   accel/kvm/kvm-pmu.c      | 386 +++++++++++++++++++++++++++++++++++++++
>   accel/kvm/meson.build    |   1 +
>   include/system/kvm-pmu.h |  44 +++++
>   include/system/kvm_int.h |   2 +
>   qapi/kvm.json            | 246 +++++++++++++++++++++++++
>   qapi/meson.build         |   1 +
>   qapi/qapi-schema.json    |   1 +
>   qapi/qom.json            |   3 +
>   target/i386/kvm/kvm.c    | 176 ++++++++++++++++++
>   10 files changed, 861 insertions(+)
>   create mode 100644 accel/kvm/kvm-pmu.c
>   create mode 100644 include/system/kvm-pmu.h
>   create mode 100644 qapi/kvm.json
> 

-- 
Shaoqin



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 0/5] accel/kvm: Support KVM PMU filter
  2025-03-18  7:35 ` Shaoqin Huang
@ 2025-03-21  3:43   ` Zhao Liu
  2025-03-31  6:32     ` Shaoqin Huang
  0 siblings, 1 reply; 27+ messages in thread
From: Zhao Liu @ 2025-03-21  3:43 UTC (permalink / raw)
  To: Shaoqin Huang
  Cc: Paolo Bonzini, Eric Blake, Markus Armbruster, Michael Roth,
	Daniel P . Berrangé, Eduardo Habkost, Marcelo Tosatti,
	Eric Auger, Peter Maydell, Laurent Vivier, Thomas Huth,
	Sebastian Ott, Gavin Shan, qemu-devel, kvm, qemu-arm, Dapeng Mi,
	Yi Lai

Hi Shaoqin,

Thank you very much for testing!

> I tried your series on ARM64, but it reports error at compile time, here is
> the error output:
> 
> qapi/kvm.json:59:Unexpected indentation.

I guess this is caused by my invalid format and sphinx complains that,
as Markus figured out :-(

What about the following change?

diff --git a/qapi/kvm.json b/qapi/kvm.json
index 31447dfeffb0..b383dfd9a788 100644
--- a/qapi/kvm.json
+++ b/qapi/kvm.json
@@ -54,11 +54,6 @@
 ##
 # @KVMPMUX86DefalutEvent:
 #
-# x86 PMU event encoding with select and umask.
-# raw_event = ((select & 0xf00UL) << 24) | \
-#              (select) & 0xff) | \
-#              ((umask) & 0xff) << 8)
-#
 # @select: x86 PMU event select field, which is a 12-bit unsigned
 #     number.
 #

> While I compiled it on x86, everything is ok. Could you please check why it
> failed on ARM64?

Maybe your Arm64 environment doesn't have sphinx_rtd_theme?

You can check it by:

python3 -m pip show sphinx_rtd_theme

> By the mean time, I will review and test this series.

Thank you again! I also plan to refresh v3, in maybe 1 or 2 weeks.

Best Regards,
Zhao




^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [RFC v2 0/5] accel/kvm: Support KVM PMU filter
  2025-03-21  3:43   ` Zhao Liu
@ 2025-03-31  6:32     ` Shaoqin Huang
  0 siblings, 0 replies; 27+ messages in thread
From: Shaoqin Huang @ 2025-03-31  6:32 UTC (permalink / raw)
  To: Zhao Liu
  Cc: Paolo Bonzini, Eric Blake, Markus Armbruster, Michael Roth,
	Daniel P. Berrangé, Eduardo Habkost, Marcelo Tosatti,
	Eric Auger, Peter Maydell, Laurent Vivier, Thomas Huth,
	Sebastian Ott, Gavin Shan, qemu-devel, kvm, qemu-arm, Dapeng Mi,
	Yi Lai

Hi Zhao,

On 3/21/25 11:43 AM, Zhao Liu wrote:
> Hi Shaoqin,
> 
> Thank you very much for testing!
> 
>> I tried your series on ARM64, but it reports error at compile time, here is
>> the error output:
>>
>> qapi/kvm.json:59:Unexpected indentation.
> 
> I guess this is caused by my invalid format and sphinx complains that,
> as Markus figured out :-(
> 
> What about the following change?
> 
> diff --git a/qapi/kvm.json b/qapi/kvm.json
> index 31447dfeffb0..b383dfd9a788 100644
> --- a/qapi/kvm.json
> +++ b/qapi/kvm.json
> @@ -54,11 +54,6 @@
>   ##
>   # @KVMPMUX86DefalutEvent:
>   #
> -# x86 PMU event encoding with select and umask.
> -# raw_event = ((select & 0xf00UL) << 24) | \
> -#              (select) & 0xff) | \
> -#              ((umask) & 0xff) << 8)
> -#
>   # @select: x86 PMU event select field, which is a 12-bit unsigned
>   #     number.
>   #
> 

This doesn't work for me.

But this works on ARM:

-#              (select) & 0xff) | \
-#              ((umask) & 0xff) << 8)
+# (select) & 0xff) | \
+# ((umask) & 0xff) << 8)


>> While I compiled it on x86, everything is ok. Could you please check why it
>> failed on ARM64?
> 
> Maybe your Arm64 environment doesn't have sphinx_rtd_theme?
> 
> You can check it by:
> 
> python3 -m pip show sphinx_rtd_theme
> 
>> By the mean time, I will review and test this series.
> 
> Thank you again! I also plan to refresh v3, in maybe 1 or 2 weeks.

Ok, Waiting for your new respin. :)

Thanks,
Shaoqin

> 
> Best Regards,
> Zhao
> 
> 

-- 
Shaoqin



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object
  2025-02-07 13:02           ` Markus Armbruster
@ 2025-04-01  7:47             ` Zhao Liu
  2025-04-08  5:51               ` Markus Armbruster
  0 siblings, 1 reply; 27+ messages in thread
From: Zhao Liu @ 2025-04-01  7:47 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Paolo Bonzini, Eric Blake, Michael Roth, Daniel P . Berrangé,
	Eduardo Habkost, Marcelo Tosatti, Shaoqin Huang, Eric Auger,
	Peter Maydell, Laurent Vivier, Thomas Huth, Sebastian Ott,
	Gavin Shan, qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai,
	Zhao Liu

Hi Mrkus,

I'm really sorry I completely missed your reply (and your patient
advice). It wasn't until I looked back at the lore archives that I
realized my mistake. Thinking it over again, I see that your reply,
which I missed, really helped clear up my confusion:

On Fri, Feb 07, 2025 at 02:02:44PM +0100, Markus Armbruster wrote:
> Date: Fri, 07 Feb 2025 14:02:44 +0100
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> >> Let's ignore how to place it for now, and focus on where we would *like*
> >> to place it.
> >> 
> >> Is it related to anything other than ObjectType / ObjectOptions in the
> >> QMP reference manual?
> >
> > Yes!
> 
> Now I'm confused :)
> 
> It is related to ObjectType / ObjectType.
> 
> Is it related to anything else in the QMP reference manual, and if yes,
> to what exactly is it related?

I misunderstood your point. The PMU stuff and the QAPI definitions for
ObjectType/ObjectOptions are not related. They should belong to separate
categories or sections.

> >> I guess qapi/kvm.json is for KVM-specific stuff in general, not just the
> >> KVM PMU filter.  Should we have a section for accelerator-specific
> >> stuff, with subsections for the various accelerators?
> >> 
> >> [...]
> >
> > If we consider the accelerator from a top-down perspective, I understand
> > that we need to add accelerator.json, kvm.json, and kvm-pmu-filter.json.
> >
> > The first two files are just to include subsections without any additional
> > content. Is this overkill? Could we just add a single kvm-pmu-filter.json
> > (I also considered this name, thinking that kvm might need to add more
> > things in the future)?
> >
> > Of course, I lack experience with the file organization here. If you think
> > the three-level sections (accelerator.json, kvm.json, and kvm-pmu-filter.json)
> > is necessary, I am happy to try this way. :-)
> 
> We don't have to create files just to get a desired section structure.
> 
> I'll show you how in a jiffie, but before I do that, let me stress: we
> should figure out what we want *first*, and only then how to get it.
> So, what section structure would make the most sense for the QMP
> reference manual?

Thank you for your patience. I have revisited and carefully considered
the "QEMU QMP Reference Manual," especially from a reader's perspective.
Indeed, I agree that, as you mentioned, a three-level directory
(accelerator - kvm - kvm stuff) is more readable and easier to maintain.

For this question "what we want *first*, and only then how to get it", I
think my thought is:

First, the structure should be considered, and then the specific content
can be added. Once the structure is clearly defined, categorizing items
into their appropriate places becomes a natural process...

Then for this question "what section structure would make the most sense
for the QMP reference manual?", I understand that a top-down, clearly
defined hierarchical directory makes the most sense, allowing readers to
follow the structure to find what they want. Directly adding
kvm-pmu-filter.json or kvm.json would disrupt the entire structure, because
KVM is just one of the accelerators supported by QEMU. Using "accelerator"
as the entry point for the documentation, similar to the "accel" directory
in QEMU's source code, would make indexing more convenient.

> A few hints on how...
> 
> Consider how qapi/block.json includes qapi/block-core.json:
> 
>     ##
>     # = Block devices
>     ##
> 
>     { 'include': 'block-core.json' }
> 
>     ##
>     # == Additional block stuff (VM related)
>     ##
> 
> block-core.json starts with
> 
>     ##
>     # == Block core (VM unrelated)
>     ##
> 
> Together, this produces this section structure
> 
>     = Block devices
>     == 
>     ##
> 
> Together, this produces this section structure
> 
>     = Block devices
>     == Block core (VM unrelated)
>     == Additional block stuff (VM related)
> 
> Note that qapi/block-core.json isn't included anywhere else.
> qapi/qapi-schema.json advises:
> 
>     # Documentation generated with qapi-gen.py is in source order, with
>     # included sub-schemas inserted at the first include directive
>     # (subsequent include directives have no effect).  To get a sane and
>     # stable order, it's best to include each sub-schema just once, or
>     # include it first right here.

Thank you very much!!

Based on your inspiration, I think the ideal section structure for my
issue could be:

    = accelerator
    == KVM
    === PMU

Firstly, I should have a new accelerator.json () to include KVM stuff:

    ##
    # = Accelerator
    ##

    { 'include': 'kvm.json' }

Next, in kvm.json, I could organize stuffs like:

    ##
    # == KVM
    ##

    ##
    # === PMU stuff
    ##

    ... (the below are my current QPAI definitions.)

Is such a structure reasonable?

Thank you again for your guidance!

Regards,
Zhao




^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter
  2025-02-07 13:06               ` Markus Armbruster
@ 2025-04-01  7:53                 ` Zhao Liu
  0 siblings, 0 replies; 27+ messages in thread
From: Zhao Liu @ 2025-04-01  7:53 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Daniel P. Berrang�, Paolo Bonzini, Eric Blake,
	Michael Roth, Eduardo Habkost, Marcelo Tosatti, Shaoqin Huang,
	Eric Auger, Peter Maydell, Laurent Vivier, Thomas Huth,
	Sebastian Ott, Gavin Shan, qemu-devel, kvm, qemu-arm, Dapeng Mi,
	Yi Lai, Zhao Liu

> > +##
> > +# @KVMPMUX86SelectUmaskEvent:
> > +#
> > +# x86 PMU event encoding with select and umask.  Using the X86_PMU_RAW_EVENT
> > +# macro, the select and umask fields will be encoded into raw foramt and
> > +# delivered to KVM.
> 
> Doc comments are for the QMP reference manual, i.e. for users of QMP.
> Explaining the QMP interface in terms of its implementation in QEMU is
> not nice.

Thank you! I could add doc comments in kvm-pmu.h and delete redundant
comments from QMP reference manual.

Regards,
Zhao



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object
  2025-04-01  7:47             ` Zhao Liu
@ 2025-04-08  5:51               ` Markus Armbruster
  0 siblings, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2025-04-08  5:51 UTC (permalink / raw)
  To: Zhao Liu
  Cc: Paolo Bonzini, Eric Blake, Michael Roth, Daniel P . Berrangé,
	Eduardo Habkost, Marcelo Tosatti, Shaoqin Huang, Eric Auger,
	Peter Maydell, Laurent Vivier, Thomas Huth, Sebastian Ott,
	Gavin Shan, qemu-devel, kvm, qemu-arm, Dapeng Mi, Yi Lai

Zhao Liu <zhao1.liu@intel.com> writes:

> Hi Mrkus,
>
> I'm really sorry I completely missed your reply (and your patient
> advice). It wasn't until I looked back at the lore archives that I
> realized my mistake. Thinking it over again, I see that your reply,
> which I missed, really helped clear up my confusion:

I'm glad I was able to help some!

> On Fri, Feb 07, 2025 at 02:02:44PM +0100, Markus Armbruster wrote:
>> Date: Fri, 07 Feb 2025 14:02:44 +0100
>> From: Markus Armbruster <armbru@redhat.com>
>> Subject: Re: [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object
>> 
>> Zhao Liu <zhao1.liu@intel.com> writes:
>> 
>> >> Let's ignore how to place it for now, and focus on where we would *like*
>> >> to place it.
>> >> 
>> >> Is it related to anything other than ObjectType / ObjectOptions in the
>> >> QMP reference manual?
>> >
>> > Yes!
>> 
>> Now I'm confused :)
>> 
>> It is related to ObjectType / ObjectType.
>> 
>> Is it related to anything else in the QMP reference manual, and if yes,
>> to what exactly is it related?
>
> I misunderstood your point. The PMU stuff and the QAPI definitions for
> ObjectType/ObjectOptions are not related. They should belong to separate
> categories or sections.
>
>> >> I guess qapi/kvm.json is for KVM-specific stuff in general, not just the
>> >> KVM PMU filter.  Should we have a section for accelerator-specific
>> >> stuff, with subsections for the various accelerators?
>> >> 
>> >> [...]
>> >
>> > If we consider the accelerator from a top-down perspective, I understand
>> > that we need to add accelerator.json, kvm.json, and kvm-pmu-filter.json.
>> >
>> > The first two files are just to include subsections without any additional
>> > content. Is this overkill? Could we just add a single kvm-pmu-filter.json
>> > (I also considered this name, thinking that kvm might need to add more
>> > things in the future)?
>> >
>> > Of course, I lack experience with the file organization here. If you think
>> > the three-level sections (accelerator.json, kvm.json, and kvm-pmu-filter.json)
>> > is necessary, I am happy to try this way. :-)
>> 
>> We don't have to create files just to get a desired section structure.
>> 
>> I'll show you how in a jiffie, but before I do that, let me stress: we
>> should figure out what we want *first*, and only then how to get it.
>> So, what section structure would make the most sense for the QMP
>> reference manual?
>
> Thank you for your patience. I have revisited and carefully considered
> the "QEMU QMP Reference Manual," especially from a reader's perspective.
> Indeed, I agree that, as you mentioned, a three-level directory
> (accelerator - kvm - kvm stuff) is more readable and easier to maintain.

Sounds good to me.

> For this question "what we want *first*, and only then how to get it", I
> think my thought is:
>
> First, the structure should be considered, and then the specific content
> can be added. Once the structure is clearly defined, categorizing items
> into their appropriate places becomes a natural process...
>
> Then for this question "what section structure would make the most sense
> for the QMP reference manual?", I understand that a top-down, clearly
> defined hierarchical directory makes the most sense, allowing readers to
> follow the structure to find what they want. Directly adding
> kvm-pmu-filter.json or kvm.json would disrupt the entire structure, because
> KVM is just one of the accelerators supported by QEMU. Using "accelerator"
> as the entry point for the documentation, similar to the "accel" directory
> in QEMU's source code, would make indexing more convenient.

I think so, too.

>> A few hints on how...
>> 
>> Consider how qapi/block.json includes qapi/block-core.json:
>> 
>>     ##
>>     # = Block devices
>>     ##
>> 
>>     { 'include': 'block-core.json' }
>> 
>>     ##
>>     # == Additional block stuff (VM related)
>>     ##
>> 
>> block-core.json starts with
>> 
>>     ##
>>     # == Block core (VM unrelated)
>>     ##
>> 
>> Together, this produces this section structure
>> 
>>     = Block devices
>>     == 
>>     ##
>> 
>> Together, this produces this section structure
>> 
>>     = Block devices
>>     == Block core (VM unrelated)
>>     == Additional block stuff (VM related)
>> 
>> Note that qapi/block-core.json isn't included anywhere else.
>> qapi/qapi-schema.json advises:
>> 
>>     # Documentation generated with qapi-gen.py is in source order, with
>>     # included sub-schemas inserted at the first include directive
>>     # (subsequent include directives have no effect).  To get a sane and
>>     # stable order, it's best to include each sub-schema just once, or
>>     # include it first right here.
>
> Thank you very much!!
>
> Based on your inspiration, I think the ideal section structure for my
> issue could be:
>
>     = accelerator
>     == KVM
>     === PMU
>
> Firstly, I should have a new accelerator.json () to include KVM stuff:
>
>     ##
>     # = Accelerator
>     ##
>
>     { 'include': 'kvm.json' }
>
> Next, in kvm.json, I could organize stuffs like:
>
>     ##
>     # == KVM
>     ##
>
>     ##
>     # === PMU stuff
>     ##
>
>     ... (the below are my current QPAI definitions.)
>
> Is such a structure reasonable?

Yes.

I'm not a fan of schema files with nothing but includes, like
accelerator.json.  But the alternative here would be putting its
contents into qapi-schema.json, which I really don't like, or keeping
the KVM contents there instead of in a separate kvm.json, which would
interfere with tracking maintainers in the MAINTAINERS file.

> Thank you again for your guidance!

You're welcome!



^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2025-04-08  5:52 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-22  9:05 [RFC v2 0/5] accel/kvm: Support KVM PMU filter Zhao Liu
2025-01-22  9:05 ` [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object Zhao Liu
2025-02-05 10:03   ` Markus Armbruster
2025-02-06 10:19     ` Zhao Liu
2025-02-06 10:27       ` Zhao Liu
2025-02-06 12:13       ` Markus Armbruster
2025-02-06 14:32         ` Zhao Liu
2025-02-07 13:02           ` Markus Armbruster
2025-04-01  7:47             ` Zhao Liu
2025-04-08  5:51               ` Markus Armbruster
2025-01-22  9:05 ` [RFC v2 2/5] i386/kvm: Support basic KVM PMU filter Zhao Liu
2025-01-22  9:05 ` [RFC v2 3/5] i386/kvm: Support event with select & umask format in " Zhao Liu
2025-02-05 10:07   ` Markus Armbruster
2025-02-06  9:54     ` Zhao Liu
2025-02-06  9:42       ` Daniel P. Berrangé
2025-02-06 10:23         ` Zhao Liu
2025-02-06 10:24         ` Markus Armbruster
2025-02-06 14:22           ` Zhao Liu
2025-02-06 14:54             ` Zhao Liu
2025-02-07 13:06               ` Markus Armbruster
2025-04-01  7:53                 ` Zhao Liu
2025-01-22  9:05 ` [RFC v2 4/5] i386/kvm: Support event with masked entry " Zhao Liu
2025-01-22  9:05 ` [RFC v2 5/5] i386/kvm: Support fixed counter " Zhao Liu
2025-01-24  8:00 ` [RFC v2 0/5] accel/kvm: Support " Lai, Yi
2025-03-18  7:35 ` Shaoqin Huang
2025-03-21  3:43   ` Zhao Liu
2025-03-31  6:32     ` Shaoqin Huang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).