[PATCH 0/5] accel/kvm: Support KVM PMU filter

[PATCH 0/5] accel/kvm: Support KVM PMU filter

[Zhao Liu]

Hi all,

Now I've converted the previous RFC (v2) to PATCH.

Compared with RFC v2 [1], this version mianly have the following
changes:
 * Make PMU related QAPIs accept decimal value instead of string.
 * Introduce a three-level QAPI section to organize KVM PMU stuff.
 * Fix QAPI related style issues.
 * Rename "x86-default" format to "x86-select-umask".

Current pmu-filter QOM design could meet the requirements of both x86
and ARM sides.


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":196}]}'

For x86, 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 '{"qom-type":"kvm-pmu-filter","id":"f0","action":"allow","x86-fixed-counter":0,"events":[{"format":"x86-masked-entry","select":196,"mask":255,"match":0,"exclude":true},{"format":"x86-masked-entry","select":197,"mask":255,"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-select-umask 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.


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/20250122090517.294083-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              |   2 +
 accel/kvm/kvm-pmu.c      | 177 +++++++++++++++++++++++++++++++++++++++
 accel/kvm/meson.build    |   1 +
 include/system/kvm-pmu.h |  51 +++++++++++
 include/system/kvm_int.h |   2 +
 qapi/accelerator.json    |  14 ++++
 qapi/kvm.json            | 130 ++++++++++++++++++++++++++++
 qapi/meson.build         |   1 +
 qapi/qapi-schema.json    |   1 +
 qapi/qom.json            |   3 +
 qemu-options.hx          |  67 ++++++++++++++-
 target/i386/kvm/kvm.c    | 176 ++++++++++++++++++++++++++++++++++++++
 12 files changed, 624 insertions(+), 1 deletion(-)
 create mode 100644 accel/kvm/kvm-pmu.c
 create mode 100644 include/system/kvm-pmu.h
 create mode 100644 qapi/accelerator.json
 create mode 100644 qapi/kvm.json

-- 
2.34.1

[PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object

[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.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Tested-by: Yi Lai <yi1.lai@intel.com>
---
Changes since RFC v2:
 * Drop hexadecimal variants and support numeric version in QAPI
   directly. (Daniel)
 * Define three-level sections with new accelerator.json. (Markus)
 * QAPI style fixes:
   - KVMPMU* stuff -> KvmPmu*.
   - KVMPMUFilterProperty -> KVMPMUFilterProperties.
   - KVMPMUEventEncodeFmt -> KvmPmuEventFormat.
   - drop prefix in KvmPmuFilterAction and KvmPmuEventFormat.
 * Bump up the supported QAPI version to v10.1.
 * Add Tested-by from Yi.

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 v10.0.
---
 MAINTAINERS              |   2 +
 accel/kvm/kvm-pmu.c      | 114 +++++++++++++++++++++++++++++++++++++++
 accel/kvm/meson.build    |   1 +
 include/system/kvm-pmu.h |  35 ++++++++++++
 qapi/accelerator.json    |  14 +++++
 qapi/kvm.json            |  84 +++++++++++++++++++++++++++++
 qapi/meson.build         |   1 +
 qapi/qapi-schema.json    |   1 +
 qapi/qom.json            |   3 ++
 9 files changed, 255 insertions(+)
 create mode 100644 accel/kvm/kvm-pmu.c
 create mode 100644 include/system/kvm-pmu.h
 create mode 100644 qapi/accelerator.json
 create mode 100644 qapi/kvm.json

diff --git a/MAINTAINERS b/MAINTAINERS
index d54b5578f883..3ca551025fb8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -434,6 +434,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
@@ -503,6 +504,7 @@ F: accel/Makefile.objs
 F: accel/stubs/Makefile.objs
 F: cpu-common.c
 F: cpu-target.c
+F: qapi/accelerator.c
 F: system/cpus.c
 
 Apple Silicon HVF CPUs
diff --git a/accel/kvm/kvm-pmu.c b/accel/kvm/kvm-pmu.c
new file mode 100644
index 000000000000..22f749bf9183
--- /dev/null
+++ b/accel/kvm/kvm-pmu.c
@@ -0,0 +1,114 @@
+/*
+ * QEMU KVM PMU Related Abstractions
+ *
+ * Copyright (C) 2025 Intel Corporation.
+ *
+ * Author: Zhao Liu <zhao1.liu@intel.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#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);
+
+    visit_type_KvmPmuFilterEventList(v, name, &filter->events, errp);
+}
+
+static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
+                                     void *opaque, Error **errp)
+{
+    KVMPMUFilter *filter = KVM_PMU_FILTER(obj);
+    KvmPmuFilterEventList *head = NULL, *old_head, *node;
+    int nevents = 0;
+
+    old_head = filter->events;
+    if (!visit_type_KvmPmuFilterEventList(v, name, &head, errp)) {
+        return;
+    }
+
+    for (node = head; node; node = node->next) {
+        switch (node->value->format) {
+        case KVM_PMU_EVENT_FORMAT_RAW:
+            break;
+        default:
+            g_assert_not_reached();
+        }
+
+        nevents++;
+    }
+
+    filter->nevents = nevents;
+    filter->events = head;
+    qapi_free_KvmPmuFilterEventList(old_head);
+    return;
+}
+
+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", "KvmPmuFilterEventList",
+                              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);
+
+    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..818fa309c191
--- /dev/null
+++ b/include/system/kvm-pmu.h
@@ -0,0 +1,35 @@
+/*
+ * QEMU KVM PMU Related Abstraction Header
+ *
+ * Copyright (C) 2025 Intel Corporation.
+ *
+ * Author: Zhao Liu <zhao1.liu@intel.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#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)
+
+/**
+ * KVMPMUFilter:
+ * @action: action that KVM PMU filter will take for selected PMU events.
+ * @nevents: number of PMU event entries listed in @events
+ * @events: list of PMU event entries.  A PMU event entry may represent one
+ *    event or multiple events due to its format.
+ */
+struct KVMPMUFilter {
+    Object parent_obj;
+
+    KvmPmuFilterAction action;
+    uint32_t nevents;
+    KvmPmuFilterEventList *events;
+};
+
+#endif /* KVM_PMU_H */
diff --git a/qapi/accelerator.json b/qapi/accelerator.json
new file mode 100644
index 000000000000..1fe0d64be113
--- /dev/null
+++ b/qapi/accelerator.json
@@ -0,0 +1,14 @@
+# -*- Mode: Python -*-
+# vim: filetype=python
+#
+# Copyright (C) 2025 Intel Corporation.
+#
+# Author: Zhao Liu <zhao1.liu@intel.com>
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+##
+# = Accelerators
+##
+
+{ 'include': 'kvm.json' }
diff --git a/qapi/kvm.json b/qapi/kvm.json
new file mode 100644
index 000000000000..1861d86a9726
--- /dev/null
+++ b/qapi/kvm.json
@@ -0,0 +1,84 @@
+# -*- Mode: Python -*-
+# vim: filetype=python
+#
+# Copyright (C) 2025 Intel Corporation.
+#
+# Author: Zhao Liu <zhao1.liu@intel.com>
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+##
+# == KVM
+##
+
+##
+# === PMU stuff (KVM related)
+##
+
+##
+# @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.1
+##
+{ 'enum': 'KvmPmuFilterAction',
+  'data': ['allow', 'deny'] }
+
+##
+# @KvmPmuEventFormat:
+#
+# Encoding formats of PMU event that QEMU/KVM supports.
+#
+# @raw: the encoded event code that KVM can directly consume.
+#
+# Since 10.1
+##
+{ 'enum': 'KvmPmuEventFormat',
+  'data': ['raw'] }
+
+##
+# @KvmPmuRawEvent:
+#
+# Raw PMU event code.
+#
+# @code: the raw value that has been encoded, and QEMU could deliver
+#     to KVM directly.
+#
+# Since 10.1
+##
+{ 'struct': 'KvmPmuRawEvent',
+  'data': { 'code': 'uint64' } }
+
+##
+# @KvmPmuFilterEvent:
+#
+# PMU event filtered by KVM.
+#
+# @format: PMU event format.
+#
+# Since 10.1
+##
+{ 'union': 'KvmPmuFilterEvent',
+  'base': { 'format': 'KvmPmuEventFormat' },
+  'discriminator': 'format',
+  'data': { 'raw': 'KvmPmuRawEvent' } }
+
+##
+# @KvmPmuFilterProperties:
+#
+# Properties of KVM PMU Filter.
+#
+# @action: action that KVM PMU filter will take for selected PMU events.
+#
+# @events: list of selected PMU events.
+#
+# Since 10.1
+##
+{ 'struct': 'KvmPmuFilterProperties',
+  'data': { 'action': 'KvmPmuFilterAction',
+            '*events': ['KvmPmuFilterEvent'] } }
diff --git a/qapi/meson.build b/qapi/meson.build
index eadde4db307f..dba27ebc7489 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 c41c01eb2ab9..c7fed7940af7 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -66,6 +66,7 @@
 { 'include': 'compat.json' }
 { 'include': 'control.json' }
 { 'include': 'introspect.json' }
+{ 'include': 'accelerator.json' }
 { 'include': 'qom.json' }
 { 'include': 'qdev.json' }
 { 'include': 'machine-common.json' }
diff --git a/qapi/qom.json b/qapi/qom.json
index 28ce24cd8d08..517f4c06c260 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':             'KvmPmuFilterProperties',
       'main-loop':                  'MainLoopProperties',
       'memory-backend-epc':         { 'type': 'MemoryBackendEpcProperties',
                                       'if': 'CONFIG_LINUX' },
-- 
2.34.1

[PATCH 2/5] i386/kvm: Support basic KVM PMU filter

[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>
Tested-by: Yi Lai <yi1.lai@intel.com>
---
Changes since RFC v2:
 * Add documentation in qemu-options.hx.
 * Add Tested-by from Yi.

Changes since RFC 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 +
 qemu-options.hx          |  47 ++++++++++++++-
 target/i386/kvm/kvm.c    | 127 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 175 insertions(+), 1 deletion(-)

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/qemu-options.hx b/qemu-options.hx
index dc694a99a30a..51a7c61ce0b0 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -232,7 +232,8 @@ DEF("accel", HAS_ARG, QEMU_OPTION_accel,
     "                eager-split-size=n (KVM Eager Page Split chunk size, default 0, disabled. ARM only)\n"
     "                notify-vmexit=run|internal-error|disable,notify-window=n (enable notify VM exit and set notify window, x86 only)\n"
     "                thread=single|multi (enable multi-threaded TCG)\n"
-    "                device=path (KVM device path, default /dev/kvm)\n", QEMU_ARCH_ALL)
+    "                device=path (KVM device path, default /dev/kvm)\n"
+    "                pmu-filter=id (configure KVM PMU filter)\n", QEMU_ARCH_ALL)
 SRST
 ``-accel name[,prop=value[,...]]``
     This is used to enable an accelerator. Depending on the target
@@ -318,6 +319,10 @@ SRST
         option can be used to pass the KVM device to use via a file descriptor
         by setting the value to ``/dev/fdset/NN``.
 
+    ``pmu-filter=id``
+        Sets the id of KVM PMU filter object. This option can be used to set
+        whitelist or blacklist of PMU events for Guest.
+
 ERST
 
 DEF("smp", HAS_ARG, QEMU_OPTION_smp,
@@ -6144,6 +6149,46 @@ SRST
         ::
 
             (qemu) qom-set /objects/iothread1 poll-max-ns 100000
+
+    ``-object '{"qom-type":"kvm-pmu-filter","id":id,"action":action,"events":[entry_list]}'``
+        Create a kvm-pmu-filter object that configures KVM to filter
+        selected PMU events for Guest.
+
+        This option must be written in JSON format to support ``events``
+        JSON list.
+
+        The ``action`` parameter sets the action that KVM will take for
+        the selected PMU events. It accepts ``allow`` or ``deny``. If
+        the action is set to ``allow``, all PMU events except the
+        selected ones will be disabled and blocked in the Guest. But if
+        the action is set to ``deny``, then only the selected events
+        will be denied, while all other events can be accessed normally
+        in the Guest.
+
+        The ``events`` parameter accepts a list of PMU event entries in
+        JSON format. Event entries, based on different encoding formats,
+        have the following types:
+
+        ``{"format":"raw","code":raw_code}``
+            Encode the single PMU event with raw format. The ``code``
+            parameter accepts raw code of a PMU event. For x86, the raw
+            code represents a combination of umask and event select:
+
+        ::
+
+            (((select & 0xf00UL) << 24) | \
+             ((select) & 0xff) | \
+             ((umask) & 0xff) << 8)
+
+        An example KVM PMU filter object would look like:
+
+        .. parsed-literal::
+
+             # |qemu_system| \\
+                 ... \\
+                 -accel kvm,pmu-filter=id \\
+                 -object '{"qom-type":"kvm-pmu-filter","id":"f0","action":"allow","events":[{"format":"raw","code":196}]}' \\
+                 ...
 ERST
 
 
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 6c749d4ee812..fa3a696654cb 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,18 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
         }
     }
 
+    /*
+     * TODO: Move this chunk to kvm_arch_pre_create_vcpu() and check
+     * whether pmu is enabled there.
+     */
+    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 +5956,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_FORMAT_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 +6627,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_FORMAT_RAW:
+            break;
+        default:
+            error_setg(errp,
+                       "Unsupported PMU event format %s.",
+                       KvmPmuEventFormat_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 +6695,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

[PATCH 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter

[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>
Tested-by: Yi Lai <yi1.lai@intel.com>
---
Changes since RFC v2:
 * Drop hexadecimal variants and support numeric version in QAPI
   directly. (Daniel)
 * Rename "x86-default" format to "x86-select-umask". (Markus)
 * Add Tested-by from Yi.
 * Add documentation in qemu-options.hx.
 * QAPI style fix:
   - KVMPMU* stuff -> KvmPmu*.
 * Bump up the supported QAPI version to v10.1.

Changes since RFC v1:
 * Bump up the supported QAPI version to v10.0.
---
 accel/kvm/kvm-pmu.c      | 20 +++++++++++++++++++-
 include/system/kvm-pmu.h | 13 +++++++++++++
 qapi/kvm.json            | 21 +++++++++++++++++++--
 qemu-options.hx          |  3 +++
 target/i386/kvm/kvm.c    |  5 +++++
 5 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/accel/kvm/kvm-pmu.c b/accel/kvm/kvm-pmu.c
index 22f749bf9183..fa73ef428e59 100644
--- a/accel/kvm/kvm-pmu.c
+++ b/accel/kvm/kvm-pmu.c
@@ -16,6 +16,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)
 {
@@ -53,9 +55,22 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
     }
 
     for (node = head; node; node = node->next) {
-        switch (node->value->format) {
+        KvmPmuFilterEvent *event = node->value;
+
+        switch (event->format) {
         case KVM_PMU_EVENT_FORMAT_RAW:
             break;
+        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
+            if (event->u.x86_select_umask.select > UINT12_MAX) {
+                error_setg(errp,
+                           "Parameter 'select' out of range (%d).",
+                           UINT12_MAX);
+                goto fail;
+            }
+
+            /* No need to check the range of umask since it's uint8_t. */
+            break;
+        }
         default:
             g_assert_not_reached();
         }
@@ -67,6 +82,9 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
     filter->events = head;
     qapi_free_KvmPmuFilterEventList(old_head);
     return;
+
+fail:
+    qapi_free_KvmPmuFilterEventList(head);
 }
 
 static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
diff --git a/include/system/kvm-pmu.h b/include/system/kvm-pmu.h
index 818fa309c191..6abc0d037aee 100644
--- a/include/system/kvm-pmu.h
+++ b/include/system/kvm-pmu.h
@@ -32,4 +32,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 1861d86a9726..cb151ca82e5c 100644
--- a/qapi/kvm.json
+++ b/qapi/kvm.json
@@ -36,10 +36,12 @@
 #
 # @raw: the encoded event code that KVM can directly consume.
 #
+# @x86-select-umask: standard x86 encoding format with select and umask.
+#
 # Since 10.1
 ##
 { 'enum': 'KvmPmuEventFormat',
-  'data': ['raw'] }
+  'data': ['raw', 'x86-select-umask'] }
 
 ##
 # @KvmPmuRawEvent:
@@ -54,6 +56,20 @@
 { 'struct': 'KvmPmuRawEvent',
   'data': { 'code': 'uint64' } }
 
+##
+# @KvmPmuX86SelectUmaskEvent:
+#
+# @select: x86 PMU event select field, which is a 12-bit unsigned
+#     number.
+#
+# @umask: x86 PMU event umask field.
+#
+# Since 10.1
+##
+{ 'struct': 'KvmPmuX86SelectUmaskEvent',
+  'data': { 'select': 'uint16',
+            'umask': 'uint8' } }
+
 ##
 # @KvmPmuFilterEvent:
 #
@@ -66,7 +82,8 @@
 { 'union': 'KvmPmuFilterEvent',
   'base': { 'format': 'KvmPmuEventFormat' },
   'discriminator': 'format',
-  'data': { 'raw': 'KvmPmuRawEvent' } }
+  'data': { 'raw': 'KvmPmuRawEvent',
+            'x86-select-umask': 'KvmPmuX86SelectUmaskEvent' } }
 
 ##
 # @KvmPmuFilterProperties:
diff --git a/qemu-options.hx b/qemu-options.hx
index 51a7c61ce0b0..5dcce067d8dd 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -6180,6 +6180,9 @@ SRST
              ((select) & 0xff) | \
              ((umask) & 0xff) << 8)
 
