From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 076285C97 for ; Mon, 15 Jan 2024 03:39:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="CX7qGyTk" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1705289986; x=1736825986; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=6g1CYHWdbZvGH8CrxRqJNFkQ90+0kq+qox0Ow1zDj4Y=; b=CX7qGyTk4UW1pPOt8Us8oZk8PcYvQQgRO/LB5VmCJjPQjIUvy4B3oHU5 lKo3HkEau9lyJBRTgJVaCEEPOncWbVkX90RdS11PKXFraCqoTF/Q1qEDN +T2BjtRJT2lBn1BXl/Z2eP7jhlyvV479iz3fF5H7LP0PArWQ3FYYbXXaX Wk5BaBuCpDKOzij+Oh0qqQjZMr8hEBndLSG085acbNwlQI6JnVjOyRPz1 Mw1kT00PFPllGs5Mi8iUilmh6sd9SqJd2eK1VB2UILtLAf5yOq+/1xAuV 6p4KRlFA8uTQE+JWTnViTpagXVF8ovULJ7ZjZGdLaKptycxZj6fyDVYIw A==; X-IronPort-AV: E=McAfee;i="6600,9927,10953"; a="465907924" X-IronPort-AV: E=Sophos;i="6.04,195,1695711600"; d="scan'208";a="465907924" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Jan 2024 19:39:45 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10953"; a="786962111" X-IronPort-AV: E=Sophos;i="6.04,195,1695711600"; d="scan'208";a="786962111" Received: from liuzhao-optiplex-7080.sh.intel.com (HELO localhost) ([10.239.160.36]) by fmsmga007.fm.intel.com with ESMTP; 14 Jan 2024 19:39:41 -0800 Date: Mon, 15 Jan 2024 11:52:39 +0800 From: Zhao Liu To: Xiaoyao Li Cc: Eduardo Habkost , Marcel Apfelbaum , "Michael S . Tsirkin" , Richard Henderson , Paolo Bonzini , Marcelo Tosatti , qemu-devel@nongnu.org, kvm@vger.kernel.org, Zhenyu Wang , Zhuocheng Ding , Zhao Liu , Babu Moger , Yongwei Ma Subject: Re: [PATCH v7 09/16] i386: Support module_id in X86CPUTopoIDs Message-ID: References: <20240108082727.420817-1-zhao1.liu@linux.intel.com> <20240108082727.420817-10-zhao1.liu@linux.intel.com> <1b2dd718-5968-4036-8b80-fd456058dce6@intel.com> Precedence: bulk X-Mailing-List: kvm@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1b2dd718-5968-4036-8b80-fd456058dce6@intel.com> Hi Xiaoyao, On Sun, Jan 14, 2024 at 08:42:00PM +0800, Xiaoyao Li wrote: > Date: Sun, 14 Jan 2024 20:42:00 +0800 > From: Xiaoyao Li > Subject: Re: [PATCH v7 09/16] i386: Support module_id in X86CPUTopoIDs > > On 1/8/2024 4:27 PM, Zhao Liu wrote: > > From: Zhuocheng Ding > > > > Add module_id member in X86CPUTopoIDs. > > > > module_id can be parsed from APIC ID, so also update APIC ID parsing > > rule to support module level. With this support, the conversions with > > module level between X86CPUTopoIDs, X86CPUTopoInfo and APIC ID are > > completed. > > > > module_id can be also generated from cpu topology, and before i386 > > supports "clusters" in smp, the default "clusters per die" is only 1, > > thus the module_id generated in this way is 0, so that it will not > > conflict with the module_id generated by APIC ID. > > > > Signed-off-by: Zhuocheng Ding > > Co-developed-by: Zhao Liu > > Signed-off-by: Zhao Liu > > Tested-by: Babu Moger > > Tested-by: Yongwei Ma > > Acked-by: Michael S. Tsirkin > > --- > > Changes since v1: > > * Merge the patch "i386: Update APIC ID parsing rule to support module > > level" into this one. (Yanan) > > * Move the apicid_module_width() and apicid_module_offset() support > > into the previous modules_per_die related patch. (Yanan) > > --- > > hw/i386/x86.c | 28 +++++++++++++++++++++------- > > include/hw/i386/topology.h | 17 +++++++++++++---- > > 2 files changed, 34 insertions(+), 11 deletions(-) > > > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > > index 85b847ac7914..5269aae3a5c2 100644 > > --- a/hw/i386/x86.c > > +++ b/hw/i386/x86.c > > @@ -315,11 +315,11 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev, > > /* > > * If APIC ID is not set, > > - * set it based on socket/die/core/thread properties. > > + * set it based on socket/die/cluster/core/thread properties. > > */ > > if (cpu->apic_id == UNASSIGNED_APIC_ID) { > > - int max_socket = (ms->smp.max_cpus - 1) / > > - smp_threads / smp_cores / ms->smp.dies; > > + int max_socket = (ms->smp.max_cpus - 1) / smp_threads / smp_cores / > > + ms->smp.clusters / ms->smp.dies; > > /* > > * die-id was optional in QEMU 4.0 and older, so keep it optional > > @@ -366,17 +366,27 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev, > > topo_ids.die_id = cpu->die_id; > > topo_ids.core_id = cpu->core_id; > > topo_ids.smt_id = cpu->thread_id; > > + > > + /* > > + * TODO: This is the temporary initialization for topo_ids.module_id to > > + * avoid "maybe-uninitialized" compilation errors. Will remove when > > + * X86CPU supports cluster_id. > > + */ > > + topo_ids.module_id = 0; > > > > if you put patch 10 before this patch, then we don't need this trick. Then, we need another trick to resolve "cpu->cluster_id = topo_ids.module_id;" in patch 10. ;-) Thanks, Zhao > > > cpu->apic_id = x86_apicid_from_topo_ids(&topo_info, &topo_ids); > > } > > cpu_slot = x86_find_cpu_slot(MACHINE(x86ms), cpu->apic_id, &idx); > > if (!cpu_slot) { > > x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids); > > + > > error_setg(errp, > > - "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with" > > - " APIC ID %" PRIu32 ", valid index range 0:%d", > > - topo_ids.pkg_id, topo_ids.die_id, topo_ids.core_id, topo_ids.smt_id, > > - cpu->apic_id, ms->possible_cpus->len - 1); > > + "Invalid CPU [socket: %u, die: %u, module: %u, core: %u, thread: %u]" > > + " with APIC ID %" PRIu32 ", valid index range 0:%d", > > + topo_ids.pkg_id, topo_ids.die_id, topo_ids.module_id, > > + topo_ids.core_id, topo_ids.smt_id, cpu->apic_id, > > + ms->possible_cpus->len - 1); > > return; > > } > > @@ -502,6 +512,10 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms) > > ms->possible_cpus->cpus[i].props.has_die_id = true; > > ms->possible_cpus->cpus[i].props.die_id = topo_ids.die_id; > > } > > + if (ms->smp.clusters > 1) { > > + ms->possible_cpus->cpus[i].props.has_cluster_id = true; > > + ms->possible_cpus->cpus[i].props.cluster_id = topo_ids.module_id; > > + } > > ms->possible_cpus->cpus[i].props.has_core_id = true; > > ms->possible_cpus->cpus[i].props.core_id = topo_ids.core_id; > > ms->possible_cpus->cpus[i].props.has_thread_id = true; > > diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h > > index 517e51768c13..ed1f3d6c1d5e 100644 > > --- a/include/hw/i386/topology.h > > +++ b/include/hw/i386/topology.h > > @@ -50,6 +50,7 @@ typedef uint32_t apic_id_t; > > typedef struct X86CPUTopoIDs { > > unsigned pkg_id; > > unsigned die_id; > > + unsigned module_id; > > unsigned core_id; > > unsigned smt_id; > > } X86CPUTopoIDs; > > @@ -127,6 +128,7 @@ static inline apic_id_t x86_apicid_from_topo_ids(X86CPUTopoInfo *topo_info, > > { > > return (topo_ids->pkg_id << apicid_pkg_offset(topo_info)) | > > (topo_ids->die_id << apicid_die_offset(topo_info)) | > > + (topo_ids->module_id << apicid_module_offset(topo_info)) | > > (topo_ids->core_id << apicid_core_offset(topo_info)) | > > topo_ids->smt_id; > > } > > @@ -140,12 +142,16 @@ static inline void x86_topo_ids_from_idx(X86CPUTopoInfo *topo_info, > > X86CPUTopoIDs *topo_ids) > > { > > unsigned nr_dies = topo_info->dies_per_pkg; > > - unsigned nr_cores = topo_info->cores_per_module * > > - topo_info->modules_per_die; > > + unsigned nr_modules = topo_info->modules_per_die; > > + unsigned nr_cores = topo_info->cores_per_module; > > unsigned nr_threads = topo_info->threads_per_core; > > - topo_ids->pkg_id = cpu_index / (nr_dies * nr_cores * nr_threads); > > - topo_ids->die_id = cpu_index / (nr_cores * nr_threads) % nr_dies; > > + topo_ids->pkg_id = cpu_index / (nr_dies * nr_modules * > > + nr_cores * nr_threads); > > + topo_ids->die_id = cpu_index / (nr_modules * nr_cores * > > + nr_threads) % nr_dies; > > + topo_ids->module_id = cpu_index / (nr_cores * nr_threads) % > > + nr_modules; > > topo_ids->core_id = cpu_index / nr_threads % nr_cores; > > topo_ids->smt_id = cpu_index % nr_threads; > > } > > @@ -163,6 +169,9 @@ static inline void x86_topo_ids_from_apicid(apic_id_t apicid, > > topo_ids->core_id = > > (apicid >> apicid_core_offset(topo_info)) & > > ~(0xFFFFFFFFUL << apicid_core_width(topo_info)); > > + topo_ids->module_id = > > + (apicid >> apicid_module_offset(topo_info)) & > > + ~(0xFFFFFFFFUL << apicid_module_width(topo_info)); > > topo_ids->die_id = > > (apicid >> apicid_die_offset(topo_info)) & > > ~(0xFFFFFFFFUL << apicid_die_width(topo_info)); >