Radim, Thanks for your comments. I am working on the changes. But, I need few clarifications on your comments. Please see inline. > -----Original Message----- > From: Radim Krčmář [mailto:rkrcmar@xxxxxxxxxx] > Sent: Wednesday, February 28, 2018 12:09 PM > To: Moger, Babu <Babu.Moger@xxxxxxx> > Cc: pbonzini@xxxxxxxxxx; rth@xxxxxxxxxxx; ehabkost@xxxxxxxxxx; > mtosatti@xxxxxxxxxx; qemu-devel@xxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; > pixo@xxxxxxxxxxxx; Hook, Gary <Gary.Hook@xxxxxxx> > Subject: Re: [PATCH v2 2/5] target/i386: Populate AMD Processor Cache > Information > > 2018-02-23 21:30-0500, Babu Moger: > > From: Stanislav Lanci <pixo@xxxxxxxxxxxx> > > > > Adds information about cache size and topology from cpuid 0x8000001D > leaf > > for different cache types on AMD processors. > > > > Signed-off-by: Stanislav Lanci <pixo@xxxxxxxxxxxx> > > Signed-off-by: Babu Moger <babu.moger@xxxxxxx> > > --- > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > @@ -3590,6 +3594,78 @@ void cpu_x86_cpuid(CPUX86State *env, > uint32_t index, uint32_t count, > > *edx = 0; > > } > > break; > > + case 0x8000001D: /* AMD TOPOEXT cache info */ > > + if (cpu->cache_info_passthrough) { > > + host_cpuid(index, count, eax, ebx, ecx, edx); > > + break; > > + } else if (env->features[FEAT_8000_0001_ECX] & > CPUID_EXT3_TOPOEXT) { > > + *eax = 0; > > + switch (count) { > > + case 0: /* L1 dcache info */ > > + *eax |= CPUID_4_TYPE_DCACHE | \ > > + CPUID_4_LEVEL(1) | \ > > + CPUID_4_SELF_INIT_LEVEL | \ > > + ((cs->nr_threads - 1) << 14); > > CPUID_4 uses the same format even for bits 25-14, so this would look > better with a macro. Yes. We can do that. > > > + *ebx = (L1D_LINE_SIZE - 1) | \ > > + ((L1D_PARTITIONS - 1) << 12) | \ > > + ((L1D_ASSOCIATIVITY - 1) << 22); > > + *ecx = L1D_SETS - 1; > > These numbers seem to have the same meaning as CPUID 4, but have > conflicting values. I am not sure about conflicting values. Looking at the specs(page 78 CPUID_Fn8000001D_EBX_x00) https://support.amd.com/TechDocs/54945_PPR_Family_17h_Models_00h-0Fh.pdf It looks correct to me. > > I think we should not expose CPUID 4 with AMD CPUs or at least when they > have CPUID_EXT3_TOPOEXT (the latter is easier wrt. compatibility). Can you please elaborate on these comments? Did you mean we should remove the check CPUID_EXT3_TOPOEXT and remove all CPUID 4 references? > > > + *edx = 0; > > + break; > > + case 1: /* L1 icache info */ > > + *eax |= CPUID_4_TYPE_ICACHE | \ > > + CPUID_4_LEVEL(1) | \ > > + CPUID_4_SELF_INIT_LEVEL | \ > > + ((cs->nr_threads - 1) << 14); > > + *ebx = (L1I_LINE_SIZE - 1) | \ > > + ((L1I_PARTITIONS - 1) << 12) | \ > > + ((L1I_ASSOCIATIVITY_AMD - 1) << 22); > > + *ecx = L1I_SETS_AMD - 1; > > + *edx = 0; > > + break; > > + case 2: /* L2 cache info */ > > + *eax |= CPUID_4_TYPE_UNIFIED | \ > > + CPUID_4_LEVEL(2) | \ > > + CPUID_4_SELF_INIT_LEVEL | \ > > + ((cs->nr_threads - 1) << 14); > > + *ebx = (L2_LINE_SIZE - 1) | \ > > + ((L2_PARTITIONS - 1) << 12) | \ > > + ((L2_ASSOCIATIVITY_AMD - 1) << 22); > > + *ecx = L2_SETS_AMD - 1; > > + *edx = CPUID_4_INCLUSIVE; > > + break; > > + case 3: /* L3 cache info */ > > + if (!cpu->enable_l3_cache) { > > + *eax = 0; > > + *ebx = 0; > > + *ecx = 0; > > + *edx = 0; > > + break; > > + } > > + *eax |= CPUID_4_TYPE_UNIFIED | \ > > + CPUID_4_LEVEL(3) | \ > > + CPUID_4_SELF_INIT_LEVEL | \ > > + ((cs->nr_cores * cs->nr_threads - 1) << 14); > > This number seems to be the only difference that isn't just a difference > constant. It tempts me to merge the cases for 4 and 0x8000001D as it > seems that vendors try to be compatible. Yes. We could merge cases for 4 and 0x8000001D. > > > + *ebx = (L3_N_LINE_SIZE - 1) | \ > > + ((L3_N_PARTITIONS - 1) << 12) | \ > > + ((L3_N_ASSOCIATIVITY - 1) << 22); > > + *ecx = L3_N_SETS_AMD - 1; > > + *edx = CPUID_4_NO_INVD_SHARING; > > + break; > > + default: /* end of info */ > > + *eax = 0; > > + *ebx = 0; > > + *ecx = 0; > > + *edx = 0; > > + break; > > + } > > + } else { > > + *eax = 0; > > + *ebx = 0; > > + *ecx = 0; > > + *edx = 0; > > + } > > + break; > > case 0xC0000000: > > *eax = env->cpuid_xlevel2; > > *ebx = 0; > > The numbers looks like real hardware, Do you want me to change anything here? > > thanks.