+        ``{"format":"x86-select-umask","select":event_select,"umask":event_umask}``
+            Specify the single x86 PMU event with select and umask fields.
+
         An example KVM PMU filter object would look like:
 
         .. parsed-literal::
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index fa3a696654cb..0d36ccf250ed 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -5974,6 +5974,10 @@ static bool kvm_config_pmu_event(KVMPMUFilter *filter,
         case KVM_PMU_EVENT_FORMAT_RAW:
             code = event->u.raw.code;
             break;
+        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
+            code = X86_PMU_RAW_EVENT(event->u.x86_select_umask.select,
+                                     event->u.x86_select_umask.umask);
+            break;
         default:
             g_assert_not_reached();
         }
@@ -6644,6 +6648,7 @@ static void kvm_arch_check_pmu_filter(const Object *obj, const char *name,
 
         switch (event->format) {
         case KVM_PMU_EVENT_FORMAT_RAW:
+        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
             break;
         default:
             error_setg(errp,
-- 
2.34.1

[PATCH 4/5] i386/kvm: Support event with masked entry format in KVM PMU filter

[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>
Tested-by: Yi Lai <yi1.lai@intel.com>
---
Changes since RFC v2:
 * Drop hexadecimal variants and support numeric version in QAPI
   directly. (Daniel)
 * Add Tested-by from Yi.
 * Add documentation in qemu-options.hx.
 * QAPI style fix:
   - KVMPMU* stuff -> KvmPmu*.
 * Bump up the supported QAPI version to v10.1.

Changes since RFC v1:
 * Bump up the supported QAPI version to v10.0.
---
 accel/kvm/kvm-pmu.c   | 14 ++++++++++++++
 qapi/kvm.json         | 29 +++++++++++++++++++++++++++--
 qemu-options.hx       | 13 +++++++++++++
 target/i386/kvm/kvm.c | 35 +++++++++++++++++++++++++++++++++++
 4 files changed, 89 insertions(+), 2 deletions(-)

diff --git a/accel/kvm/kvm-pmu.c b/accel/kvm/kvm-pmu.c
index fa73ef428e59..9205907d1779 100644
--- a/accel/kvm/kvm-pmu.c
+++ b/accel/kvm/kvm-pmu.c
@@ -71,6 +71,20 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
             /* No need to check the range of umask since it's uint8_t. */
             break;
         }
+        case KVM_PMU_EVENT_FORMAT_X86_MASKED_ENTRY: {
+            if (event->u.x86_masked_entry.select > UINT12_MAX) {
+                error_setg(errp,
+                           "Parameter 'select' out of range (%d).",
+                           UINT12_MAX);
+                goto fail;
+            }
+
+            /*
+             * No need to check the range of match or mask fields since
+             * they're both uint8_t.
+             */
+            break;
+        }
         default:
             g_assert_not_reached();
         }
diff --git a/qapi/kvm.json b/qapi/kvm.json
index cb151ca82e5c..1b523e058731 100644
--- a/qapi/kvm.json
+++ b/qapi/kvm.json
@@ -38,10 +38,13 @@
 #
 # @x86-select-umask: 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.1
 ##
 { 'enum': 'KvmPmuEventFormat',
-  'data': ['raw', 'x86-select-umask'] }
+  'data': ['raw', 'x86-select-umask', 'x86-masked-entry'] }
 
 ##
 # @KvmPmuRawEvent:
@@ -70,6 +73,27 @@
   'data': { 'select': 'uint16',
             'umask': 'uint8' } }
 
+##
+# @KvmPmuX86MaskedEntry:
+#
+# x86 PMU events encoding in KVM masked entry format.
+#
+# @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.1
+##
+{ 'struct': 'KvmPmuX86MaskedEntry',
+  'data': { 'select': 'uint16',
+            'match': 'uint8',
+            'mask': 'uint8',
+            'exclude': 'bool' } }
+
 ##
 # @KvmPmuFilterEvent:
 #
@@ -83,7 +107,8 @@
   'base': { 'format': 'KvmPmuEventFormat' },
   'discriminator': 'format',
   'data': { 'raw': 'KvmPmuRawEvent',
-            'x86-select-umask': 'KvmPmuX86SelectUmaskEvent' } }
+            'x86-select-umask': 'KvmPmuX86SelectUmaskEvent',
+            'x86-masked-entry': 'KvmPmuX86MaskedEntry' } }
 
 ##
 # @KvmPmuFilterProperties:
diff --git a/qemu-options.hx b/qemu-options.hx
index 5dcce067d8dd..bb89198971e0 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -6183,6 +6183,19 @@ SRST
         ``{"format":"x86-select-umask","select":event_select,"umask":event_umask}``
             Specify the single x86 PMU event with select and umask fields.
 
+        ``{"format":"x86-masked-entry","select":event_select,"mask":entry_mask,"match":entry_match,"exclude":exclude}``
+            Configure a set of x86 PMU events that share the same
+            ``select`` field. The events are determined by a formula
+            that checks if an event's umask is included:
+
+        ::
+
+            event_umask & entry_mask == entry_match
+
+            The "exclude" parameter controls whether to exclude the
+            events selected based on the above formula, under the given
+            "select" field.
+
         An example KVM PMU filter object would look like:
 
         .. parsed-literal::
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 0d36ccf250ed..8786501e9c7e 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -5978,6 +5978,13 @@ static bool kvm_config_pmu_event(KVMPMUFilter *filter,
             code = X86_PMU_RAW_EVENT(event->u.x86_select_umask.select,
                                      event->u.x86_select_umask.umask);
             break;
+        case KVM_PMU_EVENT_FORMAT_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();
         }
@@ -6009,6 +6016,17 @@ static int kvm_install_pmu_event_filter(KVMState *s)
         g_assert_not_reached();
     }
 
+    kvm_filter->flags = filter->events->value->format ==
+                        KVM_PMU_EVENT_FORMAT_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;
     }
@@ -6636,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,
@@ -6643,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_FORMAT_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_FORMAT_RAW:
         case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
