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

[Sebastian Ott]

Hi,

On Mon, 13 Nov 2023, Shaoqin Huang wrote:
> +    ``pmu-filter={A,D}:start-end[;...]``
> +        KVM implements pmu event filtering to prevent a guest from being able 
> to
> +       sample certain events. It has the following format:
> +
> +       pmu-filter="{A,D}:start-end[;{A,D}:start-end...]"
> +
> +       The A means "allow" and D means "deny", start if the first event of the
                                                      ^
                                                      is

Also it should be stated that the first filter action defines if the whole
list is an allow or a deny list.

> +static void kvm_arm_pmu_filter_init(CPUState *cs)
> +{
> +    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,
> +    };
> +    KVMState *kvm_state = cs->kvm_state;
> +    char *tmp;
> +    char *str, act;
> +
> +    if (!kvm_state->kvm_pmu_filter)
> +        return;
> +
> +    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,
> +        };
> +
> +        attr.addr = (uint64_t)&filter;

That could move to the initialization of attr (the address of filter
doesn't change).

> +        if (!kvm_arm_set_device_attr(cs, &attr, "PMU Event Filter")) {
> +            error_report("Failed to init PMU Event Filter\n");
> +            abort();
> +        }
> +    }
> +
> +    g_free(tmp);
> +}
> +
> void kvm_arm_pmu_init(CPUState *cs)
> {
>     struct kvm_device_attr attr = {
>         .group = KVM_ARM_VCPU_PMU_V3_CTRL,
>         .attr = KVM_ARM_VCPU_PMU_V3_INIT,
>     };
> +    static bool pmu_filter_init = false;
>
>     if (!ARM_CPU(cs)->has_pmu) {
>         return;
>     }
> +    if (!pmu_filter_init) {
> +        kvm_arm_pmu_filter_init(cs);
> +        pmu_filter_init = true;

pmu_filter_init could move inside kvm_arm_pmu_filter_init() - maybe
together with a comment that this only needs to be called for 1 vcpu.

Thanks,
Sebastian