Re: [PATCH] i386/cpu: Adjust cpuid addresable id count to match the spec

[Ilya Oleinik]

I'm not aware of any bug reports. L1 cache is typically shared between logical threads, so it seemed reasonable to correct this.

On Fri, Oct 28, 2022 at 9:54 AM Wang, Lei <lei4.wang@intel.com> wrote:

On 10/10/2022 10:49 AM, Ilya Oleinik wrote:
> For EBX bits 23 - 16 in CPUID[01] Intel's manual states:
> "
> *   The nearest power-of-2 integer that is not smaller than EBX[23:16]
>     is the number of unique initial APICIDs reserved for addressing
>     different logical processors in a physical package. This field is
>     only valid if CPUID.1.EDX.HTT[bit 28]= 1.
> "
> Ensure this condition is met.
>
> Additionally, apply efb3934adf9ee7794db7e0ade9f576c634592891 to
> non passthrough cache mode.
>
> Signed-off-by: Ilya Oleinik <ilyaoleinik1@gmail.com>
> ---
>  target/i386/cpu.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index ad623d91e4..e793bcc03f 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -245,8 +245,8 @@ static void encode_cache_cpuid4(CPUCacheInfo *cache,
>      *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);
> +           ((pow2ceil(num_cores) - 1) << 26) |
> +           ((pow2ceil(num_apic_ids) - 1) << 14);
> 
>      assert(cache->line_size > 0);
>      assert(cache->partitions > 0);
> @@ -5258,7 +5258,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          }
>          *edx = env->features[FEAT_1_EDX];
>          if (cs->nr_cores * cs->nr_threads > 1) {
> -            *ebx |= (cs->nr_cores * cs->nr_threads) << 16;
> +            *ebx |= (pow2ceil(cs->nr_cores * cs->nr_threads)) << 16;
>              *edx |= CPUID_HT;
>          }
>          if (!cpu->enable_pmu) {
> @@ -5313,12 +5313,12 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>              switch (count) {
>              case 0: /* L1 dcache info */
>                  encode_cache_cpuid4(env->cache_info_cpuid4.l1d_cache,
> -                                    1, cs->nr_cores,

Hi Ilya,

Just curious why the origin implementation is hard-coded to 1 and is there any
bug reported related to this?

> +                                    cs->nr_threads, cs->nr_cores,
>                                      eax, ebx, ecx, edx);
>                  break;
>              case 1: /* L1 icache info */
>                  encode_cache_cpuid4(env->cache_info_cpuid4.l1i_cache,
> -                                    1, cs->nr_cores,
> +                                    cs->nr_threads, cs->nr_cores,
>                                      eax, ebx, ecx, edx);
>                  break;
>              case 2: /* L2 cache info */