+            flag = 0;
+            break;
+        case KVM_PMU_EVENT_FORMAT_X86_MASKED_ENTRY:
+            flag = KVM_PMU_EVENT_FLAG_MASKED_EVENTS;
             break;
         default:
             error_setg(errp,
@@ -6657,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

[PATCH 5/5] i386/kvm: Support fixed counter in KVM PMU filter

[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>
Tested-by: Yi Lai <yi1.lai@intel.com>
---
Changes since RFC v2:
 * Drop KVMPMUX86FixedCounter structure and use uint32_t to represent
   bitmap in QAPI directly.
 * Add Tested-by from Yi.
 * Add documentation in qemu-options.hx.
 * Bump up the supported QAPI version to v10.1.

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 v10.0.
---
 accel/kvm/kvm-pmu.c      | 31 +++++++++++++++++++++++++++++++
 include/system/kvm-pmu.h |  5 ++++-
 qapi/kvm.json            |  6 +++++-
 qemu-options.hx          |  6 +++++-
 target/i386/kvm/kvm.c    | 39 ++++++++++++++++++++++++---------------
 5 files changed, 69 insertions(+), 18 deletions(-)

diff --git a/accel/kvm/kvm-pmu.c b/accel/kvm/kvm-pmu.c
index 9205907d1779..509d69d9c515 100644
--- a/accel/kvm/kvm-pmu.c
+++ b/accel/kvm/kvm-pmu.c
@@ -101,6 +101,29 @@ 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);
+
+    visit_type_uint32(v, name, &filter->x86_fixed_counter, errp);
+}
+
+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);
+    uint32_t counter;
+
+    if (!visit_type_uint32(v, name, &counter, errp)) {
+        return;
+    }
+
+    filter->x86_fixed_counter = counter;
+}
+
 static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
 {
     object_class_property_add_enum(oc, "action", "KvmPmuFilterAction",
@@ -116,6 +139,14 @@ 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", "uint32_t",
+                              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 6abc0d037aee..5238b2b4dcc7 100644
--- a/include/system/kvm-pmu.h
+++ b/include/system/kvm-pmu.h
@@ -19,10 +19,12 @@ OBJECT_DECLARE_SIMPLE_TYPE(KVMPMUFilter, KVM_PMU_FILTER)
 
 /**
  * KVMPMUFilter:
- * @action: action that KVM PMU filter will take for selected PMU events.
+ * @action: action that KVM PMU filter will take for selected PMU events
+ *    and counters.
  * @nevents: number of PMU event entries listed in @events
  * @events: list of PMU event entries.  A PMU event entry may represent one
  *    event or multiple events due to its format.
+ * @x86_fixed_counter: bitmap of x86 fixed counter.
  */
 struct KVMPMUFilter {
     Object parent_obj;
@@ -30,6 +32,7 @@ struct KVMPMUFilter {
     KvmPmuFilterAction action;
     uint32_t nevents;
     KvmPmuFilterEventList *events;
+    uint32_t x86_fixed_counter;
 };
 
 /*
diff --git a/qapi/kvm.json b/qapi/kvm.json
index 1b523e058731..5374c8340e5a 100644
--- a/qapi/kvm.json
+++ b/qapi/kvm.json
@@ -115,7 +115,10 @@
 #
 # Properties of KVM PMU Filter.
 #
-# @action: action that KVM PMU filter will take for selected PMU events.
+# @action: action that KVM PMU filter will take for selected PMU events
+#     and counters.
+#
+# @x86-fixed-counter: enablement bitmap of x86 fixed counters.
 #
 # @events: list of selected PMU events.
 #
@@ -123,4 +126,5 @@
 ##
 { 'struct': 'KvmPmuFilterProperties',
   'data': { 'action': 'KvmPmuFilterAction',
+            '*x86-fixed-counter': 'uint32',
             '*events': ['KvmPmuFilterEvent'] } }
diff --git a/qemu-options.hx b/qemu-options.hx
index bb89198971e0..eadfb69c8876 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -6150,7 +6150,7 @@ SRST
 
             (qemu) qom-set /objects/iothread1 poll-max-ns 100000
 
-    ``-object '{"qom-type":"kvm-pmu-filter","id":id,"action":action,"events":[entry_list]}'``
+    ``-object '{"qom-type":"kvm-pmu-filter","id":id,"x86-fixed-counter":bitmap,"action":action,"events":[entry_list]}'``
         Create a kvm-pmu-filter object that configures KVM to filter
         selected PMU events for Guest.
 
@@ -6165,6 +6165,10 @@ SRST
         will be denied, while all other events can be accessed normally
         in the Guest.
 
+        The ``x86-fixed-counter`` parameter sets a bitmap of x86 fixed
+        counters, and ``action`` will also take effect on the selected
+        fixed counters.
+
         The ``events`` parameter accepts a list of PMU event entries in
         JSON format. Event entries, based on different encoding formats,
         have the following types:
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 8786501e9c7e..8b916dbb5d6f 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -6016,19 +6016,25 @@ static int kvm_install_pmu_event_filter(KVMState *s)
         g_assert_not_reached();
     }
 
-    kvm_filter->flags = filter->events->value->format ==
-                        KVM_PMU_EVENT_FORMAT_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;
     }
 
-    if (!kvm_config_pmu_event(filter, kvm_filter)) {
-        goto fail;
+    if (filter->nevents) {
+        kvm_filter->flags = filter->events->value->format ==
+                            KVM_PMU_EVENT_FORMAT_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_FORMAT_X86_MASKED_ENTRY ?
-                KVM_PMU_EVENT_FLAG_MASKED_EVENTS : 0;
+    base_flag = 0;
+    if (filter->nevents &&
+        events->value->format == KVM_PMU_EVENT_FORMAT_X86_MASKED_ENTRY) {
+        base_flag = KVM_PMU_EVENT_FLAG_MASKED_EVENTS;
+    }
+
     while (events) {
         KvmPmuFilterEvent *event = events->value;
         uint32_t flag;
-- 
2.34.1

Re: [PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object

[Markus Armbruster]

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

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

Remind me, what does the kvm-pmu-filter object do, and why would we want
to use it?

> The raw format, as a native PMU event code representation, can be used
> for several architectures.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Tested-by: Yi Lai <yi1.lai@intel.com>

Re: [PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object

[Zhao Liu]

Hi Markus

On Thu, Apr 10, 2025 at 04:21:01PM +0200, Markus Armbruster wrote:
> Date: Thu, 10 Apr 2025 16:21:01 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > Introduce the kvm-pmu-filter object and support the PMU event with raw
> > format.
> 
> Remind me, what does the kvm-pmu-filter object do, and why would we
> want to use it?

KVM PMU filter allows user space to set PMU event whitelist / blacklist
for Guest. Both ARM and x86's KVMs accept a list of PMU events, and x86
also accpets other formats & fixed counter field.

The earliest version uses custom parsing rules, which is not flexible
enough to scale. So to support such complex configuration in cli, it's
best to define and parse it via QAPI, and it's best to support the JSON
way.

Based on these considerations, I found "object" to be a suitable enough
choice.

Thus kvm-pmu-filter object handles all the complexity of parsing values
from cli, and it can include some checks that QAPI cannot include (such
as the 12-bit limit).

Thanks,
Zhao

Re: [PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object

[Markus Armbruster]

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

> Hi Markus
>
> On Thu, Apr 10, 2025 at 04:21:01PM +0200, Markus Armbruster wrote:
>> Date: Thu, 10 Apr 2025 16:21:01 +0200
>> From: Markus Armbruster <armbru@redhat.com>
>> Subject: Re: [PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object
>> 
>> Zhao Liu <zhao1.liu@intel.com> writes:
>> 
>> > Introduce the kvm-pmu-filter object and support the PMU event with raw
>> > format.
>> 
>> Remind me, what does the kvm-pmu-filter object do, and why would we
>> want to use it?
>
> KVM PMU filter allows user space to set PMU event whitelist / blacklist
> for Guest. Both ARM and x86's KVMs accept a list of PMU events, and x86
> also accpets other formats & fixed counter field.

But what does the system *do* with these event lists?

> The earliest version uses custom parsing rules, which is not flexible
> enough to scale. So to support such complex configuration in cli, it's
> best to define and parse it via QAPI, and it's best to support the JSON
> way.
>
> Based on these considerations, I found "object" to be a suitable enough
> choice.
>
> Thus kvm-pmu-filter object handles all the complexity of parsing values
> from cli, and it can include some checks that QAPI cannot include (such
> as the 12-bit limit).
>
> Thanks,
> Zhao

Re: [PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object

[Zhao Liu]

On Fri, Apr 11, 2025 at 06:38:35AM +0200, Markus Armbruster wrote:
> Date: Fri, 11 Apr 2025 06:38:35 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > Hi Markus
> >
> > On Thu, Apr 10, 2025 at 04:21:01PM +0200, Markus Armbruster wrote:
> >> Date: Thu, 10 Apr 2025 16:21:01 +0200
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Subject: Re: [PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object
> >> 
> >> Zhao Liu <zhao1.liu@intel.com> writes:
> >> 
> >> > Introduce the kvm-pmu-filter object and support the PMU event with raw
> >> > format.
> >> 
> >> Remind me, what does the kvm-pmu-filter object do, and why would we
> >> want to use it?
> >
> > KVM PMU filter allows user space to set PMU event whitelist / blacklist
> > for Guest. Both ARM and x86's KVMs accept a list of PMU events, and x86
> > also accpets other formats & fixed counter field.
> 
> But what does the system *do* with these event lists?

This is for security purposes, and can restrict Guest users from
accessing certain sensitive hardware information on the Host via perf or
PMU counter.

When a PMU event is blocked by KVM, Guest users can't get the
corresponding event count via perf/PMU counter.

EMM, if ‘system’ refers to the QEMU part, then QEMU is responsible
for checking the format and passing the list to KVM.

Thanks,
Zhao

Re: [PATCH 0/5] accel/kvm: Support KVM PMU filter

[Shaoqin Huang]

Hi Zhao,

I've added some code and test it on ARM64, it works very well.

And after reviewing the code, it looks good to me.

Reviewed-by: Shaoqin Huang <shahuang@redhat.com>

On 4/9/25 4:26 PM, Zhao Liu wrote:
> Hi all,
> 
> Now I've converted the previous RFC (v2) to PATCH.
> 
> Compared with RFC v2 [1], this version mianly have the following
> changes:
>   * Make PMU related QAPIs accept decimal value instead of string.
>   * Introduce a three-level QAPI section to organize KVM PMU stuff.
>   * Fix QAPI related style issues.
>   * Rename "x86-default" format to "x86-select-umask".
> 
> Current pmu-filter QOM design could meet the requirements of both x86
> and ARM sides.
> 
> 
> 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":196}]}'
> 
> For x86, 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 '{"qom-type":"kvm-pmu-filter","id":"f0","action":"allow","x86-fixed-counter":0,"events":[{"format":"x86-masked-entry","select":196,"mask":255,"match":0,"exclude":true},{"format":"x86-masked-entry","select":197,"mask":255,"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-select-umask 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.
> 
> 
> 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/20250122090517.294083-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              |   2 +
>   accel/kvm/kvm-pmu.c      | 177 +++++++++++++++++++++++++++++++++++++++
>   accel/kvm/meson.build    |   1 +
>   include/system/kvm-pmu.h |  51 +++++++++++
>   include/system/kvm_int.h |   2 +
>   qapi/accelerator.json    |  14 ++++
>   qapi/kvm.json            | 130 ++++++++++++++++++++++++++++
>   qapi/meson.build         |   1 +
>   qapi/qapi-schema.json    |   1 +
>   qapi/qom.json            |   3 +
>   qemu-options.hx          |  67 ++++++++++++++-
>   target/i386/kvm/kvm.c    | 176 ++++++++++++++++++++++++++++++++++++++
>   12 files changed, 624 insertions(+), 1 deletion(-)
>   create mode 100644 accel/kvm/kvm-pmu.c
>   create mode 100644 include/system/kvm-pmu.h
>   create mode 100644 qapi/accelerator.json
>   create mode 100644 qapi/kvm.json
> 

-- 
Shaoqin

Re: [PATCH 0/5] accel/kvm: Support KVM PMU filter

[Zhao Liu]

On Tue, Apr 15, 2025 at 03:49:59PM +0800, Shaoqin Huang wrote:
> Date: Tue, 15 Apr 2025 15:49:59 +0800
> From: Shaoqin Huang <shahuang@redhat.com>
> Subject: Re: [PATCH 0/5] accel/kvm: Support KVM PMU filter
> 
> Hi Zhao,
> 
> I've added some code and test it on ARM64, it works very well.
> 
> And after reviewing the code, it looks good to me.
> 
> Reviewed-by: Shaoqin Huang <shahuang@redhat.com>

Thank you Shaoqin for your support!

Regards,
Zhao

Re: [PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object

[Markus Armbruster]

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

> On Fri, Apr 11, 2025 at 06:38:35AM +0200, Markus Armbruster wrote:
>> Date: Fri, 11 Apr 2025 06:38:35 +0200
>> From: Markus Armbruster <armbru@redhat.com>
>> Subject: Re: [PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object
>> 
>> Zhao Liu <zhao1.liu@intel.com> writes:
>> 
>> > Hi Markus
>> >
>> > On Thu, Apr 10, 2025 at 04:21:01PM +0200, Markus Armbruster wrote:
>> >> Date: Thu, 10 Apr 2025 16:21:01 +0200
>> >> From: Markus Armbruster <armbru@redhat.com>
>> >> Subject: Re: [PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object
>> >> 
>> >> Zhao Liu <zhao1.liu@intel.com> writes:
>> >> 
>> >> > Introduce the kvm-pmu-filter object and support the PMU event with raw
>> >> > format.
>> >> 
>> >> Remind me, what does the kvm-pmu-filter object do, and why would we
>> >> want to use it?
>> >
>> > KVM PMU filter allows user space to set PMU event whitelist / blacklist
>> > for Guest. Both ARM and x86's KVMs accept a list of PMU events, and x86
>> > also accpets other formats & fixed counter field.
>> 
>> But what does the system *do* with these event lists?
>
> This is for security purposes, and can restrict Guest users from
> accessing certain sensitive hardware information on the Host via perf or
> PMU counter.
>
> When a PMU event is blocked by KVM, Guest users can't get the
> corresponding event count via perf/PMU counter.
>
> EMM, if ‘system’ refers to the QEMU part, then QEMU is responsible
> for checking the format and passing the list to KVM.
>
> Thanks,
> Zhao

This helped some, thanks.  To make sure I got it:

KVM can restrict the guest's access to the PMU.  This is either a
whitelist (guest can access exactly what's on this list), or a blacklist
(guest can access exactly what's not this list).

QEMU's kvm-pmu-filter object provides an interface to this KVM feature.

KVM takes "raw" list entries: an entry is a number, and the number's
meaning depends on the architecture.  The kvm-pmu-filter object can take
such entries, and passes them to straight to KVM.

On x86, we commonly use two slightly higher level formats: select &
umask, and masked.  The kvm-pmu-filter object can take entries in either
format, and maps them to "raw".

Correct?

Re: [PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object

[Zhao Liu]

Hi Markus,

> > This is for security purposes, and can restrict Guest users from
> > accessing certain sensitive hardware information on the Host via perf or
> > PMU counter.
> >
> > When a PMU event is blocked by KVM, Guest users can't get the
> > corresponding event count via perf/PMU counter.
> >
> > EMM, if ‘system’ refers to the QEMU part, then QEMU is responsible
> > for checking the format and passing the list to KVM.
> >
> > Thanks,
> > Zhao
> 
> This helped some, thanks.  To make sure I got it:
> 
> KVM can restrict the guest's access to the PMU.  This is either a
> whitelist (guest can access exactly what's on this list), or a blacklist
> (guest can access exactly what's not this list).

Yes! The "action" field controls if it's a "whitelist" (allow) or
"blacklist" (deny).

And "access" means Guest could get the event count, if "no access", then
Guest would get nothing.

For example, if we set a the whitelist ony for the event (select: 0xc4,
umask: 0) in QEMU:

pmu='{"qom-type":"kvm-pmu-filter","id":"f0","action":"allow","events":[{"format":"x86-select-umask","select":196,"umask":0}]}'

then in Guest, this command tries to get count of 2 events:

perf stat -e cpu/event=0xc4,name=branches/,cpu/event=0xc5,name=branch-misses/ sleep 1

Since another event (select: 0xc5, umask: 0) is not on whitelist, its
"access" is blocked by KVM, so user would get the result like:

 Performance counter stats for 'sleep 1':

            348709      branches
                 0      branch-misses

       1.015962921 seconds time elapsed

       0.000000000 seconds user
       0.015195000 seconds sys

The "allowed" event has the normal output, and the result of "denied"
event is zero.

> QEMU's kvm-pmu-filter object provides an interface to this KVM feature.

Yes!

> KVM takes "raw" list entries: an entry is a number, and the number's
> meaning depends on the architecture. 

Yes, and meaning also depends on format. masked-entry format has special
meaning (with a flag).

> The kvm-pmu-filter object can take such entries, and passes them to
> straight to KVM.
> 
> On x86, we commonly use two slightly higher level formats: select &
> umask, and masked.  The kvm-pmu-filter object can take entries in either
> format, and maps them to "raw".
>
> Correct?

Yes, Markus, you're right! (And sorry for late reply.)

And "raw" format as a lower level format can be used for other arches
(e.g., ARM).

Thanks,
Zhao

Re: [PATCH 5/5] i386/kvm: Support fixed counter in KVM PMU filter

[Mi, Dapeng]

On 4/9/2025 4:26 PM, Zhao Liu wrote:
> 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>
> Tested-by: Yi Lai <yi1.lai@intel.com>
> ---
> Changes since RFC v2:
>  * Drop KVMPMUX86FixedCounter structure and use uint32_t to represent
>    bitmap in QAPI directly.
>  * Add Tested-by from Yi.
>  * Add documentation in qemu-options.hx.
>  * Bump up the supported QAPI version to v10.1.
>
> 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 v10.0.
> ---
>  accel/kvm/kvm-pmu.c      | 31 +++++++++++++++++++++++++++++++
>  include/system/kvm-pmu.h |  5 ++++-
>  qapi/kvm.json            |  6 +++++-
>  qemu-options.hx          |  6 +++++-
>  target/i386/kvm/kvm.c    | 39 ++++++++++++++++++++++++---------------
>  5 files changed, 69 insertions(+), 18 deletions(-)
>
> diff --git a/accel/kvm/kvm-pmu.c b/accel/kvm/kvm-pmu.c
> index 9205907d1779..509d69d9c515 100644
> --- a/accel/kvm/kvm-pmu.c
> +++ b/accel/kvm/kvm-pmu.c
> @@ -101,6 +101,29 @@ 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);
> +
> +    visit_type_uint32(v, name, &filter->x86_fixed_counter, errp);
> +}
> +
> +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);
> +    uint32_t counter;
> +
> +    if (!visit_type_uint32(v, name, &counter, errp)) {
> +        return;
> +    }
> +
> +    filter->x86_fixed_counter = counter;
> +}
> +
>  static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
>  {
>      object_class_property_add_enum(oc, "action", "KvmPmuFilterAction",
> @@ -116,6 +139,14 @@ 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", "uint32_t",
> +                              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 6abc0d037aee..5238b2b4dcc7 100644
> --- a/include/system/kvm-pmu.h
> +++ b/include/system/kvm-pmu.h
> @@ -19,10 +19,12 @@ OBJECT_DECLARE_SIMPLE_TYPE(KVMPMUFilter, KVM_PMU_FILTER)
>  
>  /**
>   * KVMPMUFilter:
> - * @action: action that KVM PMU filter will take for selected PMU events.
> + * @action: action that KVM PMU filter will take for selected PMU events
> + *    and counters.

Maybe more accurate "fixed counters".


>   * @nevents: number of PMU event entries listed in @events
>   * @events: list of PMU event entries.  A PMU event entry may represent one
>   *    event or multiple events due to its format.
> + * @x86_fixed_counter: bitmap of x86 fixed counter.
>   */
>  struct KVMPMUFilter {
>      Object parent_obj;
> @@ -30,6 +32,7 @@ struct KVMPMUFilter {
>      KvmPmuFilterAction action;
>      uint32_t nevents;
>      KvmPmuFilterEventList *events;
> +    uint32_t x86_fixed_counter;
>  };
>  
>  /*
> diff --git a/qapi/kvm.json b/qapi/kvm.json
> index 1b523e058731..5374c8340e5a 100644
> --- a/qapi/kvm.json
> +++ b/qapi/kvm.json
> @@ -115,7 +115,10 @@
>  #
>  # Properties of KVM PMU Filter.
>  #
> -# @action: action that KVM PMU filter will take for selected PMU events.
> +# @action: action that KVM PMU filter will take for selected PMU events
> +#     and counters.
> +#
> +# @x86-fixed-counter: enablement bitmap of x86 fixed counters.
>  #
>  # @events: list of selected PMU events.
>  #
> @@ -123,4 +126,5 @@
>  ##
>  { 'struct': 'KvmPmuFilterProperties',
>    'data': { 'action': 'KvmPmuFilterAction',
> +            '*x86-fixed-counter': 'uint32',
>              '*events': ['KvmPmuFilterEvent'] } }
> diff --git a/qemu-options.hx b/qemu-options.hx
> index bb89198971e0..eadfb69c8876 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -6150,7 +6150,7 @@ SRST
>  
>              (qemu) qom-set /objects/iothread1 poll-max-ns 100000
>  
> -    ``-object '{"qom-type":"kvm-pmu-filter","id":id,"action":action,"events":[entry_list]}'``
> +    ``-object '{"qom-type":"kvm-pmu-filter","id":id,"x86-fixed-counter":bitmap,"action":action,"events":[entry_list]}'``
>          Create a kvm-pmu-filter object that configures KVM to filter
>          selected PMU events for Guest.
>  
> @@ -6165,6 +6165,10 @@ SRST
>          will be denied, while all other events can be accessed normally
>          in the Guest.
>  
> +        The ``x86-fixed-counter`` parameter sets a bitmap of x86 fixed
> +        counters, and ``action`` will also take effect on the selected
> +        fixed counters.
> +
>          The ``events`` parameter accepts a list of PMU event entries in
>          JSON format. Event entries, based on different encoding formats,
>          have the following types:
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 8786501e9c7e..8b916dbb5d6f 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -6016,19 +6016,25 @@ static int kvm_install_pmu_event_filter(KVMState *s)
>          g_assert_not_reached();
>      }
>  
> -    kvm_filter->flags = filter->events->value->format ==
> -                        KVM_PMU_EVENT_FORMAT_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;
>      }
>  
> -    if (!kvm_config_pmu_event(filter, kvm_filter)) {
> -        goto fail;
> +    if (filter->nevents) {
> +        kvm_filter->flags = filter->events->value->format ==
> +                            KVM_PMU_EVENT_FORMAT_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_FORMAT_X86_MASKED_ENTRY ?
> -                KVM_PMU_EVENT_FLAG_MASKED_EVENTS : 0;
> +    base_flag = 0;
> +    if (filter->nevents &&
> +        events->value->format == KVM_PMU_EVENT_FORMAT_X86_MASKED_ENTRY) {
> +        base_flag = KVM_PMU_EVENT_FLAG_MASKED_EVENTS;
> +    }
> +
>      while (events) {
>          KvmPmuFilterEvent *event = events->value;
>          uint32_t flag;

Re: [PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object

[Markus Armbruster]

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.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Tested-by: Yi Lai <yi1.lai@intel.com>

[...]

> diff --git a/qapi/accelerator.json b/qapi/accelerator.json
> new file mode 100644
> index 000000000000..1fe0d64be113
> --- /dev/null
> +++ b/qapi/accelerator.json
> @@ -0,0 +1,14 @@
> +# -*- Mode: Python -*-
> +# vim: filetype=python
> +#
> +# Copyright (C) 2025 Intel Corporation.
> +#
> +# Author: Zhao Liu <zhao1.liu@intel.com>
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +##
> +# = Accelerators
> +##
> +
> +{ 'include': 'kvm.json' }
> diff --git a/qapi/kvm.json b/qapi/kvm.json
> new file mode 100644
> index 000000000000..1861d86a9726
> --- /dev/null
> +++ b/qapi/kvm.json
> @@ -0,0 +1,84 @@
> +# -*- Mode: Python -*-
> +# vim: filetype=python
> +#
> +# Copyright (C) 2025 Intel Corporation.
> +#
> +# Author: Zhao Liu <zhao1.liu@intel.com>
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +##
> +# == KVM
> +##

There's KVM-specific stuff elsewhere in the schema.  Some if of it
should probably be moved here.  Can you have a look?  This is not a
demand; it's fine if you can't.  If you can: separate patch preceding
this one to create kvm.json and move stuff there.

> +
> +##
> +# === PMU stuff (KVM related)

The KVM subsection contains just this subsubsection.  Awkward.  Can we
do without this subsubsection now?  We can always add it later, when we
have enough KVM stuff to warrant structuring it into subsubsections.

If we decide we want it:

   # === KVM performance monitor unit (PMU)

> +##
> +
> +##
> +# @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.1
> +##
> +{ 'enum': 'KvmPmuFilterAction',
> +  'data': ['allow', 'deny'] }
> +
> +##
> +# @KvmPmuEventFormat:

Maybe KvmPmuFilterEventFormat?  Or is that too long?

> +#
> +# Encoding formats of PMU event that QEMU/KVM supports.
> +#
> +# @raw: the encoded event code that KVM can directly consume.

Suggest

   # @raw: raw KVM PMU event code.

> +#
> +# Since 10.1
> +##
> +{ 'enum': 'KvmPmuEventFormat',
> +  'data': ['raw'] }
> +
> +##
> +# @KvmPmuRawEvent:

Maybe KvmPmuFilterEventRaw?  Or is that too long?

> +#
> +# Raw PMU event code.
> +#
> +# @code: the raw value that has been encoded, and QEMU could deliver
> +#     to KVM directly.

Suggest

   ##
   # @KvmPmuRawEvent
   #
   # @code: raw KVM PMU event code, to be passed verbatim to KVM.

> +#
> +# Since 10.1
> +##
> +{ 'struct': 'KvmPmuRawEvent',
> +  'data': { 'code': 'uint64' } }
> +
> +##
> +# @KvmPmuFilterEvent:
> +#
> +# PMU event filtered by KVM.

Suggest

   # A KVM PMU event specification.

> +#
> +# @format: PMU event format.
> +#
> +# Since 10.1
> +##
> +{ 'union': 'KvmPmuFilterEvent',
> +  'base': { 'format': 'KvmPmuEventFormat' },
> +  'discriminator': 'format',
> +  'data': { 'raw': 'KvmPmuRawEvent' } }
> +
> +##
> +# @KvmPmuFilterProperties:
> +#
> +# Properties of KVM PMU Filter.
> +#
> +# @action: action that KVM PMU filter will take for selected PMU events.
> +#
> +# @events: list of selected PMU events.

Here's my try:

   # Properties of kvm-pmu-filter objects.  A kvm-pmu-filter object
   # restricts the guest's access to the PMU with either an allowlist or
   # a denylist.
   #
   # @action: whether @events is an allowlist or a denylist.
   #
   # @events: list of KVM PMU event specifications.

> +#
> +# Since 10.1
> +##
> +{ 'struct': 'KvmPmuFilterProperties',
> +  'data': { 'action': 'KvmPmuFilterAction',
> +            '*events': ['KvmPmuFilterEvent'] } }
> diff --git a/qapi/meson.build b/qapi/meson.build
> index eadde4db307f..dba27ebc7489 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 c41c01eb2ab9..c7fed7940af7 100644
> --- a/qapi/qapi-schema.json
> +++ b/qapi/qapi-schema.json
> @@ -66,6 +66,7 @@
>  { 'include': 'compat.json' }
>  { 'include': 'control.json' }
>  { 'include': 'introspect.json' }
> +{ 'include': 'accelerator.json' }
>  { 'include': 'qom.json' }
>  { 'include': 'qdev.json' }
>  { 'include': 'machine-common.json' }
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 28ce24cd8d08..517f4c06c260 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':             'KvmPmuFilterProperties',
>        'main-loop':                  'MainLoopProperties',
>        'memory-backend-epc':         { 'type': 'MemoryBackendEpcProperties',
>                                        'if': 'CONFIG_LINUX' },

Re: [PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object

[Zhao Liu]

> > diff --git a/qapi/kvm.json b/qapi/kvm.json
> > new file mode 100644
> > index 000000000000..1861d86a9726
> > --- /dev/null
> > +++ b/qapi/kvm.json
> > @@ -0,0 +1,84 @@
> > +# -*- Mode: Python -*-
> > +# vim: filetype=python
> > +#
> > +# Copyright (C) 2025 Intel Corporation.
> > +#
> > +# Author: Zhao Liu <zhao1.liu@intel.com>
> > +#
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +##
> > +# == KVM
> > +##
> 
> There's KVM-specific stuff elsewhere in the schema.  Some if of it
> should probably be moved here.  Can you have a look?  This is not a
> demand; it's fine if you can't.  If you can: separate patch preceding
> this one to create kvm.json and move stuff there.

Sure! That's what I should have done, and I'll be back to follow up on
this discussion when I get something new.

> > +
> > +##
> > +# === PMU stuff (KVM related)
> 
> The KVM subsection contains just this subsubsection.  Awkward.  Can we
> do without this subsubsection now?  We can always add it later, when we
> have enough KVM stuff to warrant structuring it into subsubsections.

Thanks! I agree. As I commit to do above, if I find others about KVM,
we can add this subsection you suggested below :-).

> If we decide we want it:
> 
>    # === KVM performance monitor unit (PMU)

Good name.

> > +##
> > +
> > +##
> > +# @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.1
> > +##
> > +{ 'enum': 'KvmPmuFilterAction',
> > +  'data': ['allow', 'deny'] }
> > +
> > +##
> > +# @KvmPmuEventFormat:
> 
> Maybe KvmPmuFilterEventFormat?  Or is that too long?

For another 2 formats: 'x86-select-umask' and 'x86-masked-entry', their
enum value names already have 7 words:

 - KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK
 - KVM_PMU_EVENT_FORMAT_X86_MASKED_ENTRY

With "Filter" in name,

 - KVM_PMU_FILTER_EVENT_FORMAT_X86_SELECT_UMASK
 - KVM_PMU_FILTER_EVENT_FORMAT_X86_MASKED_ENTRY

Look still okay. I'll rename it.

> > +#
> > +# Encoding formats of PMU event that QEMU/KVM supports.
> > +#
> > +# @raw: the encoded event code that KVM can directly consume.
> 
> Suggest
> 
>    # @raw: raw KVM PMU event code.

Concise. I agree.

> > +#
> > +# Since 10.1
> > +##
> > +{ 'enum': 'KvmPmuEventFormat',
> > +  'data': ['raw'] }
> > +
> > +##
> > +# @KvmPmuRawEvent:
> 
> Maybe KvmPmuFilterEventRaw?  Or is that too long?

KvmPmuFilterEventRaw is fine (not too long).

> > +#
> > +# Raw PMU event code.
> > +#
> > +# @code: the raw value that has been encoded, and QEMU could deliver
> > +#     to KVM directly.
> 
> Suggest
> 
>    ##
>    # @KvmPmuRawEvent
>    #
>    # @code: raw KVM PMU event code, to be passed verbatim to KVM.

Thanks for polishing it up, it looks much better.

> > +#
> > +# Since 10.1
> > +##
> > +{ 'struct': 'KvmPmuRawEvent',
> > +  'data': { 'code': 'uint64' } }
> > +
> > +##
> > +# @KvmPmuFilterEvent:
> > +#
> > +# PMU event filtered by KVM.
> 
> Suggest
> 
>    # A KVM PMU event specification.

Sure.

> > +#
> > +# @format: PMU event format.
> > +#
> > +# Since 10.1
> > +##
> > +{ 'union': 'KvmPmuFilterEvent',
> > +  'base': { 'format': 'KvmPmuEventFormat' },
> > +  'discriminator': 'format',
> > +  'data': { 'raw': 'KvmPmuRawEvent' } }
> > +
> > +##
> > +# @KvmPmuFilterProperties:
> > +#
> > +# Properties of KVM PMU Filter.
> > +#
> > +# @action: action that KVM PMU filter will take for selected PMU events.
> > +#
> > +# @events: list of selected PMU events.
> 
> Here's my try:
> 
>    # Properties of kvm-pmu-filter objects.  A kvm-pmu-filter object
>    # restricts the guest's access to the PMU with either an allowlist or
>    # a denylist.
>    #
>    # @action: whether @events is an allowlist or a denylist.
>    #
>    # @events: list of KVM PMU event specifications.

Thank you very much! Your description is very accurate.

Regards,
Zhao

Re: [PATCH 5/5] i386/kvm: Support fixed counter in KVM PMU filter

[Zhao Liu]

> >  /**
> >   * KVMPMUFilter:
> > - * @action: action that KVM PMU filter will take for selected PMU events.
> > + * @action: action that KVM PMU filter will take for selected PMU events
> > + *    and counters.
> 
> Maybe more accurate "fixed counters".

Yes, will fix.

Thanks,
Zhao

Re: [PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object

[Markus Armbruster]

Philippe, there's a question for you on target-specific QAPI schema.

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.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Tested-by: Yi Lai <yi1.lai@intel.com>

[...]

> diff --git a/accel/kvm/kvm-pmu.c b/accel/kvm/kvm-pmu.c
> new file mode 100644
> index 000000000000..22f749bf9183
> --- /dev/null
> +++ b/accel/kvm/kvm-pmu.c

[...]

> +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)

The new file is compiled into the binary when CONFIG_KVM.  Therefore,
object kvm-pmu-filter is available exactly then.  Makes sense.
However, ...

[...]

> diff --git a/qapi/kvm.json b/qapi/kvm.json
> new file mode 100644
> index 000000000000..1861d86a9726
> --- /dev/null
> +++ b/qapi/kvm.json
> @@ -0,0 +1,84 @@
> +# -*- Mode: Python -*-
> +# vim: filetype=python
> +#
> +# Copyright (C) 2025 Intel Corporation.
> +#
> +# Author: Zhao Liu <zhao1.liu@intel.com>
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +##
> +# == KVM
> +##
> +
> +##
> +# === PMU stuff (KVM related)
> +##
> +
> +##
> +# @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.1
> +##
> +{ 'enum': 'KvmPmuFilterAction',
> +  'data': ['allow', 'deny'] }
> +
> +##
> +# @KvmPmuEventFormat:
> +#
> +# Encoding formats of PMU event that QEMU/KVM supports.
> +#
> +# @raw: the encoded event code that KVM can directly consume.
> +#
> +# Since 10.1
> +##
> +{ 'enum': 'KvmPmuEventFormat',
> +  'data': ['raw'] }
> +
> +##
> +# @KvmPmuRawEvent:
> +#
> +# Raw PMU event code.
> +#
> +# @code: the raw value that has been encoded, and QEMU could deliver
> +#     to KVM directly.
> +#
> +# Since 10.1
> +##
> +{ 'struct': 'KvmPmuRawEvent',
> +  'data': { 'code': 'uint64' } }
> +
> +##
> +# @KvmPmuFilterEvent:
> +#
> +# PMU event filtered by KVM.
> +#
> +# @format: PMU event format.
> +#
> +# Since 10.1
> +##
> +{ 'union': 'KvmPmuFilterEvent',
> +  'base': { 'format': 'KvmPmuEventFormat' },
> +  'discriminator': 'format',
> +  'data': { 'raw': 'KvmPmuRawEvent' } }
> +
> +##
> +# @KvmPmuFilterProperties:
> +#
> +# Properties of KVM PMU Filter.
> +#
> +# @action: action that KVM PMU filter will take for selected PMU events.
> +#
> +# @events: list of selected PMU events.
> +#
> +# Since 10.1
> +##
> +{ 'struct': 'KvmPmuFilterProperties',
> +  'data': { 'action': 'KvmPmuFilterAction',
> +            '*events': ['KvmPmuFilterEvent'] } }

... the QAPI schema doesn't reflect that.

To make it reflect, we'd have to add 'if': 'CONFIG_KVM'.  Since
CONFIG_KVM can only be used in target-specific code, we'd have to put
the definitions in a target-specific schema module kvm-target.json.

This makes the headers generated for the module target-specific, which
can be inconvenient.  Whether it's inconvenient here, I can't say.

I understand target-specific QAPI modules are problematic for the single
binary / heterogeneous machine work.  Philippe, thoughts on this one?

[...]

Re: [PATCH 2/5] i386/kvm: Support basic KVM PMU filter

[Markus Armbruster]

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

> 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>
> Tested-by: Yi Lai <yi1.lai@intel.com>
> ---
> Changes since RFC v2:
>  * Add documentation in qemu-options.hx.
>  * Add Tested-by from Yi.
>
> Changes since RFC 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 +
>  qemu-options.hx          |  47 ++++++++++++++-
>  target/i386/kvm/kvm.c    | 127 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 175 insertions(+), 1 deletion(-)
>
> 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/qemu-options.hx b/qemu-options.hx
> index dc694a99a30a..51a7c61ce0b0 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -232,7 +232,8 @@ DEF("accel", HAS_ARG, QEMU_OPTION_accel,
>      "                eager-split-size=n (KVM Eager Page Split chunk size, default 0, disabled. ARM only)\n"
>      "                notify-vmexit=run|internal-error|disable,notify-window=n (enable notify VM exit and set notify window, x86 only)\n"
>      "                thread=single|multi (enable multi-threaded TCG)\n"
> -    "                device=path (KVM device path, default /dev/kvm)\n", QEMU_ARCH_ALL)
> +    "                device=path (KVM device path, default /dev/kvm)\n"
> +    "                pmu-filter=id (configure KVM PMU filter)\n", QEMU_ARCH_ALL)

As we'll see below, this property is actually available only for i386.
Other target-specific properties document this like "x86 only".  Please
do that for this one, too.

As far as I can tell, the kvm-pmu-filter object needs to be activated
with -accel pmu-filter=... to do anything.  Correct?

You can create any number of kvm-pmu-filter objects, but only one of
them can be active.  Correct?

>  SRST
>  ``-accel name[,prop=value[,...]]``
>      This is used to enable an accelerator. Depending on the target
> @@ -318,6 +319,10 @@ SRST
>          option can be used to pass the KVM device to use via a file descriptor
>          by setting the value to ``/dev/fdset/NN``.
>  
> +    ``pmu-filter=id``
> +        Sets the id of KVM PMU filter object. This option can be used to set
> +        whitelist or blacklist of PMU events for Guest.

Well, "this option" can't actually be used to set the lists.  That's to
be done with -object kvm-pmu-filter.  Perhaps:

           Activate a KVM PMU filter object.  That object can be used to
           filter guest access to PMU events.

> +
>  ERST
>  
>  DEF("smp", HAS_ARG, QEMU_OPTION_smp,
> @@ -6144,6 +6149,46 @@ SRST
>          ::
>  
>              (qemu) qom-set /objects/iothread1 poll-max-ns 100000
> +
> +    ``-object '{"qom-type":"kvm-pmu-filter","id":id,"action":action,"events":[entry_list]}'``

Should this be in the previous patch?

> +        Create a kvm-pmu-filter object that configures KVM to filter
> +        selected PMU events for Guest.

The object doesn't actually configure KVM.  It merely holds the filter
configuration.  The configuring is done by the KVM accelerator according
to configuration in the connected kvm-pmu-filter object.  Perhaps:

           Create a kvm-pmu-filter object to hold PMU event filter
           configuration.

> +
> +        This option must be written in JSON format to support ``events``
> +        JSON list.
> +
> +        The ``action`` parameter sets the action that KVM will take for
> +        the selected PMU events. It accepts ``allow`` or ``deny``. If
> +        the action is set to ``allow``, all PMU events except the
> +        selected ones will be disabled and blocked in the Guest. But if
> +        the action is set to ``deny``, then only the selected events
> +        will be denied, while all other events can be accessed normally
> +        in the Guest.

I recommend "guest" instead of "Guest".

> +
> +        The ``events`` parameter accepts a list of PMU event entries in
> +        JSON format. Event entries, based on different encoding formats,
> +        have the following types:
> +
> +        ``{"format":"raw","code":raw_code}``
> +            Encode the single PMU event with raw format. The ``code``
> +            parameter accepts raw code of a PMU event. For x86, the raw
> +            code represents a combination of umask and event select:
> +
> +        ::
> +
> +            (((select & 0xf00UL) << 24) | \
> +             ((select) & 0xff) | \
> +             ((umask) & 0xff) << 8)

Does it?  Could the code also represent a combination of select, match,
and mask (masked entry format)?

> +
> +        An example KVM PMU filter object would look like:
> +
> +        .. parsed-literal::
> +
> +             # |qemu_system| \\
> +                 ... \\
> +                 -accel kvm,pmu-filter=id \\
> +                 -object '{"qom-type":"kvm-pmu-filter","id":"f0","action":"allow","events":[{"format":"raw","code":196}]}' \\
> +                 ...
>  ERST
>  
>  
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 6c749d4ee812..fa3a696654cb 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,18 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>          }
>      }
>  
> +    /*
> +     * TODO: Move this chunk to kvm_arch_pre_create_vcpu() and check

I can't see a function kvm_arch_pre_create_vcpu().

> +     * whether pmu is enabled there.

PMU

> +     */
> +    if (s->pmu_filter) {
> +        ret = kvm_filter_pmu_event(s);
> +        if (ret < 0) {
> +            error_report("Could not set KVM PMU filter");

When kvm_filter_pmu_event() failed, it already reported an error.
Reporting it another time can be confusing.

> +            return ret;
> +        }
> +    }
> +
>      return 0;
>  }
>  
> @@ -5942,6 +5956,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_FORMAT_RAW:
> +            code = event->u.raw.code;
> +            break;
> +        default:
> +            g_assert_not_reached();
> +        }
> +
> +        kvm_filter->events[idx++] = code;
> +        events = events->next;
> +    }
> +
> +    return true;
> +}

