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

[Babu Moger <babu.moger@amd.com>]

On 1/28/20 2:04 PM, Eduardo Habkost wrote:
> Hi,
> 
> Sorry for taking so long.  I was away from the office for a
> month, and now I'm finally back.

no worries.

> 
> 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 <babu.moger@amd.com>
>> ---
>>  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

Yes. you did mention before. I was not sure how to achieve  without
comparing the model string

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

Yes. We need to have a special behavior for specific model.
I will look at both these above approaches closely. Challenge is this
needs to be done much early in the initialization(before parse_numa_opts
or machine_run_board_init). Will research more on this.

> 
> 
> 
>> +        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
>>   *
>>
>>
>