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);
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, 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?
> 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).
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 51a7c61ce0b0..5dcce067d8dd 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -6180,6 +6180,9 @@ SRST
> > ((select) & 0xff) | \
> > ((umask) & 0xff) << 8)
> >
> > +
> > ``{"format":"x86-select-umask","select":event_select,"umask":event_umask}``
> > + Specify the single x86 PMU event with select and umask fields.
> > +
> > An example KVM PMU filter object would look like:
> >
> > .. parsed-literal::
> > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > index fa3a696654cb..0d36ccf250ed 100644
> > --- a/target/i386/kvm/kvm.c
> > +++ b/target/i386/kvm/kvm.c
> > @@ -5974,6 +5974,10 @@ static bool kvm_config_pmu_event(KVMPMUFilter
> > *filter,
> > case KVM_PMU_EVENT_FORMAT_RAW:
> > code = event->u.raw.code;
> > break;
> > + case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
> > + code = X86_PMU_RAW_EVENT(event->u.x86_select_umask.select,
> > + event->u.x86_select_umask.umask);
> > + break;
> > default:
> > g_assert_not_reached();
> > }
> > @@ -6644,6 +6648,7 @@ static void kvm_arch_check_pmu_filter(const Object
> > *obj, const char *name,
> >
> > switch (event->format) {
> > case KVM_PMU_EVENT_FORMAT_RAW:
> > + case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
Here's the current format check I mentioned above. But I agree your idea
and will check in the parsing of pmu-filter object.
> > break;
> > default:
> > error_setg(errp,
>
