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

[Mi, Dapeng]

On 7/19/2024 4:40 PM, Zhao Liu wrote:
> Hi Dapeng,
>
> On Thu, Jul 18, 2024 at 01:28:25PM +0800, Mi, Dapeng wrote:
>
> [snip]
>
>>> +        case KVM_PMU_EVENT_FMT_X86_DEFAULT: {
>>> +            uint64_t select, umask;
>>> +
>>> +            ret = qemu_strtou64(str_event->u.x86_default.select, NULL,
>>> +                                0, &select);
>>> +            if (ret < 0) {
>>> +                error_setg(errp,
>>> +                           "Invalid %s PMU event (select: %s): %s. "
>>> +                           "The select must be a "
>>> +                           "12-bit unsigned number string.",
>>> +                           KVMPMUEventEncodeFmt_str(str_event->format),
>>> +                           str_event->u.x86_default.select,
>>> +                           strerror(-ret));
>>> +                g_free(event);
>>> +                goto fail;
>>> +            }
>>> +            if (select > UINT12_MAX) {
>>> +                error_setg(errp,
>>> +                           "Invalid %s PMU event (select: %s): "
>>> +                           "Numerical result out of range. "
>>> +                           "The select must be a "
>>> +                           "12-bit unsigned number string.",
>>> +                           KVMPMUEventEncodeFmt_str(str_event->format),
>>> +                           str_event->u.x86_default.select);
>>> +                g_free(event);
>>> +                goto fail;
>>> +            }
>>> +            event->u.x86_default.select = select;
>>> +
>>> +            ret = qemu_strtou64(str_event->u.x86_default.umask, NULL,
>>> +                                0, &umask);
>>> +            if (ret < 0) {
>>> +                error_setg(errp,
>>> +                           "Invalid %s PMU event (umask: %s): %s. "
>>> +                           "The umask must be a uint8 string.",
>>> +                           KVMPMUEventEncodeFmt_str(str_event->format),
>>> +                           str_event->u.x86_default.umask,
>>> +                           strerror(-ret));
>>> +                g_free(event);
>>> +                goto fail;
>>> +            }
>>> +            if (umask > UINT8_MAX) {
>> umask is extended to 16 bits from Perfmon v6+. Please notice we need to
>> upgrade this to 16 bits in the future. More details can be found here.
>> [PATCH V3 00/13] Support Lunar Lake and Arrow Lake core PMU - kan.liang
>> (kernel.org)
>> <https://lore.kernel.org/all/20240626143545.480761-1-kan.liang@linux.intel.com/>
> It's tricky...now I referred the RAW_EVENT format in tools/testing/
> selftests/kvm/include/x86_64/pmu.h, which is used in KVM PMU and is
> compatible with AMD and Intel.
>
> The current KVM PMU filter for raw code doesn't define the layout in
> the standard API like masked entries (KVM_PMU_ENCODE_MASKED_ENTRY), but
> actually uses the RAW_EVENT format. So I even plan to move RAW_EVENT
> macro into arch/x86/include/uapi/asm/kvm.h...
For the raw event format, I suppose user should know the event layout and
would directly input the raw event code, qemu may not concern this.


>
> For the changes you mentioned, I think it would be better for the raw
> code layout design not to break RAW_EVENT, so that AMD and Intel can
> equally use the same macro to encode. Is it possible for a unified
> layout macro?
>
> What about extending RAW_EVENT as the following example? I understand
> the umask2 is at bit 40-47.
>
> #define X86_PMU_RAW_EVENT(eventsel, umask) (((eventsel & 0xf00UL) << 24) | \
>                                             ((eventsel) & 0xff) | \
>                                             ((umask) & 0xff) << 8) | \
>                                           ((umask & 0xff00UL << 32)

It's always good if Intel and AMD can share same event layout, the extended
umask field occupies bits [40:47]. So I think we'd better follow this for
the RAW_EVENT as well.


>
> Thanks,
> Zhao
>