Re: [PATCH 3/8] KVM: x86/pmu: Add support for hardware-switched PMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Hardware-switched PMU state (PMCs, event selectors) is not synchronized with the vendor save area during MSR access, VM reset, and event emulation, causing data loss across live migration and incorrect event filtering.
--

--- Patch [3]: [PATCH 3/8] KVM: x86/pmu: Add support for hardware-switched PMU ---
commit 388311da6f37e672745e8b6b16a96e9812fb8ddd
Author: Sandipan Das <sandipan.das@amd.com>

KVM: x86/pmu: Add support for hardware-switched PMU

This patch introduces the hardware-switched mode for the mediated PMU.
It transitions the guest PMU state to reside in a vendor save area
instead of the software kvm_pmu struct. The load and put functionality
is simplified to schedule host events in and out.

> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -844,10 +890,12 @@ int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	switch (msr) {
>  	case MSR_CORE_PERF_GLOBAL_STATUS:
>  	case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS:
> +		kvm_pmu_get_vendor_state(vcpu, kvm_pmu_ops.PERF_GLOBAL_STATUS);
>  		msr_info->data = pmu->global_status;
>  		break;
>  	case MSR_AMD64_PERF_CNTR_GLOBAL_CTL:
>  	case MSR_CORE_PERF_GLOBAL_CTRL:
> +		kvm_pmu_get_vendor_state(vcpu, kvm_pmu_ops.PERF_GLOBAL_CTRL);
>  		msr_info->data = pmu->global_ctrl;
>  		break;

[Severity: High]
Does this miss synchronizing the PMCs and Event Selectors in the default
switch cases for kvm_pmu_get_msr() and kvm_pmu_set_msr()?

When userspace reads or writes the guest PMU state via KVM_GET_MSRS and
KVM_SET_MSRS, the default block falls through to vendor code:

	default:
		return kvm_pmu_call(get_msr)(vcpu, msr_info);

Without calling kvm_pmu_get_vendor_state() or kvm_pmu_set_vendor_state() for
these MSRs, KVM might return stale software values or fail to commit restored
values to the hardware save area. Since kvm_mediated_pmu_load() skips loading
software state into hardware for the hardware-switched mode, could this cause
data loss across live migration?

[ ... ]

> @@ -1140,11 +1202,17 @@ static void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu,
>  
>  	idx = srcu_read_lock(&vcpu->kvm->srcu);
>  	kvm_for_each_pmc(pmu, pmc, i, bitmap) {
> +		kvm_pmu_get_vendor_state(vcpu, pmc_counter_msr(pmc));
> +
>  		if (!pmc_is_event_allowed(pmc) || !cpl_is_matched(pmc))
>  			continue;
>  
>  		kvm_pmu_incr_counter(pmc);
> +		kvm_pmu_set_vendor_state(vcpu, pmc_counter_msr(pmc));
>  	}

[Severity: High]
Are we evaluating the event filter with stale event selector state here?

While we fetch the latest counter values via kvm_pmu_get_vendor_state(),
pmc_is_event_allowed() and cpl_is_matched() still rely on pmc->eventsel
and pmu->fixed_ctr_ctrl. Without syncing the event selector or fixed counter
control from the vendor save area, might these checks use outdated software
state?

Additionally, how does KVM handle VM resets and instruction emulation tracking
with hardware-switched PMUs?

For resets, kvm_pmu_reset() zeroes out the PMCs and event selectors in the
software structures:

	pmc_stop_counter(pmc);
	pmc->counter = 0;
	pmc->emulated_counter = 0;

	if (pmc_is_gp(pmc)) {
		pmc->eventsel = 0;
		pmc->eventsel_hw = 0;

It doesn't seem to push this reset state down to the vendor save area.

For emulation, MSR writes bypass KVM interception in the hardware-switched
mode. Since KVM_REQ_PMU is never triggered, it appears that
kvm_pmu_recalc_pmc_emulation() won't run, leaving pmc_counting_instructions
and pmc_counting_branches empty. Could this cause KVM to silently fail to
increment counters during instruction and branch emulation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786345201.git.sandipan.das@amd.com?part=3