Re: [PATCH v6 09/10] i386: Simplify CPUID_8000_001E for AMD

[Babu Moger]

On 9/1/20 6:52 AM, Igor Mammedov wrote:
> On Mon, 31 Aug 2020 13:43:01 -0500
> Babu Moger <babu.moger@amd.com> wrote:
> 
>> Remove all the hardcoded values and replace with generalized
>> fields.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  target/i386/cpu.c |   31 +++++++++++++++++--------------
>>  1 file changed, 17 insertions(+), 14 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index ba4667b33c..d434c8545a 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -395,9 +395,10 @@ static int cores_in_core_complex(int nr_cores)
>>  }
>>  
>>  /* Encode cache info for CPUID[8000001D] */
>> -static void encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
>> -                                uint32_t *eax, uint32_t *ebx,
>> -                                uint32_t *ecx, uint32_t *edx)
>> +static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
>> +                                       X86CPUTopoInfo *topo_info,
>> +                                       uint32_t *eax, uint32_t *ebx,
>> +                                       uint32_t *ecx, uint32_t *edx)
>>  {
>>      uint32_t l3_cores;
>>      assert(cache->size == cache->line_size * cache->associativity *
>> @@ -408,10 +409,12 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo 
>> *cache, CPUState *cs,
>>  
>>      /* L3 is shared among multiple cores */
>>      if (cache->level == 3) {
>> -        l3_cores = cores_in_core_complex(cs->nr_cores);
>> -        *eax |= ((l3_cores * cs->nr_threads) - 1) << 14;
>> +        l3_cores = DIV_ROUND_UP((topo_info->cores_per_die *
>> +                                 topo_info->threads_per_core),
>> +                                 topo_info->dies_per_pkg);
> 
> from spec:
> "
> NumSharingCache: number of '''logical processors''' sharing cache.
> "
> 
> s/l3_cores/l3_vcpus|l3_threads/
> 

Sure.

> Also why not use just:
> 
>   val = topo_info->cores_per_die * topo_info->threads_per_core

Yes. You are right. Will correct it. thanks

> 
> 
> 
>> +        *eax |= (l3_cores - 1) << 14;
>>      } else {
>> -        *eax |= ((cs->nr_threads - 1) << 14);
>> +        *eax |= ((topo_info->threads_per_core - 1) << 14);
>>      }
>>  
>>      assert(cache->line_size > 0);
>> @@ -5994,20 +5997,20 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
>> uint32_t count,
>>          }
>>          switch (count) {
>>          case 0: /* L1 dcache info */
>> -            encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache, cs,
>> -                                       eax, ebx, ecx, edx);
>> +            encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache,
>> +                                       &topo_info, eax, ebx, ecx, edx);
>>              break;
>>          case 1: /* L1 icache info */
>> -            encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache, cs,
>> -                                       eax, ebx, ecx, edx);
>> +            encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache,
>> +                                       &topo_info, eax, ebx, ecx, edx);
>>              break;
>>          case 2: /* L2 cache info */
>> -            encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache, cs,
>> -                                       eax, ebx, ecx, edx);
>> +            encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache,
>> +                                       &topo_info, eax, ebx, ecx, edx);
>>              break;
>>          case 3: /* L3 cache info */
>> -            encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache, cs,
>> -                                       eax, ebx, ecx, edx);
>> +            encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache,
>> +                                       &topo_info, eax, ebx, ecx, edx);
>>              break;
>>          default: /* end of info */
>>              *eax = *ebx = *ecx = *edx = 0;
>>
>