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

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>
---
 accel/kvm/kvm-pmu.c      | 62 ++++++++++++++++++++++++++++++++++++++++
 include/sysemu/kvm-pmu.h | 13 +++++++++
 qapi/kvm.json            | 46 +++++++++++++++++++++++++++--
 target/i386/kvm/kvm.c    |  5 ++++
 4 files changed, 123 insertions(+), 3 deletions(-)

On 7/10/2024 12:51 PM, Zhao Liu wrote:
> 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>
> ---
>  accel/kvm/kvm-pmu.c      | 62 ++++++++++++++++++++++++++++++++++++++++
>  include/sysemu/kvm-pmu.h | 13 +++++++++
>  qapi/kvm.json            | 46 +++++++++++++++++++++++++++--
>  target/i386/kvm/kvm.c    |  5 ++++
>  4 files changed, 123 insertions(+), 3 deletions(-)
>
> diff --git a/accel/kvm/kvm-pmu.c b/accel/kvm/kvm-pmu.c
> index 483d1bdf4807..51d3fba5a72a 100644
> --- a/accel/kvm/kvm-pmu.c
> +++ b/accel/kvm/kvm-pmu.c
> @@ -17,6 +17,8 @@
>  #include "qom/object_interfaces.h"
>  #include "sysemu/kvm-pmu.h"
>  
> +#define UINT12_MAX (4095)
> +
>  static void kvm_pmu_filter_get_event(Object *obj, Visitor *v, const char *name,
>                                       void *opaque, Error **errp)
>  {
> @@ -38,6 +40,12 @@ static void kvm_pmu_filter_get_event(Object *obj, Visitor *v, const char *name,
>              str_event->u.raw.code = g_strdup_printf("0x%lx",
>                                                      event->u.raw.code);
>              break;
> +        case KVM_PMU_EVENT_FMT_X86_DEFAULT:
> +            str_event->u.x86_default.select =
> +                g_strdup_printf("0x%x", event->u.x86_default.select);
> +            str_event->u.x86_default.umask =
> +                g_strdup_printf("0x%x", event->u.x86_default.umask);
> +            break;
>          default:
>              g_assert_not_reached();
>          }
> @@ -83,6 +91,60 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
>                  goto fail;
>              }
>              break;
> +        case KVM_PMU_EVENT_FMT_X86_DEFAULT: {
> +            uint64_t select, umask;
> +
> +            ret = qemu_strtou64(str_event->u.x86_default.select, NULL,
> +                                0, &select);
> +            if (ret < 0) {
> +                error_setg(errp,
> +                           "Invalid %s PMU event (select: %s): %s. "
> +                           "The select must be a "
> +                           "12-bit unsigned number string.",
> +                           KVMPMUEventEncodeFmt_str(str_event->format),
> +                           str_event->u.x86_default.select,
> +                           strerror(-ret));
> +                g_free(event);
> +                goto fail;
> +            }
> +            if (select > UINT12_MAX) {
> +                error_setg(errp,
> +                           "Invalid %s PMU event (select: %s): "
> +                           "Numerical result out of range. "
> +                           "The select must be a "
> +                           "12-bit unsigned number string.",
> +                           KVMPMUEventEncodeFmt_str(str_event->format),
> +                           str_event->u.x86_default.select);
> +                g_free(event);
> +                goto fail;
> +            }
> +            event->u.x86_default.select = select;
> +
> +            ret = qemu_strtou64(str_event->u.x86_default.umask, NULL,
> +                                0, &umask);
> +            if (ret < 0) {
> +                error_setg(errp,
> +                           "Invalid %s PMU event (umask: %s): %s. "
> +                           "The umask must be a uint8 string.",
> +                           KVMPMUEventEncodeFmt_str(str_event->format),
> +                           str_event->u.x86_default.umask,
> +                           strerror(-ret));
> +                g_free(event);
> +                goto fail;
> +            }
> +            if (umask > UINT8_MAX) {

umask is extended to 16 bits from Perfmon v6+. Please notice we need to
upgrade this to 16 bits in the future. More details can be found here.
[PATCH V3 00/13] Support Lunar Lake and Arrow Lake core PMU - kan.liang
(kernel.org)
<https://lore.kernel.org/all/20240626143545.480761-1-kan.liang@linux.intel.com/>


> +                error_setg(errp,
> +                           "Invalid %s PMU event (umask: %s): "
> +                           "Numerical result out of range. "
> +                           "The umask must be a uint8 string.",
> +                           KVMPMUEventEncodeFmt_str(str_event->format),
> +                           str_event->u.x86_default.umask);
> +                g_free(event);
> +                goto fail;
> +            }
> +            event->u.x86_default.umask = umask;
> +            break;
> +        }
>          default:
>              g_assert_not_reached();
>          }
> diff --git a/include/sysemu/kvm-pmu.h b/include/sysemu/kvm-pmu.h
> index 4707759761f1..707f33d604fd 100644
> --- a/include/sysemu/kvm-pmu.h
> +++ b/include/sysemu/kvm-pmu.h
> @@ -26,4 +26,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 0619da83c123..0d759884c229 100644
> --- a/qapi/kvm.json
> +++ b/qapi/kvm.json
> @@ -27,11 +27,13 @@
>  #
>  # @raw: the encoded event code that KVM can directly consume.
>  #
> +# @x86-default: standard x86 encoding format with select and umask.
> +#
>  # Since 9.1
>  ##
>  { 'enum': 'KVMPMUEventEncodeFmt',
>    'prefix': 'KVM_PMU_EVENT_FMT',
> -  'data': ['raw'] }
> +  'data': ['raw', 'x86-default'] }
>  
>  ##
>  # @KVMPMURawEvent:
> @@ -46,6 +48,25 @@
>  { 'struct': 'KVMPMURawEvent',
>    'data': { 'code': 'uint64' } }
>  
> +##
> +# @KVMPMUX86DefalutEvent:
> +#
> +# x86 PMU event encoding with select and umask.
> +# raw_event = ((select & 0xf00UL) << 24) | \
> +#              (select) & 0xff) | \
> +#              ((umask) & 0xff) << 8)
> +#
> +# @select: x86 PMU event select field, which is a 12-bit unsigned
> +#     number.
> +#
> +# @umask: x86 PMU event umask field.
> +#
> +# Since 9.1
> +##
> +{ 'struct': 'KVMPMUX86DefalutEvent',
> +  'data': { 'select': 'uint16',
> +            'umask': 'uint8' } }
> +
>  ##
>  # @KVMPMUFilterEvent:
>  #
> @@ -61,7 +82,8 @@
>    'base': { 'action': 'KVMPMUFilterAction',
>              'format': 'KVMPMUEventEncodeFmt' },
>    'discriminator': 'format',
> -  'data': { 'raw': 'KVMPMURawEvent' } }
> +  'data': { 'raw': 'KVMPMURawEvent',
> +            'x86-default': 'KVMPMUX86DefalutEvent' } }
>  
>  ##
>  # @KVMPMUFilterProperty:
> @@ -89,6 +111,23 @@
>  { 'struct': 'KVMPMURawEventVariant',
>    'data': { 'code': 'str' } }
>  
> +##
> +# @KVMPMUX86DefalutEventVariant:
> +#
> +# The variant of KVMPMUX86DefalutEvent with the string, rather than
> +# the numeric value.
> +#
> +# @select: x86 PMU event select field.  This field is a 12-bit
> +#     unsigned number string.
> +#
> +# @umask: x86 PMU event umask field. This field is a uint8 string.
> +#
> +# Since 9.1
> +##
> +{ 'struct': 'KVMPMUX86DefalutEventVariant',
> +  'data': { 'select': 'str',
> +            'umask': 'str' } }
> +
>  ##
>  # @KVMPMUFilterEventVariant:
>  #
> @@ -104,7 +143,8 @@
>    'base': { 'action': 'KVMPMUFilterAction',
>              'format': 'KVMPMUEventEncodeFmt' },
>    'discriminator': 'format',
> -  'data': { 'raw': 'KVMPMURawEventVariant' } }
> +  'data': { 'raw': 'KVMPMURawEventVariant',
> +            'x86-default': 'KVMPMUX86DefalutEventVariant' } }
>  
>  ##
>  # @KVMPMUFilterPropertyVariant:
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index e9bf79782316..391531c036a6 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -5393,6 +5393,10 @@ kvm_config_pmu_event(KVMPMUFilter *filter,
>          case KVM_PMU_EVENT_FMT_RAW:
>              code = event->u.raw.code;
>              break;
> +        case KVM_PMU_EVENT_FMT_X86_DEFAULT:
> +            code = X86_PMU_RAW_EVENT(event->u.x86_default.select,
> +                                     event->u.x86_default.umask);
> +            break;
>          default:
>              g_assert_not_reached();
>          }
> @@ -6073,6 +6077,7 @@ static void kvm_arch_check_pmu_filter(const Object *obj, const char *name,
>  
>          switch (event->format) {
>          case KVM_PMU_EVENT_FMT_RAW:
> +        case KVM_PMU_EVENT_FMT_X86_DEFAULT:
>              break;
>          default:
>              error_setg(errp,

Hi Dapeng,

On Thu, Jul 18, 2024 at 01:28:25PM +0800, Mi, Dapeng wrote:

[snip]

> > +        case KVM_PMU_EVENT_FMT_X86_DEFAULT: {
> > +            uint64_t select, umask;
> > +
> > +            ret = qemu_strtou64(str_event->u.x86_default.select, NULL,
> > +                                0, &select);
> > +            if (ret < 0) {
> > +                error_setg(errp,
> > +                           "Invalid %s PMU event (select: %s): %s. "
> > +                           "The select must be a "
> > +                           "12-bit unsigned number string.",
> > +                           KVMPMUEventEncodeFmt_str(str_event->format),
> > +                           str_event->u.x86_default.select,
> > +                           strerror(-ret));
> > +                g_free(event);
> > +                goto fail;
> > +            }
> > +            if (select > UINT12_MAX) {
> > +                error_setg(errp,
> > +                           "Invalid %s PMU event (select: %s): "
> > +                           "Numerical result out of range. "
> > +                           "The select must be a "
> > +                           "12-bit unsigned number string.",
> > +                           KVMPMUEventEncodeFmt_str(str_event->format),
> > +                           str_event->u.x86_default.select);
> > +                g_free(event);
> > +                goto fail;
> > +            }
> > +            event->u.x86_default.select = select;
> > +
> > +            ret = qemu_strtou64(str_event->u.x86_default.umask, NULL,
> > +                                0, &umask);
> > +            if (ret < 0) {
> > +                error_setg(errp,
> > +                           "Invalid %s PMU event (umask: %s): %s. "
> > +                           "The umask must be a uint8 string.",
> > +                           KVMPMUEventEncodeFmt_str(str_event->format),
> > +                           str_event->u.x86_default.umask,
> > +                           strerror(-ret));
> > +                g_free(event);
> > +                goto fail;
> > +            }
> > +            if (umask > UINT8_MAX) {
> 
> umask is extended to 16 bits from Perfmon v6+. Please notice we need to
> upgrade this to 16 bits in the future. More details can be found here.
> [PATCH V3 00/13] Support Lunar Lake and Arrow Lake core PMU - kan.liang
> (kernel.org)
> <https://lore.kernel.org/all/20240626143545.480761-1-kan.liang@linux.intel.com/>

It's tricky...now I referred the RAW_EVENT format in tools/testing/
selftests/kvm/include/x86_64/pmu.h, which is used in KVM PMU and is
compatible with AMD and Intel.

The current KVM PMU filter for raw code doesn't define the layout in
the standard API like masked entries (KVM_PMU_ENCODE_MASKED_ENTRY), but
actually uses the RAW_EVENT format. So I even plan to move RAW_EVENT
macro into arch/x86/include/uapi/asm/kvm.h...

For the changes you mentioned, I think it would be better for the raw
code layout design not to break RAW_EVENT, so that AMD and Intel can
equally use the same macro to encode. Is it possible for a unified
layout macro?

What about extending RAW_EVENT as the following example? I understand
the umask2 is at bit 40-47.

#define X86_PMU_RAW_EVENT(eventsel, umask) (((eventsel & 0xf00UL) << 24) | \
                                            ((eventsel) & 0xff) | \
                                            ((umask) & 0xff) << 8) | \
					    ((umask & 0xff00UL << 32)

Thanks,
Zhao

On 7/19/2024 4:40 PM, Zhao Liu wrote:
> Hi Dapeng,
>
> On Thu, Jul 18, 2024 at 01:28:25PM +0800, Mi, Dapeng wrote:
>
> [snip]
>
>>> +        case KVM_PMU_EVENT_FMT_X86_DEFAULT: {
>>> +            uint64_t select, umask;
>>> +
>>> +            ret = qemu_strtou64(str_event->u.x86_default.select, NULL,
>>> +                                0, &select);
>>> +            if (ret < 0) {
>>> +                error_setg(errp,
>>> +                           "Invalid %s PMU event (select: %s): %s. "
>>> +                           "The select must be a "
>>> +                           "12-bit unsigned number string.",
>>> +                           KVMPMUEventEncodeFmt_str(str_event->format),
>>> +                           str_event->u.x86_default.select,
>>> +                           strerror(-ret));
>>> +                g_free(event);
>>> +                goto fail;
>>> +            }
>>> +            if (select > UINT12_MAX) {
>>> +                error_setg(errp,
>>> +                           "Invalid %s PMU event (select: %s): "
>>> +                           "Numerical result out of range. "
>>> +                           "The select must be a "
>>> +                           "12-bit unsigned number string.",
>>> +                           KVMPMUEventEncodeFmt_str(str_event->format),
>>> +                           str_event->u.x86_default.select);
>>> +                g_free(event);
>>> +                goto fail;
>>> +            }
>>> +            event->u.x86_default.select = select;
>>> +
>>> +            ret = qemu_strtou64(str_event->u.x86_default.umask, NULL,
>>> +                                0, &umask);
>>> +            if (ret < 0) {
>>> +                error_setg(errp,
>>> +                           "Invalid %s PMU event (umask: %s): %s. "
>>> +                           "The umask must be a uint8 string.",
>>> +                           KVMPMUEventEncodeFmt_str(str_event->format),
>>> +                           str_event->u.x86_default.umask,
>>> +                           strerror(-ret));
>>> +                g_free(event);
>>> +                goto fail;
>>> +            }
>>> +            if (umask > UINT8_MAX) {
>> umask is extended to 16 bits from Perfmon v6+. Please notice we need to
>> upgrade this to 16 bits in the future. More details can be found here.
>> [PATCH V3 00/13] Support Lunar Lake and Arrow Lake core PMU - kan.liang
>> (kernel.org)
>> <https://lore.kernel.org/all/20240626143545.480761-1-kan.liang@linux.intel.com/>
> It's tricky...now I referred the RAW_EVENT format in tools/testing/
> selftests/kvm/include/x86_64/pmu.h, which is used in KVM PMU and is
> compatible with AMD and Intel.
>
> The current KVM PMU filter for raw code doesn't define the layout in
> the standard API like masked entries (KVM_PMU_ENCODE_MASKED_ENTRY), but
> actually uses the RAW_EVENT format. So I even plan to move RAW_EVENT
> macro into arch/x86/include/uapi/asm/kvm.h...
For the raw event format, I suppose user should know the event layout and
would directly input the raw event code, qemu may not concern this.


>
> For the changes you mentioned, I think it would be better for the raw
> code layout design not to break RAW_EVENT, so that AMD and Intel can
> equally use the same macro to encode. Is it possible for a unified
> layout macro?
>
> What about extending RAW_EVENT as the following example? I understand
> the umask2 is at bit 40-47.
>
> #define X86_PMU_RAW_EVENT(eventsel, umask) (((eventsel & 0xf00UL) << 24) | \
>                                             ((eventsel) & 0xff) | \
>                                             ((umask) & 0xff) << 8) | \
> 					    ((umask & 0xff00UL << 32)

It's always good if Intel and AMD can share same event layout, the extended
umask field occupies bits [40:47]. So I think we'd better follow this for
the RAW_EVENT as well.


>
> Thanks,
> Zhao
>