Re: [Qemu-devel] [RFC 2 PATCH 13/16] machine: Add new epyc property in PCMachineState
On Fri, Oct 11, 2019 at 04:59:37PM +, Moger, Babu wrote:
>
> On 10/10/19 10:59 PM, Eduardo Habkost wrote:
> > On Fri, Sep 06, 2019 at 07:13:09PM +, 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
> >> ---
> > [...]
> >> 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 = >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
Re: [Qemu-devel] [RFC 2 PATCH 13/16] machine: Add new epyc property in PCMachineState
On 10/10/19 10:59 PM, Eduardo Habkost wrote:
> On Fri, Sep 06, 2019 at 07:13:09PM +, 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
>> ---
> [...]
>> 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.
>
Re: [Qemu-devel] [RFC 2 PATCH 13/16] machine: Add new epyc property in PCMachineState
On 10/10/19 10:59 PM, Eduardo Habkost wrote:
> On Fri, Sep 06, 2019 at 07:13:09PM +, 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
>> ---
> [...]
>> 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.
>
Re: [Qemu-devel] [RFC 2 PATCH 13/16] machine: Add new epyc property in PCMachineState
On Fri, Sep 06, 2019 at 07:13:09PM +, 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
> ---
[...]
> 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.
--
Eduardo
[Qemu-devel] [RFC 2 PATCH 13/16] machine: Add new epyc property in PCMachineState
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
---
hw/i386/pc.c | 23 +++
include/hw/boards.h |2 ++
include/hw/i386/pc.h |1 +
3 files changed, 26 insertions(+)
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, _abort);
+
+object_class_property_add_bool(oc, "epyc",
+pc_machine_get_epyc, pc_machine_set_epyc, _abort);
+
+object_class_property_set_description(oc, "epyc",
+"Set on/off to use epyc mode", _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);
