From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 4E998EB64DD for ; Mon, 7 Aug 2023 10:09:01 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qSxAF-0006Yr-UI; Mon, 07 Aug 2023 06:08:39 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qSxAD-0006Yj-OC for qemu-devel@nongnu.org; Mon, 07 Aug 2023 06:08:38 -0400 Received: from mgamail.intel.com ([134.134.136.20]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qSxAA-00084n-IQ for qemu-devel@nongnu.org; Mon, 07 Aug 2023 06:08:37 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1691402914; x=1722938914; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to; bh=RPPkZOEXQkJxaarBuJTbiKtTrCrmDondyVCJseq/XF4=; b=HbCIwGiyzLK6rIdGNa08L9vWx9Hvw5JANcPrXtgbzzbk8Fq9j6z8CSka ij2QZ5DBKCWjn980/NUYzRSb6tKtB0BC7BVk/IeCQOLxW6+enXR1tvO+s 4trUudoDFvnwD/ZhEIaC3wR5AK3OQX/uodJ0nZSLakkfFQDcewZ+LzGY5 ZQ8NsGx6Mo4b7JxtDBtsgKbTIaiWZCLsbHRGV0r2Beoyl0V+X/X7hTBjO lOigQUmugpwzoGK+2Bvfw+6Y7c0caVBttVIG2/QdWDP0ELBV0O1mX9mhh WSsuw9pRwFIqMwuCzZYsBE7Zlpl9ArIQwtbdw2YXKgKtCUD6AUSEBPAzK g==; X-IronPort-AV: E=McAfee;i="6600,9927,10794"; a="360588778" X-IronPort-AV: E=Sophos;i="6.01,261,1684825200"; d="scan'208,217";a="360588778" Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Aug 2023 03:08:30 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10794"; a="854612472" X-IronPort-AV: E=Sophos;i="6.01,261,1684825200"; d="scan'208,217";a="854612472" Received: from qianwen-mobl1.ccr.corp.intel.com (HELO [10.238.5.29]) ([10.238.5.29]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Aug 2023 03:08:28 -0700 Content-Type: multipart/alternative; boundary="------------BveG2m08j62bXEYQ7FLKi7fR" Message-ID: Date: Mon, 7 Aug 2023 18:08:26 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.14.0 Subject: Re: [PATCH] target/i386: Avoid cpu number overflow in legacy topology Content-Language: en-US To: Xiaoyao Li , qemu-devel@nongnu.org Cc: zhao1.liu@intel.com, Paolo Bonzini , richard.henderson@linaro.org, babu.moger@amd.com References: <20230728080150.2958048-1-qian.wen@intel.com> <8b4fd5e9-8c0b-1ece-ebb1-85d027cc1155@intel.com> From: "Wen, Qian" In-Reply-To: <8b4fd5e9-8c0b-1ece-ebb1-85d027cc1155@intel.com> Received-SPF: pass client-ip=134.134.136.20; envelope-from=qian.wen@intel.com; helo=mgamail.intel.com X-Spam_score_int: -85 X-Spam_score: -8.6 X-Spam_bar: -------- X-Spam_report: (-8.6 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=0.001, NICE_REPLY_A=-4.139, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org This is a multi-part message in MIME format. --------------BveG2m08j62bXEYQ7FLKi7fR Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 >> --- >>   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; >>           } >>           /* > --------------BveG2m08j62bXEYQ7FLKi7fR Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit
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;
          }
          /*

--------------BveG2m08j62bXEYQ7FLKi7fR--