Re: [RFC 41/52] machine: Introduce core_type() hook

[Zhao Liu]

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