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  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 
> > ---
> >  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 */
>

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

[Wang, Lei]

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 
> ---
>  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 */

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

[Ilya Oleinik]

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 
---
 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,
+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 */
-- 
2.37.2.windows.2