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@xxxxxxxxxx>
> 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' },
>
