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

[Shaoqin Huang]

Hi Sebastian,

On 11/15/23 20:17, Sebastian Ott wrote:
> 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

Thanks for point it out.

> 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).

It looks better. Will change it.

> > +        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.

Good idea. Will do that.

Thanks,
Shaoqin

> Thanks,
> Sebastian