Re: [PATCH v3 16/18] hw/i386: Introduce EPYC mode function handlers

[Eduardo Habkost]

Hi,

Sorry for taking so long.  I was away from the office for a
month, and now I'm finally back.

On Tue, Dec 03, 2019 at 06:38:46PM -0600, Babu Moger wrote:
> Introduce following handlers for new epyc mode.
> x86_apicid_from_cpu_idx_epyc: Generate apicid from cpu index.
> x86_topo_ids_from_apicid_epyc: Generate topo ids from apic id.
> x86_apicid_from_topo_ids_epyc: Generate apicid from topo ids.
> 
> Signed-off-by: Babu Moger <address@hidden>
> ---
>  hw/i386/pc.c               |   12 ++++++++++++
>  include/hw/i386/topology.h |    4 ++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e6c8a458e7..64e3658873 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2819,6 +2819,17 @@ static bool pc_hotplug_allowed(MachineState *ms, 
> DeviceState *dev, Error **errp)
>      return true;
>  }
>  
> +static void pc_init_apicid_fn(MachineState *ms)
> +{
> +    PCMachineState *pcms = PC_MACHINE(ms);
> +
> +    if (!strncmp(ms->cpu_type, "EPYC", 4)) {

Please never use string comparison to introduce device-specific
behavior.  I had already pointed this out at
https://lore.kernel.org/qemu-devel/address@hidden/

If you need a CPU model to provide special behavior,
you have two options:

* Add a method pointer to X86CPUClass and/or X86CPUDefinition
* Add a QOM property to enable/disable special behavior, and
  include the property in the CPU model definition.

The second option might be preferable long term, but might
require more work because the property would become visible in
query-cpu-model-expansion and in the command line.  The first
option may be acceptable to avoid extra user-visible complexity
in the first version.



> +        pcms->apicid_from_cpu_idx = x86_apicid_from_cpu_idx_epyc;
> +        pcms->topo_ids_from_apicid = x86_topo_ids_from_apicid_epyc;
> +        pcms->apicid_from_topo_ids = x86_apicid_from_topo_ids_epyc;

Why do you need to override the function pointers in
PCMachineState instead of just looking up the relevant info at
X86CPUClass?

If both machine-types and CPU models are supposed to override the
APIC ID calculation functions, the interaction between
machine-type and CPU model needs to be better documented
(preferably with simple test cases) to ensure we won't break
compatibility later.

> +    }
> +}
> +
>  static void pc_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -2847,6 +2858,7 @@ static void pc_machine_class_init(ObjectClass *oc, void 
> *data)
>      mc->cpu_index_to_instance_props = pc_cpu_index_to_props;
>      mc->get_default_cpu_node_id = pc_get_default_cpu_node_id;
>      mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
> +    mc->init_apicid_fn = pc_init_apicid_fn;
>      mc->auto_enable_numa_with_memhp = true;
>      mc->has_hotpluggable_cpus = true;
>      mc->default_boot_order = "cad";
> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
> index b2b9e93a06..f028d2332a 100644
> --- a/include/hw/i386/topology.h
> +++ b/include/hw/i386/topology.h
> @@ -140,7 +140,7 @@ static inline unsigned 
> apicid_pkg_offset_epyc(X86CPUTopoInfo *topo_info)
>   *
>   * The caller must make sure core_id < nr_cores and smt_id < nr_threads.
>   */
> -static inline apic_id_t apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info,
> +static inline apic_id_t x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo 
> *topo_info,
>                                                    const X86CPUTopoIDs 
> *topo_ids)
>  {
>      return (topo_ids->pkg_id  << apicid_pkg_offset_epyc(topo_info)) |
> @@ -200,7 +200,7 @@ static inline apic_id_t 
> x86_apicid_from_cpu_idx_epyc(X86CPUTopoInfo *topo_info,
>  {
>      X86CPUTopoIDs topo_ids;
>      x86_topo_ids_from_idx_epyc(topo_info, cpu_index, &topo_ids);
> -    return apicid_from_topo_ids_epyc(topo_info, &topo_ids);
> +    return x86_apicid_from_topo_ids_epyc(topo_info, &topo_ids);
>  }
>  /* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
>   *
> 
> 

-- 
Eduardo