Hi Shaoqin,
On 1/15/24 09:01, Shaoqin Huang wrote:
> The KVM_ARM_VCPU_PMU_V3_FILTER provides the ability to let the VMM decide
> which PMU events are provided to the guest. Add a new option
> `kvm-pmu-filter` as -cpu sub-option to set the PMU Event Filtering.
> Without the filter, all PMU events are exposed from host to guest by
> default. The usage of the new sub-option can be found from the updated
> document (docs/system/arm/cpu-features.rst).
do not hesitate to cc qemu-arm@xxxxxxxxxx for ARM specific topics.
>
> Here is an example shows how to use the PMU Event Filtering, when
which shows
> we launch a guest by use kvm, add such command line:
>
> # qemu-system-aarch64 \
> -accel kvm \
> -cpu host,kvm-pmu-filter="D:0x11-0x11"
>
> Since the first action is deny, we have a global allow policy. This
> disables the filtering of the cycle counter (event 0x11 being CPU_CYCLES).
Actually it filters it ;-) It would rather say this filters out the
cycle counter. But I am not a native speaker either ;-)
>
> And then in guest, use the perf to count the cycle:
>
> # perf stat sleep 1
>
> Performance counter stats for 'sleep 1':
>
> 1.22 msec task-clock # 0.001 CPUs utilized
> 1 context-switches # 820.695 /sec
> 0 cpu-migrations # 0.000 /sec
> 55 page-faults # 45.138 K/sec
> <not supported> cycles
> 1128954 instructions
> 227031 branches # 186.323 M/sec
> 8686 branch-misses # 3.83% of all branches
>
> 1.002492480 seconds time elapsed
>
> 0.001752000 seconds user
> 0.000000000 seconds sys
>
> As we can see, the cycle counter has been disabled in the guest, but
> other pmu events are still work.
do still work
>
> Signed-off-by: Shaoqin Huang <shahuang@xxxxxxxxxx>
> ---
> v4->v5:
> - Change the kvm-pmu-filter as a -cpu sub-option. [Eric]
> - Comment tweak. [Gavin]
> - Rebase to the latest branch.
>
> v3->v4:
> - Fix the wrong check for pmu_filter_init. [Sebastian]
> - Fix multiple alignment issue. [Gavin]
> - Report error by warn_report() instead of error_report(), and don't use
> abort() since the PMU Event Filter is an add-on and best-effort feature.
> [Gavin]
> - Add several missing { } for single line of code. [Gavin]
> - Use the g_strsplit() to replace strtok(). [Gavin]
>
> v2->v3:
> - Improve commits message, use kernel doc wording, add more explaination on
> filter example, fix some typo error. [Eric]
> - Add g_free() in kvm_arch_set_pmu_filter() to prevent memory leak. [Eric]
> - Add more precise error message report. [Eric]
> - In options doc, add pmu-filter rely on KVM_ARM_VCPU_PMU_V3_FILTER support in
> KVM. [Eric]
>
> v1->v2:
> - Add more description for allow and deny meaning in
> commit message. [Sebastian]
> - Small improvement. [Sebastian]
>
> docs/system/arm/cpu-features.rst | 23 ++++++++++
> include/sysemu/kvm_int.h | 1 +
> target/arm/cpu.h | 3 ++
> target/arm/kvm.c | 78 ++++++++++++++++++++++++++++++++
> 4 files changed, 105 insertions(+)
>
> diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
> index a5fb929243..44a797c50e 100644
> --- a/docs/system/arm/cpu-features.rst
> +++ b/docs/system/arm/cpu-features.rst
> @@ -204,6 +204,29 @@ the list of KVM VCPU features and their descriptions.
> the guest scheduler behavior and/or be exposed to the guest
> userspace.
>
> +``kvm-pmu-filter``
> + By default kvm-pmu-filter is disabled. This means that by default all pmu
> + events will be exposed to guest.
> +
> + KVM implements PMU Event Filtering to prevent a guest from being able to
> + sample certain events. It depends on the KVM_ARM_VCPU_PMU_V3_FILTER
> + attribute supported in KVM. It has the following format:
> +
> + kvm-pmu-filter="{A,D}:start-end[;{A,D}:start-end...]"
> +
> + The A means "allow" and D means "deny", start is the first event of the
> + range and the end is the last one. The first registered range defines
> + the global policy(global ALLOW if the first @action is DENY, global DENY
> + if the first @action is ALLOW). The start and end only support hexadecimal
> + format now. For example:
> +
> + kvm-pmu-filter="A:0x11-0x11;A:0x23-0x3a;D:0x30-0x30"
> +
> + Since the first action is allow, we have a global deny policy. It
> + will allow event 0x11 (The cycle counter), events 0x23 to 0x3a is
s/is/are
> + also allowed except the event 0x30 is denied, and all the other events
0x30 is/0x30 which is
> + are disallowed.
s/disallowed/denied just to match the above terminology.
> +
> TCG VCPU Features
> =================
>
> diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
> index fd846394be..8f4601474f 100644
> --- a/include/sysemu/kvm_int.h
> +++ b/include/sysemu/kvm_int.h
> @@ -120,6 +120,7 @@ struct KVMState
> uint32_t xen_caps;
> uint16_t xen_gnttab_max_frames;
> uint16_t xen_evtchn_max_pirq;
> + char *kvm_pmu_filter;
I think this is not needed anymore
> };
>
> void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 8c3ca2e231..1be7dca4aa 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -971,6 +971,9 @@ struct ArchCPU {
>
> /* KVM steal time */
> OnOffAuto kvm_steal_time;
> +
> + /* KVM PMU Filter */
> + char *kvm_pmu_filter;
> #endif /* CONFIG_KVM */
>
> /* Uniprocessor system with MP extensions */
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index c5a3183843..413ee2720e 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -495,6 +495,22 @@ static void kvm_steal_time_set(Object *obj, bool value, Error **errp)
> ARM_CPU(obj)->kvm_steal_time = value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> }
>
> +static char *kvm_pmu_filter_get(Object *obj, Error **errp)
> +{
> + ARMCPU *cpu = ARM_CPU(obj);
> +
> + return g_strdup(cpu->kvm_pmu_filter);
> +}
> +
> +static void kvm_pmu_filter_set(Object *obj, const char *pmu_filter,
> + Error **errp)
> +{
> + ARMCPU *cpu = ARM_CPU(obj);
> +
> + g_free(cpu->kvm_pmu_filter);
> + cpu->kvm_pmu_filter = g_strdup(pmu_filter);
> +}
> +
> /* KVM VCPU properties should be prefixed with "kvm-". */
> void kvm_arm_add_vcpu_properties(ARMCPU *cpu)
> {
> @@ -516,6 +532,12 @@ void kvm_arm_add_vcpu_properties(ARMCPU *cpu)
> kvm_steal_time_set);
> object_property_set_description(obj, "kvm-steal-time",
> "Set off to disable KVM steal time.");
> +
> + object_property_add_str(obj, "kvm-pmu-filter", kvm_pmu_filter_get,
> + kvm_pmu_filter_set);
> + object_property_set_description(obj, "kvm-pmu-filter",
> + "PMU Event Filtering description for "
> + "guest PMU. (default: NULL, disabled)");
> }
>
> bool kvm_arm_pmu_supported(void)
> @@ -1705,6 +1727,60 @@ static bool kvm_arm_set_device_attr(ARMCPU *cpu, struct kvm_device_attr *attr,
> return true;
> }
>
> +static void kvm_arm_pmu_filter_init(ARMCPU *cpu)
> +{
> + static bool pmu_filter_init;
> + struct kvm_pmu_event_filter filter;
> + struct kvm_device_attr attr = {
> + .group = KVM_ARM_VCPU_PMU_V3_CTRL,
> + .attr = KVM_ARM_VCPU_PMU_V3_FILTER,
> + .addr = (uint64_t)&filter,
> + };
> + int i;
> + gchar **event_filters;
wonder if you couldn't use g_auto(GStrv) event_filters with auto
cleenup? examples in qom/object.c for instance
> +
> + if (!cpu->kvm_pmu_filter) {
> + return;
> + }
> + if (kvm_vcpu_ioctl(CPU(cpu), KVM_HAS_DEVICE_ATTR, &attr)) {
> + warn_report("The KVM doesn't support the PMU Event Filter!");
> + return;
> + }
> +
> + /*
> + * The filter only needs to be initialized through one vcpu ioctl and it
> + * will affect all other vcpu in the vm.
> + */
> + if (pmu_filter_init) {
I think I commented on that on the v4. Maybe I missed your reply. You
sure you don't need to call it for each vcpu?
> + return;
> + } else {
> + pmu_filter_init = true;
> + }
> +
> + event_filters = g_strsplit(cpu->kvm_pmu_filter, ";", -1);
> + for (i = 0; event_filters[i]; i++) {
> + unsigned short start = 0, end = 0;
> + char act;
> +
> + sscanf(event_filters[i], "%c:%hx-%hx", &act, &start, &end);
> + if ((act != 'A' && act != 'D') || (!start && !end)) {
> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
> + continue;
> + }
> +
> + filter.base_event = start;
> + filter.nevents = end - start + 1;
> + filter.action = (act == 'A') ? KVM_PMU_EVENT_ALLOW :
> + KVM_PMU_EVENT_DENY;
> +
> + if (!kvm_arm_set_device_attr(cpu, &attr, "PMU Event Filter")) {
Nit: maybe s/PMU Event Filter/PMU_V3_FILTER
> + break;
> + }
> + }
> +
> + g_strfreev(event_filters);
> +}
> +
> void kvm_arm_pmu_init(ARMCPU *cpu)
> {
> struct kvm_device_attr attr = {
> @@ -1715,6 +1791,8 @@ void kvm_arm_pmu_init(ARMCPU *cpu)
> if (!cpu->has_pmu) {
> return;
> }
> +
> + kvm_arm_pmu_filter_init(cpu);
> if (!kvm_arm_set_device_attr(cpu, &attr, "PMU")) {
> error_report("failed to init PMU");
> abort();
Along with this new vcpu feature you may want to add a test like what
was done in 68970d1e0d07 ("hw/arm/virt: Implement kvm-steal-time"), in
tests/qtest/arm-cpu-features.c
Thanks
Eric
