Re: [PATCH v5 4/7] kvm: x86/pmu: Introduce masked events to the pmu event filter

[Sean Christopherson <seanjc@google.com>]

On Fri, Oct 21, 2022, Aaron Lewis wrote:
> Here's what I came up with.  Let me know if this is what you were thinking:

A purely mechanical suggestions, but overall looks awesome! 

> static int filter_sort_cmp(const void *pa, const void *pb)
> {
>         u64 a = *(u64 *)pa & (KVM_PMU_MASKED_ENTRY_EVENT_SELECT |
>                               KVM_PMU_MASKED_ENTRY_EXCLUDE);
>         u64 b = *(u64 *)pb & (KVM_PMU_MASKED_ENTRY_EVENT_SELECT |
>                               KVM_PMU_MASKED_ENTRY_EXCLUDE);
> 
>         return (a > b) - (a < b);
> }
> 
> /*
>  * For the event filter, searching is done on the 'includes' list and
>  * 'excludes' list separately rather than on the 'events' list (which
>  * has both).  As a result the exclude bit can be ignored.
>  */
> static int filter_event_cmp(const void *pa, const void *pb)
> {
>         u64 a = *(u64 *)pa & KVM_PMU_MASKED_ENTRY_EVENT_SELECT;
>         u64 b = *(u64 *)pb & KVM_PMU_MASKED_ENTRY_EVENT_SELECT;
> 
>         return (a > b) - (a < b);
> }


To dedup code slightly and make this a little more readable, what about adding a
common helper to do the compare?  That also makes it quite obvious that the only
difference is the inclusion (heh) of the EXCLUDE flag.

static int filter_cmp(u64 *pa, u64 *pb, u64 mask)
{
        u64 a = *pa & mask;
        u64 b = *pb & mask;

        return (a > b) - (a < b);
}

