[v6] i386/cpu: fixup number of addressable IDs for logical processors in the physical package

Commit Message

Chuang Xu Oct. 9, 2024, 3:56 a.m. UTC

When QEMU is started with:
-cpu host,migratable=on,host-cache-info=on,l3-cache=off
-smp 180,sockets=2,dies=1,cores=45,threads=2

On Intel platform:
CPUID.01H.EBX[23:16] is defined as "max number of addressable IDs for
logical processors in the physical package".

When executing "cpuid -1 -l 1 -r" in the guest, we obtain a value of 90 for
CPUID.01H.EBX[23:16], whereas the expected value is 128. Additionally,
executing "cpuid -1 -l 4 -r" in the guest yields a value of 63 for
CPUID.04H.EAX[31:26], which matches the expected result.

As (1+CPUID.04H.EAX[31:26]) rounds up to the nearest power-of-2 integer,
we'd beter round up CPUID.01H.EBX[23:16] to the nearest power-of-2
integer too. Otherwise we may encounter unexpected results in guest.

For example, when QEMU is started with CLI above and xtopology is disabled,
guest kernel 5.15.120 uses CPUID.01H.EBX[23:16]/(1+CPUID.04H.EAX[31:26]) to
calculate threads-per-core in detect_ht(). Then guest will get "90/(1+63)=1"
as the result, even though threads-per-core should actually be 2.

And on AMD platform:
CPUID.01H.EBX[23:16] is defined as "Logical processor count". Current
result meets our expectation.

So let us round up CPUID.01H.EBX[23:16] to the nearest power-of-2 integer
only for Intel platform to solve the unexpected result.

Reviewed-by: Zhao Liu <[e-mail]>
Acked-by: Igor Mammedov <[e-mail]>
Signed-off-by: Guixiong Wei <[e-mail]>
Signed-off-by: Yipeng Yin <[e-mail]>
Signed-off-by: Chuang Xu <[e-mail]>
---
 target/i386/cpu.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Zhao Liu Oct. 9, 2024, 4:21 a.m. UTC | #1

On Wed, Oct 09, 2024 at 11:56:38AM +0800, Chuang Xu wrote:
> Date: Wed,  9 Oct 2024 11:56:38 +0800
> From: Chuang Xu <[e-mail]>
> Subject: [PATCH v6] i386/cpu: fixup number of addressable IDs for logical
>  processors in the physical package
> X-Mailer: git-send-email 2.39.3 (Apple Git-146)
> 
> When QEMU is started with:
> -cpu host,migratable=on,host-cache-info=on,l3-cache=off
> -smp 180,sockets=2,dies=1,cores=45,threads=2
> 
> On Intel platform:
> CPUID.01H.EBX[23:16] is defined as "max number of addressable IDs for
> logical processors in the physical package".
> 
> When executing "cpuid -1 -l 1 -r" in the guest, we obtain a value of 90 for
> CPUID.01H.EBX[23:16], whereas the expected value is 128. Additionally,
> executing "cpuid -1 -l 4 -r" in the guest yields a value of 63 for
> CPUID.04H.EAX[31:26], which matches the expected result.
> 
> As (1+CPUID.04H.EAX[31:26]) rounds up to the nearest power-of-2 integer,
> we'd beter round up CPUID.01H.EBX[23:16] to the nearest power-of-2
> integer too. Otherwise we may encounter unexpected results in guest.
> 
> For example, when QEMU is started with CLI above and xtopology is disabled,
> guest kernel 5.15.120 uses CPUID.01H.EBX[23:16]/(1+CPUID.04H.EAX[31:26]) to
> calculate threads-per-core in detect_ht(). Then guest will get "90/(1+63)=1"
> as the result, even though threads-per-core should actually be 2.
> 
> And on AMD platform:
> CPUID.01H.EBX[23:16] is defined as "Logical processor count". Current
> result meets our expectation.
> 
> So let us round up CPUID.01H.EBX[23:16] to the nearest power-of-2 integer
> only for Intel platform to solve the unexpected result.
> 
> Reviewed-by: Zhao Liu <[e-mail]>
> Acked-by: Igor Mammedov <[e-mail]>
> Signed-off-by: Guixiong Wei <[e-mail]>
> Signed-off-by: Yipeng Yin <[e-mail]>
> Signed-off-by: Chuang Xu <[e-mail]>
> ---
>  target/i386/cpu.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)

This version is fine for me, thanks.

-Zhao

Xiaoyao Li Oct. 12, 2024, 7:13 a.m. UTC | #2

On 10/9/2024 11:56 AM, Chuang Xu wrote:
> When QEMU is started with:
> -cpu host,migratable=on,host-cache-info=on,l3-cache=off
> -smp 180,sockets=2,dies=1,cores=45,threads=2
> 
> On Intel platform:
> CPUID.01H.EBX[23:16] is defined as "max number of addressable IDs for
> logical processors in the physical package".
> 
> When executing "cpuid -1 -l 1 -r" in the guest, we obtain a value of 90 for
> CPUID.01H.EBX[23:16], whereas the expected value is 128. Additionally,
> executing "cpuid -1 -l 4 -r" in the guest yields a value of 63 for
> CPUID.04H.EAX[31:26], which matches the expected result.
> 
> As (1+CPUID.04H.EAX[31:26]) rounds up to the nearest power-of-2 integer,
> we'd beter round up CPUID.01H.EBX[23:16] to the nearest power-of-2
> integer too. Otherwise we may encounter unexpected results in guest.
> 
> For example, when QEMU is started with CLI above and xtopology is disabled,
> guest kernel 5.15.120 uses CPUID.01H.EBX[23:16]/(1+CPUID.04H.EAX[31:26]) to
> calculate threads-per-core in detect_ht(). Then guest will get "90/(1+63)=1"
> as the result, even though threads-per-core should actually be 2.
> 
> And on AMD platform:
> CPUID.01H.EBX[23:16] is defined as "Logical processor count". Current
> result meets our expectation.

So for AMD platform, what's result for the same situation with xtopology 
disabled? Does AMD uses another algorithm to calculate other than 
CPUID.01H.EBX[23:16]/(1+CPUID.04H.EAX[31:26]) ?

> So let us round up CPUID.01H.EBX[23:16] to the nearest power-of-2 integer
> only for Intel platform to solve the unexpected result.
> 
> Reviewed-by: Zhao Liu <[e-mail]>
> Acked-by: Igor Mammedov <[e-mail]>
> Signed-off-by: Guixiong Wei <[e-mail]>
> Signed-off-by: Yipeng Yin <[e-mail]>
> Signed-off-by: Chuang Xu <[e-mail]>
> ---
>   target/i386/cpu.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index ff227a8c5c..641d4577b0 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6462,7 +6462,15 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>           }
>           *edx = env->features[FEAT_1_EDX];
>           if (threads_per_pkg > 1) {
> -            *ebx |= threads_per_pkg << 16;
> +            /*
> +             * AMD requires logical processor count, but Intel needs maximum
> +             * number of addressable IDs for logical processors per package.
> +             */
> +            if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) {
> +                *ebx |= threads_per_pkg << 16;
> +            } else {
> +                *ebx |= 1 << apicid_pkg_offset(&topo_info) << 16;
> +            }

you need to handle the overflow case when the number of logical 
processors > 255.

