Re: [PATCH v2 2/5] target/i386: Populate AMD Processor Cache Information

["Radim Krčmář" <rkrcmar@redhat.com>]

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.