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?
Note there is v8 but most of your comments still apply
https://lore.kernel.org/all/20240312074849.71475-1-shahuang@xxxxxxxxxx/
Thanks
Eric
>
> With regards,
> Daniel