This function cannot fail.  Please return void, and simplify its caller.

> +
> +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));

Should we use sizeof(filter->events[0])?

> +
> +    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));

Suggest something like "can't set KVM PMU event filter".

> +        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.");

Error message should be a single phrase with no trailing punctuation.
More of the same below.

> +        return -1;
> +    }
> +
> +    return kvm_install_pmu_event_filter(s);
> +}
> +
>  static bool has_sgx_provisioning;
>  
>  static bool __kvm_enable_sgx_provisioning(KVMState *s)
> @@ -6537,6 +6627,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.");

Why is this an error?

action=allow with an empty would be the obvious way to allow nothing,
wouldn't it?

> +        return;
> +    }
> +
> +    while (events) {
> +        KvmPmuFilterEvent *event = events->value;
> +
> +        switch (event->format) {
> +        case KVM_PMU_EVENT_FORMAT_RAW:
> +            break;
> +        default:
> +            error_setg(errp,
> +                       "Unsupported PMU event format %s.",
> +                       KvmPmuEventFormat_str(events->value->format));

Unreachable.

> +            return;
> +        }
> +
> +        events = events->next;
> +    }
> +}
> +
>  void kvm_arch_accel_class_init(ObjectClass *oc)
>  {
>      object_class_property_add_enum(oc, "notify-vmexit", "NotifyVMexitOption",
> @@ -6576,6 +6695,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)

target/i386/kvm/kvm.c is compiled into the binary only for i386 target
with CONFIG_KVM.

The kvm-pmu-filter-object exists for any target with CONFIG_KVM.  But
it's usable only for i386.

I think the previous patch's commit message should state the role of the
kvm-pmu-filter-object more clearly: hold KVM PMU filter configuration
for any target with KVM.  This patch's commit message should then
explain what the patch does: enable actual use of the
kvm-pmu-filter-object for i386 only.  Other targets are left for another
day.

Re: [PATCH 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter

[Markus Armbruster]

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.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Tested-by: Yi Lai <yi1.lai@intel.com>
> ---
> Changes since RFC v2:
>  * Drop hexadecimal variants and support numeric version in QAPI
>    directly. (Daniel)
>  * Rename "x86-default" format to "x86-select-umask". (Markus)
>  * Add Tested-by from Yi.
>  * Add documentation in qemu-options.hx.
>  * QAPI style fix:
>    - KVMPMU* stuff -> KvmPmu*.
>  * Bump up the supported QAPI version to v10.1.
>
> Changes since RFC v1:
>  * Bump up the supported QAPI version to v10.0.
> ---
>  accel/kvm/kvm-pmu.c      | 20 +++++++++++++++++++-
>  include/system/kvm-pmu.h | 13 +++++++++++++
>  qapi/kvm.json            | 21 +++++++++++++++++++--
>  qemu-options.hx          |  3 +++
>  target/i386/kvm/kvm.c    |  5 +++++
>  5 files changed, 59 insertions(+), 3 deletions(-)
>
> diff --git a/accel/kvm/kvm-pmu.c b/accel/kvm/kvm-pmu.c
> index 22f749bf9183..fa73ef428e59 100644
> --- a/accel/kvm/kvm-pmu.c
> +++ b/accel/kvm/kvm-pmu.c
> @@ -16,6 +16,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)
>  {
> @@ -53,9 +55,22 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
>      }
>  
>      for (node = head; node; node = node->next) {
> -        switch (node->value->format) {
> +        KvmPmuFilterEvent *event = node->value;
> +
> +        switch (event->format) {
>          case KVM_PMU_EVENT_FORMAT_RAW:
>              break;
> +        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
> +            if (event->u.x86_select_umask.select > UINT12_MAX) {
> +                error_setg(errp,
> +                           "Parameter 'select' out of range (%d).",
> +                           UINT12_MAX);
> +                goto fail;
> +            }
> +
> +            /* No need to check the range of umask since it's uint8_t. */
> +            break;
> +        }

As we'll see below, the new x86-specific format is defined in the QAPI
schema regardless of target.

It is accepted here also regardless of target.  Doesn't matter much
right now, as the object is effectively useless for targets other than
x86, but I understand that will change.

Should we reject it unless the target is x86?

If not, I feel the behavior should be noted in the commit message.

>          default:
>              g_assert_not_reached();
>          }
> @@ -67,6 +82,9 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
>      filter->events = head;
>      qapi_free_KvmPmuFilterEventList(old_head);
>      return;
> +
> +fail:
> +    qapi_free_KvmPmuFilterEventList(head);
>  }
>  
>  static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
> diff --git a/include/system/kvm-pmu.h b/include/system/kvm-pmu.h
> index 818fa309c191..6abc0d037aee 100644
> --- a/include/system/kvm-pmu.h
> +++ b/include/system/kvm-pmu.h
> @@ -32,4 +32,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 1861d86a9726..cb151ca82e5c 100644
> --- a/qapi/kvm.json
> +++ b/qapi/kvm.json
> @@ -36,10 +36,12 @@
>  #
>  # @raw: the encoded event code that KVM can directly consume.
>  #
> +# @x86-select-umask: standard x86 encoding format with select and umask.
> +#
>  # Since 10.1
>  ##
>  { 'enum': 'KvmPmuEventFormat',
> -  'data': ['raw'] }
> +  'data': ['raw', 'x86-select-umask'] }
>  
>  ##
>  # @KvmPmuRawEvent:
> @@ -54,6 +56,20 @@
>  { 'struct': 'KvmPmuRawEvent',
>    'data': { 'code': 'uint64' } }
>  
> +##
> +# @KvmPmuX86SelectUmaskEvent:
> +#
> +# @select: x86 PMU event select field, which is a 12-bit unsigned
> +#     number.
> +#
> +# @umask: x86 PMU event umask field.
> +#
> +# Since 10.1
> +##
> +{ 'struct': 'KvmPmuX86SelectUmaskEvent',
> +  'data': { 'select': 'uint16',
> +            'umask': 'uint8' } }
> +
>  ##
>  # @KvmPmuFilterEvent:
>  #
> @@ -66,7 +82,8 @@
>  { 'union': 'KvmPmuFilterEvent',
>    'base': { 'format': 'KvmPmuEventFormat' },
>    'discriminator': 'format',
> -  'data': { 'raw': 'KvmPmuRawEvent' } }
> +  'data': { 'raw': 'KvmPmuRawEvent',
> +            'x86-select-umask': 'KvmPmuX86SelectUmaskEvent' } }
>  
>  ##
>  # @KvmPmuFilterProperties:

