Re: [PATCH v3] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER

[Shaoqin Huang <shahuang@redhat.com>]

Hi Gavin,

On 12/1/23 13:37, Gavin Shan wrote:
> Hi Shaoqin,
> 
> On 11/29/23 14:08, Shaoqin Huang wrote:
>> The KVM_ARM_VCPU_PMU_V3_FILTER provide the ability to let the VMM decide
>> which PMU events are provided to the guest. Add a new option
>> `pmu-filter` as -accel sub-option to set the PMU Event Filtering.
>> Without the filter, the KVM will expose all events from the host to
>> guest by default.
>>
>> The `pmu-filter` has such format:
>>
>>    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 hex
>> format now. For example:
>>
>>    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
>> also allowed except the event 0x30 is denied, and all the other events
>> are disallowed.
>>
>> Here is an real example shows how to use the PMU Event Filtering, when
>> we launch a guest by use kvm, add such command line:
>>
>>    # qemu-system-aarch64 \
>>     -accel 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).
>>
>> 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.
>>
>> Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
>> ---
>> 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]
>>
>> v2: 
>> https://lore.kernel.org/all/20231117060838.39723-1-shahuang@redhat.com/
>> v1: 
>> https://lore.kernel.org/all/20231113081713.153615-1-shahuang@redhat.com/
>> ---
>>   include/sysemu/kvm_int.h |  1 +
>>   qemu-options.hx          | 21 +++++++++++++
>>   target/arm/kvm.c         | 23 ++++++++++++++
>>   target/arm/kvm64.c       | 68 ++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 113 insertions(+)
>>
>> 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;
>>   };
>>   void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 42fd09e4de..8b721d6668 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -187,6 +187,7 @@ DEF("accel", HAS_ARG, QEMU_OPTION_accel,
>>       "                tb-size=n (TCG translation block cache size)\n"
>>       "                dirty-ring-size=n (KVM dirty ring GFN count, 
>> default 0)\n"
>>       "                eager-split-size=n (KVM Eager Page Split chunk 
>> size, default 0, disabled. ARM only)\n"
>> +    "                pmu-filter={A,D}:start-end[;...] (KVM PMU Event 
>> Filter, default no filter. ARM only)\n"
>    ^^^^^^^
> 
> Potential alignment issue, or the email isn't shown for me correctly.
> Besides, why not follow the pattern in the commit log, which is nicer
> than what's of being:
> 
> pmu-filter={A,D}:start-end[;...]
> 
> to
> 
> pmu-filter="{A,D}:start-end[;{A,D}:start-end...]
> 

Ok. I can replace it with the better format.

>>       "                
>> 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", QEMU_ARCH_ALL)
>>   SRST
>> @@ -259,6 +260,26 @@ SRST
>>           impact on the memory. By default, this feature is disabled
>>           (eager-split-size=0).
>> +    ``pmu-filter={A,D}:start-end[;...]``
>> +        KVM implements pmu event filtering to prevent a guest from 
>> being able to
>         ^^^^               ^^^^^^^^^^^^^^^^^^^
>         Alignment          "PMU Event Filtering" to be consistent
> 

Thanks for pointing it out. It should be an alignment issue. I will fix it.

