On 11/19/2024 9:44 PM, Sean Christopherson wrote:
> On Tue, Nov 19, 2024, Dapeng Mi wrote:
>> On 11/19/2024 9:46 AM, Sean Christopherson wrote:
>>> On Fri, Oct 25, 2024, Dapeng Mi wrote:
>>>> On 10/25/2024 4:26 AM, Chen, Zide wrote:
>>>>> On 7/31/2024 9:58 PM, Mingwei Zhang wrote:
>>>>>
>>>>>> @@ -7295,6 +7299,46 @@ static void atomic_switch_perf_msrs(struct vcpu_vmx *vmx)
>>>>>> msrs[i].host, false);
>>>>>> }
>>>>>>
>>>>>> +static void save_perf_global_ctrl_in_passthrough_pmu(struct vcpu_vmx *vmx)
>>>>>> +{
>>>>>> + struct kvm_pmu *pmu = vcpu_to_pmu(&vmx->vcpu);
>>>>>> + int i;
>>>>>> +
>>>>>> + if (vm_exit_controls_get(vmx) & VM_EXIT_SAVE_IA32_PERF_GLOBAL_CTRL) {
>>>>>> + pmu->global_ctrl = vmcs_read64(GUEST_IA32_PERF_GLOBAL_CTRL);
>>>>> As commented in patch 26, compared with MSR auto save/store area
>>>>> approach, the exec control way needs one relatively expensive VMCS read
>>>>> on every VM exit.
>>>> Anyway, let us have a evaluation and data speaks.
>>> No, drop the unconditional VMREAD and VMWRITE, one way or another. No benchmark
>>> will notice ~50 extra cycles, but if we write poor code for every feature, those
>>> 50 cycles per feature add up.
>>>
>>> Furthermore, checking to see if the CPU supports the load/save VMCS controls at
>>> runtime beyond ridiculous. The mediated PMU requires ***VERSION 4***; if a CPU
>>> supports PMU version 4 and doesn't support the VMCS controls, KVM should yell and
>>> disable the passthrough PMU. The amount of complexity added here to support a
>>> CPU that should never exist is silly.
>>>
>>>>>> +static void load_perf_global_ctrl_in_passthrough_pmu(struct vcpu_vmx *vmx)
>>>>>> +{
>>>>>> + struct kvm_pmu *pmu = vcpu_to_pmu(&vmx->vcpu);
>>>>>> + u64 global_ctrl = pmu->global_ctrl;
>>>>>> + int i;
>>>>>> +
>>>>>> + if (vm_entry_controls_get(vmx) & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL) {
>>>>>> + vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL, global_ctrl);
>>>>> ditto.
>>>>>
>>>>> We may optimize it by introducing a new flag pmu->global_ctrl_dirty and
>>>>> update GUEST_IA32_PERF_GLOBAL_CTRL only when it's needed. But this
>>>>> makes the code even more complicated.
>>> I haven't looked at surrounding code too much, but I guarantee there's _zero_
>>> reason to eat a VMWRITE+VMREAD on every transition. If, emphasis on *if*, KVM
>>> accesses PERF_GLOBAL_CTRL frequently, e.g. on most exits, then add a VCPU_EXREG_XXX
>>> and let KVM's caching infrastructure do the heavy lifting. Don't reinvent the
>>> wheel. But first, convince the world that KVM actually accesses the MSR somewhat
>>> frequently.
>> Sean, let me give more background here.
>>
>> VMX supports two ways to save/restore PERF_GLOBAL_CTRL MSR, one is to
>> leverage VMCS_EXIT_CTRL/VMCS_ENTRY_CTRL to save/restore guest
>> PERF_GLOBAL_CTRL value to/from VMCS guest state. The other is to use the
>> VMCS MSR auto-load/restore bitmap to save/restore guest PERF_GLOBAL_CTRL.
> I know.
>
>> Currently we prefer to use the former way to save/restore guest
>> PERF_GLOBAL_CTRL as long as HW supports it. There is a limitation on the
>> MSR auto-load/restore feature. When there are multiple MSRs, the MSRs are
>> saved/restored in the order of MSR index. As the suggestion of SDM,
>> PERF_GLOBAL_CTRL should always be written at last after all other PMU MSRs
>> are manipulated. So if there are some PMU MSRs whose index is larger than
>> PERF_GLOBAL_CTRL (It would be true in archPerfmon v6+, all PMU MSRs in the
>> new MSR range have larger index than PERF_GLOBAL_CTRL),
> No, the entries in the load/store lists are processed in sequential order as they
> appear in the lists. Ordering them based on their MSR index would be insane and
> would make the lists useless.
>
> VM entries may load MSRs from the VM-entry MSR-load area (see Section 25.8.2).
> Specifically each entry in that area (up to the number specified in the VM-entry
> MSR-load count) is processed in order by loading the MSR indexed by bits 31:0
> with the contents of bits 127:64 as they would be written by WRMSR.1
Thanks, just look at the SDM again. It seems I misunderstood the words. :(
>
>> these PMU MSRs would be restored after PERF_GLOBAL_CTRL. That would break the
>> rule. Of course, it's good to save/restore PERF_GLOBAL_CTRL right now with
>> the VMCS VMCS MSR auto-load/restore bitmap feature since only one PMU MSR
>> PERF_GLOBAL_CTRL is saved/restored in current implementation.
> No, it's never good to use the load/store lists. They're slow as mud, because
> they're essentially just wrappers to the standard WRMSR/RDMSR ucode. Whereas
> dedicated VMCS fields have dedicated, streamlined ucode to make loads and stores
> as fast as possible.
>
> I haven't measured PERF_GLOBAL_CTRL specifically, at least not in recent memory,
> but generally speaking using a load/store entry is 100+ cycles, whereas using a
> dedicated VMCS field is <20 cyles (often far less).
>
> So what I am saying is that the mediated PMU should _require_ support for loading
> and saving PERF_GLOBAL_CTRL via dedicated fields, and WARN if a CPU with a v4+
> PMU doesn't support said fields. E.g.
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index a4b2b0b69a68..cab8305e7bf0 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -8620,6 +8620,15 @@ __init int vmx_hardware_setup(void)
> enable_sgx = false;
> #endif
>
> + /*
> + * All CPUs that support a mediated PMU are expected to support loading
> + * and saving PERF_GLOBAL_CTRL via dedicated VMCS fields.
> + */
> + if (enable_passthrough_pmu &&
> + (WARN_ON_ONCE(!cpu_has_load_perf_global_ctrl() ||
> + !cpu_has_save_perf_global_ctrl())))
> + enable_passthrough_pmu = false;
> +
> /*
> * set_apic_access_page_addr() is used to reload apic access
> * page upon invalidation. No need to do anything if not
>
> That will provide better, more consistent performance, and will eliminate a big
> pile of non-trivial code.
Sure. When the mediated vPMU is proposed at the beginning, we considered to
support all kinds of HWs including some old may not broadly used HWs
nowadays, so we have to consider all kinds of corner cases. But think
twice, it may not be worthy...
>
>> PERF_GLOBAL_CTRL MSR could be frequently accessed by perf/pmu driver, e.g.
>> on each task switch, so PERF_GLOBAL_CTRL MSR is configured to passthrough
>> to reduce the performance impact in mediated vPMU proposal if guest own all
>> PMU HW resource. But if guest only owns part of PMU HW resource,
>> PERF_GLOBAL_CTRL would be set to interception mode.
> Again, I know. What I am saying is that propagating PERF_GLOBAL_CTRL to/from the
> VMCS on every entry and exit is extremely wasteful and completely unnecessary.
Yes, I understood your meaning. For passthough mode, KVM should not need
to access the guest PERF_GLOBAL_CTRL value. For interception mode, KVM
would maintain an emulated guest PERF_GLOBAL_CTRL value and don't need to
read/write it from/to VMCS guest fields.
Anyway, it looks these two helpers are useless, it can be removed.
>
>> I suppose KVM doesn't need access PERF_GLOBAL_CTRL in passthrough mode.
>> This piece of code is intently just for PERF_GLOBAL_CTRL interception mode,
> No, it's even more useless if PERF_GLOBAL_CTRL is intercepted, because in that
> case the _only_ time KVM needs move the guest's value to/from the VMCS is when
> the guest (or host userspace) is explicitly accessing the field.
>
>> but think twice it looks unnecessary to save/restore PERF_GLOBAL_CTRL via
>> VMCS as KVM would always maintain the guest PERF_GLOBAL_CTRL value? Anyway,
>> this part of code can be optimized.