Documentation could perhaps be more explicit about this making sense
only for x86.

> diff --git a/qemu-options.hx b/qemu-options.hx
> index 51a7c61ce0b0..5dcce067d8dd 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -6180,6 +6180,9 @@ SRST
>               ((select) & 0xff) | \
>               ((umask) & 0xff) << 8)
>  
> +        ``{"format":"x86-select-umask","select":event_select,"umask":event_umask}``
> +            Specify the single x86 PMU event with select and umask fields.
> +
>          An example KVM PMU filter object would look like:
>  
>          .. parsed-literal::
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index fa3a696654cb..0d36ccf250ed 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -5974,6 +5974,10 @@ static bool kvm_config_pmu_event(KVMPMUFilter *filter,
>          case KVM_PMU_EVENT_FORMAT_RAW:
>              code = event->u.raw.code;
>              break;
> +        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
> +            code = X86_PMU_RAW_EVENT(event->u.x86_select_umask.select,
> +                                     event->u.x86_select_umask.umask);
> +            break;
>          default:
>              g_assert_not_reached();
>          }
> @@ -6644,6 +6648,7 @@ static void kvm_arch_check_pmu_filter(const Object *obj, const char *name,
>  
>          switch (event->format) {
>          case KVM_PMU_EVENT_FORMAT_RAW:
> +        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
>              break;
>          default:
>              error_setg(errp,

Re: [PATCH 4/5] i386/kvm: Support event with masked entry format in KVM PMU filter

[Markus Armbruster]

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

> 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>
> Tested-by: Yi Lai <yi1.lai@intel.com>

My review comments on PATCH 3 apply here as well.

Re: [PATCH 5/5] i386/kvm: Support fixed counter in KVM PMU filter

[Philippe Mathieu-Daudé]

On 9/4/25 10:26, Zhao Liu wrote:
> 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>
> Tested-by: Yi Lai <yi1.lai@intel.com>
> ---
> Changes since RFC v2:
>   * Drop KVMPMUX86FixedCounter structure and use uint32_t to represent
>     bitmap in QAPI directly.
>   * Add Tested-by from Yi.
>   * Add documentation in qemu-options.hx.
>   * Bump up the supported QAPI version to v10.1.
> 
> 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 v10.0.
> ---
>   accel/kvm/kvm-pmu.c      | 31 +++++++++++++++++++++++++++++++
>   include/system/kvm-pmu.h |  5 ++++-
>   qapi/kvm.json            |  6 +++++-
>   qemu-options.hx          |  6 +++++-
>   target/i386/kvm/kvm.c    | 39 ++++++++++++++++++++++++---------------
>   5 files changed, 69 insertions(+), 18 deletions(-)
> 
> diff --git a/accel/kvm/kvm-pmu.c b/accel/kvm/kvm-pmu.c
> index 9205907d1779..509d69d9c515 100644
> --- a/accel/kvm/kvm-pmu.c
> +++ b/accel/kvm/kvm-pmu.c
> @@ -101,6 +101,29 @@ 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);
> +
> +    visit_type_uint32(v, name, &filter->x86_fixed_counter, errp);
> +}
> +
> +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);
> +    uint32_t counter;
> +
> +    if (!visit_type_uint32(v, name, &counter, errp)) {
> +        return;
> +    }
> +
> +    filter->x86_fixed_counter = counter;
> +}
> +
>   static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
>   {
>       object_class_property_add_enum(oc, "action", "KvmPmuFilterAction",
> @@ -116,6 +139,14 @@ 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", "uint32_t",
> +                              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");

Adding that x86-specific field to all architectures is a bit dubious.

>   }
>   
>   static void kvm_pmu_filter_instance_init(Object *obj)
> diff --git a/include/system/kvm-pmu.h b/include/system/kvm-pmu.h
> index 6abc0d037aee..5238b2b4dcc7 100644
> --- a/include/system/kvm-pmu.h
> +++ b/include/system/kvm-pmu.h
> @@ -19,10 +19,12 @@ OBJECT_DECLARE_SIMPLE_TYPE(KVMPMUFilter, KVM_PMU_FILTER)
>   
>   /**
>    * KVMPMUFilter:
> - * @action: action that KVM PMU filter will take for selected PMU events.
> + * @action: action that KVM PMU filter will take for selected PMU events
> + *    and counters.
>    * @nevents: number of PMU event entries listed in @events
>    * @events: list of PMU event entries.  A PMU event entry may represent one
>    *    event or multiple events due to its format.
> + * @x86_fixed_counter: bitmap of x86 fixed counter.
>    */
>   struct KVMPMUFilter {
>       Object parent_obj;
> @@ -30,6 +32,7 @@ struct KVMPMUFilter {
>       KvmPmuFilterAction action;
>       uint32_t nevents;
>       KvmPmuFilterEventList *events;
> +    uint32_t x86_fixed_counter;
>   };

Re: [PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object

[Philippe Mathieu-Daudé]

Hi Zhao,

On 24/4/25 08:33, Zhao Liu wrote:
> Hi Markus,
> 
>>> This is for security purposes, and can restrict Guest users from
>>> accessing certain sensitive hardware information on the Host via perf or
>>> PMU counter.
>>>
>>> When a PMU event is blocked by KVM, Guest users can't get the
>>> corresponding event count via perf/PMU counter.
>>>
>>> EMM, if ‘system’ refers to the QEMU part, then QEMU is responsible
>>> for checking the format and passing the list to KVM.
>>>
>>> Thanks,
>>> Zhao
>>
>> This helped some, thanks.  To make sure I got it:
>>
>> KVM can restrict the guest's access to the PMU.  This is either a
>> whitelist (guest can access exactly what's on this list), or a blacklist
>> (guest can access exactly what's not this list).
> 
> Yes! The "action" field controls if it's a "whitelist" (allow) or
> "blacklist" (deny).
> 
> And "access" means Guest could get the event count, if "no access", then
> Guest would get nothing.
> 
> For example, if we set a the whitelist ony for the event (select: 0xc4,
> umask: 0) in QEMU:
> 
> pmu='{"qom-type":"kvm-pmu-filter","id":"f0","action":"allow","events":[{"format":"x86-select-umask","select":196,"umask":0}]}'
> 
> then in Guest, this command tries to get count of 2 events:
> 
> perf stat -e cpu/event=0xc4,name=branches/,cpu/event=0xc5,name=branch-misses/ sleep 1
> 
> Since another event (select: 0xc5, umask: 0) is not on whitelist, its
> "access" is blocked by KVM, so user would get the result like:
> 
>   Performance counter stats for 'sleep 1':
> 
>              348709      branches
>                   0      branch-misses
> 
>         1.015962921 seconds time elapsed
> 
>         0.000000000 seconds user
>         0.015195000 seconds sys
> 
> The "allowed" event has the normal output, and the result of "denied"
> event is zero.
> 
>> QEMU's kvm-pmu-filter object provides an interface to this KVM feature.
> 
> Yes!
> 
>> KVM takes "raw" list entries: an entry is a number, and the number's
>> meaning depends on the architecture.
> 
> Yes, and meaning also depends on format. masked-entry format has special
> meaning (with a flag).
> 
>> The kvm-pmu-filter object can take such entries, and passes them to
>> straight to KVM.
>>
>> On x86, we commonly use two slightly higher level formats: select &
>> umask, and masked.  The kvm-pmu-filter object can take entries in either
>> format, and maps them to "raw".
>>
>> Correct?
> 
> Yes, Markus, you're right! (And sorry for late reply.)
> 
> And "raw" format as a lower level format can be used for other arches
> (e.g., ARM).

Since you provide the ability to use a raw format, are we sure other
accelerators will never be interested in such PMU filtering?

I'm pretty sure HVF could benefit of it (whether we implement it there
is another story).

What do you think about adding this as a generic accelerator feature.
If a particular accel doesn't support it and we ask to filter, we simply
report an error.

Re: [PATCH 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter

[Zhao Liu]

Hi Markus,

> > +        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
> > +            if (event->u.x86_select_umask.select > UINT12_MAX) {
> > +                error_setg(errp,
> > +                           "Parameter 'select' out of range (%d).",
> > +                           UINT12_MAX);
> > +                goto fail;
> > +            }
> > +
> > +            /* No need to check the range of umask since it's uint8_t. */
> > +            break;
> > +        }
> 
> As we'll see below, the new x86-specific format is defined in the QAPI
> schema regardless of target.
> 
> It is accepted here also regardless of target.  Doesn't matter much
> right now, as the object is effectively useless for targets other than
> x86, but I understand that will change.
> 
> Should we reject it unless the target is x86?

I previously supposed that different architectures should implement
their own kvm_arch_check_pmu_filter(), which is the `check` hook of
object_class_property_add_link():

    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);

For x86, I implemented kvm_arch_check_pmu_filter() in target/i386/kvm/
kvm.c and checked the supported formats (I also supposed arch-specific
PMU filter could reject the unsupported format in
kvm_arch_check_pmu_filter().)

But I think your idea is better, i.e., rejecting unsupported format
early in pmu-filter parsing.

Well, IIUC, there is no way to specify in QAPI that certain enumerations
are generic and certain enumerations are arch-specific, so rejecting
unsupported format can only happen in parsing code. For example, wrap
the above code in "#if defined(TARGET_I386)":

    for (node = head; node; node = node->next) {
        KvmPmuFilterEvent *event = node->value;

        switch (event->format) {
        case KVM_PMU_EVENT_FORMAT_RAW:
            break;
#if defined(TARGET_I386)
        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
            ...
            break;
        }
        case KVM_PMU_EVENT_FORMAT_X86_MASKED_ENTRY: {
            ...
	    break;
        }
#endif
        default:
	    error_setg(errp,
                       "Unsupported format.");
            goto fail;
        }

        ...
    }

EMM, do you like this idea?

> If not, I feel the behavior should be noted in the commit message.

With the above change, I think it's possible to reject x86-specific
format on non-x86 arch. And I can also note this behavior in commit
message.

> >          default:
> >              g_assert_not_reached();
> >          }
> > @@ -67,6 +82,9 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
> >      filter->events = head;
> >      qapi_free_KvmPmuFilterEventList(old_head);
> >      return;
> > +
> > +fail:
> > +    qapi_free_KvmPmuFilterEventList(head);
> >  }
> >  
> >  static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)