static int filter_sort_cmp(const void *pa, const void *pb)
{
        return filter_cmp(pa, pb, (KVM_PMU_MASKED_ENTRY_EVENT_SELECT |
                                   KVM_PMU_MASKED_ENTRY_EXCLUDE);
}

/*
 * For the event filter, searching is done on the 'includes' list and
 * 'excludes' list separately rather than on the 'events' list (which
 * has both).  As a result the exclude bit can be ignored.
 */
static int filter_event_cmp(const void *pa, const void *pb)
{
        return filter_cmp(pa, pb, (KVM_PMU_MASKED_ENTRY_EVENT_SELECT);
}

> static bool filter_contains_match(u64 *events, u64 nevents, u64 eventsel)
> {
>         u64 event_select = eventsel & kvm_pmu_ops.EVENTSEL_EVENT;
>         u64 umask = eventsel & ARCH_PERFMON_EVENTSEL_UMASK;
>         int i, index;
> 
>         index = find_filter_index(events, nevents, event_select);
>         if (index < 0)
>                 return false;
> 
>         /*
>          * Entries are sorted by the event select.  Walk the list in both
>          * directions to process all entries with the targeted event select.
>          */
>         for (i = index; i < nevents; i++) {
>                 if (filter_event_cmp(&events[i], &event_select) != 0)

Preferred kernel style is to omit comparisons against zero, i.e. just

		if (filter_event_cmp(&events[i], &event_select))
			break;

>                         break;
> 
>                 if (is_filter_entry_match(events[i], umask))
>                         return true;
>         }
> 
>         for (i = index - 1; i >= 0; i--) {
>                 if (filter_event_cmp(&events[i], &event_select) != 0)
>                         break;
> 
>                 if (is_filter_entry_match(events[i], umask))
>                         return true;
>         }
> 
>         return false;
> }
> 
> static bool is_gp_event_allowed(struct kvm_x86_pmu_event_filter *filter,
>                                 u64 eventsel)
> {
>         if (filter_contains_match(filter->includes,
> filter->nr_includes, eventsel) &&
>             !filter_contains_match(filter->excludes,
> filter->nr_excludes, eventsel))
>                 return filter->action == KVM_PMU_EVENT_ALLOW;
> 
>         return filter->action == KVM_PMU_EVENT_DENY;

Might be worth using a single char for the filter param, e.g. 'f' yields:

static bool is_gp_event_allowed(struct kvm_x86_pmu_event_filter *f,
                                u64 eventsel)
{
        if (filter_contains_match(f->includes, f->nr_includes, eventsel) &&
            !filter_contains_match(f->excludes, f->nr_excludes, eventsel))
                return f->action == KVM_PMU_EVENT_ALLOW;

        return f->action == KVM_PMU_EVENT_DENY;
}

> static void setup_filter_lists(struct kvm_x86_pmu_event_filter *filter)
> {
>         int i;
> 
>         for (i = 0; i < filter->nevents; i++) {
>                 if(filter->events[i] & KVM_PMU_MASKED_ENTRY_EXCLUDE)a

Space after the if

		if (filter-> ...)

>                         break;
>         }
> 
>         filter->nr_includes = i;
>         filter->nr_excludes = filter->nevents - filter->nr_includes;
>         filter->includes = filter->events;
>         filter->excludes = filter->events + filter->nr_includes;
> }
> 

...

> static void prepare_filter_events(struct kvm_x86_pmu_event_filter *filter)
> {
>         int i, j;
> 
>         if (filter->flags & KVM_PMU_EVENT_FLAG_MASKED_EVENTS)
>                 return;
> 
>         for (i = 0, j = 0; i < filter->nevents; i++) {
>                 /*
>                  * Skip events that are impossible to match against a guest
>                  * event.  When filtering, only the event select + unit mask
>                  * of the guest event is used.

This is a good place for calling out that impossible filters can't be rejected
for backwards compatibility reasons.

>                  */
>                 if (filter->events[i] & ~(kvm_pmu_ops.EVENTSEL_EVENT |
>                                           ARCH_PERFMON_EVENTSEL_UMASK))
>                         continue;
> 
>                 /*
>                  * Convert userspace events to a common in-kernel event so
>                  * only one code path is needed to support both events.  For
>                  * the in-kernel events use masked events because they are
>                  * flexible enough to handle both cases.  To convert to masked
>                  * events all that's needed is to add the umask_mask.

I think it's worth calling out this creates an "all ones" umask_mask, and that
EXCLUDE isn't supported.

>                  */
>                 filter->events[j++] =
>                         filter->events[i] |
>                         (0xFFULL << KVM_PMU_MASKED_ENTRY_UMASK_MASK_SHIFT);
>         }
> 
>         filter->nevents = j;
> }

...

> -       /* Restore the verified state to guard against TOCTOU attacks. */
> -       *filter = tmp;
> +       r = -EINVAL;
> +       if (!is_filter_valid(filter))
> +               goto cleanup;
> 
> -       remove_impossible_events(filter);
> +       prepare_filter_events(filter);
> 
>         /*
> -        * Sort the in-kernel list so that we can search it with bsearch.
> +        * Sort entries by event select so that all entries for a given
> +        * event select can be processed efficiently during filtering.
>          */
> -       sort(&filter->events, filter->nevents, sizeof(__u64), cmp_u64, NULL);
> +       sort(&filter->events, filter->nevents, sizeof(filter->events[0]),
> +            filter_sort_cmp, NULL);
> +
> +       setup_filter_lists(filter);

The sort+setup should definitely go in a single helper.  Rather than have the
helpers deal with masked vs. legacy, what about putting that logic in a top-level
helper?  Then this code is simply:

	r = prepare_filter_lists(filter);
	if (r)
		goto cleanup;

And the helper names can be more explicit, i.e. can call out that they validate
a masked filter and convert to a masked filter.

E.g. (completely untested)

static bool is_masked_filter_valid(const struct kvm_x86_pmu_event_filter *filter)
{
        u64 mask = kvm_pmu_ops.EVENTSEL_EVENT |
                   KVM_PMU_MASKED_ENTRY_UMASK_MASK |
                   KVM_PMU_MASKED_ENTRY_UMASK_MATCH |
                   KVM_PMU_MASKED_ENTRY_EXCLUDE;
        int i;

        for (i = 0; i < filter->nevents; i++) {
                if (filter->events[i] & ~mask)
                        return false;
        }

        return true;
}
static void convert_to_masked_filter(struct kvm_x86_pmu_event_filter *filter)
{
        int i, j;

        for (i = 0, j = 0; i < filter->nevents; i++) {
                /*
                 * Skip events that are impossible to match against a guest
                 * event.  When filtering, only the event select + unit mask
                 * of the guest event is used.  To maintain backwards
                 * compatibility, impossible filters can't be rejected :-(
                 */
                if (filter->events[i] & ~(kvm_pmu_ops.EVENTSEL_EVENT |
                                          ARCH_PERFMON_EVENTSEL_UMASK))
                        continue;
                /*
                 * Convert userspace events to a common in-kernel event so
                 * only one code path is needed to support both events.  For
                 * the in-kernel events use masked events because they are
                 * flexible enough to handle both cases.  To convert to masked
                 * events all that's needed is to add an "all ones" umask_mask,
                 * (unmasked filter events don't support EXCLUDE).
                 */
                filter->events[j++] = filter->events[i] |
                                      (0xFFULL << KVM_PMU_MASKED_ENTRY_UMASK_MASK_SHIFT);
	}

        filter->nevents = j;
}

static int prepare_filter_lists(struct kvm_x86_pmu_event_filter *filter)
{
        int i;

        if (!(filter->flags & KVM_PMU_EVENT_FLAG_MASKED_EVENTS)
                convert_to_masked_filter(filter)
        else if (!is_masked_filter_valid(filter))
                return -EINVAL;

        /*
         * Sort entries by event select and includes vs. excludes so that all
         * entries for a given event select can be processed efficiently during
         * filtering.  The EXCLUDE flag uses a more significant bit than the
         * event select, and so the sorted list is also effectively split into
         * includes and excludes sub-lists.
         */
        sort(&filter->events, filter->nevents, sizeof(filter->events[0]),
             filter_sort_cmp, NULL);

        /* Find the first EXCLUDE event (only supported for masked events). */
        if (filter->flags & KVM_PMU_EVENT_FLAG_MASKED_EVENTS) {
                for (i = 0; i < filter->nevents; i++) {
                        if (filter->events[i] & KVM_PMU_MASKED_ENTRY_EXCLUDE)
                                break;
                }
        }

        filter->nr_includes = i;
        filter->nr_excludes = filter->nevents - filter->nr_includes;
        filter->includes = filter->events;
        filter->excludes = filter->events + filter->nr_includes;
}