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

[Zhao Liu <zhao1.liu@intel.com>]

On Wed, Feb 05, 2025 at 11:03:51AM +0100, Markus Armbruster wrote:
> Date: Wed, 05 Feb 2025 11:03:51 +0100
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [RFC v2 1/5] qapi/qom: Introduce kvm-pmu-filter object
> 
> Quick & superficial review for now.

Thanks!

> > 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?

Do you mean it's not in the right place in the qapi-schema.json?

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

Because qom.json includes kvm.json, so I have to place it before
qom.json.

It doesn't have any dependencies itself, so placing it in the previous
position should be fine, where do you prefer?

> > +##
> > +
> > +##
> > +# @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.

OK, will fix.

> >  ##
> >  # = 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.

IIUC, then I should use the name "KvmPmuFilterProperties" (string version
for QAPI), and the name "KvmPmuFilterPropertiesVariant" (numeric version
in codes), do you agree?
 
> >        'main-loop':                  'MainLoopProperties',
> >        'memory-backend-epc':         { 'type': 'MemoryBackendEpcProperties',
> >                                        'if': 'CONFIG_LINUX' },
>