>               *edx |= CPUID_HT;
>           }
>           if (!cpu->enable_pmu) {

Chuang Xu Oct. 12, 2024, 8:10 a.m. UTC | #3

Hi, Xiaoyao

On 10/12/24 下午3:13, Xiaoyao Li wrote:
> On 10/9/2024 11:56 AM, Chuang Xu wrote:
>> When QEMU is started with:
>> -cpu host,migratable=on,host-cache-info=on,l3-cache=off
>> -smp 180,sockets=2,dies=1,cores=45,threads=2
>>
>> On Intel platform:
>> CPUID.01H.EBX[23:16] is defined as "max number of addressable IDs for
>> logical processors in the physical package".
>>
>> When executing "cpuid -1 -l 1 -r" in the guest, we obtain a value of 
>> 90 for
>> CPUID.01H.EBX[23:16], whereas the expected value is 128. Additionally,
>> executing "cpuid -1 -l 4 -r" in the guest yields a value of 63 for
>> CPUID.04H.EAX[31:26], which matches the expected result.
>>
>> As (1+CPUID.04H.EAX[31:26]) rounds up to the nearest power-of-2 integer,
>> we'd beter round up CPUID.01H.EBX[23:16] to the nearest power-of-2
>> integer too. Otherwise we may encounter unexpected results in guest.
>>
>> For example, when QEMU is started with CLI above and xtopology is 
>> disabled,
>> guest kernel 5.15.120 uses 
>> CPUID.01H.EBX[23:16]/(1+CPUID.04H.EAX[31:26]) to
>> calculate threads-per-core in detect_ht(). Then guest will get 
>> "90/(1+63)=1"
>> as the result, even though threads-per-core should actually be 2.
>>
>> And on AMD platform:
>> CPUID.01H.EBX[23:16] is defined as "Logical processor count". Current
>> result meets our expectation.
>
> So for AMD platform, what's result for the same situation with 
> xtopology disabled? Does AMD uses another algorithm to calculate other 
> than CPUID.01H.EBX[23:16]/(1+CPUID.04H.EAX[31:26]) ?
>
For amd platform, CPUID.04H is reserved, so it uses 
CPUID.8000001E.EAX[15:8] (fied ThreadsPerComputeUnit) to obtain the result.
>> So let us round up CPUID.01H.EBX[23:16] to the nearest power-of-2 
>> integer
>> only for Intel platform to solve the unexpected result.
>>
>> Reviewed-by: Zhao Liu <[e-mail]>
>> Acked-by: Igor Mammedov <[e-mail]>
>> Signed-off-by: Guixiong Wei <[e-mail]>
>> Signed-off-by: Yipeng Yin <[e-mail]>
>> Signed-off-by: Chuang Xu <[e-mail]>
>> ---
>>   target/i386/cpu.c | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index ff227a8c5c..641d4577b0 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -6462,7 +6462,15 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t 
>> index, uint32_t count,
>>           }
>>           *edx = env->features[FEAT_1_EDX];
>>           if (threads_per_pkg > 1) {
>> -            *ebx |= threads_per_pkg << 16;
>> +            /*
>> +             * AMD requires logical processor count, but Intel needs 
>> maximum
>> +             * number of addressable IDs for logical processors per 
>> package.
>> +             */
>> +            if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) {
>> +                *ebx |= threads_per_pkg << 16;
>> +            } else {
>> +                *ebx |= 1 << apicid_pkg_offset(&topo_info) << 16;
>> +            }
>
> you need to handle the overflow case when the number of logical 
> processors > 255.
>
It seems other cpuid cases of bit shifting don't condiser the overflow 
case too..

Since intel only reserves 8bits for this field, do you have any 
suggestions to make sure this field emulated

correctly?

>>               *edx |= CPUID_HT;
>>           }
>>           if (!cpu->enable_pmu) {
>

Xiaoyao Li Oct. 12, 2024, 8:21 a.m. UTC | #4

On 10/9/2024 11:56 AM, Chuang Xu wrote:
> When QEMU is started with:
> -cpu host,migratable=on,host-cache-info=on,l3-cache=off
> -smp 180,sockets=2,dies=1,cores=45,threads=2
> 
> On Intel platform:
> CPUID.01H.EBX[23:16] is defined as "max number of addressable IDs for
> logical processors in the physical package".
> 
> When executing "cpuid -1 -l 1 -r" in the guest, we obtain a value of 90 for
> CPUID.01H.EBX[23:16], whereas the expected value is 128. Additionally,
> executing "cpuid -1 -l 4 -r" in the guest yields a value of 63 for
> CPUID.04H.EAX[31:26], which matches the expected result.
> 
> As (1+CPUID.04H.EAX[31:26]) rounds up to the nearest power-of-2 integer,
> we'd beter round up CPUID.01H.EBX[23:16] to the nearest power-of-2
> integer too. Otherwise we may encounter unexpected results in guest.
> 
> For example, when QEMU is started with CLI above and xtopology is disabled,
> guest kernel 5.15.120 uses CPUID.01H.EBX[23:16]/(1+CPUID.04H.EAX[31:26]) to
> calculate threads-per-core in detect_ht(). Then guest will get "90/(1+63)=1"
> as the result, even though threads-per-core should actually be 2.

It's kernel's bug instead.

In 1.5.3 "Sub ID Extraction Parameters for initial APIC ID" of "Intel 64 
Architecture Processor Topology Enumeration" [1], it is

   - SMT_Mask_Width = Log2(RoundToNearestPof2(CPUID.1:EBX[23:16])/ 
(CPUID.(EAX=4,ECX=0):EAX[31:26]) + 1))

The value of CPUID.1:EBX[23:16] needs to be *rounded* to the neartest 
power-of-two integer instead of itself being the power-of-two.

This also is consistency with the SDM, where the comment for bit 23-16 
of CPUID.1:EBX is:

   The nearest power-of-2 integer that is not smaller than EBX[23:16] is
   the number of unique initial APIC IDs reserved for addressing
   different logical processors in a physical package.

What I read from this is, the nearest power-of-2 integer that is not 
smaller than EBX[23:16] is a different thing than EBX[23:16]. i.e.,

- EBX[23:16]: Maximum number of addressable IDs for logical processors
   in this physical package

- pow2ceil(EBX[23:16]): the number of unique initial APIC IDs reserved
   for addressing different logical processors in a physical package.

[1] 
https://cdrdv2-public.intel.com/759067/intel-64-architecture-processor-topology-enumeration.pdf

> And on AMD platform:
> CPUID.01H.EBX[23:16] is defined as "Logical processor count". Current
> result meets our expectation.
> 
> So let us round up CPUID.01H.EBX[23:16] to the nearest power-of-2 integer
> only for Intel platform to solve the unexpected result.
> 
> Reviewed-by: Zhao Liu <[e-mail]>
> Acked-by: Igor Mammedov <[e-mail]>
> Signed-off-by: Guixiong Wei <[e-mail]>
> Signed-off-by: Yipeng Yin <[e-mail]>
> Signed-off-by: Chuang Xu <[e-mail]>
> ---
>   target/i386/cpu.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index ff227a8c5c..641d4577b0 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6462,7 +6462,15 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>           }
>           *edx = env->features[FEAT_1_EDX];
>           if (threads_per_pkg > 1) {
> -            *ebx |= threads_per_pkg << 16;
> +            /*
> +             * AMD requires logical processor count, but Intel needs maximum
> +             * number of addressable IDs for logical processors per package.
> +             */
> +            if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) {
> +                *ebx |= threads_per_pkg << 16;
> +            } else {
> +                *ebx |= 1 << apicid_pkg_offset(&topo_info) << 16;
> +            }
>               *edx |= CPUID_HT;
>           }
>           if (!cpu->enable_pmu) {

Xiaoyao Li Oct. 12, 2024, 8:32 a.m. UTC | #5

On 10/12/2024 4:10 PM, Chuang Xu wrote:
> Hi, Xiaoyao
> 
> On 10/12/24 下午3:13, Xiaoyao Li wrote:
>> On 10/9/2024 11:56 AM, Chuang Xu wrote:
>>> When QEMU is started with:
>>> -cpu host,migratable=on,host-cache-info=on,l3-cache=off
>>> -smp 180,sockets=2,dies=1,cores=45,threads=2
>>>
>>> On Intel platform:
>>> CPUID.01H.EBX[23:16] is defined as "max number of addressable IDs for
>>> logical processors in the physical package".
>>>
>>> When executing "cpuid -1 -l 1 -r" in the guest, we obtain a value of 
>>> 90 for
>>> CPUID.01H.EBX[23:16], whereas the expected value is 128. Additionally,
>>> executing "cpuid -1 -l 4 -r" in the guest yields a value of 63 for
>>> CPUID.04H.EAX[31:26], which matches the expected result.
>>>
>>> As (1+CPUID.04H.EAX[31:26]) rounds up to the nearest power-of-2 integer,
>>> we'd beter round up CPUID.01H.EBX[23:16] to the nearest power-of-2
>>> integer too. Otherwise we may encounter unexpected results in guest.
>>>
>>> For example, when QEMU is started with CLI above and xtopology is 
>>> disabled,
>>> guest kernel 5.15.120 uses CPUID.01H.EBX[23:16]/ 
>>> (1+CPUID.04H.EAX[31:26]) to
>>> calculate threads-per-core in detect_ht(). Then guest will get "90/ 
>>> (1+63)=1"
>>> as the result, even though threads-per-core should actually be 2.
>>>
>>> And on AMD platform:
>>> CPUID.01H.EBX[23:16] is defined as "Logical processor count". Current
>>> result meets our expectation.
>>
>> So for AMD platform, what's result for the same situation with 
>> xtopology disabled? Does AMD uses another algorithm to calculate other 
>> than CPUID.01H.EBX[23:16]/(1+CPUID.04H.EAX[31:26]) ?
>>
> For amd platform, CPUID.04H is reserved, so it uses 
> CPUID.8000001E.EAX[15:8] (fied ThreadsPerComputeUnit) to obtain the result.

Does AMD support leaf 8000001E at the beginning when it starts to 
support multi-threads/multi-cores? (just my curiosity)

>>> So let us round up CPUID.01H.EBX[23:16] to the nearest power-of-2 
>>> integer
>>> only for Intel platform to solve the unexpected result.
>>>
>>> Reviewed-by: Zhao Liu <[e-mail]>
>>> Acked-by: Igor Mammedov <[e-mail]>
>>> Signed-off-by: Guixiong Wei <[e-mail]>
>>> Signed-off-by: Yipeng Yin <[e-mail]>
>>> Signed-off-by: Chuang Xu <[e-mail]>
>>> ---
>>>   target/i386/cpu.c | 10 +++++++++-
>>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>>> index ff227a8c5c..641d4577b0 100644
>>> --- a/target/i386/cpu.c
>>> +++ b/target/i386/cpu.c
>>> @@ -6462,7 +6462,15 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t 
>>> index, uint32_t count,
>>>           }
>>>           *edx = env->features[FEAT_1_EDX];
>>>           if (threads_per_pkg > 1) {
>>> -            *ebx |= threads_per_pkg << 16;
>>> +            /*
>>> +             * AMD requires logical processor count, but Intel needs 
>>> maximum
>>> +             * number of addressable IDs for logical processors per 
>>> package.
>>> +             */
>>> +            if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) {
>>> +                *ebx |= threads_per_pkg << 16;
>>> +            } else {
>>> +                *ebx |= 1 << apicid_pkg_offset(&topo_info) << 16;
>>> +            }
>>
>> you need to handle the overflow case when the number of logical 
>> processors > 255.
>>
> It seems other cpuid cases of bit shifting don't condiser the overflow 
> case too..
> 
> Since intel only reserves 8bits for this field, do you have any 
> suggestions to make sure this field emulated
> 
> correctly?

