Re: [PATCH v3 14/17] i386: Use CPUCacheInfo.share_level to encode CPUID[4]

[Zhao Liu <zhao1.liu@linux.intel.com>]

Hi Babu,

On Thu, Aug 03, 2023 at 11:41:40AM -0500, Moger, Babu wrote:
> Date: Thu, 3 Aug 2023 11:41:40 -0500
> From: "Moger, Babu" <babu.moger@amd.com>
> Subject: Re: [PATCH v3 14/17] i386: Use CPUCacheInfo.share_level to encode
>  CPUID[4]
> 
> Hi Zhao,
> 
> On 8/2/23 18:49, Moger, Babu wrote:
> > Hi Zhao,
> > 
> > Hitting this error after this patch.
> > 
> > ERROR:../target/i386/cpu.c:257:max_processor_ids_for_cache: code should
> > not be reached
> > Bail out! ERROR:../target/i386/cpu.c:257:max_processor_ids_for_cache: code
> > should not be reached
> > Aborted (core dumped)
> > 
> > Looks like share_level for all the caches for AMD is not initialized.

I missed these change when I rebase. Sorry for that.

BTW, could I ask a question? From a previous discussion[1], I understand
that the cache info is used to show the correct cache information in
new machine. And from [2], the wrong cache info may cause "compatibility
issues".

Is this "compatibility issues" AMD specific? I'm not sure if Intel should
update the cache info like that. thanks!

[1]: https://patchwork.kernel.org/project/kvm/patch/CY4PR12MB1768A3CBE42AAFB03CB1081E95AA0@CY4PR12MB1768.namprd12.prod.outlook.com/
[2]: https://lore.kernel.org/qemu-devel/20180510204148.11687-1-babu.moger@amd.com/

