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

[Shaoqin Huang <shahuang@redhat.com>]

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
>