Re: [PATCH 2/5] i386/kvm: Support basic KVM PMU filter

[Markus Armbruster <armbru@redhat.com>]

Zhao Liu <zhao1.liu@intel.com> writes:

> ...
>
>> > diff --git a/qemu-options.hx b/qemu-options.hx
>> > index dc694a99a30a..51a7c61ce0b0 100644
>> > --- a/qemu-options.hx
>> > +++ b/qemu-options.hx
>> > @@ -232,7 +232,8 @@ DEF("accel", HAS_ARG, QEMU_OPTION_accel,
>> >      "                eager-split-size=n (KVM Eager Page Split chunk size, default 0, disabled. ARM only)\n"
>> >      "                notify-vmexit=run|internal-error|disable,notify-window=n (enable notify VM exit and set notify window, x86 only)\n"
>> >      "                thread=single|multi (enable multi-threaded TCG)\n"
>> > -    "                device=path (KVM device path, default /dev/kvm)\n", QEMU_ARCH_ALL)
>> > +    "                device=path (KVM device path, default /dev/kvm)\n"
>> > +    "                pmu-filter=id (configure KVM PMU filter)\n", QEMU_ARCH_ALL)
>> 
>> As we'll see below, this property is actually available only for i386.
>> Other target-specific properties document this like "x86 only".  Please
>> do that for this one, too.
>
> Thanks! I'll change QEMU_ARCH_ALL to QEMU_ARCH_I386.

That would be wrong :)

QEMU_ARCH_ALL is the last argument passed to macro DEF().  It applies to
the entire option, in this case -accel.

I'd like you to mark the option parameter as "(x86 only)", like
notify-vmexit right above, and several more elsewhere.

>> As far as I can tell, the kvm-pmu-filter object needs to be activated
>> with -accel pmu-filter=... to do anything.  Correct?
>
> Yes,
>
>> You can create any number of kvm-pmu-filter objects, but only one of
>> them can be active.  Correct?
>
> Yes! I'll try to report error when user repeats to set this object, or
> mention this rule in doc.

Creating kvm-pmu-filter objects without using them should be harmless,
shouldn't it?  I think users can already create other kinds of unused
objects.

>> > +
>> > +static int kvm_install_pmu_event_filter(KVMState *s)
>> > +{
>> > +    struct kvm_pmu_event_filter *kvm_filter;
>> > +    KVMPMUFilter *filter = s->pmu_filter;
>> > +    int ret;
>> > +
>> > +    kvm_filter = g_malloc0(sizeof(struct kvm_pmu_event_filter) +
>> > +                           filter->nevents * sizeof(uint64_t));
>> 
>> Should we use sizeof(filter->events[0])?
>
> No, here I'm trying to constructing the memory accepted in kvm interface
> (with the specific layout), which is not the same as the KVMPMUFilter
> object.

You're right.  What about sizeof(kvm_filter->events[0])?

[...]