Re: [PATCH v2 11/12] KVM: SVM: Add LBR/PMC freeze support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] KVM: SVM: AMD_LBR_PMC_FREEZE advertised without gating in svm_set_cpu_caps()
- [High] KVM: SVM: amd_pmu_refresh() leaves freeze bits reserved in global_status_rsvd
- [High] KVM: SVM: svm_vcpu_run() loses hardware PMC freeze when GLOBAL_CTL intercepted
- [High] KVM: SVM: svm_need_lbr_virtualization() omits DEBUGCTL freeze-on-PMI bits
--

commit e9a3e4432aa85c33133e5cfe1c272c751022082d
Author: Shivansh Dhiman <shivansh.dhiman@amd.com>

KVM: SVM: Add LBR/PMC freeze support

This commit advertises X86_FEATURE_AMD_LBR_PMC_FREEZE (CPUID
0x80000022.EAX[2]) to guests and allows guests with the feature to set
DEBUGCTLMSR_FREEZE_LBRS_ON_PMI and DEBUGCTLMSR_FREEZE_PERFMON_ON_PMI in
MSR_IA32_DEBUGCTLMSR.

> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index bb24d83621328..37da587ede716 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1259,6 +1259,7 @@ void kvm_initialize_cpu_caps(void)
>  	kvm_cpu_cap_init(CPUID_8000_0022_EAX,
>  		F(PERFMON_V2),
>  		SCATTERED_F(AMD_LBR_V2),
> +		SCATTERED_F(AMD_LBR_PMC_FREEZE),
>  	);

[Severity: High]
Should X86_FEATURE_AMD_LBR_PMC_FREEZE also be gated in svm_set_cpu_caps()
(and in svm_get_supported_debugctl()) on lbrv, enable_mediated_pmu, and
X86_FEATURE_PERFMON_V2?

In arch/x86/kvm/svm/svm.c, svm_set_cpu_caps() clears X86_FEATURE_AMD_LBR_V2
and X86_FEATURE_BUS_LOCK_DETECT when LBR virtualization or the mediated PMU
is unavailable:

arch/x86/kvm/svm/svm.c:svm_set_cpu_caps() {
	...
		if (!lbrv || !enable_mediated_pmu ||
		    !kvm_cpu_cap_has(X86_FEATURE_PERFMON_V2) ||
		    kvm_pmu_cap.num_branches_lbr != SVM_LBR_V2_STACK_SIZE)
			kvm_cpu_cap_clear(X86_FEATURE_AMD_LBR_V2);
	}
	...
	if (!lbrv)
		kvm_cpu_cap_clear(X86_FEATURE_BUS_LOCK_DETECT);
}

Without similar gating for X86_FEATURE_AMD_LBR_PMC_FREEZE:

1. When !lbrv, MSR_IA32_DEBUGCTLMSR is not virtualized in svm_set_msr(),
   which ignores writes via kvm_pr_unimpl_wrmsr(), leaving the advertised
   feature non-functional:

arch/x86/kvm/svm/svm.c:svm_set_msr() {
	case MSR_IA32_DEBUGCTLMSR:
		if (!lbrv) {
			kvm_pr_unimpl_wrmsr(vcpu, ecx, data);
			break;
		}
}

2. When lbrv is enabled but !enable_mediated_pmu (or when
   !kvm_vcpu_has_mediated_pmu(vcpu)), the perf_event-backed vPMU does not
   emulate freeze-on-PMI in __kvm_perf_overflow(). Because
   svm_get_supported_debugctl() still permits setting
   DEBUGCTLMSR_FREEZE_LBRS_ON_PMI and DEBUGCTLMSR_FREEZE_PERFMON_ON_PMI in
   svm->vmcb->save.dbgctl, those bits are loaded into the physical
   MSR_IA32_DEBUGCTLMSR while the hardware PMCs and LBRv2 remain owned by
   host perf, causing host PMIs during guest execution to freeze host PMCs
   and LBRs (and trigger the reserved PerfCntrGlobalStatus warning in
   amd_pmu_v2_handle_irq() if bit 59 is set).

[ ... ]

> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 01be2fc05d701..9b65728ee0d2f 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -801,6 +801,10 @@ static inline u64 svm_get_supported_debugctl(struct kvm_vcpu *vcpu)
>  	if (guest_cpu_cap_has(vcpu, X86_FEATURE_BUS_LOCK_DETECT))
>  		debugctl |= DEBUGCTLMSR_BUS_LOCK_DETECT;
>  
> +	if (guest_cpu_cap_has(vcpu, X86_FEATURE_AMD_LBR_PMC_FREEZE))
> +		debugctl |= DEBUGCTLMSR_FREEZE_LBRS_ON_PMI |
> +			    DEBUGCTLMSR_FREEZE_PERFMON_ON_PMI;
> +
>  	return debugctl;
>  }

[Severity: High]
When X86_FEATURE_AMD_LBR_PMC_FREEZE is exposed to the guest, does
amd_pmu_refresh() also need to clear the freeze status bits in
pmu->global_status_rsvd?

When DEBUGCTLMSR_FREEZE_LBRS_ON_PMI is enabled and a PMC overflows in the
guest, hardware sets GLOBAL_STATUS_LBRS_FROZEN (bit 58) in
MSR_AMD64_PERF_CNTR_GLOBAL_STATUS, which amd_mediated_pmu_put() saves into
pmu->global_status:

arch/x86/kvm/svm/pmu.c:amd_mediated_pmu_put() {
	...
	wrmsrq(MSR_AMD64_PERF_CNTR_GLOBAL_CTL, 0);
	rdmsrq(MSR_AMD64_PERF_CNTR_GLOBAL_STATUS, pmu->global_status);
}