> 
> The following patch fixes the problem.
> 
> ======================================================
> 
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index f4c48e19fa..976a2755d8 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -528,6 +528,7 @@ static CPUCacheInfo legacy_l2_cache_cpuid2 = {
>      .size = 2 * MiB,
>      .line_size = 64,
>      .associativity = 8,
> +    .share_level = CPU_TOPO_LEVEL_CORE,

This "legacy_l2_cache_cpuid2" is not used to encode cache topology.
I should explicitly set this default topo level as CPU_TOPO_LEVEL_UNKNOW.

>  };
> 
> 
> @@ -1904,6 +1905,7 @@ static CPUCaches epyc_v4_cache_info = {
>          .lines_per_tag = 1,
>          .self_init = 1,
>          .no_invd_sharing = true,
> +        .share_level = CPU_TOPO_LEVEL_CORE,
>      },
>      .l1i_cache = &(CPUCacheInfo) {
>          .type = INSTRUCTION_CACHE,
> @@ -1916,6 +1918,7 @@ static CPUCaches epyc_v4_cache_info = {
>          .lines_per_tag = 1,
>          .self_init = 1,
>          .no_invd_sharing = true,
> +        .share_level = CPU_TOPO_LEVEL_CORE,
>      },
>      .l2_cache = &(CPUCacheInfo) {
>          .type = UNIFIED_CACHE,
> @@ -1926,6 +1929,7 @@ static CPUCaches epyc_v4_cache_info = {
>          .partitions = 1,
>          .sets = 1024,
>          .lines_per_tag = 1,
> +        .share_level = CPU_TOPO_LEVEL_CORE,
>      },
>      .l3_cache = &(CPUCacheInfo) {
>          .type = UNIFIED_CACHE,
> @@ -1939,6 +1943,7 @@ static CPUCaches epyc_v4_cache_info = {
>          .self_init = true,
>          .inclusive = true,
>          .complex_indexing = false,
> +        .share_level = CPU_TOPO_LEVEL_DIE,
>      },
>  };
> 
> @@ -2008,6 +2013,7 @@ static const CPUCaches epyc_rome_v3_cache_info = {
>          .lines_per_tag = 1,
>          .self_init = 1,
>          .no_invd_sharing = true,
> +        .share_level = CPU_TOPO_LEVEL_CORE,
>      },
>      .l1i_cache = &(CPUCacheInfo) {
>          .type = INSTRUCTION_CACHE,
> @@ -2020,6 +2026,7 @@ static const CPUCaches epyc_rome_v3_cache_info = {
>          .lines_per_tag = 1,
>          .self_init = 1,
>          .no_invd_sharing = true,
> +        .share_level = CPU_TOPO_LEVEL_CORE,
>      },
>      .l2_cache = &(CPUCacheInfo) {
>          .type = UNIFIED_CACHE,
> @@ -2030,6 +2037,7 @@ static const CPUCaches epyc_rome_v3_cache_info = {
>          .partitions = 1,
>          .sets = 1024,
>          .lines_per_tag = 1,
> +        .share_level = CPU_TOPO_LEVEL_CORE,
>      },
>      .l3_cache = &(CPUCacheInfo) {
>          .type = UNIFIED_CACHE,
> @@ -2043,6 +2051,7 @@ static const CPUCaches epyc_rome_v3_cache_info = {
>          .self_init = true,
>          .inclusive = true,
>          .complex_indexing = false,
> +        .share_level = CPU_TOPO_LEVEL_DIE,
>      },
>  };
> 
> @@ -2112,6 +2121,7 @@ static const CPUCaches epyc_milan_v2_cache_info = {
>          .lines_per_tag = 1,
>          .self_init = 1,
>          .no_invd_sharing = true,
> +        .share_level = CPU_TOPO_LEVEL_CORE,
>      },
>      .l1i_cache = &(CPUCacheInfo) {
>          .type = INSTRUCTION_CACHE,
> @@ -2124,6 +2134,7 @@ static const CPUCaches epyc_milan_v2_cache_info = {
>          .lines_per_tag = 1,
>          .self_init = 1,
>          .no_invd_sharing = true,
> +        .share_level = CPU_TOPO_LEVEL_CORE,
>      },
>      .l2_cache = &(CPUCacheInfo) {
>          .type = UNIFIED_CACHE,
> @@ -2134,6 +2145,7 @@ static const CPUCaches epyc_milan_v2_cache_info = {
>          .partitions = 1,
>          .sets = 1024,
>          .lines_per_tag = 1,
> +        .share_level = CPU_TOPO_LEVEL_CORE,
>      },
>      .l3_cache = &(CPUCacheInfo) {
>          .type = UNIFIED_CACHE,
> @@ -2147,6 +2159,7 @@ static const CPUCaches epyc_milan_v2_cache_info = {
>          .self_init = true,
>          .inclusive = true,
>          .complex_indexing = false,
> +        .share_level = CPU_TOPO_LEVEL_DIE,
>      },
>  };
> 
> @@ -2162,6 +2175,7 @@ static const CPUCaches epyc_genoa_cache_info = {
>          .lines_per_tag = 1,
>          .self_init = 1,
>          .no_invd_sharing = true,
> +        .share_level = CPU_TOPO_LEVEL_CORE,
>      },
>      .l1i_cache = &(CPUCacheInfo) {
>          .type = INSTRUCTION_CACHE,
> @@ -2174,6 +2188,7 @@ static const CPUCaches epyc_genoa_cache_info = {
>          .lines_per_tag = 1,
>          .self_init = 1,
>          .no_invd_sharing = true,
> +        .share_level = CPU_TOPO_LEVEL_CORE,
>      },
>      .l2_cache = &(CPUCacheInfo) {
>          .type = UNIFIED_CACHE,
> @@ -2184,6 +2199,7 @@ static const CPUCaches epyc_genoa_cache_info = {
>          .partitions = 1,
>          .sets = 2048,
>          .lines_per_tag = 1,
> +        .share_level = CPU_TOPO_LEVEL_CORE,
>      },
>      .l3_cache = &(CPUCacheInfo) {
>          .type = UNIFIED_CACHE,
> @@ -2197,6 +2213,7 @@ static const CPUCaches epyc_genoa_cache_info = {
>          .self_init = true,
>          .inclusive = true,
>          .complex_indexing = false,
> +        .share_level = CPU_TOPO_LEVEL_DIE,
>      },
>  };
> 
> 
> =========================================================================


Look good to me except legacy_l2_cache_cpuid2, thanks very much!
I'll add this in next version.

-Zhao

> 
> Thanks
> Babu
> > 
> > On 8/1/23 05:35, Zhao Liu wrote:
> >> From: Zhao Liu <zhao1.liu@intel.com>
> >>
> >> CPUID[4].EAX[bits 25:14] is used to represent the cache topology for
> >> intel CPUs.
> >>
> >> After cache models have topology information, we can use
> >> CPUCacheInfo.share_level to decide which topology level to be encoded
> >> into CPUID[4].EAX[bits 25:14].
> >>
> >> And since maximum_processor_id (original "num_apic_ids") is parsed
> >> based on cpu topology levels, which are verified when parsing smp, it's
> >> no need to check this value by "assert(num_apic_ids > 0)" again, so
> >> remove this assert.
> >>
> >> Additionally, wrap the encoding of CPUID[4].EAX[bits 31:26] into a
> >> helper to make the code cleaner.
> >>
> >> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> >> ---
> >> Changes since v1:
> >>  * Use "enum CPUTopoLevel share_level" as the parameter in
> >>    max_processor_ids_for_cache().
> >>  * Make cache_into_passthrough case also use
> >>    max_processor_ids_for_cache() and max_core_ids_in_package() to
> >>    encode CPUID[4]. (Yanan)
> >>  * Rename the title of this patch (the original is "i386: Use
> >>    CPUCacheInfo.share_level to encode CPUID[4].EAX[bits 25:14]").
> >> ---
> >>  target/i386/cpu.c | 70 +++++++++++++++++++++++++++++------------------
> >>  1 file changed, 43 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> >> index 55aba4889628..c9897c0fe91a 100644
> >> --- a/target/i386/cpu.c
> >> +++ b/target/i386/cpu.c
> >> @@ -234,22 +234,53 @@ static uint8_t cpuid2_cache_descriptor(CPUCacheInfo *cache)
> >>                         ((t) == UNIFIED_CACHE) ? CACHE_TYPE_UNIFIED : \
> >>                         0 /* Invalid value */)
> >>  
> >> +static uint32_t max_processor_ids_for_cache(X86CPUTopoInfo *topo_info,
> >> +                                            enum CPUTopoLevel share_level)
> >> +{
> >> +    uint32_t num_ids = 0;
> >> +
> >> +    switch (share_level) {
> >> +    case CPU_TOPO_LEVEL_CORE:
> >> +        num_ids = 1 << apicid_core_offset(topo_info);
> >> +        break;
> >> +    case CPU_TOPO_LEVEL_DIE:
> >> +        num_ids = 1 << apicid_die_offset(topo_info);
> >> +        break;
> >> +    case CPU_TOPO_LEVEL_PACKAGE:
> >> +        num_ids = 1 << apicid_pkg_offset(topo_info);
> >> +        break;
> >> +    default:
> >> +        /*
> >> +         * Currently there is no use case for SMT and MODULE, so use
> >> +         * assert directly to facilitate debugging.
> >> +         */
> >> +        g_assert_not_reached();
> >> +    }
> >> +
> >> +    return num_ids - 1;
> >> +}
> >> +
> >> +static uint32_t max_core_ids_in_package(X86CPUTopoInfo *topo_info)
> >> +{
> >> +    uint32_t num_cores = 1 << (apicid_pkg_offset(topo_info) -
> >> +                               apicid_core_offset(topo_info));
> >> +    return num_cores - 1;
> >> +}
> >>  
> >>  /* Encode cache info for CPUID[4] */
> >>  static void encode_cache_cpuid4(CPUCacheInfo *cache,
> >> -                                int num_apic_ids, int num_cores,
> >> +                                X86CPUTopoInfo *topo_info,
> >>                                  uint32_t *eax, uint32_t *ebx,
> >>                                  uint32_t *ecx, uint32_t *edx)
> >>  {
> >>      assert(cache->size == cache->line_size * cache->associativity *
> >>                            cache->partitions * cache->sets);
> >>  
> >> -    assert(num_apic_ids > 0);
> >>      *eax = CACHE_TYPE(cache->type) |
> >>             CACHE_LEVEL(cache->level) |
> >>             (cache->self_init ? CACHE_SELF_INIT_LEVEL : 0) |
> >> -           ((num_cores - 1) << 26) |
> >> -           ((num_apic_ids - 1) << 14);
> >> +           (max_core_ids_in_package(topo_info) << 26) |
> >> +           (max_processor_ids_for_cache(topo_info, cache->share_level) << 14);
> >>  
> >>      assert(cache->line_size > 0);
> >>      assert(cache->partitions > 0);
> >> @@ -6116,56 +6147,41 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> >>                  int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
> >>  
> >>                  if (cores_per_pkg > 1) {
> >> -                    int addressable_cores_offset =
> >> -                                                apicid_pkg_offset(&topo_info) -
> >> -                                                apicid_core_offset(&topo_info);
> >> -
> >>                      *eax &= ~0xFC000000;
> >> -                    *eax |= (1 << addressable_cores_offset - 1) << 26;
> >> +                    *eax |= max_core_ids_in_package(&topo_info) << 26;
> >>                  }
> >>                  if (host_vcpus_per_cache > cpus_per_pkg) {
> >> -                    int pkg_offset = apicid_pkg_offset(&topo_info);
> >> -
> >>                      *eax &= ~0x3FFC000;
> >> -                    *eax |= (1 << pkg_offset - 1) << 14;
> >> +                    *eax |=
> >> +                        max_processor_ids_for_cache(&topo_info,
> >> +                                                CPU_TOPO_LEVEL_PACKAGE) << 14;
> >>                  }
> >>              }
> >>          } else if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) {
> >>              *eax = *ebx = *ecx = *edx = 0;
> >>          } else {
> >>              *eax = 0;
> >> -            int addressable_cores_offset = apicid_pkg_offset(&topo_info) -
> >> -                                           apicid_core_offset(&topo_info);
> >> -            int core_offset, die_offset;
> >>  
> >>              switch (count) {
> >>              case 0: /* L1 dcache info */
> >> -                core_offset = apicid_core_offset(&topo_info);
> >>                  encode_cache_cpuid4(env->cache_info_cpuid4.l1d_cache,
> >> -                                    (1 << core_offset),
> >> -                                    (1 << addressable_cores_offset),
> >> +                                    &topo_info,
> >>                                      eax, ebx, ecx, edx);
> >>                  break;
> >>              case 1: /* L1 icache info */
> >> -                core_offset = apicid_core_offset(&topo_info);
> >>                  encode_cache_cpuid4(env->cache_info_cpuid4.l1i_cache,
> >> -                                    (1 << core_offset),
> >> -                                    (1 << addressable_cores_offset),
> >> +                                    &topo_info,
> >>                                      eax, ebx, ecx, edx);
> >>                  break;
> >>              case 2: /* L2 cache info */
> >> -                core_offset = apicid_core_offset(&topo_info);
> >>                  encode_cache_cpuid4(env->cache_info_cpuid4.l2_cache,
> >> -                                    (1 << core_offset),
> >> -                                    (1 << addressable_cores_offset),
> >> +                                    &topo_info,
> >>                                      eax, ebx, ecx, edx);
> >>                  break;
> >>              case 3: /* L3 cache info */
> >> -                die_offset = apicid_die_offset(&topo_info);
> >>                  if (cpu->enable_l3_cache) {
> >>                      encode_cache_cpuid4(env->cache_info_cpuid4.l3_cache,
> >> -                                        (1 << die_offset),
> >> -                                        (1 << addressable_cores_offset),
> >> +                                        &topo_info,
> >>                                          eax, ebx, ecx, edx);
> >>                      break;
> >>                  }
> > 
> 
> -- 
> Thanks
> Babu Moger