the usual option can be masking the value to only 8 bits before 
shifting, like

	((1 << apicid_pkg_offset(&topo_info)) & 0xff) << 16

but when the value is greater than 255, it will be truncated, so we need 
something like below to reflect the hardware behavior:

	MIN((1 << apicid_pkg_offset(&topo_info)), 255) << 16

This is what Qian's patch [1] wanted to fix last year, but that patch 
never gets merged.

[1] 
https://lore.kernel.org/qemu-devel/[e-mail]/

>>>               *edx |= CPUID_HT;
>>>           }
>>>           if (!cpu->enable_pmu) {
>>

Zhao Liu Oct. 12, 2024, 8:56 a.m. UTC | #6

> > > > +            if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) {
> > > > +                *ebx |= threads_per_pkg << 16;
> > > > +            } else {
> > > > +                *ebx |= 1 << apicid_pkg_offset(&topo_info) << 16;
> > > > +            }
> > > 
> > > you need to handle the overflow case when the number of logical
> > > processors > 255.
> > > 
> > It seems other cpuid cases of bit shifting don't condiser the overflow
> > case too..
> > 
> > Since intel only reserves 8bits for this field, do you have any
> > suggestions to make sure this field emulated
> > 
> > correctly?
> 
> the usual option can be masking the value to only 8 bits before shifting,
> like
> 
> 	((1 << apicid_pkg_offset(&topo_info)) & 0xff) << 16
> 
> but when the value is greater than 255, it will be truncated, so we need
> something like below to reflect the hardware behavior:
> 
> 	MIN((1 << apicid_pkg_offset(&topo_info)), 255) << 16
> 
> This is what Qian's patch [1] wanted to fix last year, but that patch never
> gets merged.
> 
> [1] https://lore.kernel.org/qemu-devel/[e-mail]/
> 

That's on my list. I had the plan to help Qian pick up it again and
rebase that series on Chuang's change.

-Zhao

Zhao Liu Oct. 12, 2024, 9:28 a.m. UTC | #7

> This also is consistency with the SDM, where the comment for bit 23-16 of
> CPUID.1:EBX is:
> 
>   The nearest power-of-2 integer that is not smaller than EBX[23:16] is
>   the number of unique initial APIC IDs reserved for addressing
>   different logical processors in a physical package.
> 
> What I read from this is, the nearest power-of-2 integer that is not smaller
> than EBX[23:16] is a different thing than EBX[23:16]. i.e.,

“not smaller” means “greater than or equal to” (≥).

Chuang Xu Oct. 12, 2024, 9:35 a.m. UTC | #8

On 10/12/24 下午4:21, Xiaoyao Li wrote:
> On 10/9/2024 11:56 AM, Chuang Xu wrote:
>> When QEMU is started with:
>> -cpu host,migratable=on,host-cache-info=on,l3-cache=off
>> -smp 180,sockets=2,dies=1,cores=45,threads=2
>>
>> On Intel platform:
>> CPUID.01H.EBX[23:16] is defined as "max number of addressable IDs for
>> logical processors in the physical package".
>>
>> When executing "cpuid -1 -l 1 -r" in the guest, we obtain a value of 
>> 90 for
>> CPUID.01H.EBX[23:16], whereas the expected value is 128. Additionally,
>> executing "cpuid -1 -l 4 -r" in the guest yields a value of 63 for
>> CPUID.04H.EAX[31:26], which matches the expected result.
>>
>> As (1+CPUID.04H.EAX[31:26]) rounds up to the nearest power-of-2 integer,
>> we'd beter round up CPUID.01H.EBX[23:16] to the nearest power-of-2
>> integer too. Otherwise we may encounter unexpected results in guest.
>>
>> For example, when QEMU is started with CLI above and xtopology is 
>> disabled,
>> guest kernel 5.15.120 uses 
>> CPUID.01H.EBX[23:16]/(1+CPUID.04H.EAX[31:26]) to
>> calculate threads-per-core in detect_ht(). Then guest will get 
>> "90/(1+63)=1"
>> as the result, even though threads-per-core should actually be 2.
>
> It's kernel's bug instead.
>
> In 1.5.3 "Sub ID Extraction Parameters for initial APIC ID" of "Intel 
> 64 Architecture Processor Topology Enumeration" [1], it is
>
>   - SMT_Mask_Width = Log2(RoundToNearestPof2(CPUID.1:EBX[23:16])/ 
> (CPUID.(EAX=4,ECX=0):EAX[31:26]) + 1))
>
> The value of CPUID.1:EBX[23:16] needs to be *rounded* to the neartest 
> power-of-two integer instead of itself being the power-of-two.
>
> This also is consistency with the SDM, where the comment for bit 23-16 
> of CPUID.1:EBX is:
>
>   The nearest power-of-2 integer that is not smaller than EBX[23:16] is
>   the number of unique initial APIC IDs reserved for addressing
>   different logical processors in a physical package.
>
> What I read from this is, the nearest power-of-2 integer that is not 
> smaller than EBX[23:16] is a different thing than EBX[23:16]. i.e.,

Yes, when I read sdm, I also thought it was a kernel bug. But on my 
192ht spr host, the value of CPUID.1:EBX[23:16] was indeed rounded up

to the nearest power of 2 by the hardware. After communicating with 
Intel technical support staff, we thought that perhaps the description 
in sdm

is not so accurate, and rounding up CPUID.1:EBX[23:16] to the power of 2 
in qemu may be more in line with the hardware behavior.

