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

[Markus Armbruster <armbru@redhat.com>]

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.

> 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' } ] }

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.

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

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

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

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