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

["Wen, Qian" <qian.wen@intel.com>]

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

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;
>>           }
>>           /*
>

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