>
> - EBX[23:16]: Maximum number of addressable IDs for logical processors
>   in this physical package
>
> - pow2ceil(EBX[23:16]): the number of unique initial APIC IDs reserved
>   for addressing different logical processors in a physical package.
>
> [1] 
> https://cdrdv2-public.intel.com/759067/intel-64-architecture-processor-topology-enumeration.pdf
>
>> And on AMD platform:
>> CPUID.01H.EBX[23:16] is defined as "Logical processor count". Current
>> result meets our expectation.
>>
>> So let us round up CPUID.01H.EBX[23:16] to the nearest power-of-2 
>> integer
>> only for Intel platform to solve the unexpected result.
>>
>> Reviewed-by: Zhao Liu <[e-mail]>
>> Acked-by: Igor Mammedov <[e-mail]>
>> Signed-off-by: Guixiong Wei <[e-mail]>
>> Signed-off-by: Yipeng Yin <[e-mail]>
>> Signed-off-by: Chuang Xu <[e-mail]>
>> ---
>>   target/i386/cpu.c | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index ff227a8c5c..641d4577b0 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -6462,7 +6462,15 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t 
>> index, uint32_t count,
>>           }
>>           *edx = env->features[FEAT_1_EDX];
>>           if (threads_per_pkg > 1) {
>> -            *ebx |= threads_per_pkg << 16;
>> +            /*
>> +             * AMD requires logical processor count, but Intel needs 
>> maximum
>> +             * number of addressable IDs for logical processors per 
>> package.
>> +             */
>> +            if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) {
>> +                *ebx |= threads_per_pkg << 16;
>> +            } else {
>> +                *ebx |= 1 << apicid_pkg_offset(&topo_info) << 16;
>> +            }
>>               *edx |= CPUID_HT;
>>           }
>>           if (!cpu->enable_pmu) {
>

Xiaoyao Li Oct. 14, 2024, 12:36 a.m. UTC | #9

On 10/12/2024 5:35 PM, Chuang Xu wrote:
> 
> On 10/12/24 下午4:21, Xiaoyao Li wrote:
>> On 10/9/2024 11:56 AM, Chuang Xu wrote:
>>> When QEMU is started with:
>>> -cpu host,migratable=on,host-cache-info=on,l3-cache=off
>>> -smp 180,sockets=2,dies=1,cores=45,threads=2
>>>
>>> On Intel platform:
>>> CPUID.01H.EBX[23:16] is defined as "max number of addressable IDs for
>>> logical processors in the physical package".
>>>
>>> When executing "cpuid -1 -l 1 -r" in the guest, we obtain a value of 
>>> 90 for
>>> CPUID.01H.EBX[23:16], whereas the expected value is 128. Additionally,
>>> executing "cpuid -1 -l 4 -r" in the guest yields a value of 63 for
>>> CPUID.04H.EAX[31:26], which matches the expected result.
>>>
>>> As (1+CPUID.04H.EAX[31:26]) rounds up to the nearest power-of-2 integer,
>>> we'd beter round up CPUID.01H.EBX[23:16] to the nearest power-of-2
>>> integer too. Otherwise we may encounter unexpected results in guest.
>>>
>>> For example, when QEMU is started with CLI above and xtopology is 
>>> disabled,
>>> guest kernel 5.15.120 uses CPUID.01H.EBX[23:16]/ 
>>> (1+CPUID.04H.EAX[31:26]) to
>>> calculate threads-per-core in detect_ht(). Then guest will get "90/ 
>>> (1+63)=1"
>>> as the result, even though threads-per-core should actually be 2.
>>
>> It's kernel's bug instead.
>>
>> In 1.5.3 "Sub ID Extraction Parameters for initial APIC ID" of "Intel 
>> 64 Architecture Processor Topology Enumeration" [1], it is
>>
>>   - SMT_Mask_Width = Log2(RoundToNearestPof2(CPUID.1:EBX[23:16])/ 
>> (CPUID.(EAX=4,ECX=0):EAX[31:26]) + 1))
>>
>> The value of CPUID.1:EBX[23:16] needs to be *rounded* to the neartest 
>> power-of-two integer instead of itself being the power-of-two.
>>
>> This also is consistency with the SDM, where the comment for bit 23-16 
>> of CPUID.1:EBX is:
>>
>>   The nearest power-of-2 integer that is not smaller than EBX[23:16] is
>>   the number of unique initial APIC IDs reserved for addressing
>>   different logical processors in a physical package.
>>
>> What I read from this is, the nearest power-of-2 integer that is not 
>> smaller than EBX[23:16] is a different thing than EBX[23:16]. i.e.,
> 
> Yes, when I read sdm, I also thought it was a kernel bug. But on my 
> 192ht spr host, the value of CPUID.1:EBX[23:16] was indeed rounded up
> 
> to the nearest power of 2 by the hardware. After communicating with 
> Intel technical support staff, we thought that perhaps the description 
> in sdm
> 
> is not so accurate, and rounding up CPUID.1:EBX[23:16] to the power of 2 
> in qemu may be more in line with the hardware behavior.

I think above justification is important. We need to justify our changes 
with the fact and correct reason.

I somehow agree to set EBX[23:16] to a value of power-of-2, because the 
1.5.3 "Sub ID Extraction Parameters for initial APIC ID" of "Intel 64 
Architecture Processor Topology Enumeration" spec says

     CPUID.1:EBX[23:16] represents the maximum number of addressable IDs
     (initial APIC ID) that can be assigned to logical processors in a
     physical package. The value may not be the same as the number of
     logical processors that are present in the hardware of a physical
     package.

It uses the word "may not".

However, the justification of the change cannot be "it leads to 
unexpected results in guest" because the guest implementation is not 
correct.

