On 11/21/2024 1:09 AM, Sean Christopherson wrote: > On Wed, Nov 20, 2024, Dapeng Mi wrote: >> On 11/19/2024 11:37 PM, Sean Christopherson wrote: >>>> --- >>>> arch/x86/kvm/pmu.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c >>>> index 5768ea2935e9..e656f72fdace 100644 >>>> --- a/arch/x86/kvm/pmu.c >>>> +++ b/arch/x86/kvm/pmu.c >>>> @@ -787,7 +787,7 @@ void kvm_pmu_refresh(struct kvm_vcpu *vcpu) >>>> * in the global controls). Emulate that behavior when refreshing the >>>> * PMU so that userspace doesn't need to manually set PERF_GLOBAL_CTRL. >>>> */ >>>> - if (kvm_pmu_has_perf_global_ctrl(pmu) && pmu->nr_arch_gp_counters) >>>> + if ((pmu->passthrough || kvm_pmu_has_perf_global_ctrl(pmu)) && pmu->nr_arch_gp_counters) >>>> pmu->global_ctrl = GENMASK_ULL(pmu->nr_arch_gp_counters - 1, 0); >>> This is wrong and confusing. From the guest's perspective, and therefore from >>> host userspace's perspective, PERF_GLOBAL_CTRL does not exist. Therefore, the >>> value that is tracked for the guest must be '0'. >>> >>> I see that intel_passthrough_pmu_msrs() and amd_passthrough_pmu_msrs() intercept >>> accesses to PERF_GLOBAL_CTRL if "pmu->version > 1" (which, by the by, needs to be >>> kvm_pmu_has_perf_global_ctrl()), so there's no weirdness with the guest being able >>> to access MSRs that shouldn't exist. >>> >>> But KVM shouldn't stuff pmu->global_ctrl, and doing so is a symptom of another >>> flaw. Unless I'm missing something, KVM stuffs pmu->global_ctrl so that the >>> correct value is loaded on VM-Enter, but loading and saving PERF_GLOBAL_CTRL on >>> entry/exit is unnecessary and confusing, as is loading the associated MSRs when >>> restoring (loading) the guest context. >>> >>> For PERF_GLOBAL_CTRL on Intel, KVM needs to ensure all GP counters are enabled in >>> VMCS.GUEST_IA32_PERF_GLOBAL_CTRL, but that's a "set once and forget" operation, >>> not something that needs to be done on every entry and exit. Of course, loading >>> and saving PERF_GLOBAL_CTRL on every entry/exit is unnecessary for other reasons, >>> but that's largely orthogonal. >>> >>> On AMD, amd_restore_pmu_context()[*] needs to enable a maximal value for >>> PERF_GLOBAL_CTRL, but I don't think there's any need to load the other MSRs, >>> and the maximal value should come from the above logic, not pmu->global_ctrl. >> Sean, just double confirm, you are suggesting to do one-shot initialization >> for guest PERF_GLOBAL_CTRL (VMCS.GUEST_IA32_PERF_GLOBAL_CTRL for Intel) >> after vCPU resets, right? > No, it would need to be written during refresh(). VMCS.GUEST_IA32_PERF_GLOBAL_CTRL > is only static (because it's unreachable) if the guest does NOT have version > 1. oh, yeah, refresh() instead of reset() to be exact. >
