[PATCH v3 0/2] Fix overflow of the max number of IDs for logic processor and core

[PATCH v3 0/2] Fix overflow of the max number of IDs for logic processor and core

[Qian Wen]

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

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

[Qian Wen]

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.

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

Specifically, if launching the VM with -smp 256, the value written to
EBX[23:16] is 0 because of data overflow. If the guest only supports
legacy topology, without V2 Extended Topology enumerated by CPUID.0x1f
or Extended Topology enumerated by CPUID.0x0b to support over 255 CPUs,
the return of the kernel invoking cpu_smt_allowed() is false and APs
(application processors) will fail to bring up. Then only CPU 0 is online,
and others are offline.

For example, launch VM via:
qemu-system-x86_64 -M q35,accel=kvm,kernel-irqchip=split \
    -cpu qemu64,cpuid-0xb=off -smp 256 -m 32G \
    -drive file=guest.img,if=none,id=virtio-disk0,format=raw \
    -device virtio-blk-pci,drive=virtio-disk0,bootindex=1 --nographic

The guest shows:
    CPU(s):               256
    On-line CPU(s) list:  0
    Off-line CPU(s) list: 1-255

To avoid this issue caused by overflow, limit the max value written to
EBX[23:16] to 255 as the HW does.

Signed-off-by: Qian Wen <qian.wen@intel.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/i386/cpu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 97ad229d8b..5c008b9d7e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6008,6 +6008,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;
@@ -6049,8 +6050,9 @@ 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;
+        threads_per_socket = cs->nr_cores * cs->nr_threads;
+        if (threads_per_socket > 1) {
+            *ebx |= MIN(threads_per_socket, 255) << 16;
             *edx |= CPUID_HT;
         }
         if (!cpu->enable_pmu) {
-- 
2.25.1

[PATCH v3 2/2] target/i386: Avoid overflow of the cache parameter enumerated by leaf 4

[Qian Wen]

According to SDM, CPUID.0x4:EAX[31:26] indicates the Maximum number of
addressable IDs for processor cores in the physical package. If we
launch over 64 cores VM, the 6-bit field will overflow, and the wrong
core_id number will be reported.

Since the HW reports 0x3f when the intel processor has over 64 cores,
limit the max value written to EBX[31:26] to 63, so max num_cores should
be 64.

Signed-off-by: Qian Wen <qian.wen@intel.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/i386/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 5c008b9d7e..3b6854300a 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -248,7 +248,7 @@ 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) |
+           ((MIN(num_cores, 64) - 1) << 26) |
            ((num_apic_ids - 1) << 14);
 
     assert(cache->line_size > 0);
-- 
2.25.1

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

[Xiaoyao Li]

On 8/16/2023 4:06 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.
> 
> When threads_per_socket > 255, it will 1) overwrite bits[31:24] which is
> apic_id, 2) bits [23:16] get truncated.
> 
> Specifically, if launching the VM with -smp 256, the value written to
> EBX[23:16] is 0 because of data overflow. If the guest only supports
> legacy topology, without V2 Extended Topology enumerated by CPUID.0x1f
> or Extended Topology enumerated by CPUID.0x0b to support over 255 CPUs,
> the return of the kernel invoking cpu_smt_allowed() is false and APs
> (application processors) will fail to bring up. Then only CPU 0 is online,
> and others are offline.
> 
> For example, launch VM via:
> qemu-system-x86_64 -M q35,accel=kvm,kernel-irqchip=split \
>      -cpu qemu64,cpuid-0xb=off -smp 256 -m 32G \
>      -drive file=guest.img,if=none,id=virtio-disk0,format=raw \
>      -device virtio-blk-pci,drive=virtio-disk0,bootindex=1 --nographic
> 
> The guest shows:
>      CPU(s):               256
>      On-line CPU(s) list:  0
>      Off-line CPU(s) list: 1-255
> 
> To avoid this issue caused by overflow, limit the max value written to
> EBX[23:16] to 255 as the HW does.
> 
> Signed-off-by: Qian Wen <qian.wen@intel.com>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   target/i386/cpu.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 97ad229d8b..5c008b9d7e 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6008,6 +6008,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;
> @@ -6049,8 +6050,9 @@ 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;
> +        threads_per_socket = cs->nr_cores * cs->nr_threads;
> +        if (threads_per_socket > 1) {
> +            *ebx |= MIN(threads_per_socket, 255) << 16;
>               *edx |= CPUID_HT;
>           }
>           if (!cpu->enable_pmu) {

Re: [PATCH v3 2/2] target/i386: Avoid overflow of the cache parameter enumerated by leaf 4

[Xiaoyao Li]

On 8/16/2023 4:06 PM, Qian Wen wrote:
> According to SDM, CPUID.0x4:EAX[31:26] indicates the Maximum number of
> addressable IDs for processor cores in the physical package. If we
> launch over 64 cores VM, the 6-bit field will overflow, and the wrong
> core_id number will be reported.
> 
> Since the HW reports 0x3f when the intel processor has over 64 cores,
> limit the max value written to EBX[31:26] to 63, so max num_cores should
> be 64.
> 
> Signed-off-by: Qian Wen <qian.wen@intel.com>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   target/i386/cpu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 5c008b9d7e..3b6854300a 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -248,7 +248,7 @@ 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) |
> +           ((MIN(num_cores, 64) - 1) << 26) |
>              ((num_apic_ids - 1) << 14);
>   
>       assert(cache->line_size > 0);

