Re: [PATCH 6/8] KVM: SVM: Add support for hardware-switched PMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Emulated guest reads and writes to PMU MSRs return stale data or are lost because the vendor state sync is skipped for guest-initiated accesses.
- [High] PMC virtualization is never enabled for the BSP (Boot Strap Processor), leaving its PMU completely broken.
- [High] Nested virtualization (L2) PMU is broken because SVM_MISC2_ENABLE_V_PMC is never propagated to vmcb02.
- [High] Calling `wrmsrq()` inside the `noinstr` function `svm_vcpu_enter_exit()` invokes instrumentable tracepoint code while RCU is not watching.
- [High] The `noinstr` function `svm_vcpu_enter_exit()` calls non-`__always_inline` PMU capability helpers, risking instrumentation in the VMRUN path.
--

--- Patch [6]: [PATCH 6/8] KVM: SVM: Add support for hardware-switched PMU ---
commit d7b31e3949e70dae0c322517e2056c7007227bb4
Author: Sandipan Das <sandipan.das@amd.com>
Subject: KVM: SVM: Add support for hardware-switched PMU

This patch enables PMC virtualization on Zen 5 processors to offload
guest PMU context switches to hardware. It introduces vendor state sync
operations and a module parameter to toggle the feature, integrating it
with the VMCB save area.

> diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
> index 5dccf87763686..7d3a151b6cdee 100644
> --- a/arch/x86/kvm/svm/pmu.c
> +++ b/arch/x86/kvm/svm/pmu.c

[ ... ]

> @@ -134,6 +217,10 @@ static int amd_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	/* MSR_PERFCTRn */
>  	pmc = get_gp_pmc_amd(pmu, msr, PMU_TYPE_COUNTER);
>  	if (pmc) {
> +		if (msr_info->host_initiated &&
> +		    kvm_vcpu_has_mediated_pmu_caps(vcpu, KVM_MEDIATED_PMU_CAP_HW_SWITCHED) &&
> +		    amd_get_vendor_state(vcpu, msr))
> +			kvm_pmu_warn_vendor_state(msr);
>  		msr_info->data = pmc_read_counter(pmc);
>  		return 0;
>  	}

[Severity: High]
Does gating the vendor state sync behind msr_info->host_initiated break
guest-initiated emulated reads?

When KVM emulates an instruction (e.g., forced emulation or MMIO) that
accesses a PMU counter, host_initiated is false. For reads, this skips the
vendor state sync and returns the stale value in pmc->counter.

> @@ -158,6 +245,10 @@ static int amd_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	pmc = get_gp_pmc_amd(pmu, msr, PMU_TYPE_COUNTER);
>  	if (pmc) {
>  		pmc_write_counter(pmc, data);
> +		if (msr_info->host_initiated &&
> +		    kvm_vcpu_has_mediated_pmu_caps(vcpu, KVM_MEDIATED_PMU_CAP_HW_SWITCHED) &&
> +		    amd_set_vendor_state(vcpu, msr))
> +			kvm_pmu_warn_vendor_state(msr);
>  		return 0;
>  	}

[Severity: High]
Does gating the vendor state sync behind msr_info->host_initiated break
guest-initiated emulated writes?

For emulated writes, KVM will update pmc->counter but skip updating the VMCB
save area because host_initiated is false. Since pmc_write_counter() does not
trigger a PMU refresh for mediated PMUs, the VMCB save area is never updated.
Hardware natively reloads the stale PMU state from the VMCB on the next VMRUN,
silently discarding the guest's emulated write.

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index ac3d898d21bda..2fa5d85d3c40e 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c

[ ... ]

> @@ -1271,6 +1274,9 @@ static void init_vmcb(struct kvm_vcpu *vcpu, bool init_event)
>  
>  	svm_hv_init_vmcb(vmcb);
>  
> +	if (kvm_vcpu_has_mediated_pmu_caps(vcpu, KVM_MEDIATED_PMU_CAP_HW_SWITCHED))
> +		control->misc_ctl2 |= SVM_MISC2_ENABLE_V_PMC;
> +
>  	kvm_make_request(KVM_REQ_RECALC_INTERCEPTS, vcpu);

[Severity: High]
Will PMC virtualization remain disabled for the Boot Strap Processor (BSP)?

During early VCPU creation (svm_vcpu_reset), init_vmcb() is called before
userspace sets the CPUID via KVM_SET_CPUID2. At this point, the PMU version
is 0, so kvm_vcpu_has_mediated_pmu_caps() returns false and
SVM_MISC2_ENABLE_V_PMC is not set.

Unlike APs (which receive an INIT signal that triggers a subsequent
init_vmcb() after CPUID is set), the BSP does not undergo another reset.
During runtime, KVM skips manual PMU context switching because the
capability evaluates to true, but hardware also skips it due to the missing
V_PMC bit in the BSP's VMCB, effectively breaking the PMU for the BSP.

[Severity: High]
Is nested virtualization (L2) PMU broken because SVM_MISC2_ENABLE_V_PMC is
never propagated to vmcb02?

When KVM prepares vmcb02 to run an L2 guest in
nested_vmcb02_prepare_control(), it initializes misc_ctl2 but completely omits
setting SVM_MISC2_ENABLE_V_PMC. KVM will skip manual loading of the PMU state
because kvm_vcpu_has_mediated_pmu_caps() returns true when L2 is running, but
hardware will also skip it because vmcb02 lacks the V_PMC enable bit.

[ ... ]

> @@ -4456,6 +4486,14 @@ static noinstr void svm_vcpu_enter_exit(struct kvm_vcpu *vcpu, unsigned enter_fl
>  
>  	amd_clear_divider();
>  
> +	/*
> +	 * On #VMEXIT, PerfCntrGlobalCtl goes back to its reset state since
> +	 * its save slot is of Swap Type C. All enable bits are set but PMC
> +	 * virtualization requires them to be cleared before VMRUN.
> +	 */
> +	if (kvm_vcpu_has_mediated_pmu_caps(vcpu, KVM_MEDIATED_PMU_CAP_HW_SWITCHED))
> +		wrmsrq(MSR_AMD64_PERF_CNTR_GLOBAL_CTL, 0);
> +
>  	if (is_sev_es_guest(vcpu))
>  		__svm_sev_es_vcpu_run(svm, enter_flags,

[Severity: High]
Does calling kvm_vcpu_has_mediated_pmu_caps() from a noinstr function risk
instrumentation in the VMRUN path?

Functions executed in a noinstr context must be explicitly marked noinstr or
__always_inline. kvm_vcpu_has_mediated_pmu_caps() and its dependency
kvm_vcpu_has_mediated_pmu() are only declared as static inline. If the
compiler compiles them out-of-line, the noinstr protections are stripped,
which can lead to objtool warnings and potential kernel panics if tracing
or instrumentation triggers during this VMRUN entry path.

[Severity: High]
Can calling wrmsrq() inside the noinstr function svm_vcpu_enter_exit() invoke
instrumentable tracepoint code while RCU is not watching?

The wrmsrq() macro wraps native_write_msr(), which conditionally calls
do_trace_write_msr() if the write_msr tracepoint is enabled. Tracepoints
utilize RCU to safely iterate their probe lists. Executing this inside a
noinstr section where RCU is not watching causes lockdep/RCU splats
(RCU used illegally from extended quiescent state) and potential system
panics when tracing is active.

Should this use a non-instrumented wrapper such as native_wrmsrl() or
__wrmsr()?

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