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

From: Jim Mattson <jmattson@google.com>
To: Aaron Lewis <aaronlewis@google.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, seanjc@google.com
Subject: Re: [PATCH v2 1/4] kvm: x86/pmu: Introduce masked events to the pmu event filter
Date: Fri, 1 Jul 2022 16:43:58 -0700	[thread overview]
Message-ID: <CALMp9eT3U+kLJTJJ_QP66LQPTQywVTuxucx=7JU74Xb7=xeY0g@mail.gmail.com> (raw)
In-Reply-To: <20220606175248.1884041-2-aaronlewis@google.com>

On Mon, Jun 6, 2022 at 10:53 AM Aaron Lewis <aaronlewis@google.com> wrote:
>
> When building an event list for the pmu event filter, fitting all the
> events in the limited space can be a challenge.  It becomes
> particularly challenging when trying to include various unit mask
> combinations for a particular event the guest is allow to or not allow
> to program.  Instead of increasing the size of the list to allow for
> these, add a new encoding in the pmu event filter's events field. These
> encoded events can then be used to test against the event the guest is
> attempting to program to determine if the guest should have access to
> it.
>
> The encoded values are: mask, match, and invert.  When filtering events
> the mask is applied to the guest's unit mask to see if it matches the
> match value (ie: unit_mask & mask == match).  If it does and the pmu
> event filter is an allow list the event is allowed, and denied if it's
> a deny list.  Additionally, the result is reversed if the invert flag
> is set in the encoded event.
>
> This feature is enabled by setting the flags field to
> KVM_PMU_EVENT_FLAG_MASKED_EVENTS.
>
> Events can be encoded by using KVM_PMU_EVENT_ENCODE_MASKED_EVENT().
>
> It is an error to have a bit set outside valid encoded bits, and calls
> to KVM_SET_PMU_EVENT_FILTER will return -EINVAL in such cases,
> including bits that are set in the high nybble[1] for AMD if called on
> Intel.
>
> [1] bits 35:32 in the event and bits 11:8 in the eventsel.
>
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  Documentation/virt/kvm/api.rst         |  46 +++++++--
>  arch/x86/include/asm/kvm-x86-pmu-ops.h |   1 +
>  arch/x86/include/uapi/asm/kvm.h        |   8 ++
>  arch/x86/kvm/pmu.c                     | 128 ++++++++++++++++++++++---
>  arch/x86/kvm/pmu.h                     |   1 +
>  arch/x86/kvm/svm/pmu.c                 |  12 +++
>  arch/x86/kvm/vmx/pmu_intel.c           |  12 +++
>  7 files changed, 190 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 11e00a46c610..4e904772da5b 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -5017,7 +5017,13 @@ using this ioctl.
>  :Architectures: x86
>  :Type: vm ioctl
>  :Parameters: struct kvm_pmu_event_filter (in)
> -:Returns: 0 on success, -1 on error
> +:Returns: 0 on success,
> +    -EFAULT args[0] cannot be accessed.
> +    -EINVAL args[0] contains invalid data in the filter or events field.
> +                    Note: event validation is only done for modes where
> +                    the flags field is non-zero.
> +    -E2BIG nevents is too large.
> +    -ENOMEM not enough memory to allocate the filter.
>
>  ::
>
> @@ -5030,14 +5036,42 @@ using this ioctl.
>         __u64 events[0];
>    };
>
> -This ioctl restricts the set of PMU events that the guest can program.
> -The argument holds a list of events which will be allowed or denied.
> -The eventsel+umask of each event the guest attempts to program is compared
> -against the events field to determine whether the guest should have access.
> +This ioctl restricts the set of PMU events the guest can program.  The
> +argument holds a list of events which will be allowed or denied.
> +
>  The events field only controls general purpose counters; fixed purpose
>  counters are controlled by the fixed_counter_bitmap.
>
> -No flags are defined yet, the field must be zero.
> +Valid values for 'flags'::
> +
> +``0``
> +
> +This is the default behavior for the pmu event filter, and used when the
> +flags field is clear.  In this mode the eventsel+umask for the event the
> +guest is attempting to program is compared against each event in the events
> +field to determine whether the guest should have access to it.
> +
> +``KVM_PMU_EVENT_FLAG_MASKED_EVENTS``
> +
> +In this mode each event in the events field will be encoded with mask, match,
> +and invert values in addition to an eventsel.  These encoded events will be
> +matched against the event the guest is attempting to program to determine
> +whether the guest should have access to it.  When matching an encoded event
> +with a guest event these steps are followed:
> + 1. Match the encoded eventsel to the guest eventsel.
> + 2. If that matches, match the mask and match values from the encoded event to
> +    the guest's unit mask (ie: unit_mask & mask == match).
> + 3. If that matches, the guest is allow to program the event if its an allow
> +    list or the guest is not allow to program the event if its a deny list.
> + 4. If the invert value is set in the encoded event, reverse the meaning of #3
> +    (ie: deny if its an allow list, allow if it's a deny list).

The invert flag introduces some ambiguity. What if a particular event
matches two of the masked filter entries: one with an invert flag and
one without?

> +To encode an event in the pmu_event_filter use
> +KVM_PMU_EVENT_ENCODE_MASKED_EVENT().
> +
> +If a bit is set in an encoded event that is not apart of the bits used for

Nit: "a part"?