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

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

Posted by Qian Wen 8 months, 3 weeks 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.

When 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 AP's bring-up will
fail. Then only CPU 0 is online, and others are offline.

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

Signed-off-by: Qian Wen <qian.wen@intel.com>
---
Changes v1 -> v2:
 - Revise the commit message and comment to more clearer.
 - Rebased to v8.1.0-rc2.
---
 target/i386/cpu.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 97ad229d8b..6e1d88fbd7 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,19 @@ 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;
+        /*
+         * Only bits [23:16] represent 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] get 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;
         }
         if (!cpu->enable_pmu) {
-- 
2.25.1

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

Posted by Igor Mammedov 8 months, 3 weeks ago

On Wed,  9 Aug 2023 18:27:32 +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 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 AP's bring-up will
> fail. Then only CPU 0 is online, and others are offline.
> 
> To avoid this issue caused by overflow, limit the max value written to
> EBX[23:16] to 255.

what happens on real hw or in lack of thereof what SDM says about this
value when there is more than 255 threads?.


> Signed-off-by: Qian Wen <qian.wen@intel.com>
> ---
> Changes v1 -> v2:
>  - Revise the commit message and comment to more clearer.
>  - Rebased to v8.1.0-rc2.
> ---
>  target/i386/cpu.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 97ad229d8b..6e1d88fbd7 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,19 @@ 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;
> +        /*
> +         * Only bits [23:16] represent 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] get 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;
>          }
>          if (!cpu->enable_pmu) {

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

Posted by Wen, Qian 8 months, 3 weeks ago

On 8/9/2023 7:14 PM, Igor Mammedov wrote:
> On Wed,  9 Aug 2023 18:27:32 +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 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 AP's bring-up will
>> fail. Then only CPU 0 is online, and others are offline.
>>
>> To avoid this issue caused by overflow, limit the max value written to
>> EBX[23:16] to 255.
> what happens on real hw or in lack of thereof what SDM says about this
> value when there is more than 255 threads?.
>

Current SDM doesn't specify what the value should be when APIC IDs per package exceeds 255. So we asked the internal HW architect, the response is that EBX[23:16] will report 255 instead of being truncated to a smaller value.

Thanks,
Qian

>> Signed-off-by: Qian Wen <qian.wen@intel.com>
>> ---
>> Changes v1 -> v2:
>>  - Revise the commit message and comment to more clearer.
>>  - Rebased to v8.1.0-rc2.
>> ---
>>  target/i386/cpu.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 97ad229d8b..6e1d88fbd7 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,19 @@ 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;
>> +        /*
>> +         * Only bits [23:16] represent 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] get 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;
>>          }
>>          if (!cpu->enable_pmu) {

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

Posted by Igor Mammedov 8 months, 3 weeks ago

On Wed, 9 Aug 2023 21:20:48 +0800
"Wen, Qian" <qian.wen@intel.com> wrote:

> On 8/9/2023 7:14 PM, Igor Mammedov wrote:
> > On Wed,  9 Aug 2023 18:27:32 +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 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 AP's bring-up will
> >> fail. Then only CPU 0 is online, and others are offline.
> >>
> >> To avoid this issue caused by overflow, limit the max value written to
> >> EBX[23:16] to 255.  
> > what happens on real hw or in lack of thereof what SDM says about this
> > value when there is more than 255 threads?.
> >  
> 
> Current SDM doesn't specify what the value should be when APIC IDs per package exceeds 255. So we asked the internal HW architect, the response is that EBX[23:16] will report 255 instead of being truncated to a smaller value.

then mention it in commit log so one wouldn't wonder where the value came from.

> 
> Thanks,
> Qian
> 
> >> Signed-off-by: Qian Wen <qian.wen@intel.com>
> >> ---
> >> Changes v1 -> v2:
> >>  - Revise the commit message and comment to more clearer.
> >>  - Rebased to v8.1.0-rc2.
> >> ---
> >>  target/i386/cpu.c | 16 ++++++++++++++--
> >>  1 file changed, 14 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> >> index 97ad229d8b..6e1d88fbd7 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,19 @@ 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;
> >> +        /*
> >> +         * Only bits [23:16] represent 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] get 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;
                          ^^^^^^^^^^^^^^^^^^^^^^^^^
more robust would be mask out non-relevant fields at rhs 

also perhaps double check if we could do induce similar overflow
tweaking other -smp properties (todo for another patch[es] if there are such places).

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

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

Posted by Wen, Qian 8 months, 3 weeks ago

On 8/9/2023 9:47 PM, Igor Mammedov wrote:
> On Wed, 9 Aug 2023 21:20:48 +0800
> "Wen, Qian" <qian.wen@intel.com> wrote:
>
>> On 8/9/2023 7:14 PM, Igor Mammedov wrote:
>>> On Wed,  9 Aug 2023 18:27:32 +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 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 AP's bring-up will
>>>> fail. Then only CPU 0 is online, and others are offline.
>>>>
>>>> To avoid this issue caused by overflow, limit the max value written to
>>>> EBX[23:16] to 255.  
>>> what happens on real hw or in lack of thereof what SDM says about this
>>> value when there is more than 255 threads?.
>>>  
>> Current SDM doesn't specify what the value should be when APIC IDs per package exceeds 255. So we asked the internal HW architect, the response is that EBX[23:16] will report 255 instead of being truncated to a smaller value.
> then mention it in commit log so one wouldn't wonder where the value came from.

Ok, thanks for your suggestion!

>> Thanks,
>> Qian
>>
>>>> Signed-off-by: Qian Wen <qian.wen@intel.com>
>>>> ---
>>>> Changes v1 -> v2:
>>>>  - Revise the commit message and comment to more clearer.
>>>>  - Rebased to v8.1.0-rc2.
>>>> ---
>>>>  target/i386/cpu.c | 16 ++++++++++++++--
>>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>>>> index 97ad229d8b..6e1d88fbd7 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,19 @@ 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;
>>>> +        /*
>>>> +         * Only bits [23:16] represent 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] get 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;
>                           ^^^^^^^^^^^^^^^^^^^^^^^^^
> more robust would be mask out non-relevant fields at rhs 

I think a mask for this case is a bit redundant, since the limitation of 255 already filtered non-relevant fields.
I prefer not to add the mask here and keep code style consistency with others place.

> also perhaps double check if we could do induce similar overflow
> tweaking other -smp properties (todo for another patch[es] if there are such places).

I have a check, the 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-bits field will also overflow. I will add the following fix to patch2 in v2.

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 52a2a1a1c7..9c1ae3d83d 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -243,6 +243,7 @@ static void encode_cache_cpuid4(CPUCacheInfo *cache,
                           cache->partitions * cache->sets);

     assert(num_apic_ids > 0);
+    num_cores = num_cores > 64 ? 64 : num_cores;
     *eax = CACHE_TYPE(cache->type) |
            CACHE_LEVEL(cache->level) |
            (cache->self_init ? CACHE_SELF_INIT_LEVEL : 0) |


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

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

Posted by Zhao Liu 8 months, 3 weeks ago

Hi Qian,

On Sun, Aug 13, 2023 at 06:49:40PM +0800, Wen, Qian wrote:

[snip]

> 
> > also perhaps double check if we could do induce similar overflow
> > tweaking other -smp properties (todo for another patch[es] if there are such places).
> 
> I have a check, the 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-bits field will also overflow. I will add the following fix to patch2 in v2.

Good catch!

Just discussion, I find if we use APIC ID offset to encode 0x4, then it
seems no need for an explicit check [1], right?

[1]: https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg00027.html

Thanks,
Zhao

> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 52a2a1a1c7..9c1ae3d83d 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -243,6 +243,7 @@ static void encode_cache_cpuid4(CPUCacheInfo *cache,
>                            cache->partitions * cache->sets);
> 
>      assert(num_apic_ids > 0);
> +    num_cores = num_cores > 64 ? 64 : num_cores;
>      *eax = CACHE_TYPE(cache->type) |
>             CACHE_LEVEL(cache->level) |
>             (cache->self_init ? CACHE_SELF_INIT_LEVEL : 0) |
> 
> 
> Thanks,
> Qian
> >>>>              *edx |= CPUID_HT;
> >>>>          }
> >>>>          if (!cpu->enable_pmu) {

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

Posted by Wen, Qian 8 months, 3 weeks ago

On 8/14/2023 7:59 AM, Zhao Liu wrote:
> Hi Qian,
>
> On Sun, Aug 13, 2023 at 06:49:40PM +0800, Wen, Qian wrote:
>
> [snip]
>
>>> also perhaps double check if we could do induce similar overflow
>>> tweaking other -smp properties (todo for another patch[es] if there are such places).
>> I have a check, the 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-bits field will also overflow. I will add the following fix to patch2 in v3.
> Good catch!
>
> Just discussion, I find if we use APIC ID offset to encode 0x4, then it
> seems no need for an explicit check [1], right?
>
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg00027.html

Yes. The offset is always power of 2, so the value written to the 6-bit field likes 0b1111, 0b11111, 0b111111, 0b1111111...
So, EAX[31:26] will be expected, i.e., 0x3f, when the value is overflow and truncated.

>
> Thanks,
> Zhao
>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 52a2a1a1c7..9c1ae3d83d 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -243,6 +243,7 @@ static void encode_cache_cpuid4(CPUCacheInfo *cache,
>>                            cache->partitions * cache->sets);
>>
>>      assert(num_apic_ids > 0);
>> +    num_cores = num_cores > 64 ? 64 : num_cores;
>>      *eax = CACHE_TYPE(cache->type) |
>>             CACHE_LEVEL(cache->level) |
>>             (cache->self_init ? CACHE_SELF_INIT_LEVEL : 0) |
>>
>>
>> Thanks,
>> Qian
>>>>>>              *edx |= CPUID_HT;
>>>>>>          }
>>>>>>          if (!cpu->enable_pmu) {