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:

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

Every binary we create contains target-specific code for at most one
target.  Therefore, different numerical encodings for different targets
are fine.

Same argument for struct members, by the way.  Consider

    { 'struct': 'CpuModelExpansionInfo',
      'data': { 'model': 'CpuModelInfo',
                'deprecated-props' : { 'type': ['str'],
                                       'if': 'TARGET_S390X' } },
      'if': { 'any': [ 'TARGET_S390X',
                       'TARGET_I386',
                       'TARGET_ARM',
                       'TARGET_LOONGARCH64',
                       'TARGET_RISCV' ] } }

This generates

    #if defined(TARGET_S390X) || defined(TARGET_I386) || defined(TARGET_ARM) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV)
    struct CpuModelExpansionInfo {
        CpuModelInfo *model;
    #if defined(TARGET_S390X)
        strList *deprecated_props;
    #endif /* defined(TARGET_S390X) */
    };
    #endif /* defined(TARGET_S390X) || defined(TARGET_I386) || defined(TARGET_ARM) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV) */

The struct's size depends on the target.  If we ever add members after
@deprecated_props, their offset depends on the target, too.

The single binary work will invalidate the "at most one target"
property.  We need to figure out how to best deal with that, but not in
this thread.

[...]