Re: [PATCH v7 3/9] KVM: x86/pmu: Rename and move vcpu_get_perf_capabilities() to pmu.h

On 7/27/2026 2:42 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] Missing explicit header inclusions after refactoring.
> --
> 
> --- Patch [3]: [PATCH v7 3/9] KVM: x86/pmu: Rename and move vcpu_get_perf_capabilities() to pmu.h ---
> commit fb5ce8bc24fb7de4e71c57bef960ab9beca9bc0b
> Author: Zide Chen <[email protected]>
> 
> KVM: x86/pmu: Rename and move vcpu_get_perf_capabilities() to pmu.h
> 
> This commit renames vcpu_get_perf_capabilities() to kvm_vcpu_get_perf_caps()
> and moves it from pmu_intel.h to pmu.h to make it accessible to common x86
> code.
> 
>> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
>> index a5821d7c87f93..1b2f66a2e9153 100644
>> --- a/arch/x86/kvm/pmu.h
>> +++ b/arch/x86/kvm/pmu.h
>> @@ -271,6 +271,14 @@ static inline bool kvm_pmu_is_fastpath_emulation_allowed(struct kvm_vcpu *vcpu)
>>  				  X86_PMC_IDX_MAX);
>>  }
>>  
>> +static inline u64 kvm_vcpu_get_perf_caps(struct kvm_vcpu *vcpu)
>> +{
>> +	if (!guest_cpu_cap_has(vcpu, X86_FEATURE_PDCM))
>> +		return 0;
> 
> [Severity: Low]
> Does this file need an explicit include for cpuid.h?
> 
> Since kvm_vcpu_get_perf_caps() relies on guest_cpu_cap_has() defined in 
> cpuid.h, it seems like pmu.h should now explicitly include cpuid.h to remain
> self-contained, even if it happens to compile due to implicit includes in
> the compilation units.

It sounds reasonable, but I'd prefer to keep it as-is.

Adding cpuid.h to pmu.h would make pmu.h more self-contained, but it
would also naturally lead to removing now-redundant cpuid.h includes
from PMU-related files such as vmx/pmu_intel.c, svm/pmu.c, and
potentially others.

I'd like to keep the current patch focused on the functional change and
avoid mixing it with include dependency cleanup.

>> +
>> +	return vcpu->arch.perf_capabilities;
>> +}
>> +
> 
> [ ... ]
> 
>> diff --git a/arch/x86/kvm/vmx/pmu_intel.h b/arch/x86/kvm/vmx/pmu_intel.h
>> index 5d9357640aa18..afdbbc9991d6d 100644
>> --- a/arch/x86/kvm/vmx/pmu_intel.h
>> +++ b/arch/x86/kvm/vmx/pmu_intel.h
>> @@ -6,17 +6,9 @@
>>  
>>  #include "cpuid.h"
>>  
>> -static inline u64 vcpu_get_perf_capabilities(struct kvm_vcpu *vcpu)
>> -{
>> -	if (!guest_cpu_cap_has(vcpu, X86_FEATURE_PDCM))
>> -		return 0;
>> -
>> -	return vcpu->arch.perf_capabilities;
>> -}
>> -
>>  static inline bool fw_writes_is_enabled(struct kvm_vcpu *vcpu)
>>  {
>> -	return (vcpu_get_perf_capabilities(vcpu) & PERF_CAP_FW_WRITES) != 0;
>> +	return (kvm_vcpu_get_perf_caps(vcpu) & PERF_CAP_FW_WRITES) != 0;
>>  }
> 
> [Severity: Low]
> Is the include for cpuid.h at the top of this snippet still needed now that
> the function relying on it has moved out of pmu_intel.h?

Yes, I think #include "cpuid.h" can be cleanly removed now.

> Also, since fw_writes_is_enabled() now calls kvm_vcpu_get_perf_caps(), would
> it be better to add an explicit include for pmu.h here to ensure header
> dependencies are fully self-contained?

I don't think that's necessary, since it's natural to include the common
pmu.h before the vendor-specific pmu_intel.h.