>>
>> - EBX[23:16]: Maximum number of addressable IDs for logical processors
>>   in this physical package
>>
>> - pow2ceil(EBX[23:16]): the number of unique initial APIC IDs reserved
>>   for addressing different logical processors in a physical package.
>>
>> [1] https://cdrdv2-public.intel.com/759067/intel-64-architecture- 
>> processor-topology-enumeration.pdf
>>
>>> And on AMD platform:
>>> CPUID.01H.EBX[23:16] is defined as "Logical processor count". Current
>>> result meets our expectation.
>>>
>>> So let us round up CPUID.01H.EBX[23:16] to the nearest power-of-2 
>>> integer
>>> only for Intel platform to solve the unexpected result.
>>>
>>> Reviewed-by: Zhao Liu <[e-mail]>
>>> Acked-by: Igor Mammedov <[e-mail]>
>>> Signed-off-by: Guixiong Wei <[e-mail]>
>>> Signed-off-by: Yipeng Yin <[e-mail]>
>>> Signed-off-by: Chuang Xu <[e-mail]>
>>> ---
>>>   target/i386/cpu.c | 10 +++++++++-
>>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>>> index ff227a8c5c..641d4577b0 100644
>>> --- a/target/i386/cpu.c
>>> +++ b/target/i386/cpu.c
>>> @@ -6462,7 +6462,15 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t 
>>> index, uint32_t count,
>>>           }
>>>           *edx = env->features[FEAT_1_EDX];
>>>           if (threads_per_pkg > 1) {
>>> -            *ebx |= threads_per_pkg << 16;
>>> +            /*
>>> +             * AMD requires logical processor count, but Intel needs 
>>> maximum
>>> +             * number of addressable IDs for logical processors per 
>>> package.
>>> +             */
>>> +            if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) {
>>> +                *ebx |= threads_per_pkg << 16;
>>> +            } else {
>>> +                *ebx |= 1 << apicid_pkg_offset(&topo_info) << 16;
>>> +            }
>>>               *edx |= CPUID_HT;
>>>           }
>>>           if (!cpu->enable_pmu) {
>>

Xiaoyao Li Oct. 14, 2024, 1:32 a.m. UTC | #10

On 10/14/2024 8:36 AM, Xiaoyao Li wrote:
> On 10/12/2024 5:35 PM, Chuang Xu wrote:
>>
>> On 10/12/24 下午4:21, Xiaoyao Li wrote:
>>> On 10/9/2024 11:56 AM, Chuang Xu wrote:
>>>> When QEMU is started with:
>>>> -cpu host,migratable=on,host-cache-info=on,l3-cache=off
>>>> -smp 180,sockets=2,dies=1,cores=45,threads=2
>>>>
>>>> On Intel platform:
>>>> CPUID.01H.EBX[23:16] is defined as "max number of addressable IDs for
>>>> logical processors in the physical package".
>>>>
>>>> When executing "cpuid -1 -l 1 -r" in the guest, we obtain a value of 
>>>> 90 for
>>>> CPUID.01H.EBX[23:16], whereas the expected value is 128. Additionally,
>>>> executing "cpuid -1 -l 4 -r" in the guest yields a value of 63 for
>>>> CPUID.04H.EAX[31:26], which matches the expected result.
>>>>
>>>> As (1+CPUID.04H.EAX[31:26]) rounds up to the nearest power-of-2 
>>>> integer,
>>>> we'd beter round up CPUID.01H.EBX[23:16] to the nearest power-of-2
>>>> integer too. Otherwise we may encounter unexpected results in guest.
>>>>
>>>> For example, when QEMU is started with CLI above and xtopology is 
>>>> disabled,
>>>> guest kernel 5.15.120 uses CPUID.01H.EBX[23:16]/ 
>>>> (1+CPUID.04H.EAX[31:26]) to
>>>> calculate threads-per-core in detect_ht(). Then guest will get "90/ 
>>>> (1+63)=1"
>>>> as the result, even though threads-per-core should actually be 2.
>>>
>>> It's kernel's bug instead.
>>>
>>> In 1.5.3 "Sub ID Extraction Parameters for initial APIC ID" of "Intel 
>>> 64 Architecture Processor Topology Enumeration" [1], it is
>>>
>>>   - SMT_Mask_Width = Log2(RoundToNearestPof2(CPUID.1:EBX[23:16])/ 
>>> (CPUID.(EAX=4,ECX=0):EAX[31:26]) + 1))
>>>
>>> The value of CPUID.1:EBX[23:16] needs to be *rounded* to the neartest 
>>> power-of-two integer instead of itself being the power-of-two.
>>>
>>> This also is consistency with the SDM, where the comment for bit 
>>> 23-16 of CPUID.1:EBX is:
>>>
>>>   The nearest power-of-2 integer that is not smaller than EBX[23:16] is
>>>   the number of unique initial APIC IDs reserved for addressing
>>>   different logical processors in a physical package.
>>>
>>> What I read from this is, the nearest power-of-2 integer that is not 
>>> smaller than EBX[23:16] is a different thing than EBX[23:16]. i.e.,
>>
>> Yes, when I read sdm, I also thought it was a kernel bug. But on my 
>> 192ht spr host, the value of CPUID.1:EBX[23:16] was indeed rounded up
>>
>> to the nearest power of 2 by the hardware. After communicating with 
>> Intel technical support staff, we thought that perhaps the description 
>> in sdm
>>
>> is not so accurate, and rounding up CPUID.1:EBX[23:16] to the power of 
>> 2 in qemu may be more in line with the hardware behavior.
> 
> I think above justification is important. We need to justify our changes 
> with the fact and correct reason.
> 
> I somehow agree to set EBX[23:16] to a value of power-of-2, because the 
> 1.5.3 "Sub ID Extraction Parameters for initial APIC ID" of "Intel 64 
> Architecture Processor Topology Enumeration" spec says
> 
>      CPUID.1:EBX[23:16] represents the maximum number of addressable IDs
>      (initial APIC ID) that can be assigned to logical processors in a
>      physical package. The value may not be the same as the number of
>      logical processors that are present in the hardware of a physical
>      package.
> 
> It uses the word "may not".
> 
> However, the justification of the change cannot be "it leads to 
> unexpected results in guest" because the guest implementation is not 
> correct.

FYI, latest linux already fix the issue, it calculates the shift via

   tscan->ebx1_nproc_shift = get_count_order(ebx.nproc);

>>>
>>> - EBX[23:16]: Maximum number of addressable IDs for logical processors
>>>   in this physical package
>>>
>>> - pow2ceil(EBX[23:16]): the number of unique initial APIC IDs reserved
>>>   for addressing different logical processors in a physical package.
>>>
>>> [1] https://cdrdv2-public.intel.com/759067/intel-64-architecture- 
>>> processor-topology-enumeration.pdf
>>>
>>>> And on AMD platform:
>>>> CPUID.01H.EBX[23:16] is defined as "Logical processor count". Current
>>>> result meets our expectation.
>>>>
>>>> So let us round up CPUID.01H.EBX[23:16] to the nearest power-of-2 
>>>> integer
>>>> only for Intel platform to solve the unexpected result.
>>>>
>>>> Reviewed-by: Zhao Liu <[e-mail]>
>>>> Acked-by: Igor Mammedov <[e-mail]>
>>>> Signed-off-by: Guixiong Wei <[e-mail]>
>>>> Signed-off-by: Yipeng Yin <[e-mail]>
>>>> Signed-off-by: Chuang Xu <[e-mail]>
>>>> ---
>>>>   target/i386/cpu.c | 10 +++++++++-
>>>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>>>> index ff227a8c5c..641d4577b0 100644
>>>> --- a/target/i386/cpu.c
>>>> +++ b/target/i386/cpu.c
>>>> @@ -6462,7 +6462,15 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t 
>>>> index, uint32_t count,
>>>>           }
>>>>           *edx = env->features[FEAT_1_EDX];
>>>>           if (threads_per_pkg > 1) {
>>>> -            *ebx |= threads_per_pkg << 16;
>>>> +            /*
>>>> +             * AMD requires logical processor count, but Intel 
>>>> needs maximum
>>>> +             * number of addressable IDs for logical processors per 
>>>> package.
>>>> +             */
>>>> +            if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) {
>>>> +                *ebx |= threads_per_pkg << 16;
>>>> +            } else {
>>>> +                *ebx |= 1 << apicid_pkg_offset(&topo_info) << 16;
>>>> +            }
>>>>               *edx |= CPUID_HT;
>>>>           }
>>>>           if (!cpu->enable_pmu) {
>>>
> 
>

Zhao Liu Oct. 14, 2024, 3:36 a.m. UTC | #11

> > > On 10/9/2024 11:56 AM, Chuang Xu wrote:
> > > > When QEMU is started with:
> > > > -cpu host,migratable=on,host-cache-info=on,l3-cache=off
> > > > -smp 180,sockets=2,dies=1,cores=45,threads=2
> > > > 
> > > > On Intel platform:
> > > > CPUID.01H.EBX[23:16] is defined as "max number of addressable IDs for
> > > > logical processors in the physical package".
> > > > 
> > > > When executing "cpuid -1 -l 1 -r" in the guest, we obtain a
> > > > value of 90 for
> > > > CPUID.01H.EBX[23:16], whereas the expected value is 128. Additionally,
> > > > executing "cpuid -1 -l 4 -r" in the guest yields a value of 63 for
> > > > CPUID.04H.EAX[31:26], which matches the expected result.
> > > > 
> > > > As (1+CPUID.04H.EAX[31:26]) rounds up to the nearest power-of-2 integer,
> > > > we'd beter round up CPUID.01H.EBX[23:16] to the nearest power-of-2
> > > > integer too. Otherwise we may encounter unexpected results in guest.
> > > > 
> > > > For example, when QEMU is started with CLI above and xtopology
> > > > is disabled,
> > > > guest kernel 5.15.120 uses CPUID.01H.EBX[23:16]/
> > > > (1+CPUID.04H.EAX[31:26]) to
> > > > calculate threads-per-core in detect_ht(). Then guest will get
> > > > "90/ (1+63)=1"
> > > > as the result, even though threads-per-core should actually be 2.
> > > 
> > > It's kernel's bug instead.
> > > 
> > > In 1.5.3 "Sub ID Extraction Parameters for initial APIC ID" of
> > > "Intel 64 Architecture Processor Topology Enumeration" [1], it is
> > > 
> > >   - SMT_Mask_Width = Log2(RoundToNearestPof2(CPUID.1:EBX[23:16])/
> > > (CPUID.(EAX=4,ECX=0):EAX[31:26]) + 1))
> > > 
> > > The value of CPUID.1:EBX[23:16] needs to be *rounded* to the
> > > neartest power-of-two integer instead of itself being the
> > > power-of-two.
> > > 
> > > This also is consistency with the SDM, where the comment for bit
> > > 23-16 of CPUID.1:EBX is:
> > > 
> > >   The nearest power-of-2 integer that is not smaller than EBX[23:16] is
> > >   the number of unique initial APIC IDs reserved for addressing
> > >   different logical processors in a physical package.
> > > 
> > > What I read from this is, the nearest power-of-2 integer that is not
> > > smaller than EBX[23:16] is a different thing than EBX[23:16]. i.e.,
> > 
> > Yes, when I read sdm, I also thought it was a kernel bug. But on my
> > 192ht spr host, the value of CPUID.1:EBX[23:16] was indeed rounded up
> > 
> > to the nearest power of 2 by the hardware. After communicating with
> > Intel technical support staff, we thought that perhaps the description
> > in sdm
> > 
> > is not so accurate, and rounding up CPUID.1:EBX[23:16] to the power of 2
> > in qemu may be more in line with the hardware behavior.
> 
> I think above justification is important. We need to justify our changes
> with the fact and correct reason.
> 
> I somehow agree to set EBX[23:16] to a value of power-of-2, because the
> 1.5.3 "Sub ID Extraction Parameters for initial APIC ID" of "Intel 64
> Architecture Processor Topology Enumeration" spec says
> 
>     CPUID.1:EBX[23:16] represents the maximum number of addressable IDs
>     (initial APIC ID) that can be assigned to logical processors in a
>     physical package. The value may not be the same as the number of
>     logical processors that are present in the hardware of a physical
>     package.
> 
> It uses the word "may not".

