Re: [Qemu-devel] [RFC 2 PATCH 13/16] machine: Add new epyc property in PCMachineState

[Eduardo Habkost <ehabkost@redhat.com>]

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.
> > 

-- 
Eduardo