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

[Eric Auger <eauger@redhat.com>]

Hi,

On 3/19/24 19:07, Daniel P. Berrangé wrote:
> On Tue, Mar 19, 2024 at 06:58:33PM +0100, Eric Auger wrote:
>> Hi Daniel,
>>
>> On 3/19/24 16:22, Daniel P. Berrangé wrote:
>>> On Wed, Feb 21, 2024 at 01:34:31AM -0500, 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).
>>>>
>>>> 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"
>>>
>>> snip
>>>
>>>> @@ -517,6 +533,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)");
>>>>  }
>>>
>>> Passing a string property, but....[1]
>>>
>>>>  
>>>>  bool kvm_arm_pmu_supported(void)
>>>> @@ -1706,6 +1728,62 @@ 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;
>>>> +    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!");
>>>
>>> If the user requested a filter and it can't be supported, QEMU
>>> must exit with an error, not ignore the user's request.
>>>
>>>> +        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) {
>>>> +        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;
>>>> +
>>>> +        if (sscanf(event_filters[i], "%c:%hx-%hx", &act, &start, &end) != 3) {
>>>> +            warn_report("Skipping invalid PMU filter %s", event_filters[i]);
>>>> +            continue;
>>>
>>> Warning on user syntax errors is undesirable - it should be a fatal
>>> error of the user gets this wrong.
>>>
>>>> +        }
>>>> +
>>>> +        if ((act != 'A' && act != 'D') || start > end) {
>>>> +            warn_report("Skipping invalid PMU filter %s", event_filters[i]);
>>>> +            continue;
>>>
>>> Likewise should be fatal.
>>>
>>>> +        }
>>>> +
>>>> +        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_V3_FILTER")) {
>>>> +            break;
>>>> +        }
>>>> +    }
>>>> +}
>>>
>>> ..[1] then implementing a custom parser is rather a QEMU design anti-pattern,
>>> especially when the proposed syntax is incapable of being mapped into the
>>> normal QAPI syntax for a list of structs should we want to fully convert
>>> -cpu to QAPI parsing later. I wonder if can we model this property with
>>> QAPI now ?
>> I guess you mean creating a new property like those in
>> hw/core/qdev-properties-system.c for instance  and populating an array
>> of those at CPU object level?
> 
> Yeah, something like the IOThreadVirtQueueMapping data type would
> be the more QAPI like code pattern.
OK thank you for the confirmation. Then if we create such kind of
property it would be nice that this latter also matches the need of x86
PMU filtering. I think the uapi exists at KVM level but has never been
integrated in qemu.
> 
>> Note there is v8 but most of your comments still apply
>> https://lore.kernel.org/all/20240312074849.71475-1-shahuang@redhat.com/
> 
> Yes, sorry I just saw Peter's query about libvirt on this v7 and
> didn't think to look for a newer version

no problem. Thank you for your time

Eric
> 
> With regards,
> Daniel