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 <arm...@redhat.com>
> Subject: Re: [PATCH 3/5] i386/kvm: Support event with select & umask format
> in KVM PMU filter
>
> Zhao Liu <zhao1....@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/aa3chickmt3vd...@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
