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

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

On Mon, Apr 28, 2025 at 09:19:07AM +0200, Markus Armbruster wrote:
> Date: Mon, 28 Apr 2025 09:19:07 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH 3/5] i386/kvm: Support event with select & umask format
>  in KVM PMU filter
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > Hi Markus,
> >
> >> > +        case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
> >> > +            if (event->u.x86_select_umask.select > UINT12_MAX) {
> >> > +                error_setg(errp,
> >> > +                           "Parameter 'select' out of range (%d).",
> >> > +                           UINT12_MAX);
> >> > +                goto fail;
> >> > +            }
> >> > +
> >> > +            /* No need to check the range of umask since it's uint8_t. */
> >> > +            break;
> >> > +        }
> >> 
> >> As we'll see below, the new x86-specific format is defined in the QAPI
> >> schema regardless of target.
> >> 
> >> It is accepted here also regardless of target.  Doesn't matter much
> >> right now, as the object is effectively useless for targets other than
> >> x86, but I understand that will change.
> >> 
> >> Should we reject it unless the target is x86?
> >
> > I previously supposed that different architectures should implement
> > their own kvm_arch_check_pmu_filter(), which is the `check` hook of
> > object_class_property_add_link():
> >
> >     object_class_property_add_link(oc, "pmu-filter",
> >                                    TYPE_KVM_PMU_FILTER,
> >                                    offsetof(KVMState, pmu_filter),
> >                                    kvm_arch_check_pmu_filter,
> >                                    OBJ_PROP_LINK_STRONG);
> 
> This way, the checking happens only when you actually connect the
> kvm-pmu-filter object to the accelerator.
> 
> Have you considered checking in the kvm-pmu-filter object's complete()
> method?  Simple example of how to do that: qauthz_simple_complete() in
> authz/simple.c.

Thank you, I hadn't noticed it before. Now I study it carefully, and yes,
this is a better way than `check` hook. Though in the following we are
talking about other ways to handle target-specific check, this helper
may be still useful as I proposed to help check accel-specific cases in
the reply to Philip [*].

[*]: https://lore.kernel.org/qemu-devel/aA3cHIcKmt3vdkVk@intel.com/

> > For x86, I implemented kvm_arch_check_pmu_filter() in target/i386/kvm/
> > kvm.c and checked the supported formats (I also supposed arch-specific
> > PMU filter could reject the unsupported format in
> > kvm_arch_check_pmu_filter().)
> >
> > But I think your idea is better, i.e., rejecting unsupported format
> > early in pmu-filter parsing.
> >
> > Well, IIUC, there is no way to specify in QAPI that certain enumerations
> > are generic and certain enumerations are arch-specific,
> 
> Here's how to make enum values conditional:
> 
>     { 'enum': 'KvmPmuEventFormat',
>       'data': ['raw',
>                { 'name': 'x86-select-umask', 'if': 'TARGET_I386' }
>                { 'name': 'x86-masked-entry', 'if': 'TARGET_I386' } ] }

What I'm a bit hesitant about is that, if different arches add similar
"conditional" enumerations later, it could cause the enumeration values
to change under different compilation conditions (correct? :-)). Although
it might not break anything, since we don't rely on the specific numeric
values.

> However, TARGET_I386 is usable only in target-specific code.  This has
> two consequences here:
> 
> 1. It won't compile, since QAPI schema module kvm.json is
>    target-independent.  We'd have to put it into a target-specific
>    module kvm-target.json.
> 
> 2. Target-specific QAPI schema mdoules are problematic for the single
>    binary / heterogeneous machine work.  We are discussing how to best
>    handle that.  Unclear whether adding more target-specific QAPI
>    definitions are a good idea.

And per yours feedback, CONFIG_KVM can also only be used in target-specific
code. Moreover, especially if we need to further consider multiple
accelerators, such as the HVF need mentioned by Philip, it seems that
we can't avoid target-specific issues here!

> >                                                         so rejecting
> > unsupported format can only happen in parsing code. For example, wrap
> > the above code in "#if defined(TARGET_I386)":
> >
> >     for (node = head; node; node = node->next) {
> >         KvmPmuFilterEvent *event = node->value;
> >
> >         switch (event->format) {
> >         case KVM_PMU_EVENT_FORMAT_RAW:
> >             break;
> > #if defined(TARGET_I386)
> >         case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
> >             ...
> >             break;
> >         }
> >         case KVM_PMU_EVENT_FORMAT_X86_MASKED_ENTRY: {
> >             ...
> > 	    break;
> >         }
> > #endif
> >         default:
> > 	    error_setg(errp,
> >                        "Unsupported format.");
> >             goto fail;
> >         }
> >
> >         ...
> >     }
> >
> > EMM, do you like this idea?
> 
> This is kvm_pmu_filter_set_event(), I presume.
>
> The #if is necessary when you make the enum values conditional.  The
> default: code is unreachable then, so it should stay
> g_assert_not_reached().
>
> The #if is fine even when you don't make the enum values conditional.
> The default: code is reachable then, unless you reject the unwanted
> enums earlier some other way.

Thanks for your analysis, it's very accurate! I personally prefer the
2nd way.

> >> If not, I feel the behavior should be noted in the commit message.
> >
> > With the above change, I think it's possible to reject x86-specific
> > format on non-x86 arch. And I can also note this behavior in commit
> > message.
> >
> >> >          default:
> >> >              g_assert_not_reached();
> >> >          }
> >> > @@ -67,6 +82,9 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
> >> >      filter->events = head;
> >> >      qapi_free_KvmPmuFilterEventList(old_head);
> >> >      return;
> >> > +
> >> > +fail:
> >> > +    qapi_free_KvmPmuFilterEventList(head);
> >> >  }
> >> >  
> >> >  static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
> >
> > ...
> >
> >> >  ##
> >> >  # @KvmPmuFilterEvent:
> >> >  #
> >> > @@ -66,7 +82,8 @@
> >> >  { 'union': 'KvmPmuFilterEvent',
> >> >    'base': { 'format': 'KvmPmuEventFormat' },
> >> >    'discriminator': 'format',
> >> > -  'data': { 'raw': 'KvmPmuRawEvent' } }
> >> > +  'data': { 'raw': 'KvmPmuRawEvent',
> >> > +            'x86-select-umask': 'KvmPmuX86SelectUmaskEvent' } }
> >> >  
> >> >  ##
> >> >  # @KvmPmuFilterProperties:
> >> 
> >> Documentation could perhaps be more explicit about this making sense
> >> only for x86.
> >
> > What about the following doc?
> >
> > ##
> > # @KvmPmuFilterProperties:
> > #
> > # Properties of KVM PMU Filter (only for x86).
> 
> Hmm.  Branch 'raw' make sense regardless of target, doesn't it?  It's
> actually usable only for i86 so far, because this series implements
> accelerator property "pmu-filter" only for i386.

The advantage of this single note is someone can easily mention other
arch in doc :-)

> Let's not worry about this until we decided whether to use QAPI
> conditionals or not.

OK, this is not a big deal (comparing with other issues).

Thanks,
Zhao