...

> >  ##
> >  # @KvmPmuFilterEvent:
> >  #
> > @@ -66,7 +82,8 @@
> >  { 'union': 'KvmPmuFilterEvent',
> >    'base': { 'format': 'KvmPmuEventFormat' },
> >    'discriminator': 'format',
> > -  'data': { 'raw': 'KvmPmuRawEvent' } }
> > +  'data': { 'raw': 'KvmPmuRawEvent',
> > +            'x86-select-umask': 'KvmPmuX86SelectUmaskEvent' } }
> >  
> >  ##
> >  # @KvmPmuFilterProperties:
> 
> Documentation could perhaps be more explicit about this making sense
> only for x86.

What about the following doc?

##
# @KvmPmuFilterProperties:
#
# Properties of KVM PMU Filter (only for x86).

> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 51a7c61ce0b0..5dcce067d8dd 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -6180,6 +6180,9 @@ SRST
> >               ((select) & 0xff) | \
> >               ((umask) & 0xff) << 8)
> >  
> > +        ``{"format":"x86-select-umask","select":event_select,"umask":event_umask}``
> > +            Specify the single x86 PMU event with select and umask fields.
> > +
> >          An example KVM PMU filter object would look like:
> >  
> >          .. parsed-literal::
> > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > index fa3a696654cb..0d36ccf250ed 100644
> > --- a/target/i386/kvm/kvm.c
> > +++ b/target/i386/kvm/kvm.c
> > @@ -5974,6 +5974,10 @@ static bool kvm_config_pmu_event(KVMPMUFilter *filter,
> >          case KVM_PMU_EVENT_FORMAT_RAW:
> >              code = event->u.raw.code;
> >              break;
> > +        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
> > +            code = X86_PMU_RAW_EVENT(event->u.x86_select_umask.select,
> > +                                     event->u.x86_select_umask.umask);
> > +            break;
> >          default:
> >              g_assert_not_reached();
> >          }
> > @@ -6644,6 +6648,7 @@ static void kvm_arch_check_pmu_filter(const Object *obj, const char *name,
> >  
> >          switch (event->format) {
> >          case KVM_PMU_EVENT_FORMAT_RAW:
> > +        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:

Here's the current format check I mentioned above. But I agree your idea
and will check in the parsing of pmu-filter object.

> >              break;
> >          default:
> >              error_setg(errp,
> 

Re: [PATCH 1/5] qapi/qom: Introduce kvm-pmu-filter object

[Zhao Liu]

Hi Philip and Markus,

Let's discuss how to handle compilation for different architectures as
well as different accelerators here.

> > And "raw" format as a lower level format can be used for other arches
> > (e.g., ARM).
> 
> Since you provide the ability to use a raw format, are we sure other
> accelerators will never be interested in such PMU filtering?
> 
> I'm pretty sure HVF could benefit of it (whether we implement it there
> is another story).

Nice to know it could benefit more cases.

> What do you think about adding this as a generic accelerator feature.

I can implement pmu-filter directly at the "accel" level.

> If a particular accel doesn't support it and we ask to filter, we simply
> report an error.

One of the main issues is how to organize the QAPI scheme:

First we have a "qapi/accelerator.json" like current implementation to
provide:

##
# = Accelerators
##

Then we should have a "qapi/accelerator-target.json" (which will follows
qapi/accelerator.json in qapi-schema.json, just like machine.json &
machine-target.json), and place all pmu-filter related things in this
file with specify the compilation condition, for example:

{ 'struct': 'KvmPmuFilterProperties',
  'data': { 'action': 'KvmPmuFilterAction',
            '*x86-fixed-counter': 'uint32',
            '*events': ['KvmPmuFilterEvent'] },
  'if': 'CONFIG_KVM' }

In the future, this could be expanded to: 'if': { 'any': [ 'CONFIG_HVF', 'CONFIG_KVM' ] }.

I understand that there is no way to specify the architecture here,
because it is not possible to specify a combination case like
"TARGET_I386 & CONFIG_KVM", "TARGET_ARM & CONFIG_KVM", "TARGET_ARM & CONFIG_HVF"
(please educate me if such "if" condition be implemented in QAPI :-)).

So, I will put the arch-specific format check in pmu-filter.c by adding
arch macros as I mentioned in this reply:

https://lore.kernel.org/qemu-devel/aA3TeaYG9mNMdEiW@intel.com/

And there'll need accel-specific format check (for example, maksed-entry
is KVM specific, and it is not defined in x86 spec). I can check the
accel-specific format in the `check` hook of
object_class_property_add_link(), which links the pmu-filter object to
accelerator.

Do you like this idea?

Thanks,
Zhao

Re: [PATCH 5/5] i386/kvm: Support fixed counter in KVM PMU filter

[Zhao Liu]

> >   static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
> >   {
> >       object_class_property_add_enum(oc, "action", "KvmPmuFilterAction",
> > @@ -116,6 +139,14 @@ 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", "uint32_t",
> > +                              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");
> 
> Adding that x86-specific field to all architectures is a bit dubious.

Similarly, do you think that it would be a good idea to wrap x86 related
codes in "#if defined(TARGET_I386)"? Like I said in this reply:

https://lore.kernel.org/qemu-devel/aA3TeaYG9mNMdEiW@intel.com/

Re: [PATCH 2/5] i386/kvm: Support basic KVM PMU filter

[Zhao Liu]

...

> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index dc694a99a30a..51a7c61ce0b0 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -232,7 +232,8 @@ DEF("accel", HAS_ARG, QEMU_OPTION_accel,
> >      "                eager-split-size=n (KVM Eager Page Split chunk size, default 0, disabled. ARM only)\n"
> >      "                notify-vmexit=run|internal-error|disable,notify-window=n (enable notify VM exit and set notify window, x86 only)\n"
> >      "                thread=single|multi (enable multi-threaded TCG)\n"
> > -    "                device=path (KVM device path, default /dev/kvm)\n", QEMU_ARCH_ALL)
> > +    "                device=path (KVM device path, default /dev/kvm)\n"
> > +    "                pmu-filter=id (configure KVM PMU filter)\n", QEMU_ARCH_ALL)
> 
> As we'll see below, this property is actually available only for i386.
> Other target-specific properties document this like "x86 only".  Please
> do that for this one, too.

Thanks! I'll change QEMU_ARCH_ALL to QEMU_ARCH_I386.

> As far as I can tell, the kvm-pmu-filter object needs to be activated
> with -accel pmu-filter=... to do anything.  Correct?

Yes,

> You can create any number of kvm-pmu-filter objects, but only one of
> them can be active.  Correct?

Yes! I'll try to report error when user repeats to set this object, or
mention this rule in doc.

> >  SRST
> >  ``-accel name[,prop=value[,...]]``
> >      This is used to enable an accelerator. Depending on the target
> > @@ -318,6 +319,10 @@ SRST
> >          option can be used to pass the KVM device to use via a file descriptor
> >          by setting the value to ``/dev/fdset/NN``.
> >  
> > +    ``pmu-filter=id``
> > +        Sets the id of KVM PMU filter object. This option can be used to set
> > +        whitelist or blacklist of PMU events for Guest.
> 
> Well, "this option" can't actually be used to set the lists.  That's to
> be done with -object kvm-pmu-filter.  Perhaps:
> 
>            Activate a KVM PMU filter object.  That object can be used to
>            filter guest access to PMU events.

Thank you! Nice description.

> > +
> >  ERST
> >  
> >  DEF("smp", HAS_ARG, QEMU_OPTION_smp,
> > @@ -6144,6 +6149,46 @@ SRST
> >          ::
> >  
> >              (qemu) qom-set /objects/iothread1 poll-max-ns 100000
> > +
> > +    ``-object '{"qom-type":"kvm-pmu-filter","id":id,"action":action,"events":[entry_list]}'``
> 
> Should this be in the previous patch?

Yes, I can amend this to the previous patch.

> > +        Create a kvm-pmu-filter object that configures KVM to filter
> > +        selected PMU events for Guest.
> 
> The object doesn't actually configure KVM.  It merely holds the filter
> configuration.  The configuring is done by the KVM accelerator according
> to configuration in the connected kvm-pmu-filter object.  Perhaps:
> 
>            Create a kvm-pmu-filter object to hold PMU event filter
>            configuration.

Yes, that's a very accurate statement. Will fix.

> > +
> > +        This option must be written in JSON format to support ``events``
> > +        JSON list.
> > +
> > +        The ``action`` parameter sets the action that KVM will take for
> > +        the selected PMU events. It accepts ``allow`` or ``deny``. If
> > +        the action is set to ``allow``, all PMU events except the
> > +        selected ones will be disabled and blocked in the Guest. But if
> > +        the action is set to ``deny``, then only the selected events
> > +        will be denied, while all other events can be accessed normally
> > +        in the Guest.
> 
> I recommend "guest" instead of "Guest".

OK.

> > +
> > +        The ``events`` parameter accepts a list of PMU event entries in
> > +        JSON format. Event entries, based on different encoding formats,
> > +        have the following types:
> > +
> > +        ``{"format":"raw","code":raw_code}``
> > +            Encode the single PMU event with raw format. The ``code``
> > +            parameter accepts raw code of a PMU event. For x86, the raw
> > +            code represents a combination of umask and event select:
> > +
> > +        ::
> > +
> > +            (((select & 0xf00UL) << 24) | \
> > +             ((select) & 0xff) | \
> > +             ((umask) & 0xff) << 8)
> 
> Does it?  Could the code also represent a combination of select, match,
> and mask (masked entry format)?

Yes, I'll drop this formula.

> > +
> > +        An example KVM PMU filter object would look like:
> > +
> > +        .. parsed-literal::
> > +
> > +             # |qemu_system| \\
> > +                 ... \\
> > +                 -accel kvm,pmu-filter=id \\
> > +                 -object '{"qom-type":"kvm-pmu-filter","id":"f0","action":"allow","events":[{"format":"raw","code":196}]}' \\
> > +                 ...
> >  ERST
> >  
> >  
> > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > index 6c749d4ee812..fa3a696654cb 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,18 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> >          }
> >      }
> >  
> > +    /*
> > +     * TODO: Move this chunk to kvm_arch_pre_create_vcpu() and check
> 
> I can't see a function kvm_arch_pre_create_vcpu().

I was referring this patch:
https://lore.kernel.org/qemu-devel/20250425213037.8137-4-dongli.zhang@oracle.com/.

Since it's an unmerged patch, I can drop this comment.

> > +     * whether pmu is enabled there.
> 
> PMU

Sure.

> > +     */
> > +    if (s->pmu_filter) {
> > +        ret = kvm_filter_pmu_event(s);
> > +        if (ret < 0) {
> > +            error_report("Could not set KVM PMU filter");
> 
> When kvm_filter_pmu_event() failed, it already reported an error.
> Reporting it another time can be confusing.

Good catch! Will remove this error_report().

> > +            return ret;
> > +        }
> > +    }
> > +
> >      return 0;
> >  }
> >  
> > @@ -5942,6 +5956,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_FORMAT_RAW:
> > +            code = event->u.raw.code;
> > +            break;
> > +        default:
> > +            g_assert_not_reached();
> > +        }
> > +
> > +        kvm_filter->events[idx++] = code;
> > +        events = events->next;
> > +    }
> > +
> > +    return true;
> > +}
> 
> This function cannot fail.  Please return void, and simplify its caller.

