On Thu, Aug 01, 2024, Mingwei Zhang wrote:
> Update pmc_{read,write}_counter() to disconnect perf API because
> passthrough PMU does not use host PMU on backend. Because of that
> pmc->counter contains directly the actual value of the guest VM when set by
> the host (VMM) side.
>
> Signed-off-by: Mingwei Zhang <mizhang@xxxxxxxxxx>
> Signed-off-by: Dapeng Mi <dapeng1.mi@xxxxxxxxxxxxxxx>
> ---
> arch/x86/kvm/pmu.c | 5 +++++
> arch/x86/kvm/pmu.h | 4 ++++
> 2 files changed, 9 insertions(+)
>
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index 41057d0122bd..3604cf467b34 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -322,6 +322,11 @@ static void pmc_update_sample_period(struct kvm_pmc *pmc)
>
> void pmc_write_counter(struct kvm_pmc *pmc, u64 val)
> {
> + if (pmc_to_pmu(pmc)->passthrough) {
> + pmc->counter = val;
This needs to mask the value with pmc_bitmask(pmc), otherwise emulated events
will operate on a bad value, and loading the PMU state into hardware will #GP
if the PMC is written through the sign-extended MSRs, i.e. if val = -1 and the
CPU supports full-width writes.
> + return;
> + }
> +
> /*
> * Drop any unconsumed accumulated counts, the WRMSR is a write, not a
> * read-modify-write. Adjust the counter value so that its value is
> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> index 78a7f0c5f3ba..7e006cb61296 100644
> --- a/arch/x86/kvm/pmu.h
> +++ b/arch/x86/kvm/pmu.h
> @@ -116,6 +116,10 @@ static inline u64 pmc_read_counter(struct kvm_pmc *pmc)
> {
> u64 counter, enabled, running;
>
> + counter = pmc->counter;
Using a local variable is pointless, the perf-based path immediately clobbers it.
> + if (pmc_to_pmu(pmc)->passthrough)
> + return counter & pmc_bitmask(pmc);
And then this can simply return pmc->counter. We _could_ add a WARN on pmc->counter
overlapping with pmc_bitmask(), but IMO that's unnecessary. If anything, WARN and
mask pmc->counter when loading state into hardware.
> +
> counter = pmc->counter + pmc->emulated_counter;
>
> if (pmc->perf_event && !pmc->is_paused)
> --
> 2.46.0.rc1.232.g9752f9e123-goog
>
