| Message ID | 20230213095035.158240-42-zhao1.liu@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce hybrid CPU topology | expand |
On 13/2/23 10:50, Zhao Liu wrote: > From: Zhao Liu <zhao1.liu@intel.com> > > Since supported core types are architecture specific, we need this hook > to allow archs define its own parsing or validation method. > > As the example, add the x86 core_type() which will be used in "-hybrid" > parameter parsing. What would be a "core type" for other archs? > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > --- > hw/core/machine-topo.c | 14 ++++++++++++++ > hw/core/machine.c | 1 + > hw/i386/x86.c | 15 +++++++++++++++ > include/hw/boards.h | 7 +++++++ > 4 files changed, 37 insertions(+) > index 9364c90d5f1a..34ec035b5c9f 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -36,6 +36,7 @@ void machine_set_cpu_numa_node(MachineState *machine, > Error **errp); > void machine_parse_smp_config(MachineState *ms, > const SMPConfiguration *config, Error **errp); > +int machine_parse_hybrid_core_type(MachineState *ms, const char *coretype); > > /** > * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices > @@ -199,6 +200,11 @@ typedef struct { > * Return the type of KVM corresponding to the kvm-type string option or > * computed based on other criteria such as the host kernel capabilities. > * kvm-type may be NULL if it is not needed. > + * @core_type: > + * Return the type of hybrid cores corresponding to the coretype string > + * option. The default hook only accept "none" or "" since the most generic > + * core topology should not specify any specific core type. Each arch can > + * define its own core_type() hook to override the default one. > * @numa_mem_supported: > * true if '--numa node.mem' option is supported and false otherwise > * @hotplug_allowed: > @@ -237,6 +243,7 @@ struct MachineClass { > void (*reset)(MachineState *state, ShutdownCause reason); > void (*wakeup)(MachineState *state); > int (*kvm_type)(MachineState *machine, const char *arg); > + int (*core_type)(MachineState *state, const char *type); > > BlockInterfaceType block_default_type; > int units_per_default_bus;
On 13/2/23 10:50, Zhao Liu wrote: > From: Zhao Liu <zhao1.liu@intel.com> > > Since supported core types are architecture specific, we need this hook > to allow archs define its own parsing or validation method. > > As the example, add the x86 core_type() which will be used in "-hybrid" > parameter parsing. > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > --- > hw/core/machine-topo.c | 14 ++++++++++++++ > hw/core/machine.c | 1 + > hw/i386/x86.c | 15 +++++++++++++++ > include/hw/boards.h | 7 +++++++ > 4 files changed, 37 insertions(+) > > diff --git a/hw/core/machine-topo.c b/hw/core/machine-topo.c > index 12c05510c1b5..f9ab08a1252e 100644 > --- a/hw/core/machine-topo.c > +++ b/hw/core/machine-topo.c > @@ -352,3 +352,17 @@ void machine_parse_smp_config(MachineState *ms, > return; > } > } > + > +/* > + * machine_parse_hybrid_core_type: the default hook to parse hybrid core > + * type corresponding to the coretype > + * string option. > + */ > +int machine_parse_hybrid_core_type(MachineState *ms, const char *coretype) > +{ > + if (strcmp(coretype, "") == 0 || strcmp(coretype, "none") == 0) { > + return 0; > + } > + > + return -1; Shouldn't this use mc->core_type()? Where is it used? > +}
On Mon, Feb 13, 2023 at 02:33:22PM +0100, Philippe Mathieu-Daudé wrote: > Date: Mon, 13 Feb 2023 14:33:22 +0100 > From: Philippe Mathieu-Daudé <philmd@linaro.org> > Subject: Re: [RFC 41/52] machine: Introduce core_type() hook > > On 13/2/23 10:50, Zhao Liu wrote: > > From: Zhao Liu <zhao1.liu@intel.com> > > > > Since supported core types are architecture specific, we need this hook > > to allow archs define its own parsing or validation method. > > > > As the example, add the x86 core_type() which will be used in "-hybrid" > > parameter parsing. > > What would be a "core type" for other archs? For intel, core type refers to the CPUID.1AH, and it is different with CPU model. For example, Alder Lake is a CPU model, it can be marked as "Core" or "Atom". But for other architectures, the core type may be considered "cpu type" or "cpu model". Therefore, whether "core type" is used as a mark of a big or small core, or as a different cpu model/cpu type, each architecture needs to define this by itself. I do have limited understanding of other architectures. Do you think whether this explanation and handling is generic enough? Thanks, Zhao > > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > > --- > > hw/core/machine-topo.c | 14 ++++++++++++++ > > hw/core/machine.c | 1 + > > hw/i386/x86.c | 15 +++++++++++++++ > > include/hw/boards.h | 7 +++++++ > > 4 files changed, 37 insertions(+) > > > > index 9364c90d5f1a..34ec035b5c9f 100644 > > --- a/include/hw/boards.h > > +++ b/include/hw/boards.h > > @@ -36,6 +36,7 @@ void machine_set_cpu_numa_node(MachineState *machine, > > Error **errp); > > void machine_parse_smp_config(MachineState *ms, > > const SMPConfiguration *config, Error **errp); > > +int machine_parse_hybrid_core_type(MachineState *ms, const char *coretype); > > /** > > * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices > > @@ -199,6 +200,11 @@ typedef struct { > > * Return the type of KVM corresponding to the kvm-type string option or > > * computed based on other criteria such as the host kernel capabilities. > > * kvm-type may be NULL if it is not needed. > > + * @core_type: > > + * Return the type of hybrid cores corresponding to the coretype string > > + * option. The default hook only accept "none" or "" since the most generic > > + * core topology should not specify any specific core type. Each arch can > > + * define its own core_type() hook to override the default one. > > * @numa_mem_supported: > > * true if '--numa node.mem' option is supported and false otherwise > > * @hotplug_allowed: > > @@ -237,6 +243,7 @@ struct MachineClass { > > void (*reset)(MachineState *state, ShutdownCause reason); > > void (*wakeup)(MachineState *state); > > int (*kvm_type)(MachineState *machine, const char *arg); > > + int (*core_type)(MachineState *state, const char *type); > > BlockInterfaceType block_default_type; > > int units_per_default_bus; >
On Mon, Feb 13, 2023 at 02:35:24PM +0100, Philippe Mathieu-Daudé wrote: > Date: Mon, 13 Feb 2023 14:35:24 +0100 > From: Philippe Mathieu-Daudé <philmd@linaro.org> > Subject: Re: [RFC 41/52] machine: Introduce core_type() hook > > On 13/2/23 10:50, Zhao Liu wrote: > > From: Zhao Liu <zhao1.liu@intel.com> > > > > Since supported core types are architecture specific, we need this hook > > to allow archs define its own parsing or validation method. > > > > As the example, add the x86 core_type() which will be used in "-hybrid" > > parameter parsing. > > > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > > --- > > hw/core/machine-topo.c | 14 ++++++++++++++ > > hw/core/machine.c | 1 + > > hw/i386/x86.c | 15 +++++++++++++++ > > include/hw/boards.h | 7 +++++++ > > 4 files changed, 37 insertions(+) > > > > diff --git a/hw/core/machine-topo.c b/hw/core/machine-topo.c > > index 12c05510c1b5..f9ab08a1252e 100644 > > --- a/hw/core/machine-topo.c > > +++ b/hw/core/machine-topo.c > > @@ -352,3 +352,17 @@ void machine_parse_smp_config(MachineState *ms, > > return; > > } > > } > > + > > +/* > > + * machine_parse_hybrid_core_type: the default hook to parse hybrid core > > + * type corresponding to the coretype > > + * string option. > > + */ > > +int machine_parse_hybrid_core_type(MachineState *ms, const char *coretype) > > +{ > > + if (strcmp(coretype, "") == 0 || strcmp(coretype, "none") == 0) { > > + return 0; > > + } > > + > > + return -1; > > Shouldn't this use mc->core_type()? Where is it used? This interface is used in "[RFC 44/52] machine: Add "-hybrid" parsing rule"[1] to check the "coretype" info passed from command line: static void insert_core_into_cluster(MachineState *ms, HybridCluster *cluster, const HybridCoreOptions *config, Error **errp) { ... ret = mc->core_type(ms, config->coretype); if (!ret) { error_setg(errp, "Invalid coretype=%s", config->coretype); } core_pack->core.core_type = ret; ... } I use that machine_parse_hybrid_core_type() as the default mc->core_type() implementation. Other arch can override this default core_type() if necessary, for example x86 has its own core_type implementation: x86_parse_hybrid_core_type(). This default core_type() avoids useless and wrong core_type information from the command line so that I can expose core type info in query_cpus_fast. [1]: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg03250.html Zhao > > > +}
On Thu, Feb 16, 2023 at 08:15:23PM +0800, wangyanan (Y) wrote: > Date: Thu, 16 Feb 2023 20:15:23 +0800 > From: "wangyanan (Y)" <wangyanan55@huawei.com> > Subject: Re: [RFC 41/52] machine: Introduce core_type() hook > > Hi Zhao, > > 在 2023/2/13 17:50, Zhao Liu 写道: > > From: Zhao Liu <zhao1.liu@intel.com> > > > > Since supported core types are architecture specific, we need this hook > > to allow archs define its own parsing or validation method. > > > > As the example, add the x86 core_type() which will be used in "-hybrid" > > parameter parsing. > > > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > > --- > > hw/core/machine-topo.c | 14 ++++++++++++++ > > hw/core/machine.c | 1 + > > hw/i386/x86.c | 15 +++++++++++++++ > > include/hw/boards.h | 7 +++++++ > > 4 files changed, 37 insertions(+) > > > > diff --git a/hw/core/machine-topo.c b/hw/core/machine-topo.c > > index 12c05510c1b5..f9ab08a1252e 100644 > > --- a/hw/core/machine-topo.c > > +++ b/hw/core/machine-topo.c > > @@ -352,3 +352,17 @@ void machine_parse_smp_config(MachineState *ms, > > return; > > } > > } > > + > > +/* > > + * machine_parse_hybrid_core_type: the default hook to parse hybrid core > > + * type corresponding to the coretype > > + * string option. > > + */ > > +int machine_parse_hybrid_core_type(MachineState *ms, const char *coretype) > > +{ > > + if (strcmp(coretype, "") == 0 || strcmp(coretype, "none") == 0) { > > + return 0; > > + } > > + > > + return -1; > > +} > Is it possible that coretype can be NULL? > What would *coretype be if the users don't explicitly specify coretype > in the command line? At present, the coretype field cannot be omitted, which requires other code changes to support omission (if omission is required in the future, there should be an arch-specific method to supplement the default coretype at the same time). > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > index fad990f49b03..acc32b3be5f6 100644 > > --- a/hw/core/machine.c > > +++ b/hw/core/machine.c > > @@ -926,6 +926,7 @@ static void machine_class_init(ObjectClass *oc, void *data) > > * On Linux, each node's border has to be 8MB aligned > > */ > > mc->numa_mem_align_shift = 23; > > + mc->core_type = machine_parse_hybrid_core_type; > > object_class_property_add_str(oc, "kernel", > > machine_get_kernel, machine_set_kernel); > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > > index f381fdc43180..f58a90359170 100644 > > --- a/hw/i386/x86.c > > +++ b/hw/i386/x86.c > > @@ -1569,6 +1569,20 @@ static void machine_set_sgx_epc(Object *obj, Visitor *v, const char *name, > > qapi_free_SgxEPCList(list); > > } > > +static int x86_parse_hybrid_core_type(MachineState *ms, const char *coretype) > > +{ > > + X86HybridCoreType type; > > + > > + if (strcmp(coretype, "atom") == 0) { > > + type = INTEL_ATOM_TYPE; > > + } else if (strcmp(coretype, "core") == 0) { > > + type = INTEL_CORE_TYPE; > > + } else { > > + type = INVALID_HYBRID_TYPE; > > + } > What about: > INTEL_CORE_TYPE_ATOM > INTEL_CORE_TYPE_CORE > X86_CORE_TYPE_UNKNOWN ? > just a suggestion. It looks better! Thanks. > > Thanks, > Yanan > > + return type; > > +} > > + > > static void x86_machine_initfn(Object *obj) > > { > > X86MachineState *x86ms = X86_MACHINE(obj); > > @@ -1596,6 +1610,7 @@ static void x86_machine_class_init(ObjectClass *oc, void *data) > > x86mc->save_tsc_khz = true; > > x86mc->fwcfg_dma_enabled = true; > > nc->nmi_monitor_handler = x86_nmi; > > + mc->core_type = x86_parse_hybrid_core_type; > > object_class_property_add(oc, X86_MACHINE_SMM, "OnOffAuto", > > x86_machine_get_smm, x86_machine_set_smm, > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > index 9364c90d5f1a..34ec035b5c9f 100644 > > --- a/include/hw/boards.h > > +++ b/include/hw/boards.h > > @@ -36,6 +36,7 @@ void machine_set_cpu_numa_node(MachineState *machine, > > Error **errp); > > void machine_parse_smp_config(MachineState *ms, > > const SMPConfiguration *config, Error **errp); > > +int machine_parse_hybrid_core_type(MachineState *ms, const char *coretype); > > /** > > * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices > > @@ -199,6 +200,11 @@ typedef struct { > > * Return the type of KVM corresponding to the kvm-type string option or > > * computed based on other criteria such as the host kernel capabilities. > > * kvm-type may be NULL if it is not needed. > > + * @core_type: > > + * Return the type of hybrid cores corresponding to the coretype string > > + * option. The default hook only accept "none" or "" since the most generic > > + * core topology should not specify any specific core type. Each arch can > > + * define its own core_type() hook to override the default one. > > * @numa_mem_supported: > > * true if '--numa node.mem' option is supported and false otherwise > > * @hotplug_allowed: > > @@ -237,6 +243,7 @@ struct MachineClass { > > void (*reset)(MachineState *state, ShutdownCause reason); > > void (*wakeup)(MachineState *state); > > int (*kvm_type)(MachineState *machine, const char *arg); > > + int (*core_type)(MachineState *state, const char *type); > > BlockInterfaceType block_default_type; > > int units_per_default_bus; >
diff --git a/hw/core/machine-topo.c b/hw/core/machine-topo.c index 12c05510c1b5..f9ab08a1252e 100644 --- a/hw/core/machine-topo.c +++ b/hw/core/machine-topo.c @@ -352,3 +352,17 @@ void machine_parse_smp_config(MachineState *ms, return; } } + +/* + * machine_parse_hybrid_core_type: the default hook to parse hybrid core + * type corresponding to the coretype + * string option. + */ +int machine_parse_hybrid_core_type(MachineState *ms, const char *coretype) +{ + if (strcmp(coretype, "") == 0 || strcmp(coretype, "none") == 0) { + return 0; + } + + return -1; +} diff --git a/hw/core/machine.c b/hw/core/machine.c index fad990f49b03..acc32b3be5f6 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -926,6 +926,7 @@ static void machine_class_init(ObjectClass *oc, void *data) * On Linux, each node's border has to be 8MB aligned */ mc->numa_mem_align_shift = 23; + mc->core_type = machine_parse_hybrid_core_type; object_class_property_add_str(oc, "kernel", machine_get_kernel, machine_set_kernel); diff --git a/hw/i386/x86.c b/hw/i386/x86.c index f381fdc43180..f58a90359170 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -1569,6 +1569,20 @@ static void machine_set_sgx_epc(Object *obj, Visitor *v, const char *name, qapi_free_SgxEPCList(list); } +static int x86_parse_hybrid_core_type(MachineState *ms, const char *coretype) +{ + X86HybridCoreType type; + + if (strcmp(coretype, "atom") == 0) { + type = INTEL_ATOM_TYPE; + } else if (strcmp(coretype, "core") == 0) { + type = INTEL_CORE_TYPE; + } else { + type = INVALID_HYBRID_TYPE; + } + return type; +} + static void x86_machine_initfn(Object *obj) { X86MachineState *x86ms = X86_MACHINE(obj); @@ -1596,6 +1610,7 @@ static void x86_machine_class_init(ObjectClass *oc, void *data) x86mc->save_tsc_khz = true; x86mc->fwcfg_dma_enabled = true; nc->nmi_monitor_handler = x86_nmi; + mc->core_type = x86_parse_hybrid_core_type; object_class_property_add(oc, X86_MACHINE_SMM, "OnOffAuto", x86_machine_get_smm, x86_machine_set_smm, diff --git a/include/hw/boards.h b/include/hw/boards.h index 9364c90d5f1a..34ec035b5c9f 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -36,6 +36,7 @@ void machine_set_cpu_numa_node(MachineState *machine, Error **errp); void machine_parse_smp_config(MachineState *ms, const SMPConfiguration *config, Error **errp); +int machine_parse_hybrid_core_type(MachineState *ms, const char *coretype); /** * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices @@ -199,6 +200,11 @@ typedef struct { * Return the type of KVM corresponding to the kvm-type string option or * computed based on other criteria such as the host kernel capabilities. * kvm-type may be NULL if it is not needed. + * @core_type: + * Return the type of hybrid cores corresponding to the coretype string + * option. The default hook only accept "none" or "" since the most generic + * core topology should not specify any specific core type. Each arch can + * define its own core_type() hook to override the default one. * @numa_mem_supported: * true if '--numa node.mem' option is supported and false otherwise * @hotplug_allowed: @@ -237,6 +243,7 @@ struct MachineClass { void (*reset)(MachineState *state, ShutdownCause reason); void (*wakeup)(MachineState *state); int (*kvm_type)(MachineState *machine, const char *arg); + int (*core_type)(MachineState *state, const char *type); BlockInterfaceType block_default_type; int units_per_default_bus;