| Message ID | 156779718791.21957.9675425538561156773.stgit@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | APIC ID fixes for AMD EPYC CPU models | expand |
On Fri, Sep 06, 2019 at 07:13:09PM +0000, Moger, Babu wrote: > Adds new epyc property in PCMachineState and also in MachineState. > This property will be used to initialize the mode specific handlers > to generate apic ids. > > Signed-off-by: Babu Moger <babu.moger@amd.com> > --- [...] > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 12eb5032a5..0001d42e50 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -299,6 +299,8 @@ struct MachineState { > AccelState *accelerator; > CPUArchIdList *possible_cpus; > CpuTopology smp; > + bool epyc; > + This won't scale at all when we start adding new CPU models with different topology constraints. I still have hope we can avoid having separate set of topology ID functions (see my reply to "hw/386: Add new epyc mode topology decoding functions"). But if we really have to create separate functions, we can make them part of the CPU model table, not a boolean machine property.
On 10/10/19 10:59 PM, Eduardo Habkost wrote: > On Fri, Sep 06, 2019 at 07:13:09PM +0000, Moger, Babu wrote: >> Adds new epyc property in PCMachineState and also in MachineState. >> This property will be used to initialize the mode specific handlers >> to generate apic ids. >> >> Signed-off-by: Babu Moger <babu.moger@amd.com> >> --- > [...] >> diff --git a/include/hw/boards.h b/include/hw/boards.h >> index 12eb5032a5..0001d42e50 100644 >> --- a/include/hw/boards.h >> +++ b/include/hw/boards.h >> @@ -299,6 +299,8 @@ struct MachineState { >> AccelState *accelerator; >> CPUArchIdList *possible_cpus; >> CpuTopology smp; >> + bool epyc; >> + > > This won't scale at all when we start adding new CPU models with > different topology constraints. Yes, I knew. This could cause scaling issues. Let me see if we could do anything different to avoid this. > > I still have hope we can avoid having separate set of topology ID > functions (see my reply to "hw/386: Add new epyc mode topology Yes. That was (not to have separate topology functions) my hope too. Let me think thru this bit more. > decoding functions"). But if we really have to create separate > functions, we can make them part of the CPU model table, not a > boolean machine property. >
On 10/10/19 10:59 PM, Eduardo Habkost wrote: > On Fri, Sep 06, 2019 at 07:13:09PM +0000, Moger, Babu wrote: >> Adds new epyc property in PCMachineState and also in MachineState. >> This property will be used to initialize the mode specific handlers >> to generate apic ids. >> >> Signed-off-by: Babu Moger <babu.moger@amd.com> >> --- > [...] >> diff --git a/include/hw/boards.h b/include/hw/boards.h >> index 12eb5032a5..0001d42e50 100644 >> --- a/include/hw/boards.h >> +++ b/include/hw/boards.h >> @@ -299,6 +299,8 @@ struct MachineState { >> AccelState *accelerator; >> CPUArchIdList *possible_cpus; >> CpuTopology smp; >> + bool epyc; >> + > > This won't scale at all when we start adding new CPU models with > different topology constraints. Yes, I knew. This could cause scaling issues. Let me see if we could do anything different. > > I still have hope we can avoid having separate set of topology ID > functions (see my reply to "hw/386: Add new epyc mode topology Yes. That was my hope too. Let me think thru this bit more. I will come back on this. > decoding functions"). But if we really have to create separate > functions, we can make them part of the CPU model table, not a > boolean machine property. >
On Fri, Oct 11, 2019 at 04:59:37PM +0000, Moger, Babu wrote: > > On 10/10/19 10:59 PM, Eduardo Habkost wrote: > > On Fri, Sep 06, 2019 at 07:13:09PM +0000, Moger, Babu wrote: > >> Adds new epyc property in PCMachineState and also in MachineState. > >> This property will be used to initialize the mode specific handlers > >> to generate apic ids. > >> > >> Signed-off-by: Babu Moger <babu.moger@amd.com> > >> --- > > [...] > >> diff --git a/include/hw/boards.h b/include/hw/boards.h > >> index 12eb5032a5..0001d42e50 100644 > >> --- a/include/hw/boards.h > >> +++ b/include/hw/boards.h > >> @@ -299,6 +299,8 @@ struct MachineState { > >> AccelState *accelerator; > >> CPUArchIdList *possible_cpus; > >> CpuTopology smp; > >> + bool epyc; > >> + > > > > This won't scale at all when we start adding new CPU models with > > different topology constraints. > > Yes, I knew. This could cause scaling issues. Let me see if we could > do anything different. > > > > > I still have hope we can avoid having separate set of topology ID > > functions (see my reply to "hw/386: Add new epyc mode topology > > Yes. That was my hope too. Let me think thru this bit more. I will come > back on this. If you don't manage to use a common function in the next version, it's not a big deal. My main request is to make the calculations easier to follow (e.g. avoiding any expression with more than two terms, and always using an explicit "_per_*" suffix in all variables and constants). There's one possible problem I didn't realize yesterday: we might need a mechanism to force a field width to be larger than apicid_bitwidth_for_count(number_of_ids) (e.g. having 2 bits for core ID even if there's only 1 or 2 cores per CCX). Maybe the solution is to add optional field width parameters to X86CPUTopoInfo. Then we could redefine the width functions like this: static inline unsigned apicid_core_width(X86CPUTopoInfo *topo) { return MAX(apicid_bitwidth_for_count(topo->nr_cores), topo->min_core_bits); } Maybe we could replace the collection of fields with arrays to make all calculations generic. Untested example: enum TopoField { TOPO_FIELD_THREADS = 0, TOPO_FIELD_CORES, TOPO_FIELD_CCXS, /* AMD */ TOPO_FIELD_DIES = TOPO_FIELD_CCX, /* Intel */ TOPO_FIELD_NODES, TOPO_FIELD_PKG, MAX_TOPO_FIELD, }; struct TopoFieldDefinition { /* Number of IDs at this level */ unsigned count; /* Minimum number of APIC ID bits for this level */ unsigned min_width; }; struct X86CPUTopoInfo { TopoFieldDefinition fields[MAX_TOPO_FIELD]; }; struct X85CPUTopoIDs { unsigned ids[MAX_TOPO_FIELD]; }; static inline unsigned apicid_field_width(const X86CPUTopoInfo *topo, TopoField field) { TopoFieldDefinition *def = &topo->fields[field]; return MAX(apicid_bitwidth_for_count(def->count), def->min_width); } static inline unsigned apicid_field_offset(const X86CPUTopoInfo *topo, TopoField field) { if (field == 0) { return 0; } return apicid_field_offset(topo, field - 1) + apic_id_field_width(topo, field - 1); } static inline apic_id_t apicid_from_topo_ids(const X86CPUTopoInfo *topo, const X86CPUTopoIDs *ids) { TopoField field; apic_id_t r = 0; for (field = 0; l < MAX_TOPO_FIELD; l++) { unsigned offset = apicid_field_offset(topo, field); unsigned width = apicid_field_width(topo, field); assert(apicid_bitwidth_for_count(ids->ids[field] + 1) < apicid_field_width(topo, field)); r = deposit64(r, offset, width, ids->ids[field]; } return r; } static inline void x86_topo_ids_from_apicid(apic_id_t apicid, const X86CPUTopoInfo *topo, X86CPUTopoIDs *ids) { TopoField field; for (field = 0; l < MAX_TOPO_FIELD; l++) { unsigned offset = apicid_field_offset(topo, field); unsigned width = apicid_field_width(topo, field); ids->ids[field] = extract64(apicid, offset, width); } } > > > > decoding functions"). But if we really have to create separate > > functions, we can make them part of the CPU model table, not a > > boolean machine property. > >
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 959bd3821b..14760523a9 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -2810,6 +2810,22 @@ static void pc_machine_set_pit(Object *obj, bool value, Error **errp) pcms->pit_enabled = value; } +static bool pc_machine_get_epyc(Object *obj, Error **errp) +{ + PCMachineState *pcms = PC_MACHINE(obj); + + return pcms->epyc; +} + +static void pc_machine_set_epyc(Object *obj, bool value, Error **errp) +{ + PCMachineState *pcms = PC_MACHINE(obj); + MachineState *ms = MACHINE(pcms); + + pcms->epyc = value; + ms->epyc = value; +} + static void pc_machine_initfn(Object *obj) { PCMachineState *pcms = PC_MACHINE(obj); @@ -3015,6 +3031,13 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) object_class_property_add_bool(oc, PC_MACHINE_PIT, pc_machine_get_pit, pc_machine_set_pit, &error_abort); + + object_class_property_add_bool(oc, "epyc", + pc_machine_get_epyc, pc_machine_set_epyc, &error_abort); + + object_class_property_set_description(oc, "epyc", + "Set on/off to use epyc mode", &error_abort); + } static const TypeInfo pc_machine_info = { diff --git a/include/hw/boards.h b/include/hw/boards.h index 12eb5032a5..0001d42e50 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -299,6 +299,8 @@ struct MachineState { AccelState *accelerator; CPUArchIdList *possible_cpus; CpuTopology smp; + bool epyc; + struct NVDIMMState *nvdimms_state; }; diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index d6f1189997..cf9e7b0045 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -68,6 +68,7 @@ struct PCMachineState { uint64_t *node_mem; /* Apic id specific handlers */ + bool epyc; uint32_t (*apicid_from_cpu_idx)(X86CPUTopoInfo *topo_info, unsigned cpu_index); void (*topo_ids_from_apicid)(apic_id_t apicid, X86CPUTopoInfo *topo_info, X86CPUTopoIDs *topo_ids);
Adds new epyc property in PCMachineState and also in MachineState. This property will be used to initialize the mode specific handlers to generate apic ids. Signed-off-by: Babu Moger <babu.moger@amd.com> --- hw/i386/pc.c | 23 +++++++++++++++++++++++ include/hw/boards.h | 2 ++ include/hw/i386/pc.h | 1 + 3 files changed, 26 insertions(+)