Hi Xiaoyao, On Wed, Jan 10, 2024 at 07:52:38PM +0800, Xiaoyao Li wrote: > Date: Wed, 10 Jan 2024 19:52:38 +0800 > From: Xiaoyao Li <xiaoyao.li@xxxxxxxxx> > Subject: Re: [PATCH v7 03/16] i386/cpu: Consolidate the use of topo_info in > cpu_x86_cpuid() > > On 1/8/2024 4:27 PM, Zhao Liu wrote: > > From: Zhao Liu <zhao1.liu@xxxxxxxxx> > > > > In cpu_x86_cpuid(), there are many variables in representing the cpu > > topology, e.g., topo_info, cs->nr_cores/cs->nr_threads. > > Please use comma instead of slash. cs->nr_cores/cs->nr_threads looks like > one variable. Okay. > > > Since the names of cs->nr_cores/cs->nr_threads does not accurately > > represent its meaning, the use of cs->nr_cores/cs->nr_threads is prone > > to confusion and mistakes. > > > > And the structure X86CPUTopoInfo names its members clearly, thus the > > variable "topo_info" should be preferred. > > > > In addition, in cpu_x86_cpuid(), to uniformly use the topology variable, > > replace env->dies with topo_info.dies_per_pkg as well. > > > > Suggested-by: Robert Hoo <robert.hu@xxxxxxxxxxxxxxx> > > Signed-off-by: Zhao Liu <zhao1.liu@xxxxxxxxx> > > Tested-by: Babu Moger <babu.moger@xxxxxxx> > > Tested-by: Yongwei Ma <yongwei.ma@xxxxxxxxx> > > Acked-by: Michael S. Tsirkin <mst@xxxxxxxxxx> > > --- > > Changes since v3: > > * Fix typo. (Babu) > > > > Changes since v1: > > * Extract cores_per_socket from the code block and use it as a local > > variable for cpu_x86_cpuid(). (Yanan) > > * Remove vcpus_per_socket variable and use cpus_per_pkg directly. > > (Yanan) > > * Replace env->dies with topo_info.dies_per_pkg in cpu_x86_cpuid(). > > --- > > target/i386/cpu.c | 31 ++++++++++++++++++------------- > > 1 file changed, 18 insertions(+), 13 deletions(-) > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index c8d2a585723a..6f8fa772ecf8 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -6017,11 +6017,16 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > > uint32_t limit; > > uint32_t signature[3]; > > X86CPUTopoInfo topo_info; > > + uint32_t cores_per_pkg; > > + uint32_t cpus_per_pkg; > > I prefer to lps_per_pkg or threads_per_pkg. Okay, lp is not common in QEMU code, so I would change this to threads_per_pkg. > > Other than it, > > Reviewed-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx> Thanks! -Zhao