Yes, the error handling is unnecessary here.

> > +
> > +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));
> 
> Should we use sizeof(filter->events[0])?

No, here I'm trying to constructing the memory accepted in kvm interface
(with the specific layout), which is not the same as the KVMPMUFilter
object.

> > +
> > +    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));
> 
> Suggest something like "can't set KVM PMU event filter".

Sure, sounds good!

> > +        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.");
> 
> Error message should be a single phrase with no trailing punctuation.
> More of the same below.

OK, I'll clean up them.

> > +        return -1;
> > +    }
> > +
> > +    return kvm_install_pmu_event_filter(s);
> > +}
> > +
> >  static bool has_sgx_provisioning;
> >  
> >  static bool __kvm_enable_sgx_provisioning(KVMState *s)
> > @@ -6537,6 +6627,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.");
> 
> Why is this an error?
> 
> action=allow with an empty would be the obvious way to allow nothing,
> wouldn't it?

allow nothing == deny everything!

Yes, it makes sense :-) Will drop this limitation.

> > +        return;
> > +    }
> > +
> > +    while (events) {
> > +        KvmPmuFilterEvent *event = events->value;
> > +
> > +        switch (event->format) {
> > +        case KVM_PMU_EVENT_FORMAT_RAW:
> > +            break;
> > +        default:
> > +            error_setg(errp,
> > +                       "Unsupported PMU event format %s.",
> > +                       KvmPmuEventFormat_str(events->value->format));
> 
> Unreachable.

OK, it makes sense. Thanks.

> > +            return;
> > +        }
> > +
> > +        events = events->next;
> > +    }
> > +}
> > +
> >  void kvm_arch_accel_class_init(ObjectClass *oc)
> >  {
> >      object_class_property_add_enum(oc, "notify-vmexit", "NotifyVMexitOption",
> > @@ -6576,6 +6695,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)
> 
> target/i386/kvm/kvm.c is compiled into the binary only for i386 target
> with CONFIG_KVM.
> 
> The kvm-pmu-filter-object exists for any target with CONFIG_KVM.  But
> it's usable only for i386.

As with Philip's comment, I also need to think about compatibility with
multiple accelerators, and we can continue that discussion in the mail
thread of patch 1. Thanks for your feedback!

> I think the previous patch's commit message should state the role of the
> kvm-pmu-filter-object more clearly: hold KVM PMU filter configuration
> for any target with KVM.  This patch's commit message should then
> explain what the patch does: enable actual use of the
> kvm-pmu-filter-object for i386 only.  Other targets are left for another
> day.

Thank you! I'll update the commit message.

Regards,
Zhao

Re: [PATCH 2/5] i386/kvm: Support basic KVM PMU filter

[Markus Armbruster]

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

> ...
>
>> > diff --git a/qemu-options.hx b/qemu-options.hx
>> > index dc694a99a30a..51a7c61ce0b0 100644
>> > --- a/qemu-options.hx
>> > +++ b/qemu-options.hx
>> > @@ -232,7 +232,8 @@ DEF("accel", HAS_ARG, QEMU_OPTION_accel,
>> >      "                eager-split-size=n (KVM Eager Page Split chunk size, default 0, disabled. ARM only)\n"
>> >      "                notify-vmexit=run|internal-error|disable,notify-window=n (enable notify VM exit and set notify window, x86 only)\n"
>> >      "                thread=single|multi (enable multi-threaded TCG)\n"
>> > -    "                device=path (KVM device path, default /dev/kvm)\n", QEMU_ARCH_ALL)
>> > +    "                device=path (KVM device path, default /dev/kvm)\n"
>> > +    "                pmu-filter=id (configure KVM PMU filter)\n", QEMU_ARCH_ALL)
>> 
>> As we'll see below, this property is actually available only for i386.
>> Other target-specific properties document this like "x86 only".  Please
>> do that for this one, too.
>
> Thanks! I'll change QEMU_ARCH_ALL to QEMU_ARCH_I386.

That would be wrong :)

QEMU_ARCH_ALL is the last argument passed to macro DEF().  It applies to
the entire option, in this case -accel.

I'd like you to mark the option parameter as "(x86 only)", like
notify-vmexit right above, and several more elsewhere.

>> As far as I can tell, the kvm-pmu-filter object needs to be activated
>> with -accel pmu-filter=... to do anything.  Correct?
>
> Yes,
>
>> You can create any number of kvm-pmu-filter objects, but only one of
>> them can be active.  Correct?
>
> Yes! I'll try to report error when user repeats to set this object, or
> mention this rule in doc.

Creating kvm-pmu-filter objects without using them should be harmless,
shouldn't it?  I think users can already create other kinds of unused
objects.

>> > +
>> > +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));
>> 
>> Should we use sizeof(filter->events[0])?
>
> No, here I'm trying to constructing the memory accepted in kvm interface
> (with the specific layout), which is not the same as the KVMPMUFilter
> object.

You're right.  What about sizeof(kvm_filter->events[0])?

[...]

Re: [PATCH 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter

[Markus Armbruster]

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

> Hi Markus,
>
>> > +        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
>> > +            if (event->u.x86_select_umask.select > UINT12_MAX) {
>> > +                error_setg(errp,
>> > +                           "Parameter 'select' out of range (%d).",
>> > +                           UINT12_MAX);
>> > +                goto fail;
>> > +            }
>> > +
>> > +            /* No need to check the range of umask since it's uint8_t. */
>> > +            break;
>> > +        }
>> 
>> As we'll see below, the new x86-specific format is defined in the QAPI
>> schema regardless of target.
>> 
>> It is accepted here also regardless of target.  Doesn't matter much
>> right now, as the object is effectively useless for targets other than
>> x86, but I understand that will change.
>> 
>> Should we reject it unless the target is x86?
>
> I previously supposed that different architectures should implement
> their own kvm_arch_check_pmu_filter(), which is the `check` hook of
> object_class_property_add_link():
>
>     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);

This way, the checking happens only when you actually connect the
kvm-pmu-filter object to the accelerator.

Have you considered checking in the kvm-pmu-filter object's complete()
method?  Simple example of how to do that: qauthz_simple_complete() in
authz/simple.c.

> For x86, I implemented kvm_arch_check_pmu_filter() in target/i386/kvm/
> kvm.c and checked the supported formats (I also supposed arch-specific
> PMU filter could reject the unsupported format in
> kvm_arch_check_pmu_filter().)
>
> But I think your idea is better, i.e., rejecting unsupported format
> early in pmu-filter parsing.
>
> Well, IIUC, there is no way to specify in QAPI that certain enumerations
> are generic and certain enumerations are arch-specific,

Here's how to make enum values conditional:

    { 'enum': 'KvmPmuEventFormat',
      'data': ['raw',
               { 'name': 'x86-select-umask', 'if': 'TARGET_I386' }
               { 'name': 'x86-masked-entry', 'if': 'TARGET_I386' } ] }

However, TARGET_I386 is usable only in target-specific code.  This has
two consequences here:

1. It won't compile, since QAPI schema module kvm.json is
   target-independent.  We'd have to put it into a target-specific
   module kvm-target.json.

2. Target-specific QAPI schema mdoules are problematic for the single
   binary / heterogeneous machine work.  We are discussing how to best
   handle that.  Unclear whether adding more target-specific QAPI
   definitions are a good idea.

>                                                         so rejecting
> unsupported format can only happen in parsing code. For example, wrap
> the above code in "#if defined(TARGET_I386)":
>
>     for (node = head; node; node = node->next) {
>         KvmPmuFilterEvent *event = node->value;
>
>         switch (event->format) {
>         case KVM_PMU_EVENT_FORMAT_RAW:
>             break;
> #if defined(TARGET_I386)
>         case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
>             ...
>             break;
>         }
>         case KVM_PMU_EVENT_FORMAT_X86_MASKED_ENTRY: {
>             ...
> 	    break;
>         }
> #endif
>         default:
> 	    error_setg(errp,
>                        "Unsupported format.");
>             goto fail;
>         }
>
>         ...
>     }
>
> EMM, do you like this idea?

This is kvm_pmu_filter_set_event(), I presume.

The #if is necessary when you make the enum values conditional.  The
default: code is unreachable then, so it should stay
g_assert_not_reached().

The #if is fine even when you don't make the enum values conditional.
The default: code is reachable then, unless you reject the unwanted
enums earlier some other way.

>> If not, I feel the behavior should be noted in the commit message.
>
> With the above change, I think it's possible to reject x86-specific
> format on non-x86 arch. And I can also note this behavior in commit
> message.
>
>> >          default:
>> >              g_assert_not_reached();
>> >          }
>> > @@ -67,6 +82,9 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
>> >      filter->events = head;
>> >      qapi_free_KvmPmuFilterEventList(old_head);
>> >      return;
>> > +
>> > +fail:
>> > +    qapi_free_KvmPmuFilterEventList(head);
>> >  }
>> >  
>> >  static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
>
> ...
>
>> >  ##
>> >  # @KvmPmuFilterEvent:
>> >  #
>> > @@ -66,7 +82,8 @@
>> >  { 'union': 'KvmPmuFilterEvent',
>> >    'base': { 'format': 'KvmPmuEventFormat' },
>> >    'discriminator': 'format',
>> > -  'data': { 'raw': 'KvmPmuRawEvent' } }
>> > +  'data': { 'raw': 'KvmPmuRawEvent',
>> > +            'x86-select-umask': 'KvmPmuX86SelectUmaskEvent' } }
>> >  
>> >  ##
>> >  # @KvmPmuFilterProperties:
>> 
>> Documentation could perhaps be more explicit about this making sense
>> only for x86.
>
> What about the following doc?
>
> ##
> # @KvmPmuFilterProperties:
> #
> # Properties of KVM PMU Filter (only for x86).

Hmm.  Branch 'raw' make sense regardless of target, doesn't it?  It's
actually usable only for i86 so far, because this series implements
accelerator property "pmu-filter" only for i386.

Let's not worry about this until we decided whether to use QAPI
conditionals or not.

>> > diff --git a/qemu-options.hx b/qemu-options.hx
>> > index 51a7c61ce0b0..5dcce067d8dd 100644
>> > --- a/qemu-options.hx
>> > +++ b/qemu-options.hx
>> > @@ -6180,6 +6180,9 @@ SRST
>> >               ((select) & 0xff) | \
>> >               ((umask) & 0xff) << 8)
>> >  
>> > +        ``{"format":"x86-select-umask","select":event_select,"umask":event_umask}``
>> > +            Specify the single x86 PMU event with select and umask fields.
>> > +
>> >          An example KVM PMU filter object would look like:
>> >  
>> >          .. parsed-literal::
>> > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> > index fa3a696654cb..0d36ccf250ed 100644
>> > --- a/target/i386/kvm/kvm.c
>> > +++ b/target/i386/kvm/kvm.c
>> > @@ -5974,6 +5974,10 @@ static bool kvm_config_pmu_event(KVMPMUFilter *filter,
>> >          case KVM_PMU_EVENT_FORMAT_RAW:
>> >              code = event->u.raw.code;
>> >              break;
>> > +        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
>> > +            code = X86_PMU_RAW_EVENT(event->u.x86_select_umask.select,
>> > +                                     event->u.x86_select_umask.umask);
>> > +            break;
>> >          default:
>> >              g_assert_not_reached();
>> >          }
>> > @@ -6644,6 +6648,7 @@ static void kvm_arch_check_pmu_filter(const Object *obj, const char *name,
>> >  
>> >          switch (event->format) {
>> >          case KVM_PMU_EVENT_FORMAT_RAW:
>> > +        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
>
> Here's the current format check I mentioned above. But I agree your idea
> and will check in the parsing of pmu-filter object.
>
>> >              break;
>> >          default:
>> >              error_setg(errp,
>> 

Re: [PATCH 2/5] i386/kvm: Support basic KVM PMU filter

[Zhao Liu]

On Mon, Apr 28, 2025 at 08:12:09AM +0200, Markus Armbruster wrote:
> Date: Mon, 28 Apr 2025 08:12:09 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH 2/5] i386/kvm: Support basic KVM PMU filter
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > ...
> >
> >> > diff --git a/qemu-options.hx b/qemu-options.hx
> >> > index dc694a99a30a..51a7c61ce0b0 100644
> >> > --- a/qemu-options.hx
> >> > +++ b/qemu-options.hx
> >> > @@ -232,7 +232,8 @@ DEF("accel", HAS_ARG, QEMU_OPTION_accel,
> >> >      "                eager-split-size=n (KVM Eager Page Split chunk size, default 0, disabled. ARM only)\n"
> >> >      "                notify-vmexit=run|internal-error|disable,notify-window=n (enable notify VM exit and set notify window, x86 only)\n"
> >> >      "                thread=single|multi (enable multi-threaded TCG)\n"
> >> > -    "                device=path (KVM device path, default /dev/kvm)\n", QEMU_ARCH_ALL)
> >> > +    "                device=path (KVM device path, default /dev/kvm)\n"
> >> > +    "                pmu-filter=id (configure KVM PMU filter)\n", QEMU_ARCH_ALL)
> >> 
> >> As we'll see below, this property is actually available only for i386.
> >> Other target-specific properties document this like "x86 only".  Please
> >> do that for this one, too.
> >
> > Thanks! I'll change QEMU_ARCH_ALL to QEMU_ARCH_I386.
> 
> That would be wrong :)
> 
> QEMU_ARCH_ALL is the last argument passed to macro DEF().  It applies to
> the entire option, in this case -accel.

