On 11/21/2024 5:02 AM, Sean Christopherson wrote:
> +Aaron
>
> On Thu, Aug 01, 2024, Mingwei Zhang wrote:
>> +static void amd_passthrough_pmu_msrs(struct kvm_vcpu *vcpu)
>> +{
>> + struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
>> + struct vcpu_svm *svm = to_svm(vcpu);
>> + int msr_clear = !!(is_passthrough_pmu_enabled(vcpu));
>> + int i;
>> +
>> + for (i = 0; i < min(pmu->nr_arch_gp_counters, AMD64_NUM_COUNTERS); i++) {
>> + /*
>> + * Legacy counters are always available irrespective of any
>> + * CPUID feature bits and when X86_FEATURE_PERFCTR_CORE is set,
>> + * PERF_LEGACY_CTLx and PERF_LEGACY_CTRx registers are mirrored
>> + * with PERF_CTLx and PERF_CTRx respectively.
>> + */
>> + set_msr_interception(vcpu, svm->msrpm, MSR_K7_EVNTSEL0 + i, 0, 0);
>> + set_msr_interception(vcpu, svm->msrpm, MSR_K7_PERFCTR0 + i, msr_clear, msr_clear);
>> + }
>> +
>> + for (i = 0; i < kvm_pmu_cap.num_counters_gp; i++) {
>> + /*
>> + * PERF_CTLx registers require interception in order to clear
>> + * HostOnly bit and set GuestOnly bit. This is to prevent the
>> + * PERF_CTRx registers from counting before VM entry and after
>> + * VM exit.
>> + */
>> + set_msr_interception(vcpu, svm->msrpm, MSR_F15H_PERF_CTL + 2 * i, 0, 0);
>> +
>> + /*
>> + * Pass through counters exposed to the guest and intercept
>> + * counters that are unexposed. Do this explicitly since this
>> + * function may be set multiple times before vcpu runs.
>> + */
>> + if (i >= pmu->nr_arch_gp_counters)
>> + msr_clear = 0;
> Similar to my comments on the Intel side, explicitly enable interception for
> MSRs that don't exist in the guest model in a separate for-loop, i.e. don't
> toggle msr_clear in the middle of a loop.
Sure.
>
> I would also love to de-dup the bulk of this code, which is very doable since
> the base+shift for the MSRs is going to be stashed in kvm_pmu. All that's needed
> on top is unified MSR interception logic, which is something that's been on my
> wish list for some time. SVM's inverted polarity needs to die a horrible death.
>
> Lucky for me, Aaron is picking up that torch.
>
> Aaron, what's your ETA on the MSR unification? No rush, but if you think it'll
> be ready in the next month or so, I'll plan on merging that first and landing
> this code on top.
Is there a public link for Aaron's patches? If so, we can rebase the next
version patches on top of Aaron's patches.
>
>> + set_msr_interception(vcpu, svm->msrpm, MSR_F15H_PERF_CTR + 2 * i, msr_clear, msr_clear);
>> + }
>> +
>> + /*
>> + * In mediated passthrough vPMU, intercept global PMU MSRs when guest
>> + * PMU only owns a subset of counters provided in HW or its version is
>> + * less than 2.
>> + */
>> + if (is_passthrough_pmu_enabled(vcpu) && pmu->version > 1 &&
> kvm_pmu_has_perf_global_ctrl(), no?
Yes.
>
>> + pmu->nr_arch_gp_counters == kvm_pmu_cap.num_counters_gp)
>> + msr_clear = 1;
>> + else
>> + msr_clear = 0;
>> +
>> + set_msr_interception(vcpu, svm->msrpm, MSR_AMD64_PERF_CNTR_GLOBAL_CTL, msr_clear, msr_clear);
>> + set_msr_interception(vcpu, svm->msrpm, MSR_AMD64_PERF_CNTR_GLOBAL_STATUS, msr_clear, msr_clear);
>> + set_msr_interception(vcpu, svm->msrpm, MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR, msr_clear, msr_clear);
>> + set_msr_interception(vcpu, svm->msrpm, MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_SET, msr_clear, msr_clear);
>> +}
>> +
>> struct kvm_pmu_ops amd_pmu_ops __initdata = {
>> .rdpmc_ecx_to_pmc = amd_rdpmc_ecx_to_pmc,
>> .msr_idx_to_pmc = amd_msr_idx_to_pmc,
>> @@ -258,6 +312,7 @@ struct kvm_pmu_ops amd_pmu_ops __initdata = {
>> .refresh = amd_pmu_refresh,
>> .init = amd_pmu_init,
>> .is_rdpmc_passthru_allowed = amd_is_rdpmc_passthru_allowed,
>> + .passthrough_pmu_msrs = amd_passthrough_pmu_msrs,
>> .EVENTSEL_EVENT = AMD64_EVENTSEL_EVENT,
>> .MAX_NR_GP_COUNTERS = KVM_AMD_PMC_MAX_GENERIC,
>> .MIN_NR_GP_COUNTERS = AMD64_NUM_COUNTERS,
>> --
>> 2.46.0.rc1.232.g9752f9e123-goog
>>
