| Message ID | 1519439425-27883-3-git-send-email-babu.moger@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2018-02-23 21:30-0500, Babu Moger: > From: Stanislav Lanci <pixo@polepetko.eu> > > Adds information about cache size and topology from cpuid 0x8000001D leaf > for different cache types on AMD processors. > > Signed-off-by: Stanislav Lanci <pixo@polepetko.eu> > Signed-off-by: Babu Moger <babu.moger@amd.com> > --- > 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. > + *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 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). > + *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. > + *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, thanks.
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@redhat.com] > Sent: Wednesday, February 28, 2018 12:09 PM > To: Moger, Babu <Babu.Moger@amd.com> > Cc: pbonzini@redhat.com; rth@twiddle.net; ehabkost@redhat.com; > mtosatti@redhat.com; qemu-devel@nongnu.org; kvm@vger.kernel.org; > pixo@polepetko.eu; Hook, Gary <Gary.Hook@amd.com> > 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@polepetko.eu> > > > > Adds information about cache size and topology from cpuid 0x8000001D > leaf > > for different cache types on AMD processors. > > > > Signed-off-by: Stanislav Lanci <pixo@polepetko.eu> > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > --- > > 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.
2018-03-01 15:55+0000, Moger, Babu: > 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@redhat.com] > > Sent: Wednesday, February 28, 2018 12:09 PM > > To: Moger, Babu <Babu.Moger@amd.com> > > Cc: pbonzini@redhat.com; rth@twiddle.net; ehabkost@redhat.com; > > mtosatti@redhat.com; qemu-devel@nongnu.org; kvm@vger.kernel.org; > > pixo@polepetko.eu; Hook, Gary <Gary.Hook@amd.com> > > 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@polepetko.eu> > > > > > > Adds information about cache size and topology from cpuid 0x8000001D > > leaf > > > for different cache types on AMD processors. > > > > > > Signed-off-by: Stanislav Lanci <pixo@polepetko.eu> > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > > --- > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > > @@ -3590,6 +3594,78 @@ void cpu_x86_cpuid(CPUX86State *env, > > > + *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 agree. My comment is misplaced -- it should have been under the place that uses *_AMD macros. I wanted to point out that CPUID in QEMU is very Intel-focused and always contains CPUID leaf 4, which has fields of the very same meaning, but with different values. > > 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? CPUID 4 should have never been present when emulating AMD CPUs, but it's worse now that the numbers are conflicting. I meant to hide CPUID 4 for all AMD CPUs on future machine types, or at least when CPUID_EXT3_TOPOEXT is enabled. Keeping the current logic not a big problem as CPUID 4 should never be used by operating systems on AMD nor trusted inside a VM. Bringing the emulation closer to real state would be nice, but this can definitely be done later (aka never). > > The numbers looks like real hardware, > > Do you want me to change anything here? No, I was just commending, thanks.
> -----Original Message----- > From: Radim Krčmář [mailto:rkrcmar@redhat.com] > Sent: Thursday, March 1, 2018 1:56 PM > To: Moger, Babu <Babu.Moger@amd.com> > Cc: pbonzini@redhat.com; rth@twiddle.net; ehabkost@redhat.com; > mtosatti@redhat.com; qemu-devel@nongnu.org; kvm@vger.kernel.org; > pixo@polepetko.eu; Hook, Gary <Gary.Hook@amd.com> > Subject: Re: [PATCH v2 2/5] target/i386: Populate AMD Processor Cache > Information > > 2018-03-01 15:55+0000, Moger, Babu: > > 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@redhat.com] > > > Sent: Wednesday, February 28, 2018 12:09 PM > > > To: Moger, Babu <Babu.Moger@amd.com> > > > Cc: pbonzini@redhat.com; rth@twiddle.net; ehabkost@redhat.com; > > > mtosatti@redhat.com; qemu-devel@nongnu.org; kvm@vger.kernel.org; > > > pixo@polepetko.eu; Hook, Gary <Gary.Hook@amd.com> > > > 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@polepetko.eu> > > > > > > > > Adds information about cache size and topology from cpuid 0x8000001D > > > leaf > > > > for different cache types on AMD processors. > > > > > > > > Signed-off-by: Stanislav Lanci <pixo@polepetko.eu> > > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > > > --- > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > > > @@ -3590,6 +3594,78 @@ void cpu_x86_cpuid(CPUX86State *env, > > > > + *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 agree. My comment is misplaced -- it should have been under the place > that uses *_AMD macros. > > I wanted to point out that CPUID in QEMU is very Intel-focused and > always contains CPUID leaf 4, which has fields of the very same meaning, > but with different values. > > > > 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? > > CPUID 4 should have never been present when emulating AMD CPUs, but it's > worse now that the numbers are conflicting. > > I meant to hide CPUID 4 for all AMD CPUs on future machine types, or at > least when CPUID_EXT3_TOPOEXT is enabled. Sorry, I think I created some confusion here by using CPUID 4 definitions which are mostly for intel. Let me rework on this. I will repost the patches. Thanks for the feedback. > > Keeping the current logic not a big problem as CPUID 4 should never be > used by operating systems on AMD nor trusted inside a VM. Bringing the > emulation closer to real state would be nice, but this can definitely be > done later (aka never). > > > > The numbers looks like real hardware, > > > > Do you want me to change anything here? > > No, I was just commending, > > thanks.
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index b5e431e..a5a480e 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -118,6 +118,7 @@ #define L1I_LINE_SIZE 64 #define L1I_ASSOCIATIVITY 8 #define L1I_SETS 64 +#define L1I_SETS_AMD 256 #define L1I_PARTITIONS 1 /* Size = LINE_SIZE*ASSOCIATIVITY*SETS*PARTITIONS = 32KiB */ #define L1I_DESCRIPTOR CPUID_2_L1I_32KB_8WAY_64B @@ -129,7 +130,9 @@ /* Level 2 unified cache: */ #define L2_LINE_SIZE 64 #define L2_ASSOCIATIVITY 16 +#define L2_ASSOCIATIVITY_AMD 8 #define L2_SETS 4096 +#define L2_SETS_AMD 1024 #define L2_PARTITIONS 1 /* Size = LINE_SIZE*ASSOCIATIVITY*SETS*PARTITIONS = 4MiB */ /*FIXME: CPUID leaf 2 descriptor is inconsistent with CPUID leaf 4 */ @@ -146,6 +149,7 @@ #define L3_N_LINE_SIZE 64 #define L3_N_ASSOCIATIVITY 16 #define L3_N_SETS 16384 +#define L3_N_SETS_AMD 8192 #define L3_N_PARTITIONS 1 #define L3_N_DESCRIPTOR CPUID_2_L3_16MB_16WAY_64B #define L3_N_LINES_PER_TAG 1 @@ -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); + *ebx = (L1D_LINE_SIZE - 1) | \ + ((L1D_PARTITIONS - 1) << 12) | \ + ((L1D_ASSOCIATIVITY - 1) << 22); + *ecx = L1D_SETS - 1; + *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); + *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; diff --git a/target/i386/kvm.c b/target/i386/kvm.c index 85856b6..8adf7d1 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -909,9 +909,32 @@ int kvm_arch_init_vcpu(CPUState *cs) } c = &cpuid_data.entries[cpuid_i++]; - c->function = i; - c->flags = 0; - cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx, &c->edx); + switch (i) { + case 0x8000001d: + /* Query for all AMD cache information leaves */ + for (j = 0; ; j++) { + c->function = i; + c->flags = KVM_CPUID_FLAG_SIGNIFICANT_INDEX; + c->index = j; + cpu_x86_cpuid(env, i, j, &c->eax, &c->ebx, &c->ecx, &c->edx); + + if (c->eax == 0) { + break; + } + if (cpuid_i == KVM_MAX_CPUID_ENTRIES) { + fprintf(stderr, "cpuid_data is full, no space for " + "cpuid(eax:0x%x,ecx:0x%x)\n", i, j); + abort(); + } + c = &cpuid_data.entries[cpuid_i++]; + } + break; + default: + c->function = i; + c->flags = 0; + cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx, &c->edx); + break; + } } /* Call Centaur's CPUID instructions they are supported. */