IMO, I don't quite understand your confusion regarding this. I've already
explained the meaning of addressable ID, and the spec you referenced also
clarifies its significance. The reason for this modification is not
because of the two words "may not".

Whether it is "be" or "not be" the same as the number of logical
processors, the essence is that due to topology, the actual number of
initial IDs that can be accommodated in the APIC ID may exceed the number
of logical processors.

Xiaoyao Li Oct. 17, 2024, 8:18 a.m. UTC | #12

On 10/14/2024 11:36 AM, Zhao Liu wrote:
>>>> On 10/9/2024 11:56 AM, Chuang Xu wrote:
>>>>> When QEMU is started with:
>>>>> -cpu host,migratable=on,host-cache-info=on,l3-cache=off
>>>>> -smp 180,sockets=2,dies=1,cores=45,threads=2
>>>>>
>>>>> On Intel platform:
>>>>> CPUID.01H.EBX[23:16] is defined as "max number of addressable IDs for
>>>>> logical processors in the physical package".
>>>>>
>>>>> When executing "cpuid -1 -l 1 -r" in the guest, we obtain a
>>>>> value of 90 for
>>>>> CPUID.01H.EBX[23:16], whereas the expected value is 128. Additionally,
>>>>> executing "cpuid -1 -l 4 -r" in the guest yields a value of 63 for
>>>>> CPUID.04H.EAX[31:26], which matches the expected result.
>>>>>
>>>>> As (1+CPUID.04H.EAX[31:26]) rounds up to the nearest power-of-2 integer,
>>>>> we'd beter round up CPUID.01H.EBX[23:16] to the nearest power-of-2
>>>>> integer too. Otherwise we may encounter unexpected results in guest.
>>>>>
>>>>> For example, when QEMU is started with CLI above and xtopology
>>>>> is disabled,
>>>>> guest kernel 5.15.120 uses CPUID.01H.EBX[23:16]/
>>>>> (1+CPUID.04H.EAX[31:26]) to
>>>>> calculate threads-per-core in detect_ht(). Then guest will get
>>>>> "90/ (1+63)=1"
>>>>> as the result, even though threads-per-core should actually be 2.
>>>>
>>>> It's kernel's bug instead.
>>>>
>>>> In 1.5.3 "Sub ID Extraction Parameters for initial APIC ID" of
>>>> "Intel 64 Architecture Processor Topology Enumeration" [1], it is
>>>>
>>>>    - SMT_Mask_Width = Log2(RoundToNearestPof2(CPUID.1:EBX[23:16])/
>>>> (CPUID.(EAX=4,ECX=0):EAX[31:26]) + 1))
>>>>
>>>> The value of CPUID.1:EBX[23:16] needs to be *rounded* to the
>>>> neartest power-of-two integer instead of itself being the
>>>> power-of-two.
>>>>
>>>> This also is consistency with the SDM, where the comment for bit
>>>> 23-16 of CPUID.1:EBX is:
>>>>
>>>>    The nearest power-of-2 integer that is not smaller than EBX[23:16] is
>>>>    the number of unique initial APIC IDs reserved for addressing
>>>>    different logical processors in a physical package.
>>>>
>>>> What I read from this is, the nearest power-of-2 integer that is not
>>>> smaller than EBX[23:16] is a different thing than EBX[23:16]. i.e.,
>>>
>>> Yes, when I read sdm, I also thought it was a kernel bug. But on my
>>> 192ht spr host, the value of CPUID.1:EBX[23:16] was indeed rounded up
>>>
>>> to the nearest power of 2 by the hardware. After communicating with
>>> Intel technical support staff, we thought that perhaps the description
>>> in sdm
>>>
>>> is not so accurate, and rounding up CPUID.1:EBX[23:16] to the power of 2
>>> in qemu may be more in line with the hardware behavior.
>>
>> I think above justification is important. We need to justify our changes
>> with the fact and correct reason.
>>
>> I somehow agree to set EBX[23:16] to a value of power-of-2, because the
>> 1.5.3 "Sub ID Extraction Parameters for initial APIC ID" of "Intel 64
>> Architecture Processor Topology Enumeration" spec says
>>
>>      CPUID.1:EBX[23:16] represents the maximum number of addressable IDs
>>      (initial APIC ID) that can be assigned to logical processors in a
>>      physical package. The value may not be the same as the number of
>>      logical processors that are present in the hardware of a physical
>>      package.
>>
>> It uses the word "may not".
> 
> IMO, I don't quite understand your confusion regarding this. I've already
> explained the meaning of addressable ID, and the spec you referenced also
> clarifies its significance. The reason for this modification is not
> because of the two words "may not".
> 
> Whether it is "be" or "not be" the same as the number of logical
> processors, the essence is that due to topology, the actual number of
> initial IDs that can be accommodated in the APIC ID may exceed the number
> of logical processors.

I have the confusion because no matter from SDM:

   Bit 23-16: Maximum number of addressable IDs for logical processors in
              this physical package*

   * The nearest power-of-2 integer that is not smaller than EBX[23:16]
     is the number of unique initial APIC IDs reserved for addressing
     different logical processors in a physical package.

or from "Intel 64 Architecture Processor Topology Enumeration" spec，(Jan 
2018, revision 1.1), 1.5.3 "sub ID Extraction Parameters for Inital APIC ID"

   RoundToNearestPof2(CPUID.1:EBX[23:16])

or from "Intel 64 Architecture Processor Topology Enumeration" 
spec，(April 2023, revision 2.0), 1.6.1 Legacy Extraction Algorithm

https://cdrdv2-public.intel.com/775917/intel-64-architecture-processor-topology-enumeration.pdf

   "MaximumLogicalProcessorIDsPerPackage" is calculated by rounding
    CPUID.01H.EBX[23:16] to nearest power of 2.

what I read from them is that EBX[23:16] is a different thing than 
pow2ceil(EBX[23:16]) and EBX[23:16] doesn't need to be power-of-2, but 
the patch are trying to make it power-of-2.

Then I consult it with Intel internal architect. I was told that 
EBX[23:16] used to be that software was to round to the next power of 2. 
However, software had issues a long time ago because applications could 
then compute the wrong power of 2 based on APIC ID holes or some 
applications would use it directly (without round it up to power-of-2).
So intel became to report exact power-of-2 and this behavior is not 
documented.

Zhao Liu Oct. 17, 2024, 9:03 a.m. UTC | #13

