On 5/15/2025 8:12 AM, Sean Christopherson wrote:
> On Mon, Mar 24, 2025, Mingwei Zhang wrote:
>> From: Dapeng Mi <dapeng1...@linux.intel.com>
>>
>> Check user space's PMU cpuid configuration and filter the invalid
>> configuration.
>>
>> Either legacy perf-based vPMU or mediated vPMU needs kernel to support
>> local APIC, otherwise PMI has no way to be injected into guest. If
>> kernel doesn't support local APIC, reject user space to enable PMU
>> cpuid.
>>
>> User space configured PMU version must be no larger than KVM supported
>> maximum pmu version for mediated vPMU, otherwise guest may manipulate
>> some unsupported or unallowed PMU MSRs, this is dangerous and harmful.
>>
>> If the pmu version is larger than 1 but smaller than 5, CPUID.AH.ECX
>> must be 0 as well which is required by SDM.
>>
>> Suggested-by: Zide Chen <zide.c...@intel.com>
>> Signed-off-by: Dapeng Mi <dapeng1...@linux.intel.com>
>> Signed-off-by: Mingwei Zhang <mizh...@google.com>
>> ---
>> arch/x86/kvm/cpuid.c | 15 +++++++++++++++
>> arch/x86/kvm/pmu.c | 7 +++++--
>> arch/x86/kvm/pmu.h | 1 +
>> 3 files changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 8eb3a88707f2..f849ced9deba 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -179,6 +179,21 @@ static int kvm_check_cpuid(struct kvm_vcpu *vcpu)
>> return -EINVAL;
>> }
>>
>> + best = kvm_find_cpuid_entry(vcpu, 0xa);
>> + if (vcpu->kvm->arch.enable_pmu && best) {
>> + union cpuid10_eax eax;
>> +
>> + eax.full = best->eax;
>> + if (enable_mediated_pmu &&
>> + eax.split.version_id > kvm_pmu_cap.version)
>> + return -EINVAL;
>> + if (eax.split.version_id > 0 && !vcpu_pmu_can_enable(vcpu))
>> + return -EINVAL;
>> + if (eax.split.version_id > 1 && eax.split.version_id < 5 &&
>> + best->ecx != 0)
>> + return -EINVAL;
> NAK, unless there is a really, *really* strong need for this. I do not want
> to
> get in the business of vetting the vCPU model presented to the guest. If KVM
> needs to constrain things for its own safety, then by all means, but AFAICT
> these
> are nothing more than sanity checks on userspace.
Ok.
>
>> + }
>> +
>> /*
>> * Exposing dynamic xfeatures to the guest requires additional
>> * enabling in the FPU, e.g. to expand the guest XSAVE state size.
>> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
>> index 4f455afe4009..92c742ead663 100644
>> --- a/arch/x86/kvm/pmu.c
>> +++ b/arch/x86/kvm/pmu.c
>> @@ -743,6 +743,10 @@ static void kvm_pmu_reset(struct kvm_vcpu *vcpu)
>> kvm_pmu_call(reset)(vcpu);
>> }
>>
>> +inline bool vcpu_pmu_can_enable(struct kvm_vcpu *vcpu)
>> +{
>> + return vcpu->kvm->arch.enable_pmu && lapic_in_kernel(vcpu);
> Again, the APIC check belongs in the VM enablement path, not here. Hmm, that
> may require more thought with respect to enabling the PMU by default.
Sure.