Thank you for correction! I didn't realize this point :-(.

> I'd like you to mark the option parameter as "(x86 only)", like
> notify-vmexit right above, and several more elsewhere.

Sure, I see. This option has already provided good example for me.

> >> As far as I can tell, the kvm-pmu-filter object needs to be activated
> >> with -accel pmu-filter=... to do anything.  Correct?
> >
> > Yes,
> >
> >> You can create any number of kvm-pmu-filter objects, but only one of
> >> them can be active.  Correct?
> >
> > Yes! I'll try to report error when user repeats to set this object, or
> > mention this rule in doc.
> 
> Creating kvm-pmu-filter objects without using them should be harmless,
> shouldn't it?  I think users can already create other kinds of unused
> objects.

I think I understand now. Indeed, creating an object should be allowed
regardless of whether it's used, as this helps decouple "-object" from
other options.

I can add something that:

the kvm-pmu-filter object needs to be activated with "-accel pmu-filter=id",
and only when it is activated, its filter policy can be passed to KVM.

(A single sentence is just an example; I think it needs to be carefully
refined within the context of the entire paragraph :-).)

> >> > +
> >> > +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));
> >> 
> >> Should we use sizeof(filter->events[0])?
> >
> > No, here I'm trying to constructing the memory accepted in kvm interface
> > (with the specific layout), which is not the same as the KVMPMUFilter
> > object.
> 
> You're right.  What about sizeof(kvm_filter->events[0])?

I get your point now! Yes, I should do in this way.

Thanks,
Zhao

Re: [PATCH 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter

[Zhao Liu]

On Mon, Apr 28, 2025 at 09:19:07AM +0200, Markus Armbruster wrote:
> Date: Mon, 28 Apr 2025 09:19:07 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH 3/5] i386/kvm: Support event with select & umask format
>  in KVM PMU filter
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > Hi Markus,
> >
> >> > +        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
> >> > +            if (event->u.x86_select_umask.select > UINT12_MAX) {
> >> > +                error_setg(errp,
> >> > +                           "Parameter 'select' out of range (%d).",
> >> > +                           UINT12_MAX);
> >> > +                goto fail;
> >> > +            }
> >> > +
> >> > +            /* No need to check the range of umask since it's uint8_t. */
> >> > +            break;
> >> > +        }
> >> 
> >> As we'll see below, the new x86-specific format is defined in the QAPI
> >> schema regardless of target.
> >> 
> >> It is accepted here also regardless of target.  Doesn't matter much
> >> right now, as the object is effectively useless for targets other than
> >> x86, but I understand that will change.
> >> 
> >> Should we reject it unless the target is x86?
> >
> > I previously supposed that different architectures should implement
> > their own kvm_arch_check_pmu_filter(), which is the `check` hook of
> > object_class_property_add_link():
> >
> >     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);
> 
> This way, the checking happens only when you actually connect the
> kvm-pmu-filter object to the accelerator.
> 
> Have you considered checking in the kvm-pmu-filter object's complete()
> method?  Simple example of how to do that: qauthz_simple_complete() in
> authz/simple.c.

Thank you, I hadn't noticed it before. Now I study it carefully, and yes,
this is a better way than `check` hook. Though in the following we are
talking about other ways to handle target-specific check, this helper
may be still useful as I proposed to help check accel-specific cases in
the reply to Philip [*].

[*]: https://lore.kernel.org/qemu-devel/aA3cHIcKmt3vdkVk@intel.com/

> > For x86, I implemented kvm_arch_check_pmu_filter() in target/i386/kvm/
> > kvm.c and checked the supported formats (I also supposed arch-specific
> > PMU filter could reject the unsupported format in
> > kvm_arch_check_pmu_filter().)
> >
> > But I think your idea is better, i.e., rejecting unsupported format
> > early in pmu-filter parsing.
> >
> > Well, IIUC, there is no way to specify in QAPI that certain enumerations
> > are generic and certain enumerations are arch-specific,
> 
> Here's how to make enum values conditional:
> 
>     { 'enum': 'KvmPmuEventFormat',
>       'data': ['raw',
>                { 'name': 'x86-select-umask', 'if': 'TARGET_I386' }
>                { 'name': 'x86-masked-entry', 'if': 'TARGET_I386' } ] }

What I'm a bit hesitant about is that, if different arches add similar
"conditional" enumerations later, it could cause the enumeration values
to change under different compilation conditions (correct? :-)). Although
it might not break anything, since we don't rely on the specific numeric
values.

> However, TARGET_I386 is usable only in target-specific code.  This has
> two consequences here:
> 
> 1. It won't compile, since QAPI schema module kvm.json is
>    target-independent.  We'd have to put it into a target-specific
>    module kvm-target.json.
> 
> 2. Target-specific QAPI schema mdoules are problematic for the single
>    binary / heterogeneous machine work.  We are discussing how to best
>    handle that.  Unclear whether adding more target-specific QAPI
>    definitions are a good idea.

And per yours feedback, CONFIG_KVM can also only be used in target-specific
code. Moreover, especially if we need to further consider multiple
accelerators, such as the HVF need mentioned by Philip, it seems that
we can't avoid target-specific issues here!

> >                                                         so rejecting
> > unsupported format can only happen in parsing code. For example, wrap
> > the above code in "#if defined(TARGET_I386)":
> >
> >     for (node = head; node; node = node->next) {
> >         KvmPmuFilterEvent *event = node->value;
> >
> >         switch (event->format) {
> >         case KVM_PMU_EVENT_FORMAT_RAW:
> >             break;
> > #if defined(TARGET_I386)
> >         case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
> >             ...
> >             break;
> >         }
> >         case KVM_PMU_EVENT_FORMAT_X86_MASKED_ENTRY: {
> >             ...
> > 	    break;
> >         }
> > #endif
> >         default:
> > 	    error_setg(errp,
> >                        "Unsupported format.");
> >             goto fail;
> >         }
> >
> >         ...
> >     }
> >
> > EMM, do you like this idea?
> 
> This is kvm_pmu_filter_set_event(), I presume.
>
> The #if is necessary when you make the enum values conditional.  The
> default: code is unreachable then, so it should stay
> g_assert_not_reached().
>
> The #if is fine even when you don't make the enum values conditional.
> The default: code is reachable then, unless you reject the unwanted
> enums earlier some other way.

Thanks for your analysis, it's very accurate! I personally prefer the
2nd way.

> >> If not, I feel the behavior should be noted in the commit message.
> >
> > With the above change, I think it's possible to reject x86-specific
> > format on non-x86 arch. And I can also note this behavior in commit
> > message.
> >
> >> >          default:
> >> >              g_assert_not_reached();
> >> >          }
> >> > @@ -67,6 +82,9 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
> >> >      filter->events = head;
> >> >      qapi_free_KvmPmuFilterEventList(old_head);
> >> >      return;
> >> > +
> >> > +fail:
> >> > +    qapi_free_KvmPmuFilterEventList(head);
> >> >  }
> >> >  
> >> >  static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
> >
> > ...
> >
> >> >  ##
> >> >  # @KvmPmuFilterEvent:
> >> >  #
> >> > @@ -66,7 +82,8 @@
> >> >  { 'union': 'KvmPmuFilterEvent',
> >> >    'base': { 'format': 'KvmPmuEventFormat' },
> >> >    'discriminator': 'format',
> >> > -  'data': { 'raw': 'KvmPmuRawEvent' } }
> >> > +  'data': { 'raw': 'KvmPmuRawEvent',
> >> > +            'x86-select-umask': 'KvmPmuX86SelectUmaskEvent' } }
> >> >  
> >> >  ##
> >> >  # @KvmPmuFilterProperties:
> >> 
> >> Documentation could perhaps be more explicit about this making sense
> >> only for x86.
> >
> > What about the following doc?
> >
> > ##
> > # @KvmPmuFilterProperties:
> > #
> > # Properties of KVM PMU Filter (only for x86).
> 
> Hmm.  Branch 'raw' make sense regardless of target, doesn't it?  It's
> actually usable only for i86 so far, because this series implements
> accelerator property "pmu-filter" only for i386.

The advantage of this single note is someone can easily mention other
arch in doc :-)

> Let's not worry about this until we decided whether to use QAPI
> conditionals or not.

OK, this is not a big deal (comparing with other issues).

Thanks,
Zhao

Re: [PATCH 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter

[Markus Armbruster]

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

> On Mon, Apr 28, 2025 at 09:19:07AM +0200, Markus Armbruster wrote:
>> Date: Mon, 28 Apr 2025 09:19:07 +0200
>> From: Markus Armbruster <armbru@redhat.com>
>> Subject: Re: [PATCH 3/5] i386/kvm: Support event with select & umask format
>>  in KVM PMU filter
>> 
>> Zhao Liu <zhao1.liu@intel.com> writes:
>> 
>> > Hi Markus,
>> >
>> >> > +        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
>> >> > +            if (event->u.x86_select_umask.select > UINT12_MAX) {
>> >> > +                error_setg(errp,
>> >> > +                           "Parameter 'select' out of range (%d).",
>> >> > +                           UINT12_MAX);
>> >> > +                goto fail;
>> >> > +            }
>> >> > +
>> >> > +            /* No need to check the range of umask since it's uint8_t. */
>> >> > +            break;
>> >> > +        }
>> >> 
>> >> As we'll see below, the new x86-specific format is defined in the QAPI
>> >> schema regardless of target.
>> >> 
>> >> It is accepted here also regardless of target.  Doesn't matter much
>> >> right now, as the object is effectively useless for targets other than
>> >> x86, but I understand that will change.
>> >> 
>> >> Should we reject it unless the target is x86?
>> >
>> > I previously supposed that different architectures should implement
>> > their own kvm_arch_check_pmu_filter(), which is the `check` hook of
>> > object_class_property_add_link():
>> >
>> >     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);
>> 
>> This way, the checking happens only when you actually connect the
>> kvm-pmu-filter object to the accelerator.
>> 
>> Have you considered checking in the kvm-pmu-filter object's complete()
>> method?  Simple example of how to do that: qauthz_simple_complete() in
>> authz/simple.c.
>
> Thank you, I hadn't noticed it before. Now I study it carefully, and yes,
> this is a better way than `check` hook. Though in the following we are
> talking about other ways to handle target-specific check, this helper
> may be still useful as I proposed to help check accel-specific cases in
> the reply to Philip [*].
>
> [*]: https://lore.kernel.org/qemu-devel/aA3cHIcKmt3vdkVk@intel.com/
>
>> > For x86, I implemented kvm_arch_check_pmu_filter() in target/i386/kvm/
>> > kvm.c and checked the supported formats (I also supposed arch-specific
>> > PMU filter could reject the unsupported format in
>> > kvm_arch_check_pmu_filter().)
>> >
>> > But I think your idea is better, i.e., rejecting unsupported format
>> > early in pmu-filter parsing.
>> >
>> > Well, IIUC, there is no way to specify in QAPI that certain enumerations
>> > are generic and certain enumerations are arch-specific,
>> 
>> Here's how to make enum values conditional:
>> 
>>     { 'enum': 'KvmPmuEventFormat',
>>       'data': ['raw',
>>                { 'name': 'x86-select-umask', 'if': 'TARGET_I386' }
>>                { 'name': 'x86-masked-entry', 'if': 'TARGET_I386' } ] }
>
> What I'm a bit hesitant about is that, if different arches add similar
> "conditional" enumerations later, it could cause the enumeration values
> to change under different compilation conditions (correct? :-)). Although
> it might not break anything, since we don't rely on the specific numeric
> values.

Every binary we create contains target-specific code for at most one
target.  Therefore, different numerical encodings for different targets
are fine.

Same argument for struct members, by the way.  Consider

    { 'struct': 'CpuModelExpansionInfo',
      'data': { 'model': 'CpuModelInfo',
                'deprecated-props' : { 'type': ['str'],
                                       'if': 'TARGET_S390X' } },
      'if': { 'any': [ 'TARGET_S390X',
                       'TARGET_I386',
                       'TARGET_ARM',
                       'TARGET_LOONGARCH64',
                       'TARGET_RISCV' ] } }

This generates

    #if defined(TARGET_S390X) || defined(TARGET_I386) || defined(TARGET_ARM) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV)
    struct CpuModelExpansionInfo {
        CpuModelInfo *model;
    #if defined(TARGET_S390X)
        strList *deprecated_props;
    #endif /* defined(TARGET_S390X) */
    };
    #endif /* defined(TARGET_S390X) || defined(TARGET_I386) || defined(TARGET_ARM) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV) */

The struct's size depends on the target.  If we ever add members after
@deprecated_props, their offset depends on the target, too.

The single binary work will invalidate the "at most one target"
property.  We need to figure out how to best deal with that, but not in
this thread.

[...]

Re: [PATCH 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter

[Zhao Liu]

> > What I'm a bit hesitant about is that, if different arches add similar
> > "conditional" enumerations later, it could cause the enumeration values
> > to change under different compilation conditions (correct? :-)). Although
> > it might not break anything, since we don't rely on the specific numeric
> > values.
> 
> Every binary we create contains target-specific code for at most one
> target.  Therefore, different numerical encodings for different targets
> are fine.
> 
> Same argument for struct members, by the way.  Consider
> 
>     { 'struct': 'CpuModelExpansionInfo',
>       'data': { 'model': 'CpuModelInfo',
>                 'deprecated-props' : { 'type': ['str'],
>                                        'if': 'TARGET_S390X' } },
>       'if': { 'any': [ 'TARGET_S390X',
>                        'TARGET_I386',
>                        'TARGET_ARM',
>                        'TARGET_LOONGARCH64',
>                        'TARGET_RISCV' ] } }
> 
> This generates
> 
>     #if defined(TARGET_S390X) || defined(TARGET_I386) || defined(TARGET_ARM) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV)
>     struct CpuModelExpansionInfo {
>         CpuModelInfo *model;
>     #if defined(TARGET_S390X)
>         strList *deprecated_props;
>     #endif /* defined(TARGET_S390X) */
>     };
>     #endif /* defined(TARGET_S390X) || defined(TARGET_I386) || defined(TARGET_ARM) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV) */
> 
> The struct's size depends on the target.  If we ever add members after
> @deprecated_props, their offset depends on the target, too.

Thank your for further explanation!

Regards,
Zhao