Re: [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object

[Markus Armbruster <armbru@redhat.com>]

Quick & superficial review for now.

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

> Introduce the kvm-pmu-filter object and support the PMU event with raw
> format.
>
> The raw format, as a native PMU event code representation, can be used
> for several architectures.
>
> Considering that PMU event related fields are commonly used in
> hexadecimal, define KVMPMURawEventVariant, KVMPMUFilterEventVariant, and
> KVMPMUFilterPropertyVariant in kvm.json to support hexadecimal number
> strings in JSON.
>
> Additionally, define the corresponding numeric versions of
> KVMPMURawEvent, KVMPMUFilterEvent, and KVMPMUFilterProperty in kvm.json.
> This allows to handle numeric values more effectively and take advantage
> of the qapi helpers.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

[...]

> diff --git a/qapi/kvm.json b/qapi/kvm.json
> new file mode 100644
> index 000000000000..d51aeeba7cd8
> --- /dev/null
> +++ b/qapi/kvm.json
> @@ -0,0 +1,116 @@
> +# -*- Mode: Python -*-
> +# vim: filetype=python
> +
> +##
> +# = KVM based feature API

This is a top-level section.  It ends up between sections "QMP
introspection" and "QEMU Object Model (QOM)".  Is this what we want?  Or
should it be a sub-section of something?  Or next to something else?

> +##
> +
> +##
> +# @KVMPMUFilterAction:
> +#
> +# Actions that KVM PMU filter supports.
> +#
> +# @deny: disable the PMU event/counter in KVM PMU filter.
> +#
> +# @allow: enable the PMU event/counter in KVM PMU filter.
> +#
> +# Since 10.0
> +##
> +{ 'enum': 'KVMPMUFilterAction',
> +  'prefix': 'KVM_PMU_FILTER_ACTION',
> +  'data': ['allow', 'deny'] }
> +
> +##
> +# @KVMPMUEventEncodeFmt:

Please don't abbreviate Format to Fmt.  We use Format elsewhere, and
consistency is desirable.

> +#
> +# Encoding formats of PMU event that QEMU/KVM supports.
> +#
> +# @raw: the encoded event code that KVM can directly consume.
> +#
> +# Since 10.0
> +##
> +{ 'enum': 'KVMPMUEventEncodeFmt',
> +  'prefix': 'KVM_PMU_EVENT_FMT',
> +  'data': ['raw'] }
> +
> +##
> +# @KVMPMURawEvent:
> +#
> +# Raw PMU event code.
> +#
> +# @code: the raw value that has been encoded, and QEMU could deliver
> +#     to KVM directly.
> +#
> +# Since 10.0
> +##
> +{ 'struct': 'KVMPMURawEvent',
> +  'data': { 'code': 'uint64' } }
> +
> +##
> +# @KVMPMUFilterEvent:
> +#
> +# PMU event filtered by KVM.
> +#
> +# @format: PMU event format.
> +#
> +# Since 10.0
> +##
> +{ 'union': 'KVMPMUFilterEvent',
> +  'base': { 'format': 'KVMPMUEventEncodeFmt' },
> +  'discriminator': 'format',
> +  'data': { 'raw': 'KVMPMURawEvent' } }
> +
> +##
> +# @KVMPMUFilterProperty:
> +#
> +# Property of KVM PMU Filter.
> +#
> +# @events: the KVMPMUFilterEvent list.
> +#
> +# Since 10.0
> +##
> +{ 'struct': 'KVMPMUFilterProperty',
> +  'data': { '*events': ['KVMPMUFilterEvent'] } }
> +
> +##
> +# @KVMPMURawEventVariant:
> +#
> +# The variant of KVMPMURawEvent with the string, rather than the
> +# numeric value.
> +#
> +# @code: the raw value that has been encoded, and QEMU could deliver
> +#     to KVM directly.  This field is a uint64 string.
> +#
> +# Since 10.0
> +##
> +{ 'struct': 'KVMPMURawEventVariant',
> +  'data': { 'code': 'str' } }
> +
> +##
> +# @KVMPMUFilterEventVariant:
> +#
> +# The variant of KVMPMUFilterEvent.
> +#
> +# @format: PMU event format.
> +#
> +# Since 10.0
> +##
> +{ 'union': 'KVMPMUFilterEventVariant',
> +  'base': { 'format': 'KVMPMUEventEncodeFmt' },
> +  'discriminator': 'format',
> +  'data': { 'raw': 'KVMPMURawEventVariant' } }
> +
> +##
> +# @KVMPMUFilterPropertyVariant:
> +#
> +# The variant of KVMPMUFilterProperty.
> +#
> +# @action: action that KVM PMU filter will take.
> +#
> +# @events: the KVMPMUFilterEventVariant list.
> +#
> +# Since 10.0
> +##
> +{ 'struct': 'KVMPMUFilterPropertyVariant',
> +  'data': { 'action': 'KVMPMUFilterAction',
> +            '*events': ['KVMPMUFilterEventVariant'] } }
> diff --git a/qapi/meson.build b/qapi/meson.build
> index e7bc54e5d047..856439c76b67 100644
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -37,6 +37,7 @@ qapi_all_modules = [
>    'error',
>    'introspect',
>    'job',
> +  'kvm',
>    'machine-common',
>    'machine',
>    'machine-target',
> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> index b1581988e4eb..742818d16e45 100644
> --- a/qapi/qapi-schema.json
> +++ b/qapi/qapi-schema.json
> @@ -64,6 +64,7 @@
>  { 'include': 'compat.json' }
>  { 'include': 'control.json' }
>  { 'include': 'introspect.json' }
> +{ 'include': 'kvm.json' }
>  { 'include': 'qom.json' }
>  { 'include': 'qdev.json' }
>  { 'include': 'machine-common.json' }
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 28ce24cd8d08..c75ec4b21e95 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -8,6 +8,7 @@
>  { 'include': 'block-core.json' }
>  { 'include': 'common.json' }
>  { 'include': 'crypto.json' }
> +{ 'include': 'kvm.json' }
>  
>  ##
>  # = QEMU Object Model (QOM)
> @@ -1108,6 +1109,7 @@
>        'if': 'CONFIG_LINUX' },
>      'iommufd',
>      'iothread',
> +    'kvm-pmu-filter',
>      'main-loop',
>      { 'name': 'memory-backend-epc',
>        'if': 'CONFIG_LINUX' },
> @@ -1183,6 +1185,7 @@
>                                        'if': 'CONFIG_LINUX' },
>        'iommufd':                    'IOMMUFDProperties',
>        'iothread':                   'IothreadProperties',
> +      'kvm-pmu-filter':             'KVMPMUFilterPropertyVariant',

The others are like

         'mumble': 'MumbleProperties'

Let's stick to that, and also avoid running together multiple
capitalized acronyms: KvmPmuFilterProperties.

>        'main-loop':                  'MainLoopProperties',
>        'memory-backend-epc':         { 'type': 'MemoryBackendEpcProperties',
>                                        'if': 'CONFIG_LINUX' },