| Message ID | 20250302220112.17653-3-dongli.zhang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/i386/kvm/pmu: PMU Enhancement, Bugfix and Cleanup | expand |
On 3/3/2025 6:00 AM, Dongli Zhang wrote: > Currently, AMD PMU support isn't determined based on CPUID, that is, the > "-pmu" option does not fully disable KVM AMD PMU virtualization. > > To minimize AMD PMU features, remove PERFCORE when "-pmu" is configured. > > To completely disable AMD PMU virtualization will be implemented via > KVM_CAP_PMU_CAPABILITY in upcoming patches. > > As a reminder, neither CPUID_EXT3_PERFCORE nor > CPUID_8000_0022_EAX_PERFMON_V2 is removed from env->features[] when "-pmu" > is configured. Developers should query whether they are supported via > cpu_x86_cpuid() rather than relying on env->features[] in future patches. I don't think it is the correct direction to go. env->features[] should be finalized before cpu_x86_cpuid() and env->features[] needs to be able to be exposed to guest directly. This ensures guest and QEMU have the same view of CPUIDs and it simplifies things. We can adjust env->features[] by filtering all PMU related CPUIDs based on cpu->enable_pmu in x86_cpu_realizefn(). > Suggested-by: Zhao Liu <zhao1.liu@intel.com> > Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> > --- > target/i386/cpu.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index b6d6167910..61a671028a 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -7115,6 +7115,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > !(env->hflags & HF_LMA_MASK)) { > *edx &= ~CPUID_EXT2_SYSCALL; > } > + > + if (kvm_enabled() && IS_AMD_CPU(env) && !cpu->enable_pmu) { > + *ecx &= ~CPUID_EXT3_PERFCORE; > + } > break; > case 0x80000002: > case 0x80000003:
Hi Xiaoyao, On 3/2/25 5:59 PM, Xiaoyao Li wrote: > On 3/3/2025 6:00 AM, Dongli Zhang wrote: >> Currently, AMD PMU support isn't determined based on CPUID, that is, the >> "-pmu" option does not fully disable KVM AMD PMU virtualization. >> >> To minimize AMD PMU features, remove PERFCORE when "-pmu" is configured. >> >> To completely disable AMD PMU virtualization will be implemented via >> KVM_CAP_PMU_CAPABILITY in upcoming patches. >> >> As a reminder, neither CPUID_EXT3_PERFCORE nor >> CPUID_8000_0022_EAX_PERFMON_V2 is removed from env->features[] when "-pmu" >> is configured. Developers should query whether they are supported via >> cpu_x86_cpuid() rather than relying on env->features[] in future patches. > > I don't think it is the correct direction to go. > > env->features[] should be finalized before cpu_x86_cpuid() and env- >>features[] needs to be able to be exposed to guest directly. This ensures > guest and QEMU have the same view of CPUIDs and it simplifies things. > > We can adjust env->features[] by filtering all PMU related CPUIDs based on > cpu->enable_pmu in x86_cpu_realizefn(). Thank you very much for suggestion. I see code like below in x86_cpu_realizefn() to edit env->features[]. 7982 /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on 7983 * CPUID[1].EDX. 7984 */ 7985 if (IS_AMD_CPU(env)) { 7986 env->features[FEAT_8000_0001_EDX] &= ~CPUID_EXT2_AMD_ALIASES; 7987 env->features[FEAT_8000_0001_EDX] |= (env->features[FEAT_1_EDX] 7988 & CPUID_EXT2_AMD_ALIASES); 7989 } I may do something similar to them for CPUID_EXT3_PERFCORE and CPUID_8000_0022_EAX_PERFMON_V2. Dongli Zhang > >> Suggested-by: Zhao Liu <zhao1.liu@intel.com> >> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> >> --- >> target/i386/cpu.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> index b6d6167910..61a671028a 100644 >> --- a/target/i386/cpu.c >> +++ b/target/i386/cpu.c >> @@ -7115,6 +7115,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t >> index, uint32_t count, >> !(env->hflags & HF_LMA_MASK)) { >> *edx &= ~CPUID_EXT2_SYSCALL; >> } >> + >> + if (kvm_enabled() && IS_AMD_CPU(env) && !cpu->enable_pmu) { >> + *ecx &= ~CPUID_EXT3_PERFCORE; >> + } >> break; >> case 0x80000002: >> case 0x80000003: >
On 3/4/2025 2:45 AM, dongli.zhang@oracle.com wrote: > Hi Xiaoyao, > > On 3/2/25 5:59 PM, Xiaoyao Li wrote: >> On 3/3/2025 6:00 AM, Dongli Zhang wrote: >>> Currently, AMD PMU support isn't determined based on CPUID, that is, the >>> "-pmu" option does not fully disable KVM AMD PMU virtualization. >>> >>> To minimize AMD PMU features, remove PERFCORE when "-pmu" is configured. >>> >>> To completely disable AMD PMU virtualization will be implemented via >>> KVM_CAP_PMU_CAPABILITY in upcoming patches. >>> >>> As a reminder, neither CPUID_EXT3_PERFCORE nor >>> CPUID_8000_0022_EAX_PERFMON_V2 is removed from env->features[] when "-pmu" >>> is configured. Developers should query whether they are supported via >>> cpu_x86_cpuid() rather than relying on env->features[] in future patches. >> >> I don't think it is the correct direction to go. >> >> env->features[] should be finalized before cpu_x86_cpuid() and env- >>> features[] needs to be able to be exposed to guest directly. This ensures >> guest and QEMU have the same view of CPUIDs and it simplifies things. >> >> We can adjust env->features[] by filtering all PMU related CPUIDs based on >> cpu->enable_pmu in x86_cpu_realizefn(). > > Thank you very much for suggestion. > > I see code like below in x86_cpu_realizefn() to edit env->features[]. > > 7982 /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on > 7983 * CPUID[1].EDX. > 7984 */ > 7985 if (IS_AMD_CPU(env)) { > 7986 env->features[FEAT_8000_0001_EDX] &= ~CPUID_EXT2_AMD_ALIASES; > 7987 env->features[FEAT_8000_0001_EDX] |= (env->features[FEAT_1_EDX] > 7988 & CPUID_EXT2_AMD_ALIASES); > 7989 } > > I may do something similar to them for CPUID_EXT3_PERFCORE and > CPUID_8000_0022_EAX_PERFMON_V2. I just sent a series for CPUID_EXT_PDCM[1]. I think you can put CPUID_EXT3_PERFCORE and CPUID_8000_0022_EAX_PERFMON_V2 at the same place. [1] https://lore.kernel.org/qemu-devel/20250304052450.465445-1-xiaoyao.li@intel.com/T/#m31c6777131b6361d7c3af22b09532bdc785dbc06 > Dongli Zhang > > > >> >>> Suggested-by: Zhao Liu <zhao1.liu@intel.com> >>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> >>> --- >>> target/i386/cpu.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c >>> index b6d6167910..61a671028a 100644 >>> --- a/target/i386/cpu.c >>> +++ b/target/i386/cpu.c >>> @@ -7115,6 +7115,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t >>> index, uint32_t count, >>> !(env->hflags & HF_LMA_MASK)) { >>> *edx &= ~CPUID_EXT2_SYSCALL; >>> } >>> + >>> + if (kvm_enabled() && IS_AMD_CPU(env) && !cpu->enable_pmu) { >>> + *ecx &= ~CPUID_EXT3_PERFCORE; >>> + } >>> break; >>> case 0x80000002: >>> case 0x80000003: >> >
Hi Dongli, > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index b6d6167910..61a671028a 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -7115,6 +7115,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > !(env->hflags & HF_LMA_MASK)) { > *edx &= ~CPUID_EXT2_SYSCALL; > } > + > + if (kvm_enabled() && IS_AMD_CPU(env) && !cpu->enable_pmu) { No need to check "kvm_enabled() && IS_AMD_CPU(env)" because: * "pmu" is a general CPU property option which should cover all PMU related features, and not kvm-specific/vendor-specific. * this bit is reserved on Intel. So the following operation doesn't affect Intel. I think Xiaoyao's idea about checking in x86_cpu_expand_features() is good. And I believe it's worth having another cleanup series to revisit pmu dependencies. I can help you later to consolidate and move this check to x86_cpu_expand_features(), so this patch can focus on correctly defining the current dependency relationship. With the above nit fixed, Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Hi Zhao, On 3/6/25 8:50 AM, Zhao Liu wrote: > Hi Dongli, > >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> index b6d6167910..61a671028a 100644 >> --- a/target/i386/cpu.c >> +++ b/target/i386/cpu.c >> @@ -7115,6 +7115,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, >> !(env->hflags & HF_LMA_MASK)) { >> *edx &= ~CPUID_EXT2_SYSCALL; >> } >> + >> + if (kvm_enabled() && IS_AMD_CPU(env) && !cpu->enable_pmu) { > > No need to check "kvm_enabled() && IS_AMD_CPU(env)" because: > > * "pmu" is a general CPU property option which should cover all PMU > related features, and not kvm-specific/vendor-specific. > * this bit is reserved on Intel. So the following operation doesn't > affect Intel. > > I think Xiaoyao's idea about checking in x86_cpu_expand_features() is > good. And I believe it's worth having another cleanup series to revisit > pmu dependencies. I can help you later to consolidate and move this > check to x86_cpu_expand_features(), so this patch can focus on correctly > defining the current dependency relationship. That means I don't need to change anything except: 1. Remove "kvm_enabled() && IS_AMD_CPU(env)" since the bit is reserved by Intel. 2. Add your Reviewed-by. Thank you very much! Dongli Zhang > > With the above nit fixed, > > Reviewed-by: Zhao Liu <zhao1.liu@intel.com> > > >
> 1. Remove "kvm_enabled() && IS_AMD_CPU(env)" since the bit is reserved by > Intel. > > 2. Add your Reviewed-by. Yes, this is exactly what I mean! Regards, Zhao
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index b6d6167910..61a671028a 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -7115,6 +7115,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, !(env->hflags & HF_LMA_MASK)) { *edx &= ~CPUID_EXT2_SYSCALL; } + + if (kvm_enabled() && IS_AMD_CPU(env) && !cpu->enable_pmu) { + *ecx &= ~CPUID_EXT3_PERFCORE; + } break; case 0x80000002: case 0x80000003:
Currently, AMD PMU support isn't determined based on CPUID, that is, the "-pmu" option does not fully disable KVM AMD PMU virtualization. To minimize AMD PMU features, remove PERFCORE when "-pmu" is configured. To completely disable AMD PMU virtualization will be implemented via KVM_CAP_PMU_CAPABILITY in upcoming patches. As a reminder, neither CPUID_EXT3_PERFCORE nor CPUID_8000_0022_EAX_PERFMON_V2 is removed from env->features[] when "-pmu" is configured. Developers should query whether they are supported via cpu_x86_cpuid() rather than relying on env->features[] in future patches. Suggested-by: Zhao Liu <zhao1.liu@intel.com> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> --- target/i386/cpu.c | 4 ++++ 1 file changed, 4 insertions(+)