On Thu, Oct 17, 2024 at 04:18:06PM +0800, Xiaoyao Li wrote:
> Date: Thu, 17 Oct 2024 16:18:06 +0800
> From: Xiaoyao Li <[e-mail]>
> Subject: Re: [PATCH v6] i386/cpu: fixup number of addressable IDs for
>  logical processors in the physical package
> 
> On 10/14/2024 11:36 AM, Zhao Liu wrote:
> > > > > On 10/9/2024 11:56 AM, Chuang Xu wrote:
> > > > > > When QEMU is started with:
> > > > > > -cpu host,migratable=on,host-cache-info=on,l3-cache=off
> > > > > > -smp 180,sockets=2,dies=1,cores=45,threads=2
> > > > > > 
> > > > > > On Intel platform:
> > > > > > CPUID.01H.EBX[23:16] is defined as "max number of addressable IDs for
> > > > > > logical processors in the physical package".
> > > > > > 
> > > > > > When executing "cpuid -1 -l 1 -r" in the guest, we obtain a
> > > > > > value of 90 for
> > > > > > CPUID.01H.EBX[23:16], whereas the expected value is 128. Additionally,
> > > > > > executing "cpuid -1 -l 4 -r" in the guest yields a value of 63 for
> > > > > > CPUID.04H.EAX[31:26], which matches the expected result.
> > > > > > 
> > > > > > As (1+CPUID.04H.EAX[31:26]) rounds up to the nearest power-of-2 integer,
> > > > > > we'd beter round up CPUID.01H.EBX[23:16] to the nearest power-of-2
> > > > > > integer too. Otherwise we may encounter unexpected results in guest.
> > > > > > 
> > > > > > For example, when QEMU is started with CLI above and xtopology
> > > > > > is disabled,
> > > > > > guest kernel 5.15.120 uses CPUID.01H.EBX[23:16]/
> > > > > > (1+CPUID.04H.EAX[31:26]) to
> > > > > > calculate threads-per-core in detect_ht(). Then guest will get
> > > > > > "90/ (1+63)=1"
> > > > > > as the result, even though threads-per-core should actually be 2.
> > > > > 
> > > > > It's kernel's bug instead.
> > > > > 
> > > > > In 1.5.3 "Sub ID Extraction Parameters for initial APIC ID" of
> > > > > "Intel 64 Architecture Processor Topology Enumeration" [1], it is
> > > > > 
> > > > >    - SMT_Mask_Width = Log2(RoundToNearestPof2(CPUID.1:EBX[23:16])/
> > > > > (CPUID.(EAX=4,ECX=0):EAX[31:26]) + 1))
> > > > > 
> > > > > The value of CPUID.1:EBX[23:16] needs to be *rounded* to the
> > > > > neartest power-of-two integer instead of itself being the
> > > > > power-of-two.
> > > > > 
> > > > > This also is consistency with the SDM, where the comment for bit
> > > > > 23-16 of CPUID.1:EBX is:
> > > > > 
> > > > >    The nearest power-of-2 integer that is not smaller than EBX[23:16] is
> > > > >    the number of unique initial APIC IDs reserved for addressing
> > > > >    different logical processors in a physical package.
> > > > > 
> > > > > What I read from this is, the nearest power-of-2 integer that is not
> > > > > smaller than EBX[23:16] is a different thing than EBX[23:16]. i.e.,
> > > > 
> > > > Yes, when I read sdm, I also thought it was a kernel bug. But on my
> > > > 192ht spr host, the value of CPUID.1:EBX[23:16] was indeed rounded up
> > > > 
> > > > to the nearest power of 2 by the hardware. After communicating with
> > > > Intel technical support staff, we thought that perhaps the description
> > > > in sdm
> > > > 
> > > > is not so accurate, and rounding up CPUID.1:EBX[23:16] to the power of 2
> > > > in qemu may be more in line with the hardware behavior.
> > > 
> > > I think above justification is important. We need to justify our changes
> > > with the fact and correct reason.
> > > 
> > > I somehow agree to set EBX[23:16] to a value of power-of-2, because the
> > > 1.5.3 "Sub ID Extraction Parameters for initial APIC ID" of "Intel 64
> > > Architecture Processor Topology Enumeration" spec says
> > > 
> > >      CPUID.1:EBX[23:16] represents the maximum number of addressable IDs
> > >      (initial APIC ID) that can be assigned to logical processors in a
> > >      physical package. The value may not be the same as the number of
> > >      logical processors that are present in the hardware of a physical
> > >      package.
> > > 
> > > It uses the word "may not".
> > 
> > IMO, I don't quite understand your confusion regarding this. I've already
> > explained the meaning of addressable ID, and the spec you referenced also
> > clarifies its significance. The reason for this modification is not
> > because of the two words "may not".
> > 
> > Whether it is "be" or "not be" the same as the number of logical
> > processors, the essence is that due to topology, the actual number of
> > initial IDs that can be accommodated in the APIC ID may exceed the number
> > of logical processors.
> 
> I have the confusion because no matter from SDM:
> 
>   Bit 23-16: Maximum number of addressable IDs for logical processors in
>              this physical package*
> 
>   * The nearest power-of-2 integer that is not smaller than EBX[23:16]
>     is the number of unique initial APIC IDs reserved for addressing
>     different logical processors in a physical package.
> 
> or from "Intel 64 Architecture Processor Topology Enumeration" spec，(Jan
> 2018, revision 1.1), 1.5.3 "sub ID Extraction Parameters for Inital APIC ID"
> 
>   RoundToNearestPof2(CPUID.1:EBX[23:16])
> 
> or from "Intel 64 Architecture Processor Topology Enumeration" spec，(April
> 2023, revision 2.0), 1.6.1 Legacy Extraction Algorithm
> 
> https://cdrdv2-public.intel.com/775917/intel-64-architecture-processor-topology-enumeration.pdf
> 
>   "MaximumLogicalProcessorIDsPerPackage" is calculated by rounding
>    CPUID.01H.EBX[23:16] to nearest power of 2.
> 
> what I read from them is that EBX[23:16] is a different thing than
> pow2ceil(EBX[23:16]) and EBX[23:16] doesn't need to be power-of-2, but the
> patch are trying to make it power-of-2.

Yes, no one requires it must be power-of-2. But power-of-2 is just
the result, not the reason.

The core point is not power-of-2, but is the meaning of EBX[23:16].

Sorry, I have to re-emphasize:

Pls remember it's not real number of logical processors per package,
and it's "addressable ID", which is the initial APIC ID. The maximum
capacity of addressable ID is calculated by the APIC layout, and the
final value is “power-of-2”. The calculation by APIC ID or pow2ceil()
are mathematically equivalent. That's the way to get addressable IDs.

The spec is expressed in such a way to help software understands this
value, while the QEMU is designed to emulate hardware behavior.

> Then I consult it with Intel internal architect. I was told that EBX[23:16]
> used to be that software was to round to the next power of 2. However,
> software had issues a long time ago because applications could then compute
> the wrong power of 2 based on APIC ID holes or some applications would use
> it directly (without round it up to power-of-2).
> So intel became to report exact power-of-2 and this behavior is not
> documented.

Again, I suggest you think in terms of the meaning of number of
addressable IDs, it's not a matter of how power-of-2 is calculated, you
can choose to calculate number of addressable IDs in other ways, but
the final value is still a power of 2.

-Zhao

Xiaoyao Li Oct. 28, 2024, 4:07 p.m. UTC | #14

