Re: [PATCH 2/3] target/i386: Call KVM_CAP_PMU_CAPABILITY iotcl to enable/disable PMU

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 3/27/2025 10:15 AM, Mingwei Zhang wrote:
> On Wed, Mar 26, 2025 at 5:44 PM Mi, Dapeng <dapeng1.mi@linux.intel.com> wrote:
>>
>> On 3/26/2025 2:46 PM, Dongli Zhang wrote:
>>> Hi Dapeng,
>>>
>>> PATCH 1-4 from the below patchset are already reviewed. (PATCH 5-10 are for PMU
>>> registers reset).
>>>
>>> https://lore.kernel.org/all/20250302220112.17653-1-dongli.zhang@oracle.com/
>>>
>>> They require only trivial modification. i.e.:
>>>
>>> https://github.com/finallyjustice/patchset/tree/master/qemu-amd-pmu-mid/v03
>>>
>>> Therefore, since PATCH 5-10 are for another topic, any chance if I re-send 1-4
>>> as a prerequisite for the patch to explicitly call KVM_CAP_PMU_CAPABILITY?
>> any option is fine for me, spiting it into two separated ones or still keep
>> in a whole patch series. I would rebase this this patchset on top of your
>> v3 patches.
>>
>>
>>> In addition, I have a silly question. Can mediated vPMU coexist with legacy
>>> perf-based vPMU, that is, something like tdp and tdp_mmu? Or the legacy
>>> perf-based vPMU is going to be purged from the most recent kernel?
>> No, they can't. As long as mediated vPMU is enabled, it would totally
>> replace the legacy perf-based vPMU. The legacy perf-based vPMU code would
>> still be kept in the kernel in near future, but the long-term target is to
>> totally remove the perf-based vPMU once mediated vPMU is mature.
> mediated vPMU will co-exist with legacy vPMU right? Mediated vPMU
> currently was constrained to SPR+ on Intel and Genoa+ on AMD. So
> legacy CPUs will have no choice but legacy vPMU.
>
> In the future, to fully replace legacy vPMU we need to solve the
> performance issue due to PMU context switching being located at VM
> enter/exit boundary. Once those limitations are resolved, and older
> x86 CPUs fade away, mediated vPMU can fully take over.

yeah, the code would co-exist in near feature or maybe longer, but mediated
vPMU and the legacy vPMU won't work concurrently, once mediated vPMU is
enabled, it would preempt the legacy vPMU.


>
> Thanks.
> -Mingwei
>>
>>> If they can coexist, how about add property to QEMU control between
>>> legacy/modern? i.e. by default use legacy and change to modern as default in the
>>> future once the feature is stable.

I don't prefer to add such property in Qemu. Whether KVM selects to enable
mediated vPMU or the legacy perf-based vPMU, it should be transparent for
Qemu. Qemu is unnecessary to know it.

Currently Qemu already has too much PMU related options, such as pmu,
lbr/arch_lbr. It's too complicated, it introduces too much dependency on
the code and user needs to take time to learn how to configure these
options correctly as well. 

We don't need more options, on the contrary we need to simplify the PMU
options in Qemu. The ideal situation is to keep only one "pmu" option which
is used to manage all these PMU features, like basic perfmon, LBR and PEBS
etc,. But it may cause back-compatible issues if remove these additional
options...


>>>
>>> Thank you very much!
>>>
>>> Dongli Zhang
>>>
>>> On 3/24/25 5:37 AM, Dapeng Mi wrote:
>>>> After introducing mediated vPMU, mediated vPMU must be enabled by
>>>> explicitly calling KVM_CAP_PMU_CAPABILITY to enable. Thus call
>>>> KVM_CAP_PMU_CAPABILITY to enable/disable PMU base on user configuration.
>>>>
>>>> Suggested-by: Zhao Liu <zhao1.liu@intel.com>
>>>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>>>> ---
>>>>  target/i386/kvm/kvm.c | 17 +++++++++++++++++
>>>>  1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>>>> index f41e190fb8..d3e6984844 100644
>>>> --- a/target/i386/kvm/kvm.c
>>>> +++ b/target/i386/kvm/kvm.c
>>>> @@ -2051,8 +2051,25 @@ full:
>>>>      abort();
>>>>  }
>>>>
>>>> +static bool pmu_cap_set = false;
>>>>  int kvm_arch_pre_create_vcpu(CPUState *cpu, Error **errp)
>>>>  {
>>>> +    KVMState *s = kvm_state;
>>>> +    X86CPU *x86_cpu = X86_CPU(cpu);
>>>> +
>>>> +    if (!pmu_cap_set && kvm_check_extension(s, KVM_CAP_PMU_CAPABILITY)) {
>>>> +        int r = kvm_vm_enable_cap(s, KVM_CAP_PMU_CAPABILITY, 0,
>>>> +                                  KVM_PMU_CAP_DISABLE & !x86_cpu->enable_pmu);
>>>> +        if (r < 0) {
>>>> +            error_report("kvm: Failed to %s pmu cap: %s",
>>>> +                         x86_cpu->enable_pmu ? "enable" : "disable",
>>>> +                         strerror(-r));
>>>> +            return r;
>>>> +        }
>>>> +
>>>> +        pmu_cap_set = true;
>>>> +    }
>>>> +
>>>>      return 0;
>>>>  }
>>>>