Hi Zhao, On 4/10/25 12:43 AM, Zhao Liu wrote: > ... > >> TODO: >> - This patch adds is_host_compat_vendor(), while there are something >> like is_host_cpu_intel() from target/i386/kvm/vmsr_energy.c. A rework >> may help move those helpers to target/i386/cpu*. > > vmsr_energy emulates RAPL in user space...but RAPL is not architectural > (no CPUID), so this case doesn't need to consider "compat" vendor. > >> target/i386/cpu.h | 8 ++ >> target/i386/kvm/kvm.c | 176 +++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 180 insertions(+), 4 deletions(-) > > ... > >> +static bool is_host_compat_vendor(CPUX86State *env) >> +{ >> + char host_vendor[CPUID_VENDOR_SZ + 1]; >> + uint32_t host_cpuid_vendor1; >> + uint32_t host_cpuid_vendor2; >> + uint32_t host_cpuid_vendor3; >> >> + host_cpuid(0x0, 0, NULL, &host_cpuid_vendor1, &host_cpuid_vendor3, >> + &host_cpuid_vendor2); >> + >> + x86_cpu_vendor_words2str(host_vendor, host_cpuid_vendor1, >> + host_cpuid_vendor2, host_cpuid_vendor3); > > We can use host_cpu_vendor_fms() (with a little change). If you like > this idea, pls feel free to pick my cleanup patch into your series.
Sure. I will try to use host_cpu_vendor_fms(). > >> + /* >> + * Intel and Zhaoxin are compatible. >> + */ >> + if ((g_str_equal(host_vendor, CPUID_VENDOR_INTEL) || >> + g_str_equal(host_vendor, CPUID_VENDOR_ZHAOXIN1) || >> + g_str_equal(host_vendor, CPUID_VENDOR_ZHAOXIN2)) && >> + (IS_INTEL_CPU(env) || IS_ZHAOXIN_CPU(env))) { >> + return true; >> + } >> + >> + return env->cpuid_vendor1 == host_cpuid_vendor1 && >> + env->cpuid_vendor2 == host_cpuid_vendor2 && >> + env->cpuid_vendor3 == host_cpuid_vendor3; > > Checking AMD directly makes the "compat" rule clear: > > return g_str_equal(host_vendor, CPUID_VENDOR_AMD) && > IS_AMD_CPU(env); Sure. > >> +} > > ... > >> if (env->mcg_cap) { >> kvm_msr_entry_add(cpu, MSR_MCG_STATUS, 0); >> kvm_msr_entry_add(cpu, MSR_MCG_CTL, 0); >> @@ -4871,6 +5024,21 @@ static int kvm_get_msrs(X86CPU *cpu) >> case MSR_P6_EVNTSEL0 ... MSR_P6_EVNTSEL0 + MAX_GP_COUNTERS - 1: >> env->msr_gp_evtsel[index - MSR_P6_EVNTSEL0] = msrs[i].data; >> break; >> + case MSR_K7_EVNTSEL0 ... MSR_K7_EVNTSEL0 + AMD64_NUM_COUNTERS - 1: >> + env->msr_gp_evtsel[index - MSR_K7_EVNTSEL0] = msrs[i].data; >> + break; >> + case MSR_K7_PERFCTR0 ... MSR_K7_PERFCTR0 + AMD64_NUM_COUNTERS - 1: >> + env->msr_gp_counters[index - MSR_K7_PERFCTR0] = msrs[i].data; >> + break; >> + case MSR_F15H_PERF_CTL0 ... >> + MSR_F15H_PERF_CTL0 + AMD64_NUM_COUNTERS_CORE * 2 - 1: >> + index = index - MSR_F15H_PERF_CTL0; >> + if (index & 0x1) { >> + env->msr_gp_counters[index] = msrs[i].data; >> + } else { >> + env->msr_gp_evtsel[index] = msrs[i].data; > > This msr_gp_evtsel[] array's size is 18: > > #define MAX_GP_COUNTERS (MSR_IA32_PERF_STATUS - MSR_P6_EVNTSEL0) > > This formula is based on Intel's MSR, it's best to add a note that the > current size also meets AMD's needs. (No need to adjust the size, as > it will affect migration). I will add a comment to target/i386/cpu.h, above the definition of MAX_GP_COUNTERS. Thank you very much! Dongli Zhang