CPUID.1.EBX[23:16]: Maximum number of addressable IDs for logical
processors in this physical package.
CPUID.4:EAX[31:26]: Maximum number of addressable IDs for processor cores
in the physical package.

The current qemu code doesn't limit the value written to these two fields.
If the guest has a huge number of cores, APs (application processor) will
fail to bring up and the wrong info will be reported.
According to HW behavior, setting max value written to CPUID.1.EBX[23:16]
to 255, and CPUID.4:EAX[31:26] to 63.

---
Changes v2 -> v3:
  - Add patch 2.
  - Revise the commit message and comment to be clearer.
  - Using MIN() for limitation.
Changes v1 -> v2:
  - Revise the commit message and comment to more clearer.
  - Rebased to v8.1.0-rc2.

Qian Wen (2):
  target/i386: Avoid cpu number overflow in legacy topology
  target/i386: Avoid overflow of the cache parameter enumerated by leaf 4

 target/i386/cpu.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

base-commit: 0d52116fd82cdd1f4a88837336af5b6290c364a4
-- 
2.25.1

On Wed, Aug 16, 2023 at 04:06:56PM +0800,
Qian Wen <qian.wen@intel.com> wrote:

> CPUID.1.EBX[23:16]: Maximum number of addressable IDs for logical
> processors in this physical package.
> CPUID.4:EAX[31:26]: Maximum number of addressable IDs for processor cores
> in the physical package.
> 
> The current qemu code doesn't limit the value written to these two fields.
> If the guest has a huge number of cores, APs (application processor) will
> fail to bring up and the wrong info will be reported.
> According to HW behavior, setting max value written to CPUID.1.EBX[23:16]
> to 255, and CPUID.4:EAX[31:26] to 63.
> 
> ---
> Changes v2 -> v3:
>   - Add patch 2.
>   - Revise the commit message and comment to be clearer.
>   - Using MIN() for limitation.
> Changes v1 -> v2:
>   - Revise the commit message and comment to more clearer.
>   - Rebased to v8.1.0-rc2.
> 
> Qian Wen (2):
>   target/i386: Avoid cpu number overflow in legacy topology
>   target/i386: Avoid overflow of the cache parameter enumerated by leaf 4
> 
>  target/i386/cpu.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> base-commit: 0d52116fd82cdd1f4a88837336af5b6290c364a4
> -- 
> 2.25.1
> 

The patch itself looks good. Can we add test cases?
We have some in qemu/tests/unit/test-x86-cpuid.c.
-- 
Isaku Yamahata <isaku.yamahata@linux.intel.com>

On 8/18/2023 3:33 AM, Isaku Yamahata wrote:
> On Wed, Aug 16, 2023 at 04:06:56PM +0800,
> Qian Wen <qian.wen@intel.com> wrote:
>
>> CPUID.1.EBX[23:16]: Maximum number of addressable IDs for logical
>> processors in this physical package.
>> CPUID.4:EAX[31:26]: Maximum number of addressable IDs for processor cores
>> in the physical package.
>>
>> The current qemu code doesn't limit the value written to these two fields.
>> If the guest has a huge number of cores, APs (application processor) will
>> fail to bring up and the wrong info will be reported.
>> According to HW behavior, setting max value written to CPUID.1.EBX[23:16]
>> to 255, and CPUID.4:EAX[31:26] to 63.
>>
>> ---
>> Changes v2 -> v3:
>>   - Add patch 2.
>>   - Revise the commit message and comment to be clearer.
>>   - Using MIN() for limitation.
>> Changes v1 -> v2:
>>   - Revise the commit message and comment to more clearer.
>>   - Rebased to v8.1.0-rc2.
>>
>> Qian Wen (2):
>>   target/i386: Avoid cpu number overflow in legacy topology
>>   target/i386: Avoid overflow of the cache parameter enumerated by leaf 4
>>
>>  target/i386/cpu.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> base-commit: 0d52116fd82cdd1f4a88837336af5b6290c364a4
>> -- 
>> 2.25.1
>>
> The patch itself looks good. Can we add test cases?
> We have some in qemu/tests/unit/test-x86-cpuid.c.


Hi Isaku, thanks for your comments!

I take a look, the test-x86-cpuid.c has some tests for topology functions, e.g., apicid_smt/core/die_width, apicid_core/die/pkg_offset, x86_apicid_from_cpu_idx...

Do you mean adding another test for function cpu_x86_cpuid() like below? If so, it seems that some effort is required to instantiate CPUX86State for input of cpu_x86_cpuid, but the result of this test case is obvious. So, is it necessary to add this test?

+    uint32_t unused, eax, ebx;
+    /* CPUID.1.EBX[23:16]: Maximum number of addressable IDs for logical
+       processors in this physical package.
+    */
+    cpu_x86_cpuid(env, 1, 0, &unused, &ebx, &unused, &unused);
+    g_assert_cmpuint(ebx & 0xFF0000, ==, 0xFF0000);
+
+    /* CPUID.4:EAX[31:26]: Maximum number of addressable IDs for processor
+       cores in the physical package.
+    */
+    cpu_x86_cpuid(env, 4, 0, &eax, &unused, &unused, &unused);
+    g_assert_cmpuint(ebx & 0xFC000000, ==, 0xFC000000);


Thanks,
Qian