Re: [PATCH v3 0/2] Fix overflow of the max number of IDs for logic processor and core

[Isaku Yamahata]

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>

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

[Isaku Yamahata]

On Wed, Aug 16, 2023 at 04:06:57PM +0800,
Qian Wen <qian.wen@intel.com> 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.
> 
> When threads_per_socket > 255, it will 1) overwrite bits[31:24] which is
> apic_id, 2) bits [23:16] get truncated.
> 
> Specifically, if launching the VM with -smp 256, the value written to
> EBX[23:16] is 0 because of data overflow. If the guest only supports
> legacy topology, without V2 Extended Topology enumerated by CPUID.0x1f
> or Extended Topology enumerated by CPUID.0x0b to support over 255 CPUs,
> the return of the kernel invoking cpu_smt_allowed() is false and APs
> (application processors) will fail to bring up. Then only CPU 0 is online,
> and others are offline.
> 
> For example, launch VM via:
> qemu-system-x86_64 -M q35,accel=kvm,kernel-irqchip=split \
>     -cpu qemu64,cpuid-0xb=off -smp 256 -m 32G \
>     -drive file=guest.img,if=none,id=virtio-disk0,format=raw \
>     -device virtio-blk-pci,drive=virtio-disk0,bootindex=1 --nographic
> 
> The guest shows:
>     CPU(s):               256
>     On-line CPU(s) list:  0
>     Off-line CPU(s) list: 1-255
> 
> To avoid this issue caused by overflow, limit the max value written to
> EBX[23:16] to 255 as the HW does.
> 
> Signed-off-by: Qian Wen <qian.wen@intel.com>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  target/i386/cpu.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 97ad229d8b..5c008b9d7e 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6008,6 +6008,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;
> @@ -6049,8 +6050,9 @@ 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;
> +        threads_per_socket = cs->nr_cores * cs->nr_threads;
> +        if (threads_per_socket > 1) {
> +            *ebx |= MIN(threads_per_socket, 255) << 16;
>              *edx |= CPUID_HT;
>          }
>          if (!cpu->enable_pmu) {
> -- 
> 2.25.1
> 
> 

Reviewed-by: Isaku Yamahata <isaku.yamahata@intel.com>
-- 
Isaku Yamahata <isaku.yamahata@linux.intel.com>

Re: [PATCH v3 2/2] target/i386: Avoid overflow of the cache parameter enumerated by leaf 4

[Isaku Yamahata]

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

> According to SDM, CPUID.0x4:EAX[31:26] indicates the Maximum number of
> addressable IDs for processor cores in the physical package. If we
> launch over 64 cores VM, the 6-bit field will overflow, and the wrong
> core_id number will be reported.
> 
> Since the HW reports 0x3f when the intel processor has over 64 cores,
> limit the max value written to EBX[31:26] to 63, so max num_cores should
> be 64.
> 
> Signed-off-by: Qian Wen <qian.wen@intel.com>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  target/i386/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 5c008b9d7e..3b6854300a 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -248,7 +248,7 @@ 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) |
> +           ((MIN(num_cores, 64) - 1) << 26) |
>             ((num_apic_ids - 1) << 14);
>  
>      assert(cache->line_size > 0);
> -- 
> 2.25.1
> 
> 

Reviewed-by: Isaku Yamahata <isaku.yamahata@intel.com>
-- 
Isaku Yamahata <isaku.yamahata@linux.intel.com>

Re: [PATCH v3 0/2] Fix overflow of the max number of IDs for logic processor and core

[Wen, Qian]

[-- Attachment #1: Type: text/plain, Size: 2407 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 3416 bytes --]