On 11/21/2024 4:19 AM, Sean Christopherson wrote:
> 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.
Sure.
>
>> + 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.
Sure. would drop it and directly return pmc->counter.
>
>> + 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
>>
