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;
}