On 10/17/2024 5:03 PM, Zhao Liu wrote:
> On Thu, Oct 17, 2024 at 04:18:06PM +0800, Xiaoyao Li wrote:
>> Date: Thu, 17 Oct 2024 16:18:06 +0800
>> From: Xiaoyao Li <[e-mail]>
>> Subject: Re: [PATCH v6] i386/cpu: fixup number of addressable IDs for
>>   logical processors in the physical package
>>
>> On 10/14/2024 11:36 AM, Zhao Liu wrote:
>>>>>> On 10/9/2024 11:56 AM, Chuang Xu wrote:
>>>>>>> When QEMU is started with:
>>>>>>> -cpu host,migratable=on,host-cache-info=on,l3-cache=off
>>>>>>> -smp 180,sockets=2,dies=1,cores=45,threads=2
>>>>>>>
>>>>>>> On Intel platform:
>>>>>>> CPUID.01H.EBX[23:16] is defined as "max number of addressable IDs for
>>>>>>> logical processors in the physical package".
>>>>>>>
>>>>>>> When executing "cpuid -1 -l 1 -r" in the guest, we obtain a
>>>>>>> value of 90 for
>>>>>>> CPUID.01H.EBX[23:16], whereas the expected value is 128. Additionally,
>>>>>>> executing "cpuid -1 -l 4 -r" in the guest yields a value of 63 for
>>>>>>> CPUID.04H.EAX[31:26], which matches the expected result.
>>>>>>>
>>>>>>> As (1+CPUID.04H.EAX[31:26]) rounds up to the nearest power-of-2 integer,
>>>>>>> we'd beter round up CPUID.01H.EBX[23:16] to the nearest power-of-2
>>>>>>> integer too. Otherwise we may encounter unexpected results in guest.
>>>>>>>
>>>>>>> For example, when QEMU is started with CLI above and xtopology
>>>>>>> is disabled,
>>>>>>> guest kernel 5.15.120 uses CPUID.01H.EBX[23:16]/
>>>>>>> (1+CPUID.04H.EAX[31:26]) to
>>>>>>> calculate threads-per-core in detect_ht(). Then guest will get
>>>>>>> "90/ (1+63)=1"
>>>>>>> as the result, even though threads-per-core should actually be 2.
>>>>>>
>>>>>> It's kernel's bug instead.
>>>>>>
>>>>>> In 1.5.3 "Sub ID Extraction Parameters for initial APIC ID" of
>>>>>> "Intel 64 Architecture Processor Topology Enumeration" [1], it is
>>>>>>
>>>>>>     - SMT_Mask_Width = Log2(RoundToNearestPof2(CPUID.1:EBX[23:16])/
>>>>>> (CPUID.(EAX=4,ECX=0):EAX[31:26]) + 1))
>>>>>>
>>>>>> The value of CPUID.1:EBX[23:16] needs to be *rounded* to the
>>>>>> neartest power-of-two integer instead of itself being the
>>>>>> power-of-two.
>>>>>>
>>>>>> This also is consistency with the SDM, where the comment for bit
>>>>>> 23-16 of CPUID.1:EBX is:
>>>>>>
>>>>>>     The nearest power-of-2 integer that is not smaller than EBX[23:16] is
>>>>>>     the number of unique initial APIC IDs reserved for addressing
>>>>>>     different logical processors in a physical package.
>>>>>>
>>>>>> What I read from this is, the nearest power-of-2 integer that is not
>>>>>> smaller than EBX[23:16] is a different thing than EBX[23:16]. i.e.,
>>>>>
>>>>> Yes, when I read sdm, I also thought it was a kernel bug. But on my
>>>>> 192ht spr host, the value of CPUID.1:EBX[23:16] was indeed rounded up
>>>>>
>>>>> to the nearest power of 2 by the hardware. After communicating with
>>>>> Intel technical support staff, we thought that perhaps the description
>>>>> in sdm
>>>>>
>>>>> is not so accurate, and rounding up CPUID.1:EBX[23:16] to the power of 2
>>>>> in qemu may be more in line with the hardware behavior.
>>>>
>>>> I think above justification is important. We need to justify our changes
>>>> with the fact and correct reason.
>>>>
>>>> I somehow agree to set EBX[23:16] to a value of power-of-2, because the
>>>> 1.5.3 "Sub ID Extraction Parameters for initial APIC ID" of "Intel 64
>>>> Architecture Processor Topology Enumeration" spec says
>>>>
>>>>       CPUID.1:EBX[23:16] represents the maximum number of addressable IDs
>>>>       (initial APIC ID) that can be assigned to logical processors in a
>>>>       physical package. The value may not be the same as the number of
>>>>       logical processors that are present in the hardware of a physical
>>>>       package.
>>>>
>>>> It uses the word "may not".
>>>
>>> IMO, I don't quite understand your confusion regarding this. I've already
>>> explained the meaning of addressable ID, and the spec you referenced also
>>> clarifies its significance. The reason for this modification is not
>>> because of the two words "may not".
>>>
>>> Whether it is "be" or "not be" the same as the number of logical
>>> processors, the essence is that due to topology, the actual number of
>>> initial IDs that can be accommodated in the APIC ID may exceed the number
>>> of logical processors.
>>
>> I have the confusion because no matter from SDM:
>>
>>    Bit 23-16: Maximum number of addressable IDs for logical processors in
>>               this physical package*
>>
>>    * The nearest power-of-2 integer that is not smaller than EBX[23:16]
>>      is the number of unique initial APIC IDs reserved for addressing
>>      different logical processors in a physical package.
>>
>> or from "Intel 64 Architecture Processor Topology Enumeration" spec，(Jan
>> 2018, revision 1.1), 1.5.3 "sub ID Extraction Parameters for Inital APIC ID"
>>
>>    RoundToNearestPof2(CPUID.1:EBX[23:16])
>>
>> or from "Intel 64 Architecture Processor Topology Enumeration" spec，(April
>> 2023, revision 2.0), 1.6.1 Legacy Extraction Algorithm
>>
>> https://cdrdv2-public.intel.com/775917/intel-64-architecture-processor-topology-enumeration.pdf
>>
>>    "MaximumLogicalProcessorIDsPerPackage" is calculated by rounding
>>     CPUID.01H.EBX[23:16] to nearest power of 2.
>>
>> what I read from them is that EBX[23:16] is a different thing than
>> pow2ceil(EBX[23:16]) and EBX[23:16] doesn't need to be power-of-2, but the
>> patch are trying to make it power-of-2.
> 
> Yes, no one requires it must be power-of-2. But power-of-2 is just
> the result, not the reason.
> 
> The core point is not power-of-2, but is the meaning of EBX[23:16].
> 
> Sorry, I have to re-emphasize:
> 
> Pls remember it's not real number of logical processors per package,
> and it's "addressable ID", which is the initial APIC ID. The maximum
> capacity of addressable ID is calculated by the APIC layout, and the
> final value is “power-of-2”. The calculation by APIC ID or pow2ceil()
> are mathematically equivalent. That's the way to get addressable IDs.
> 
> The spec is expressed in such a way to help software understands this
> value, while the QEMU is designed to emulate hardware behavior.

To me, what SDM describes are ambiguous and misleading. It has

- EBX[23:16]: Maximum number of addressable IDs for logical processors
               in this physical package.

- The nearest power-of-2 integer that is not smaller than EBX[23:16]:
	      The number of unique initial APIC IDs reserved for
               addressing different logical processors in a physical
               package.

To me, the latter is much more equal to what you called "the maximum 
capacity of addressable ID".

Anyway, as what SDM describes are confusing (at least to me), and based 
on the real value modern Intel CPUs report, I won't argue against it 
anymore.

However, back to the patch, I think we cannot change it as this patch 
directly. Instead, we need a compat_props for the changed behavior, 
because this isn't a bug fix and it introduces guest-visible differences.

For ancient Intel CPUs, EBX[23:16] did represent the number of Logical 
processor per package. I believe this should be the reason why QEMU 
implemented it as is:

   - on SDM version 013, EBX[23:16]: Number of logical processors per 
physical processor; two for the Pentium 4 processor supporting 
Hyper-Threading Technology.

   - on SDM version 015, it changed to: Number of initial APIC IDs 
reserved for this physical package. Normally, this is the number of 
logical processors per physical package.

   - on SDM version 016, it changed to: Maximum number of logical 
processors in this physical package.

   - finally, starting from SDM version 026, it changed to what reads 
now: Maximum number of addressable IDs for logical processors in this 
physical package.

>> Then I consult it with Intel internal architect. I was told that EBX[23:16]
>> used to be that software was to round to the next power of 2. However,
>> software had issues a long time ago because applications could then compute
>> the wrong power of 2 based on APIC ID holes or some applications would use
>> it directly (without round it up to power-of-2).
>> So intel became to report exact power-of-2 and this behavior is not
>> documented.
> 
> Again, I suggest you think in terms of the meaning of number of
> addressable IDs, it's not a matter of how power-of-2 is calculated, you
> can choose to calculate number of addressable IDs in other ways, but
> the final value is still a power of 2.
> 
> -Zhao
>

Patch

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index ff227a8c5c..641d4577b0 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6462,7 +6462,15 @@  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         }
         *edx = env->features[FEAT_1_EDX];
         if (threads_per_pkg > 1) {
-            *ebx |= threads_per_pkg << 16;
+            /*
+             * AMD requires logical processor count, but Intel needs maximum
+             * number of addressable IDs for logical processors per package.
+             */
+            if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) {
+                *ebx |= threads_per_pkg << 16;
+            } else {
+                *ebx |= 1 << apicid_pkg_offset(&topo_info) << 16;
+            }
             *edx |= CPUID_HT;
         }
         if (!cpu->enable_pmu) {