On Fri, 21 Feb 2020 11:51:15 -0600
Babu Moger <babu.mo...@amd.com> wrote:
> On 2/21/20 11:05 AM, Igor Mammedov wrote:
> > On Thu, 13 Feb 2020 12:16:51 -0600
> > Babu Moger <babu.mo...@amd.com> 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;
> >>
> >
>
