Re: [PATCH v4 04/16] hw/i386: Introduce init_topo_info to initialize X86CPUTopoInfo

[Igor Mammedov]

On Fri, 21 Feb 2020 11:51:15 -0600
Babu Moger <address@hidden> wrote:

> On 2/21/20 11:05 AM, Igor Mammedov wrote:
> > On Thu, 13 Feb 2020 12:16:51 -0600
> > Babu Moger <address@hidden> wrote:
> >   
> >> Initialize all the parameters in one function init_topo_info.  
> > 
> > is it possible to squash it in 2/16
> >   
> Sure. We can do that.
> >   
> >>
> >> Move the data structure X86CPUTopoIDs and X86CPUTopoInfo into
> >> x86.h.  
> > A reason why it's moved should be here.  
> 
> Apicid functions will be part of X86MachineState data structure(patches
> introduced later).These functions will use X86CPUTopoIDs and
> X86CPUTopoInfo definition. Will add these details. Thanks

why not just include topology.h into the X86MachineState header,
and keep topo structures/functions where they are now?
(I dislike a little scattering consolidated pieces across multiple files,
but what worries me more is that it makes target/i386/cpu.c via
topology.h -> x86.h chain pull in a lot of unrelated dependencies)

So I'd keep X86CPUTopoIDs and X86CPUTopoInfo in topology.h 

[...]
> >> +static inline void init_topo_info(X86CPUTopoInfo *topo_info,
> >> +                                  const X86MachineState *x86ms)
> >> +{
> >> +    MachineState *ms = MACHINE(x86ms);
> >> +
> >> +    topo_info->dies_per_pkg = x86ms->smp_dies;
> >> +    topo_info->cores_per_die = ms->smp.cores;
> >> +    topo_info->threads_per_core = ms->smp.threads;
> >> +}

this is pure machine specific helper, and aren't used anywhere else
beside machine code.
Suggest to put it in pc.c or x86.c to keep topology.h machine independent.

> >>  
> >>  /* Return the bit width needed for 'count' IDs
> >>   */
> >> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
> >> index 4b84917885..ad62b01cf2 100644
> >> --- a/include/hw/i386/x86.h
> >> +++ b/include/hw/i386/x86.h
> >> @@ -36,6 +36,23 @@ typedef struct {
> >>      bool compat_apic_id_mode;
> >>  } X86MachineClass;
> >>  
> >> +/* APIC IDs can be 32-bit, but beware: APIC IDs > 255 require x2APIC 
> >> support
> >> + */
> >> +typedef uint32_t apic_id_t;
> >> +
> >> +typedef struct X86CPUTopoIDs {
> >> +    unsigned pkg_id;
> >> +    unsigned die_id;
> >> +    unsigned core_id;
> >> +    unsigned smt_id;
> >> +} X86CPUTopoIDs;
> >> +
> >> +typedef struct X86CPUTopoInfo {
> >> +    unsigned dies_per_pkg;
> >> +    unsigned cores_per_die;
> >> +    unsigned threads_per_core;
> >> +} X86CPUTopoInfo;
> >> +
> >>  typedef struct {
> >>      /*< private >*/
> >>      MachineState parent;
> >>  
> >   
>