[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/5] target/i386: Populate AMD Processor Cach
From: |
Moger, Babu |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/5] target/i386: Populate AMD Processor Cache Information |
Date: |
Thu, 1 Mar 2018 15:55:50 +0000 |
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:address@hidden
> Sent: Wednesday, February 28, 2018 12:09 PM
> To: Moger, Babu <address@hidden>
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; Hook, Gary <address@hidden>
> Subject: Re: [PATCH v2 2/5] target/i386: Populate AMD Processor Cache
> Information
>
> 2018-02-23 21:30-0500, Babu Moger:
> > From: Stanislav Lanci <address@hidden>
> >
> > Adds information about cache size and topology from cpuid 0x8000001D
> leaf
> > for different cache types on AMD processors.
> >
> > Signed-off-by: Stanislav Lanci <address@hidden>
> > Signed-off-by: Babu Moger <address@hidden>
> > ---
> > 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.
- Re: [Qemu-devel] [PATCH v2 2/5] target/i386: Populate AMD Processor Cache Information,
Moger, Babu <=