>> +    sample certain events. It depends on the 
>> KVM_ARM_VCPU_PMU_V3_FILTER attr
>                                                                              ^^^^
>                                                                              attribute
>> +    supported in KVM. It has the following format:
>> +
>> +    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 hex
>> +    format now. For example:
>> +
>> +    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
>> +    also allowed except the event 0x30 is denied, and all the other 
>> events
>> +    are disallowed.
>> +
>>       ``notify-vmexit=run|internal-error|disable,notify-window=n``
>>           Enables or disables notify VM exit support on x86 host and 
>> specify
>>           the corresponding notify window to trigger the VM exit if 
>> enabled.
>> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
>> index 7903e2ddde..116a0d3d2b 100644
>> --- a/target/arm/kvm.c
>> +++ b/target/arm/kvm.c
>> @@ -1108,6 +1108,22 @@ static void 
>> kvm_arch_set_eager_split_size(Object *obj, Visitor *v,
>>       s->kvm_eager_split_size = value;
>>   }
>> +static char *kvm_arch_get_pmu_filter(Object *obj, Error **errp)
>> +{
>> +    KVMState *s = KVM_STATE(obj);
>> +
>> +    return g_strdup(s->kvm_pmu_filter);
>> +}
>> +
>> +static void kvm_arch_set_pmu_filter(Object *obj, const char *pmu_filter,
>> +                                    Error **errp)
>> +{
>> +    KVMState *s = KVM_STATE(obj);
>> +
>> +    g_free(s->kvm_pmu_filter);
>> +    s->kvm_pmu_filter = g_strdup(pmu_filter);
>> +}
>> +
>>   void kvm_arch_accel_class_init(ObjectClass *oc)
>>   {
>>       object_class_property_add(oc, "eager-split-size", "size",
>> @@ -1116,4 +1132,11 @@ void kvm_arch_accel_class_init(ObjectClass *oc)
>>       object_class_property_set_description(oc, "eager-split-size",
>>           "Eager Page Split chunk size for hugepages. (default: 0, 
>> disabled)");
>> +
>> +    object_class_property_add_str(oc, "pmu-filter",
>> +                                  kvm_arch_get_pmu_filter,
>> +                                  kvm_arch_set_pmu_filter);
>> +
>> +    object_class_property_set_description(oc, "pmu-filter",
>> +        "PMU Event Filtering description for guest pmu. (default: 
>> NULL, disabled)");
>                                                        ^^^
>                                                        PMU

Got it.

>>   }
>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
>> index 3c175c93a7..7947b83b36 100644
>> --- a/target/arm/kvm64.c
>> +++ b/target/arm/kvm64.c
>> @@ -10,6 +10,7 @@
>>    */
>>   #include "qemu/osdep.h"
>> +#include <asm-arm64/kvm.h>
>>   #include <sys/ioctl.h>
>>   #include <sys/ptrace.h>
>> @@ -131,6 +132,70 @@ static bool kvm_arm_set_device_attr(CPUState *cs, 
>> struct kvm_device_attr *attr,
>>       return true;
>>   }
>> +static void kvm_arm_pmu_filter_init(CPUState *cs)
>> +{
>> +    static bool pmu_filter_init = false;
>> +    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,
>> +    };
>> +    KVMState *kvm_state = cs->kvm_state;
> 
> I would move @kvm_state to the beginning of the function since it's the 
> container
> to everything else.
> 

Ok. I can move it to the first.

>> +    char *tmp;
>> +    char *str, act;
>> +
>> +    if (!kvm_state->kvm_pmu_filter)
>> +        return;
>> +
>> +    if (kvm_vcpu_ioctl(cs, KVM_HAS_DEVICE_ATTR, attr)) {
>> +        error_report("The kernel doesn't support the pmu event 
>> filter!\n");
>> +        abort();
>> +    }
>> +
> 
> s/attr/&attr ?
> 

It should be &attr.

> The connection between vCPU and attribute query was set up in Linux 
> v4.10 by
> commit f577f6c2a6a ("arm64: KVM: Introduce per-vcpu kvm device 
> controls"), and
> the capability depends on KVM_CAP_VCPU_ATTRIBUTES. I think 
> KVM_CAP_VCPU_ATTRIBUTES
> needs to be checked prior to kvm_vcpu_ioctl(cs, KVM_HAS_DEVICE_ATTR, ...)
> 

I searched the KVM_CAP_VCPU_ATTRIBUTES, no where check the capability. 
And the current usage of KVM_HAS_DEVICE_ATTR never check the 
KVM_CAP_VCPU_ATTRIBUTES. I guess we don't need it.

> Besides, the PMU Event Filtering was introduced to Linux v4.10. It means 
> the user
> can crash qemu when "pmu-filter" is provided on Linux v4.9. So the 
> correct behavior
> would be warning and ignore "pmu-filter" since it's an add-on and 
> best-effort
> feature.

Thanks for pointing this out, I think it's reasonable. I can replace all 
error_report with wran_report and replace delete all abort().

> 
> 
>> +    /* The filter only needs to be initialized for 1 vcpu. */
>> +    if (!pmu_filter_init)
>> +        pmu_filter_init = true;
>> +
> 
> { } has been missed. QEMU needs it even for the block with single line 
> of code.
> 

Ok. I will add it.

>> +    tmp = g_strdup(kvm_state->kvm_pmu_filter);
>> +
>> +    for (str = strtok(tmp, ";"); str != NULL; str = strtok(NULL, ";")) {
>> +        unsigned short start = 0, end = 0;
>> +
>> +        sscanf(str, "%c:%hx-%hx", &act, &start, &end);
>> +        if ((act != 'A' && act != 'D') || (!start && !end)) {
>> +            error_report("skipping invalid filter %s\n", str);
>> +            continue;
>> +        }
>> +
>> +        filter = (struct kvm_pmu_event_filter) {
>> +            .base_event     = start,
>> +            .nevents        = end - start + 1,
>> +            .action         = act == 'A' ? KVM_PMU_EVENT_ALLOW :
>> +                                           KVM_PMU_EVENT_DENY,
>> +        };
>> +
>> +        if (!kvm_arm_set_device_attr(cs, &attr, "PMU Event Filter")) {
>> +            if (errno == EINVAL)
>> +                error_report("Invalid filter range [0x%x-0x%x]. "
>> +                             "ARMv8.0 support 10 bits event space, "
>> +                             "ARMv8.1 support 16 bits event space",
>> +                             start, end);
>> +            else if (errno == ENODEV)
>> +                error_report("GIC not initialized");
>> +            else if (errno == ENXIO)
>> +                error_report("PMUv3 not properly configured or 
>> in-kernel irqchip "
>> +                             "not configured.");
>> +            else if (errno == EBUSY)
>> +                error_report("PMUv3 already initialized or a VCPU has 
>> already run");
>> +
>> +            abort();
>> +        }
>> +    }
>> +
>> +    g_free(tmp);
>> +}
>> +
> 
> { } has been missed.
> 
> g_strsplit() may be good fit to parse "pmu-filter". 
> cpu-target.c::parse_cpu_option()
> is the example for its usage.
> 

Ok. I can follow the implementation. It actually much simpler.

> As I explained above, it wouldn't a "abort()" since "pmu-filter" is an 
> add-on and
> best-effort attempt. We probably just warn done by warn_report() instead 
> of raising
> error if the PMU Event Filter fails to be set.
> 

Will do that.

Thanks,
Shaoqin

>>   void kvm_arm_pmu_init(CPUState *cs)
>>   {
>>       struct kvm_device_attr attr = {
>> @@ -141,6 +206,9 @@ void kvm_arm_pmu_init(CPUState *cs)
>>       if (!ARM_CPU(cs)->has_pmu) {
>>           return;
>>       }
>> +
>> +    kvm_arm_pmu_filter_init(cs);
>> +
>>       if (!kvm_arm_set_device_attr(cs, &attr, "PMU")) {
>>           error_report("failed to init PMU");
>>           abort();
> 
> Thanks,
> Gavin
> 

-- 
Shaoqin