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

[Daniel P . Berrangé]

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.

> Note there is v8 but most of your comments still apply
> 20240312074849.71475-1-shahuang@redhat.com/">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

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|