Hi Xiaoyao,
On Mon, Aug 07, 2023 at 04:43:32PM +0800, Xiaoyao Li wrote:
> Date: Mon, 7 Aug 2023 16:43:32 +0800
> From: Xiaoyao Li <xiaoyao...@intel.com>
> Subject: Re: [PATCH v3 03/17] softmmu: Fix CPUSTATE.nr_cores' calculation
>
> On 8/7/2023 3:53 PM, Zhao Liu wrote:
> > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > > index 97ad229d8ba3..50613cd04612 100644
> > > > --- a/target/i386/cpu.c
> > > > +++ b/target/i386/cpu.c
> > > > @@ -6011,7 +6011,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t
> > > > index, uint32_t count,
> > > > X86CPUTopoInfo topo_info;
> > > > topo_info.dies_per_pkg = env->nr_dies;
> > > > - topo_info.cores_per_die = cs->nr_cores;
> > > > + topo_info.cores_per_die = cs->nr_cores / env->nr_dies;
> > > This and below things make me think that, it looks ugly that @nr_dies is
> > > added separately in struct CPUArchState for i386 because CPUState only has
> > > @nr_cores and nr_threads. Further, for i386, it defines a specific struct
> > > X86CPUTopoInfo to contain topology info when setting up CPUID. To me,
> > > struct
> > > X86CPUTopoInfo is redundant as struct CpuTopology.
> > >
> > > maybe we can carry a struct CpuTopology in CPUState, so that we can drop
> > > @nr_threads, @nr_cores in CPUState for all ARCHes, and @nr_dies in struct
> > > CPUArchState for i386. As well, topo_info can be dropped here.
> > Yeah, I agree. We think the same way, as did in [1].
> >
> > About X86CPUTopoInfo, it's still necessary to keep to help encode
> > APICID.
>
> typedef struct X86CPUTopoInfo {
> unsigned dies_per_pkg;
> unsigned cores_per_die;
> unsigned threads_per_core;
> } X86CPUTopoInfo;
>
> /**
> * CpuTopology:
> * @cpus: the number of present logical processors on the machine
> * @sockets: the number of sockets on the machine
> * @dies: the number of dies in one socket
> * @clusters: the number of clusters in one die
> * @cores: the number of cores in one cluster
> * @threads: the number of threads in one core
> * @max_cpus: the maximum number of logical processors on the machine
> */
> typedef struct CpuTopology {
> unsigned int cpus;
> unsigned int sockets;
> unsigned int dies;
> unsigned int clusters;
> unsigned int cores;
> unsigned int threads;
> unsigned int max_cpus;
> } CpuTopology;
>
> I think 'struct X86CPUTopoInfo' is just a subset of 'struct CpuTopology'
For smp topology, it's correct.
>
> IIUC, currently the value of X86CPUTopoInfo::dies_per_pkg should equal with
> CpuTopology::dies, and the same for cores_per_die and threads_per_core.
>
> So it's OK to keep an copy of 'struct CpuTopology' in CPUState and drop
> 'struct X86CPUTopoInfo'
>
> > For hybrid topology case, the APICID is likely discontinuous,
> > and the width of each CPU level in APICID depends on the maximum number
> > of elements in this level. So I also proposed to rename it to
> > X86ApicidTopoInfo [2] and count the maximum number of elements in each
> > CPU level.
>
> Do you mean, for example, for hybrid topology, X86CPUTopoInfo::dies_per_pkg
> != CpuTopology::dies? Or after rename
> X86CPUTopoInfo::max_dies != CpuTopology::dies?
I mean the latter.
A more typical example nowadays is thread level.
X86CPUTopoInfo::max_threads may not euqal to CpuTopology::threads,
since P core has 2 threads per core and E core doesn't support SMT.
The CpuTopology in CPUState should reflect the topology information for
current CPU, so CpuTopology::threads is 2 for P core and
CpuTopology::threads = 1 for E core.
But the width of the SMT level in APICID must be fixed, so that SMT width
should be determined by X86CPUTopoInfo::max_threads. Current hybrid
platforms implement it the same way.
Thanks,
Zhao
>
> > [2]:https://mail.gnu.org/archive/html/qemu-devel/2023-02/msg03237.html
> >
> > Thanks,
> > Zhao
> >
>
