On Mon, Apr 28, 2025 at 08:12:09AM +0200, Markus Armbruster wrote:
> Date: Mon, 28 Apr 2025 08:12:09 +0200
> From: Markus Armbruster <arm...@redhat.com>
> Subject: Re: [PATCH 2/5] i386/kvm: Support basic KVM PMU filter
>
> Zhao Liu <zhao1....@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.
Thank you for correction! I didn't realize this point :-(.
> I'd like you to mark the option parameter as "(x86 only)", like
> notify-vmexit right above, and several more elsewhere.
Sure, I see. This option has already provided good example for me.
> >> 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.
I think I understand now. Indeed, creating an object should be allowed
regardless of whether it's used, as this helps decouple "-object" from
other options.
I can add something that:
the kvm-pmu-filter object needs to be activated with "-accel pmu-filter=id",
and only when it is activated, its filter policy can be passed to KVM.
(A single sentence is just an example; I think it needs to be carefully
refined within the context of the entire paragraph :-).)
> >> > +
> >> > +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])?
I get your point now! Yes, I should do in this way.
Thanks,
Zhao
