[v1] target/i386: Avoid cpu number overflow in legacy topology

[PATCH] target/i386: Avoid cpu number overflow in legacy topology

Posted by Qian Wen 9 months, 1 week ago

The legacy topology enumerated by CPUID.1.EBX[23:16] is defined in SDM
Vol2:

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

To avoid data overflow, limit the max value written to EBX[23:16] to
255.

Signed-off-by: Qian Wen <qian.wen@intel.com>
---
 target/i386/cpu.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 1294be374ab2..70589a58b727 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5356,6 +5356,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
     uint32_t die_offset;
     uint32_t limit;
     uint32_t signature[3];
+    uint32_t threads_per_socket;
     X86CPUTopoInfo topo_info;
 
     topo_info.dies_per_pkg = env->nr_dies;
@@ -5397,8 +5398,18 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             *ecx |= CPUID_EXT_OSXSAVE;
         }
         *edx = env->features[FEAT_1_EDX];
-        if (cs->nr_cores * cs->nr_threads > 1) {
-            *ebx |= (cs->nr_cores * cs->nr_threads) << 16;
+        /*
+         * The vCPU number more than 255 needs support of V2 Extended
+         * Topology enumerated by CPUID.0x1f or Extended Topology
+         * enumerated by CPUID.0x0b.
+         */
+        threads_per_socket = cs->nr_cores * cs->nr_threads;
+        if (threads_per_socket > 255) {
+            threads_per_socket = 255;
+        }
+
+        if (threads_per_socket > 1) {
+            *ebx |= threads_per_socket << 16;
             *edx |= CPUID_HT;
         }
         /*
-- 
2.25.1

Re: [PATCH] target/i386: Avoid cpu number overflow in legacy topology

Posted by Zhao Liu 8 months, 4 weeks ago

On Fri, Jul 28, 2023 at 04:01:50PM +0800, Qian Wen wrote:
> Date: Fri, 28 Jul 2023 16:01:50 +0800
> From: Qian Wen <qian.wen@intel.com>
> Subject: [PATCH] target/i386: Avoid cpu number overflow in legacy topology
> X-Mailer: git-send-email 2.25.1
> 
> The legacy topology enumerated by CPUID.1.EBX[23:16] is defined in SDM
> Vol2:
> 
> Bits 23-16: Maximum number of addressable IDs for logical processors in
> this physical package.
> 
> To avoid data overflow, limit the max value written to EBX[23:16] to
> 255.
> 
> Signed-off-by: Qian Wen <qian.wen@intel.com>
> ---
>  target/i386/cpu.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 1294be374ab2..70589a58b727 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5356,6 +5356,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>      uint32_t die_offset;
>      uint32_t limit;
>      uint32_t signature[3];
> +    uint32_t threads_per_socket;
>      X86CPUTopoInfo topo_info;
>  
>      topo_info.dies_per_pkg = env->nr_dies;
> @@ -5397,8 +5398,18 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>              *ecx |= CPUID_EXT_OSXSAVE;
>          }
>          *edx = env->features[FEAT_1_EDX];
> -        if (cs->nr_cores * cs->nr_threads > 1) {
> -            *ebx |= (cs->nr_cores * cs->nr_threads) << 16;
> +        /*
> +         * The vCPU number more than 255 needs support of V2 Extended
> +         * Topology enumerated by CPUID.0x1f or Extended Topology
> +         * enumerated by CPUID.0x0b.
> +         */
> +        threads_per_socket = cs->nr_cores * cs->nr_threads;
> +        if (threads_per_socket > 255) {
> +            threads_per_socket = 255;

Straight encoding to 255 is good for me!

-Zhao

> +        }
> +
> +        if (threads_per_socket > 1) {
> +            *ebx |= threads_per_socket << 16;
>              *edx |= CPUID_HT;
>          }
>          /*
> -- 
> 2.25.1
>

Re: [PATCH] target/i386: Avoid cpu number overflow in legacy topology

Posted by Wen, Qian 8 months, 4 weeks ago

On 8/7/2023 4:08 PM, Zhao Liu wrote:
> On Fri, Jul 28, 2023 at 04:01:50PM +0800, Qian Wen wrote:
>> Date: Fri, 28 Jul 2023 16:01:50 +0800
>> From: Qian Wen <qian.wen@intel.com>
>> Subject: [PATCH] target/i386: Avoid cpu number overflow in legacy topology
>> X-Mailer: git-send-email 2.25.1
>>
>> The legacy topology enumerated by CPUID.1.EBX[23:16] is defined in SDM
>> Vol2:
>>
>> Bits 23-16: Maximum number of addressable IDs for logical processors in
>> this physical package.
>>
>> To avoid data overflow, limit the max value written to EBX[23:16] to
>> 255.
>>
>> Signed-off-by: Qian Wen <qian.wen@intel.com>
>> ---
>>  target/i386/cpu.c | 15 +++++++++++++--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 1294be374ab2..70589a58b727 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -5356,6 +5356,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>>      uint32_t die_offset;
>>      uint32_t limit;
>>      uint32_t signature[3];
>> +    uint32_t threads_per_socket;
>>      X86CPUTopoInfo topo_info;
>>  
>>      topo_info.dies_per_pkg = env->nr_dies;
>> @@ -5397,8 +5398,18 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>>              *ecx |= CPUID_EXT_OSXSAVE;
>>          }
>>          *edx = env->features[FEAT_1_EDX];
>> -        if (cs->nr_cores * cs->nr_threads > 1) {
>> -            *ebx |= (cs->nr_cores * cs->nr_threads) << 16;
>> +        /*
>> +         * The vCPU number more than 255 needs support of V2 Extended
>> +         * Topology enumerated by CPUID.0x1f or Extended Topology
>> +         * enumerated by CPUID.0x0b.
>> +         */
>> +        threads_per_socket = cs->nr_cores * cs->nr_threads;
>> +        if (threads_per_socket > 255) {
>> +            threads_per_socket = 255;
> Straight encoding to 255 is good for me!
>
> -Zhao


Got it, thanks!

Thanks,
Qian

>> +        }
>> +
>> +        if (threads_per_socket > 1) {
>> +            *ebx |= threads_per_socket << 16;
>>              *edx |= CPUID_HT;
>>          }
>>          /*
>> -- 
>> 2.25.1
>>

Re: [PATCH] target/i386: Avoid cpu number overflow in legacy topology

Posted by Xiaoyao Li 8 months, 4 weeks ago

On 7/28/2023 4:01 PM, Qian Wen wrote:
> The legacy topology enumerated by CPUID.1.EBX[23:16] is defined in SDM
> Vol2:
> 
> Bits 23-16: Maximum number of addressable IDs for logical processors in
> this physical package.
> 
> To avoid data overflow, limit the max value written to EBX[23:16] to
> 255.

It's better explain what's issue when overflow happens.

> Signed-off-by: Qian Wen <qian.wen@intel.com>
> ---
>   target/i386/cpu.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 1294be374ab2..70589a58b727 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5356,6 +5356,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>       uint32_t die_offset;
>       uint32_t limit;
>       uint32_t signature[3];
> +    uint32_t threads_per_socket;
>       X86CPUTopoInfo topo_info;
>   
>       topo_info.dies_per_pkg = env->nr_dies;
> @@ -5397,8 +5398,18 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>               *ecx |= CPUID_EXT_OSXSAVE;
>           }
>           *edx = env->features[FEAT_1_EDX];
> -        if (cs->nr_cores * cs->nr_threads > 1) {
> -            *ebx |= (cs->nr_cores * cs->nr_threads) << 16;
> +        /*
> +         * The vCPU number more than 255 needs support of V2 Extended
> +         * Topology enumerated by CPUID.0x1f or Extended Topology
> +         * enumerated by CPUID.0x0b.
> +         */

the above comment doesn't explain why it needs below.

you can explain only bits [23:16] represents the maximum number of 
addressable IDs for logical processors in this physical package.

When thread_per_socket > 255, it will 1) overwrite bits[31:24] which is 
apic_id, 2) bits [23:16] gets truncated.

