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.mo...@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.
> >
--
Eduardo
