On 11/21/2024 4:40 AM, Sean Christopherson wrote:
> On Thu, Aug 01, 2024, Mingwei Zhang wrote:
>> Disconnect counter reprogram logic because passthrough PMU never use host
>> PMU nor does it use perf API to do anything. Instead, when passthrough PMU
>> is enabled, touching anywhere around counter reprogram part should be an
>> error.
>>
>> Signed-off-by: Mingwei Zhang <mizhang@xxxxxxxxxx>
>> Signed-off-by: Dapeng Mi <dapeng1.mi@xxxxxxxxxxxxxxx>
>> ---
>> arch/x86/kvm/pmu.c | 3 +++
>> arch/x86/kvm/pmu.h | 8 ++++++++
>> 2 files changed, 11 insertions(+)
>>
>> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
>> index 3604cf467b34..fcd188cc389a 100644
>> --- a/arch/x86/kvm/pmu.c
>> +++ b/arch/x86/kvm/pmu.c
>> @@ -478,6 +478,9 @@ static int reprogram_counter(struct kvm_pmc *pmc)
>> bool emulate_overflow;
>> u8 fixed_ctr_ctrl;
>>
>> + if (WARN_ONCE(pmu->passthrough, "Passthrough PMU never reprogram counter\n"))
> Eh, a WARN_ON_ONCE() is enough.
>
> That said, this isn't entirely correct. The mediated PMU needs to "reprogram"
> event selectors if the event filter is changed. KVM currently handles this by
> setting all __reprogram_pmi bits and blasting KVM_REQ_PMU.
>
> LOL, and there's even a reprogram_fixed_counters_in_passthrough_pmu(), so the
> above message is a lie.
>
> There is also far too much duplicate code in things like reprogram_fixed_counters()
> versus reprogram_fixed_counters_in_passthrough_pmu().
>
> Reprogramming on each write is also technically suboptimal, as _very_ theoretically
> KVM could emulate multiple WRMSRs without re-entering the guest.
>
> So, I think the mediated PMU should use the reprogramming infrastructure, and
> handle the bulk of the different behavior in reprogram_counter(), not in a half
> dozen different paths.
Sure. would handle the reprogram counter behavior of meidated vPMU
reprogram_counter(), and you comments reminds me current mediated vPMU code
missed to handle the case of event filter changing. Would add them in next
version.
>
>> + return 0;
>> +
>> emulate_overflow = pmc_pause_counter(pmc);
>>
>> if (!pmc_event_is_allowed(pmc))
>> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
>> index 7e006cb61296..10553bc1ae1d 100644
>> --- a/arch/x86/kvm/pmu.h
>> +++ b/arch/x86/kvm/pmu.h
>> @@ -256,6 +256,10 @@ static inline void kvm_init_pmu_capability(const struct kvm_pmu_ops *pmu_ops)
>>
>> static inline void kvm_pmu_request_counter_reprogram(struct kvm_pmc *pmc)
>> {
>> + /* Passthrough PMU never reprogram counters via KVM_REQ_PMU. */
>> + if (pmc_to_pmu(pmc)->passthrough)
>> + return;
>> +
>> set_bit(pmc->idx, pmc_to_pmu(pmc)->reprogram_pmi);
>> kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
>> }
>> @@ -264,6 +268,10 @@ static inline void reprogram_counters(struct kvm_pmu *pmu, u64 diff)
>> {
>> int bit;
>>
>> + /* Passthrough PMU never reprogram counters via KVM_REQ_PMU. */
>> + if (pmu->passthrough)
>> + return;
> Make up your mind :-) Either handle the mediated PMU here, or handle it in the
> callers. Don't do both.
>
> I vote to handle it here, i.e. drop the check in kvm_pmu_set_msr() when handling
> MSR_CORE_PERF_GLOBAL_CTRL, and then add a comment that reprogramming GP counters
> doesn't need to happen on control updates because KVM enforces the event filters
> when emulating writes to event selectors (and because the guest can write
> PERF_GLOBAL_CTRL directly).
:) Yeah, we found this and removed them internally. Sure. would follow your
suggestion.
>
>> +
>> if (!diff)
>> return;
>>
>> --
>> 2.46.0.rc1.232.g9752f9e123-goog
>>
