Re: [PATCH 2/4] i386/cpu: Fix cpu number overflow in CPUID.01H.EBX[23:16]

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 2/27/2025 2:25 PM, Zhao Liu wrote:
> From: Qian Wen <qian.wen@intel.com>
> 
> 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.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Qian Wen <qian.wen@intel.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

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

> ---
> Changes since original v4 [*]:
>   * Rebase on addressable ID fixup.
>   * Drop R/b tags since the code base changes.
> 
> [*] original v4: https://lore.kernel.org/qemu-devel/20230829042405.932523-2-qian.wen@intel.com/
> ---
>   target/i386/cpu.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b8a78276cd50..ae6c8bfd8b5e 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6691,16 +6691,21 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>           }
>           *edx = env->features[FEAT_1_EDX];
>           if (threads_per_pkg > 1) {
> +            uint32_t num;
> +
>               /*
>                * For CPUID.01H.EBX[Bits 23-16], AMD requires logical processor
>                * count, but Intel needs maximum number of addressable IDs for
>                * logical processors per package.
>                */
>               if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) {
> -                *ebx |= threads_per_pkg << 16;
> +                num = threads_per_pkg;
>               } else {
> -                *ebx |= 1 << apicid_pkg_offset(topo_info) << 16;
> +                num = 1 << apicid_pkg_offset(topo_info);
>               }
> +
> +            /* Fixup overflow: max value for bits 23-16 is 255. */
> +            *ebx |= MIN(num, 255) << 16;
>           }
>           if (!cpu->enable_pmu) {
>               *ecx &= ~CPUID_EXT_PDCM;