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

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

> > diff --git a/qapi/kvm.json b/qapi/kvm.json
> > new file mode 100644
> > index 000000000000..1861d86a9726
> > --- /dev/null
> > +++ b/qapi/kvm.json
> > @@ -0,0 +1,84 @@
> > +# -*- Mode: Python -*-
> > +# vim: filetype=python
> > +#
> > +# Copyright (C) 2025 Intel Corporation.
> > +#
> > +# Author: Zhao Liu <zhao1.liu@intel.com>
> > +#
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +##
> > +# == KVM
> > +##
> 
> There's KVM-specific stuff elsewhere in the schema.  Some if of it
> should probably be moved here.  Can you have a look?  This is not a
> demand; it's fine if you can't.  If you can: separate patch preceding
> this one to create kvm.json and move stuff there.

Sure! That's what I should have done, and I'll be back to follow up on
this discussion when I get something new.

> > +
> > +##
> > +# === PMU stuff (KVM related)
> 
> The KVM subsection contains just this subsubsection.  Awkward.  Can we
> do without this subsubsection now?  We can always add it later, when we
> have enough KVM stuff to warrant structuring it into subsubsections.

Thanks! I agree. As I commit to do above, if I find others about KVM,
we can add this subsection you suggested below :-).

> If we decide we want it:
> 
>    # === KVM performance monitor unit (PMU)

Good name.

> > +##
> > +
> > +##
> > +# @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.1
> > +##
> > +{ 'enum': 'KvmPmuFilterAction',
> > +  'data': ['allow', 'deny'] }
> > +
> > +##
> > +# @KvmPmuEventFormat:
> 
> Maybe KvmPmuFilterEventFormat?  Or is that too long?

For another 2 formats: 'x86-select-umask' and 'x86-masked-entry', their
enum value names already have 7 words:

 - KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK
 - KVM_PMU_EVENT_FORMAT_X86_MASKED_ENTRY

With "Filter" in name,

 - KVM_PMU_FILTER_EVENT_FORMAT_X86_SELECT_UMASK
 - KVM_PMU_FILTER_EVENT_FORMAT_X86_MASKED_ENTRY

Look still okay. I'll rename it.

> > +#
> > +# Encoding formats of PMU event that QEMU/KVM supports.
> > +#
> > +# @raw: the encoded event code that KVM can directly consume.
> 
> Suggest
> 
>    # @raw: raw KVM PMU event code.

Concise. I agree.

> > +#
> > +# Since 10.1
> > +##
> > +{ 'enum': 'KvmPmuEventFormat',
> > +  'data': ['raw'] }
> > +
> > +##
> > +# @KvmPmuRawEvent:
> 
> Maybe KvmPmuFilterEventRaw?  Or is that too long?

KvmPmuFilterEventRaw is fine (not too long).

> > +#
> > +# Raw PMU event code.
> > +#
> > +# @code: the raw value that has been encoded, and QEMU could deliver
> > +#     to KVM directly.
> 
> Suggest
> 
>    ##
>    # @KvmPmuRawEvent
>    #
>    # @code: raw KVM PMU event code, to be passed verbatim to KVM.

Thanks for polishing it up, it looks much better.

> > +#
> > +# Since 10.1
> > +##
> > +{ 'struct': 'KvmPmuRawEvent',
> > +  'data': { 'code': 'uint64' } }
> > +
> > +##
> > +# @KvmPmuFilterEvent:
> > +#
> > +# PMU event filtered by KVM.
> 
> Suggest
> 
>    # A KVM PMU event specification.

Sure.

> > +#
> > +# @format: PMU event format.
> > +#
> > +# Since 10.1
> > +##
> > +{ 'union': 'KvmPmuFilterEvent',
> > +  'base': { 'format': 'KvmPmuEventFormat' },
> > +  'discriminator': 'format',
> > +  'data': { 'raw': 'KvmPmuRawEvent' } }
> > +
> > +##
> > +# @KvmPmuFilterProperties:
> > +#
> > +# Properties of KVM PMU Filter.
> > +#
> > +# @action: action that KVM PMU filter will take for selected PMU events.
> > +#
> > +# @events: list of selected PMU events.
> 
> Here's my try:
> 
>    # Properties of kvm-pmu-filter objects.  A kvm-pmu-filter object
>    # restricts the guest's access to the PMU with either an allowlist or
>    # a denylist.
>    #
>    # @action: whether @events is an allowlist or a denylist.
>    #
>    # @events: list of KVM PMU event specifications.

Thank you very much! Your description is very accurate.

Regards,
Zhao