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

[Eric Auger]

Hi Peter,

On 2/29/24 12:00, Peter Maydell wrote:
> On Thu, 29 Feb 2024 at 02:32, Shaoqin Huang <shahuang@redhat.com> wrote:
>>
>> Hi Peter,
>>
>> On 2/22/24 22:28, Peter Maydell wrote:
>>> On Wed, 21 Feb 2024 at 06:34, Shaoqin Huang <shahuang@redhat.com> 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).
>>>>
>>>> Here is an example which 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 \
>>>>          -cpu host,kvm-pmu-filter="D:0x11-0x11"
>>>>
>>>> Since the first action is deny, we have a global allow policy. This
>>>> filters out 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 do still work.
> 
>>>
>>> The new syntax for the filter property seems quite complicated.
>>> I think it would be worth testing it with a new test in
>>> tests/qtest/arm-cpu-features.c.
>>
>> I was trying to add a test in tests/qtest/arm-cpu-features.c. But I
>> found all other cpu-feature is bool property.
>>
>> When I use the 'query-cpu-model-expansion' to query the cpu-features,
>> the kvm-pmu-filter will not shown in the returned results, just like below.
>>
>> {'execute': 'query-cpu-model-expansion', 'arguments': {'type': 'full',
>> 'model': { 'name': 'host'}}}{"return": {}}
>>
>> {"return": {"model": {"name": "host", "props": {"sve768": false,
>> "sve128": false, "sve1024": false, "sve1280": false, "sve896": false,
>> "sve256": false, "sve1536": false, "sve1792": false, "sve384": false,
>> "sve": false, "sve2048": false, "pauth": false, "kvm-no-adjvtime":
>> false, "sve512": false, "aarch64": true, "pmu": true, "sve1920": false,
>> "sve1152": false, "kvm-steal-time": true, "sve640": false, "sve1408":
>> false, "sve1664": false}}}}
>>
>> I'm not sure if it's because the `query-cpu-model-expansion` only return
>> the feature which is bool. Since the kvm-pmu-filter is a str, it won't
>> be recognized as a feature.
>>
>> So I want to ask how can I add the kvm-pmu-filter which is str property
>> into the cpu-feature.c test.
> 
> It doesn't appear because the list of properties that we advertise
> via query-cpu-model-expansion is set in the cpu_model_advertised_features[]
> array in target/arm/arm-qmp-cmds.c, and this patch doesn't add
> 'kvm-pmu-filter' to it. But you have a good point about all the
> others being bool properties: I don't know enough about that
> mechanism to know if simply adding this to the list is right.
> 
> This does raise a more general question: do we need to advertise
> the existence of this property to libvirt via QMP? Eric, Sebastian:
> do you know ?
sorry I missed this question. yes I think it is sensible to expose that
option to libvirt. There is no good default value to be set at qemu
level so to me it really depends on the upper stack to choose the
correct value (depending on the sensitiveness of the data that justified
the kernel uapi).
> 
> If we don't care about this being visible to libvirt then the
> importance of having a test case covering the command line
> syntax goes down a bit.
> 
>>>>
>>>> +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;
>>>> +    g_auto(GStrv) event_filters;
>>>> +
>>>> +    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!");
>>>
>>> Drop "The ".
>>>
>>> Should this really only be a warning, rather than an error?
>>>
>>
>> I think this is an add-on feature, and shouldn't block the qemu init
>> process. If we want to set the wrong pmu filter and it doesn't take
>> affect to the VM, it can be detected in the VM.
> 
> But if the user explicitly asked for it, it's not optional
> for them, it's something they want. We should fail if the user
> passes us an option that we can't actually carry out.
> 
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * The filter only needs to be initialized through one vcpu ioctl and 
>>>> it
>>>> +     * will affect all other vcpu in the vm.
>>>
>>> Weird. Why isn't it a VM ioctl if it affects the whole VM ?
>>>
>>  From (kernel) commit d7eec2360e3 ("KVM: arm64: Add PMU event filtering
>> infrastructure"):
>>    Note that although the ioctl is per-vcpu, the map of allowed events is
>>    global to the VM (it can be setup from any vcpu until the vcpu PMU is
>>    initialized).
> 
> That just says it is a per-vcpu ioctl, it doesn't say why...

Marc said in a his cover letter
"
The filter state is global to the guest, despite the PMU being per CPU.
I'm not sure whether it would be worth it making it CPU-private."

I guess this is because Marc wanted to reuse the
KVM_ARM_VCPU_PMU_V3_CTRL group and just added a new attribute on top of
existing
KVM_ARM_VCPU_PMU_V3_IRQ, KVM_ARM_VCPU_PMU_V3_INIT

Thanks

Eric
> 
>>>> +    event_filters = g_strsplit(cpu->kvm_pmu_filter, ";", -1);
>>>> +    for (i = 0; event_filters[i]; i++) {
>>>> +        unsigned short start = 0, end = 0;
>>>> +        char act;
>>>> +
>>>> +        if (sscanf(event_filters[i], "%c:%hx-%hx", &act, &start, &end) != 
>>>> 3) {
>>>> +            warn_report("Skipping invalid PMU filter %s", 
>>>> event_filters[i]);
>>>> +            continue;
>>>> +        }
>>>> +
>>>> +        if ((act != 'A' && act != 'D') || start > end) {
>>>> +            warn_report("Skipping invalid PMU filter %s", 
>>>> event_filters[i]);
>>>> +            continue;
>>>> +        }
>>>
>>> It would be better to do the syntax checking up-front when
>>> the user tries to set the property. Then you can make the
>>> property-setting return an error for invalid strings.
>>>
>>
>> Ok. I guess you mean to say move the syntax checking to the
>> kvm_pmu_filter_set() function. But wouldn't it cause some code
>> duplication? Since it should first check syntax of the string in
>> kvm_pmu_filter_set() and then parse the string in this function.
> 
> No, you should check syntax and parse the string in
> kvm_pmu_filter_set(), and fill in a data structure so you don't
> have to do any string parsing here. kvm_pmu_filter_get()
> will need to reconstitute a string from the data structure.
> 
> thanks
> -- PMM
>