Re: [PATCH 3/4] i386/cpu: Fix overflow of cache topology fields in CPUID.04H

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

On 2/27/2025 2:25 PM, Zhao Liu wrote:
> From: Qian Wen <qian.wen@intel.com>
> 
> 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 EAX[31:26] to 63, so max num_cores should
> be 64.
> 
> For EAX[14:25], though at present Q35 supports up to 4096 CPUs, to
> prevent potential overflow issues from further increasing the number of
> CPUs in the future, check and honor the maximum value for EAX[14:25] as
> well.
> 
> In addition, for host-cache-info case, also apply the same checks and
> fixes.
> 
> 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.
>   * Teak bits 25-14 as well and add the comment.
>   * Fix overflow for host-cache-info case.
> 
> [*]: original v4: https://lore.kernel.org/qemu-devel/20230829042405.932523-3-qian.wen@intel.com/
> ---
>   target/i386/cpu.c | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index ae6c8bfd8b5e..d75175b0850a 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -280,11 +280,17 @@ static void encode_cache_cpuid4(CPUCacheInfo *cache,
>       assert(cache->size == cache->line_size * cache->associativity *
>                             cache->partitions * cache->sets);
>   
> +    /*
> +     * The following fields have bit-width limitations, so consider the
> +     * maximum values to avoid overflow:
> +     * Bits 25-14: maximum 4095.
> +     * Bits 31-26: maximum 63.
> +     */
>       *eax = CACHE_TYPE(cache->type) |
>              CACHE_LEVEL(cache->level) |
>              (cache->self_init ? CACHE_SELF_INIT_LEVEL : 0) |
> -           (max_core_ids_in_package(topo_info) << 26) |
> -           (max_thread_ids_for_cache(topo_info, cache->share_level) << 14);
> +           (MIN(max_core_ids_in_package(topo_info), 63) << 26) |
> +           (MIN(max_thread_ids_for_cache(topo_info, cache->share_level), 4095) << 14);
>   
>       assert(cache->line_size > 0);
>       assert(cache->partitions > 0);
> @@ -6743,13 +6749,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>                   int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
>   
>                   *eax &= ~0xFC000000;
> -                *eax |= max_core_ids_in_package(topo_info) << 26;
> +                *eax |= MIN(max_core_ids_in_package(topo_info), 63) << 26;
>                   if (host_vcpus_per_cache > threads_per_pkg) {
>                       *eax &= ~0x3FFC000;
>   
>                       /* Share the cache at package level. */
> -                    *eax |= max_thread_ids_for_cache(topo_info,
> -                                CPU_TOPOLOGY_LEVEL_SOCKET) << 14;
> +                    *eax |= MIN(max_thread_ids_for_cache(topo_info,
> +                                CPU_TOPOLOGY_LEVEL_SOCKET), 4095) << 14;
>                   }
>               }
>           } else if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) {