> +        threads_per_socket = cs->nr_cores * cs->nr_threads;
> +        if (threads_per_socket > 255) {
> +            threads_per_socket = 255;
> +        }
> +
> +        if (threads_per_socket > 1) {
> +            *ebx |= threads_per_socket << 16;
>               *edx |= CPUID_HT;
>           }
>           /*

Re: [PATCH] target/i386: Avoid cpu number overflow in legacy topology

Posted by Wen, Qian 8 months, 4 weeks ago

On 8/7/2023 3:36 PM, Xiaoyao Li wrote:
> On 7/28/2023 4:01 PM, Qian Wen wrote:
>> The legacy topology enumerated by CPUID.1.EBX[23:16] is defined in SDM
>> Vol2:
>>
>> Bits 23-16: Maximum number of addressable IDs for logical processors in
>> this physical package.
>>
>> To avoid data overflow, limit the max value written to EBX[23:16] to
>> 255.
>
> It's better explain what's issue when overflow happens.
>

When launch vm with -smp 256, the value writes to EBX[23:16] is 0.
If the guest only support legacy topology, the result of kernel invokes cpu_smt_allowed() is false and AP's bring-up will fail. Then only CPU 0 is online, others offline.

>> Signed-off-by: Qian Wen <qian.wen@intel.com>
>> ---
>>   target/i386/cpu.c | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 1294be374ab2..70589a58b727 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -5356,6 +5356,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>>       uint32_t die_offset;
>>       uint32_t limit;
>>       uint32_t signature[3];
>> +    uint32_t threads_per_socket;
>>       X86CPUTopoInfo topo_info;
>>         topo_info.dies_per_pkg = env->nr_dies;
>> @@ -5397,8 +5398,18 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>>               *ecx |= CPUID_EXT_OSXSAVE;
>>           }
>>           *edx = env->features[FEAT_1_EDX];
>> -        if (cs->nr_cores * cs->nr_threads > 1) {
>> -            *ebx |= (cs->nr_cores * cs->nr_threads) << 16;
>> +        /*
>> +         * The vCPU number more than 255 needs support of V2 Extended
>> +         * Topology enumerated by CPUID.0x1f or Extended Topology
>> +         * enumerated by CPUID.0x0b.
>> +         */
>
> the above comment doesn't explain why it needs below.
>
> you can explain only bits [23:16] represents the maximum number of addressable IDs for logical processors in this physical package.
>
> When thread_per_socket > 255, it will 1) overwrite bits[31:24] which is apic_id, 2) bits [23:16] gets truncated.

Thanks for your suggestion, I will add your description in v2.

Thanks,
Qian

>
>> +        threads_per_socket = cs->nr_cores * cs->nr_threads;
>> +        if (threads_per_socket > 255) {
>> +            threads_per_socket = 255;
>> +        }
>> +
>> +        if (threads_per_socket > 1) {
>> +            *ebx |= threads_per_socket << 16;
>>               *edx |= CPUID_HT;
>>           }
>>           /*
>