However, amd_pmu_refresh() leaves all bits above nr_arch_gp_counters marked
as reserved in pmu->global_status_rsvd:

arch/x86/kvm/svm/pmu.c:amd_pmu_refresh() {
	...
	if (pmu->version > 1) {
		pmu->global_ctrl_rsvd = ~(BIT_ULL(pmu->nr_arch_gp_counters) - 1);
		pmu->global_status_rsvd = pmu->global_ctrl_rsvd;
	}
}

Won't kvm_pmu_set_msr() then reject host-initiated KVM_SET_MSRS writes to
MSR_AMD64_PERF_CNTR_GLOBAL_STATUS during live migration (and strip the freeze
bits on intercepted writes to MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_SET) whenever
GLOBAL_STATUS_LBRS_FROZEN (or GLOBAL_STATUS_COUNTERS_FROZEN) is set?

arch/x86/kvm/pmu.c:kvm_pmu_set_msr() {
	case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS:
		/* Per PPR, Read-only MSR. Writes are ignored. */
		if (!msr_info->host_initiated)
			break;

		if (data & pmu->global_status_rsvd)
			return 1;

		pmu->global_status = data;
		break;
}

[Severity: High]
Can the hardware-cleared MSR_AMD64_PERF_CNTR_GLOBAL_CTL state on PMC
freeze-on-PMI be lost across a VM-Exit when MSR_AMD64_PERF_CNTR_GLOBAL_CTL
is write-intercepted?

When DEBUGCTLMSR_FREEZE_PERFMON_ON_PMI is set and a counter overflows,
hardware clears the PerfCntrEn bits in MSR_AMD64_PERF_CNTR_GLOBAL_CTL
directly without a guest WRMSR.

On VM-Exit, svm_vcpu_run() only saves MSR_AMD64_PERF_CNTR_GLOBAL_CTL when
writes to the MSR are not intercepted, and amd_mediated_pmu_put() then zeroes
the hardware MSR:

arch/x86/kvm/svm/svm.c:svm_vcpu_run() {
	...
	if (kvm_vcpu_has_mediated_pmu(vcpu)) {
		if (!msr_write_intercepted(svm, MSR_AMD64_PERF_CNTR_GLOBAL_CTL))
			rdmsrq(MSR_AMD64_PERF_CNTR_GLOBAL_CTL,
			       vcpu_to_pmu(vcpu)->global_ctrl);
		...
	}
}

If MSR_AMD64_PERF_CNTR_GLOBAL_CTL is write-intercepted (for example when
kvm_need_perf_global_ctrl_intercept(vcpu) is true because guest
nr_arch_gp_counters differs from host num_counters_gp, or when L1 intercepts
the MSR for L2), won't KVM miss the hardware-cleared value and reload the
stale pre-overflow pmu->global_ctrl on the next amd_mediated_pmu_load(),
unfreezing the guest's counters?

[Severity: High]
Does svm_need_lbr_virtualization() also need to check for
DEBUGCTLMSR_FREEZE_LBRS_ON_PMI and DEBUGCTLMSR_FREEZE_PERFMON_ON_PMI?

Currently svm_need_lbr_virtualization() only checks for DEBUGCTLMSR_LBR,
DEBUGCTLMSR_BUS_LOCK_DETECT, and svm_lbrv2_active():

arch/x86/kvm/svm/svm.c:svm_need_lbr_virtualization() {
	struct vcpu_svm *svm = to_svm(vcpu);

	if (svm->vmcb->save.dbgctl & (DEBUGCTLMSR_LBR | DEBUGCTLMSR_BUS_LOCK_DETECT))
		return true;

	if (svm_lbrv2_active(vcpu))
		return true;

	return false;
}

If a guest sets DEBUGCTLMSR_FREEZE_PERFMON_ON_PMI without enabling LBRs or
Bus Lock Detect (or sets DEBUGCTLMSR_FREEZE_LBRS_ON_PMI before setting
DBG_EXTN_CFG_LBRV2EN), SVM_MISC2_ENABLE_V_LBR will remain clear in
svm->vmcb->control.misc_ctl2.

When SVM_MISC2_ENABLE_V_LBR is clear, svm_vcpu_run() only updates the
hardware MSR_IA32_DEBUGCTLMSR when vcpu->arch.host_debugctl differs from
svm->vmcb->save.dbgctl:

arch/x86/kvm/svm/svm.c:svm_vcpu_run() {
	if (!(svm->vmcb->control.misc_ctl2 & SVM_MISC2_ENABLE_V_LBR) &&
	    vcpu->arch.host_debugctl != svm->vmcb->save.dbgctl)
		update_debugctlmsr(svm->vmcb->save.dbgctl);
}

Because vcpu_enter_guest() captures vcpu->arch.host_debugctl =
get_debugctlmsr() before kvm_mediated_pmu_load() calls
perf_load_guest_context() -> amd_pmu_lbr_disable_all() (which clears
DEBUGCTLMSR_FREEZE_LBRS_ON_PMI in hardware MSR_IA32_DEBUGCTLMSR on the host),
if the guest's svm->vmcb->save.dbgctl matches the host's pre-disable
DEBUGCTLMSR_FREEZE_LBRS_ON_PMI value, won't svm_vcpu_run() skip
update_debugctlmsr() and leave the hardware MSR_IA32_DEBUGCTLMSR cleared?

титься

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019210532.378628-1-shivansh.dhiman@amd.com?part=11