[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 16/18] hw/i386: Introduce EPYC mode function handlers
From: |
Babu Moger |
Subject: |
Re: [PATCH v3 16/18] hw/i386: Introduce EPYC mode function handlers |
Date: |
Tue, 28 Jan 2020 15:48:15 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 |
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 <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
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
>> *
>>
>>
>