Re: [PATCH v9 06/21] i386/cpu: Use APIC ID info to encode cache topo in CPUID[4]

From: Zhao Liu <zhao1.liu@linux.intel.com>
To: Xiaoyao Li <xiaoyao.li@intel.com>
Cc: "Eduardo Habkost" <eduardo@habkost.net>,
	"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
	"Philippe Mathieu-Daud�" <philmd@linaro.org>,
	"Yanan Wang" <wangyanan55@huawei.com>,
	"Michael S . Tsirkin" <mst@redhat.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Eric Blake" <eblake@redhat.com>,
	"Markus Armbruster" <armbru@redhat.com>,
	"Marcelo Tosatti" <mtosatti@redhat.com>,
	"Daniel P . Berrang�" <berrange@redhat.com>,
	qemu-devel@nongnu.org, kvm@vger.kernel.org,
	"Zhenyu Wang" <zhenyu.z.wang@intel.com>,
	"Zhuocheng Ding" <zhuocheng.ding@intel.com>,
	"Babu Moger" <babu.moger@amd.com>,
	"Yongwei Ma" <yongwei.ma@intel.com>,
	"Zhao Liu" <zhao1.liu@intel.com>,
	"Robert Hoo" <robert.hu@linux.intel.com>
Subject: Re: [PATCH v9 06/21] i386/cpu: Use APIC ID info to encode cache topo in CPUID[4]
Date: Mon, 11 Mar 2024 16:23:57 +0800	[thread overview]
Message-ID: <Ze6/naoIIXj/DfTv@intel.com> (raw)
In-Reply-To: <Ze23y7UzGxnsyo6O@intel.com>

Hi Xiaoyao,

Did the following reason convince you? Could I take your r/b tag with
current code? ;-)

Thanks,
Zhao

On Sun, Mar 10, 2024 at 09:38:19PM +0800, Zhao Liu wrote:
> Date: Sun, 10 Mar 2024 21:38:19 +0800
> From: Zhao Liu <zhao1.liu@linux.intel.com>
> Subject: Re: [PATCH v9 06/21] i386/cpu: Use APIC ID info to encode cache
>  topo in CPUID[4]
> 
> Hi Xiaoyao,
> 
> > >               case 3: /* L3 cache info */
> > > -                die_offset = apicid_die_offset(&topo_info);
> > >                   if (cpu->enable_l3_cache) {
> > > +                    addressable_threads_width = apicid_die_offset(&topo_info);
> > 
> > Please get rid of the local variable @addressable_threads_width.
> > 
> > It is truly confusing.
> 
> There're several reasons for this:
> 
> 1. This commit is trying to use APIC ID topology layout to decode 2
> cache topology fields in CPUID[4], CPUID.04H:EAX[bits 25:14] and
> CPUID.04H:EAX[bits 31:26]. When there's a addressable_cores_width to map
> to CPUID.04H:EAX[bits 31:26], it's more clear to also map
> CPUID.04H:EAX[bits 25:14] to another variable.
> 
> 2. All these 2 variables are temporary in this commit, and they will be
> replaed by 2 helpers in follow-up cleanup of this series.
> 
> 3. Similarly, to make it easier to clean up later with the helper and
> for more people to review, it's neater to explicitly indicate the
> CPUID.04H:EAX[bits 25:14] with a variable here.
> 
> 4. I call this field as addressable_threads_width since it's "Maximum
> number of addressable IDs for logical processors sharing this cache".
> 
> Its own name is long, but given the length, only individual words could
> be selected as names.
> 
> TBH, "addressable" deserves more emphasis than "sharing". The former
> emphasizes the fact that the number of threads chosen here is based on
> the APIC ID layout and does not necessarily correspond to actual threads.
> 
> Therefore, this variable is needed here.
> 
> Thanks,
> Zhao
> 