Re: [PATCH 3/5] i386/kvm: Support event with select & umask format in KVM PMU filter

[Markus Armbruster <armbru@redhat.com>]

Zhao Liu <zhao1.liu@intel.com> writes:

> The select&umask is the common way for x86 to identify the PMU event,
> so support this way as the "x86-default" format in kvm-pmu-filter
> object.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Tested-by: Yi Lai <yi1.lai@intel.com>
> ---
> Changes since RFC v2:
>  * Drop hexadecimal variants and support numeric version in QAPI
>    directly. (Daniel)
>  * Rename "x86-default" format to "x86-select-umask". (Markus)
>  * Add Tested-by from Yi.
>  * Add documentation in qemu-options.hx.
>  * QAPI style fix:
>    - KVMPMU* stuff -> KvmPmu*.
>  * Bump up the supported QAPI version to v10.1.
>
> Changes since RFC v1:
>  * Bump up the supported QAPI version to v10.0.
> ---
>  accel/kvm/kvm-pmu.c      | 20 +++++++++++++++++++-
>  include/system/kvm-pmu.h | 13 +++++++++++++
>  qapi/kvm.json            | 21 +++++++++++++++++++--
>  qemu-options.hx          |  3 +++
>  target/i386/kvm/kvm.c    |  5 +++++
>  5 files changed, 59 insertions(+), 3 deletions(-)
>
> diff --git a/accel/kvm/kvm-pmu.c b/accel/kvm/kvm-pmu.c
> index 22f749bf9183..fa73ef428e59 100644
> --- a/accel/kvm/kvm-pmu.c
> +++ b/accel/kvm/kvm-pmu.c
> @@ -16,6 +16,8 @@
>  #include "qom/object_interfaces.h"
>  #include "system/kvm-pmu.h"
>  
> +#define UINT12_MAX (4095)
> +
>  static void kvm_pmu_filter_set_action(Object *obj, int value,
>                                        Error **errp G_GNUC_UNUSED)
>  {
> @@ -53,9 +55,22 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
>      }
>  
>      for (node = head; node; node = node->next) {
> -        switch (node->value->format) {
> +        KvmPmuFilterEvent *event = node->value;
> +
> +        switch (event->format) {
>          case KVM_PMU_EVENT_FORMAT_RAW:
>              break;
> +        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
> +            if (event->u.x86_select_umask.select > UINT12_MAX) {
> +                error_setg(errp,
> +                           "Parameter 'select' out of range (%d).",
> +                           UINT12_MAX);
> +                goto fail;
> +            }
> +
> +            /* No need to check the range of umask since it's uint8_t. */
> +            break;
> +        }

As we'll see below, the new x86-specific format is defined in the QAPI
schema regardless of target.

It is accepted here also regardless of target.  Doesn't matter much
right now, as the object is effectively useless for targets other than
x86, but I understand that will change.

Should we reject it unless the target is x86?

If not, I feel the behavior should be noted in the commit message.

>          default:
>              g_assert_not_reached();
>          }
> @@ -67,6 +82,9 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
>      filter->events = head;
>      qapi_free_KvmPmuFilterEventList(old_head);
>      return;
> +
> +fail:
> +    qapi_free_KvmPmuFilterEventList(head);
>  }
>  
>  static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
> diff --git a/include/system/kvm-pmu.h b/include/system/kvm-pmu.h
> index 818fa309c191..6abc0d037aee 100644
> --- a/include/system/kvm-pmu.h
> +++ b/include/system/kvm-pmu.h
> @@ -32,4 +32,17 @@ struct KVMPMUFilter {
>      KvmPmuFilterEventList *events;
>  };
>  
> +/*
> + * Stolen from Linux kernel (RAW_EVENT at tools/testing/selftests/kvm/include/
> + * x86_64/pmu.h).
> + *
> + * Encode an eventsel+umask pair into event-select MSR format.  Note, this is
> + * technically AMD's format, as Intel's format only supports 8 bits for the
> + * event selector, i.e. doesn't use bits 24:16 for the selector.  But, OR-ing
> + * in '0' is a nop and won't clobber the CMASK.
> + */
> +#define X86_PMU_RAW_EVENT(eventsel, umask) (((eventsel & 0xf00UL) << 24) | \
> +                                            ((eventsel) & 0xff) | \
> +                                            ((umask) & 0xff) << 8)
> +
>  #endif /* KVM_PMU_H */
> diff --git a/qapi/kvm.json b/qapi/kvm.json
> index 1861d86a9726..cb151ca82e5c 100644
> --- a/qapi/kvm.json
> +++ b/qapi/kvm.json
> @@ -36,10 +36,12 @@
>  #
>  # @raw: the encoded event code that KVM can directly consume.
>  #
> +# @x86-select-umask: standard x86 encoding format with select and umask.
> +#
>  # Since 10.1
>  ##
>  { 'enum': 'KvmPmuEventFormat',
> -  'data': ['raw'] }
> +  'data': ['raw', 'x86-select-umask'] }
>  
>  ##
>  # @KvmPmuRawEvent:
> @@ -54,6 +56,20 @@
>  { 'struct': 'KvmPmuRawEvent',
>    'data': { 'code': 'uint64' } }
>  
> +##
> +# @KvmPmuX86SelectUmaskEvent:
> +#
> +# @select: x86 PMU event select field, which is a 12-bit unsigned
> +#     number.
> +#
> +# @umask: x86 PMU event umask field.
> +#
> +# Since 10.1
> +##
> +{ 'struct': 'KvmPmuX86SelectUmaskEvent',
> +  'data': { 'select': 'uint16',
> +            'umask': 'uint8' } }
> +
>  ##
>  # @KvmPmuFilterEvent:
>  #
> @@ -66,7 +82,8 @@
>  { 'union': 'KvmPmuFilterEvent',
>    'base': { 'format': 'KvmPmuEventFormat' },
>    'discriminator': 'format',
> -  'data': { 'raw': 'KvmPmuRawEvent' } }
> +  'data': { 'raw': 'KvmPmuRawEvent',
> +            'x86-select-umask': 'KvmPmuX86SelectUmaskEvent' } }
>  
>  ##
>  # @KvmPmuFilterProperties:

Documentation could perhaps be more explicit about this making sense
only for x86.

> diff --git a/qemu-options.hx b/qemu-options.hx
> index 51a7c61ce0b0..5dcce067d8dd 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -6180,6 +6180,9 @@ SRST
>               ((select) & 0xff) | \
>               ((umask) & 0xff) << 8)
>  
> +        ``{"format":"x86-select-umask","select":event_select,"umask":event_umask}``
> +            Specify the single x86 PMU event with select and umask fields.
> +
>          An example KVM PMU filter object would look like:
>  
>          .. parsed-literal::
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index fa3a696654cb..0d36ccf250ed 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -5974,6 +5974,10 @@ static bool kvm_config_pmu_event(KVMPMUFilter *filter,
>          case KVM_PMU_EVENT_FORMAT_RAW:
>              code = event->u.raw.code;
>              break;
> +        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
> +            code = X86_PMU_RAW_EVENT(event->u.x86_select_umask.select,
> +                                     event->u.x86_select_umask.umask);
> +            break;
>          default:
>              g_assert_not_reached();
>          }
> @@ -6644,6 +6648,7 @@ static void kvm_arch_check_pmu_filter(const Object *obj, const char *name,
>  
>          switch (event->format) {
>          case KVM_PMU_EVENT_FORMAT_RAW:
> +        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
>              break;
>          default:
>              error_setg(errp,