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

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

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,
>