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.
> + 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).
> +
> if (!diff)
> return;
>
> --
> 2.46.0.rc1.232.g9752f9e123-goog
>
