From: Eduardo Habkost <ehabkost@redhat.com>
To: Babu Moger <babu.moger@amd.com>
Cc: mst@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org,
imammedo@redhat.com, pbonzini@redhat.com, rth@twiddle.net
Subject: Re: [PATCH v3 16/18] hw/i386: Introduce EPYC mode function handlers
Date: Tue, 28 Jan 2020 15:04:38 -0500 [thread overview]
Message-ID: <20200128200438.GJ18770@habkost.net> (raw)
In-Reply-To: <157541992659.46157.18191224973398213624.stgit@naples-babu.amd.com>
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 <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
https://lore.kernel.org/qemu-devel/20190801192830.GD20035@habkost.net/
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
next prev parent reply other threads:[~2020-01-28 20:05 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-04 0:36 [PATCH v3 00/18] APIC ID fixes for AMD EPYC CPU models Babu Moger
2019-12-04 0:37 ` [PATCH v3 01/18] hw/i386: Rename X86CPUTopoInfo structure to X86CPUTopoIDs Babu Moger
2020-02-03 15:08 ` Igor Mammedov
2020-02-03 18:25 ` Babu Moger
2019-12-04 0:37 ` [PATCH v3 02/18] hw/i386: Introduce X86CPUTopoInfo to contain topology info Babu Moger
2020-01-28 15:44 ` Igor Mammedov
2019-12-04 0:37 ` [PATCH v3 03/18] hw/i386: Consolidate topology functions Babu Moger
2020-01-28 15:46 ` Igor Mammedov
2019-12-04 0:37 ` [PATCH v3 04/18] hw/i386: Introduce initialize_topo_info to initialize X86CPUTopoInfo Babu Moger
2020-01-28 15:49 ` Igor Mammedov
2020-01-28 16:42 ` Babu Moger
2019-12-04 0:37 ` [PATCH v3 05/18] machine: Add SMP Sockets in CpuTopology Babu Moger
2019-12-04 0:37 ` [PATCH v3 06/18] hw/core: Add core complex id in X86CPU topology Babu Moger
2020-01-28 16:27 ` Igor Mammedov
2020-01-28 16:44 ` Babu Moger
2020-01-28 16:31 ` Eric Blake
2020-01-28 16:44 ` Babu Moger
2019-12-04 0:37 ` [PATCH v3 07/18] machine: Add a new function init_apicid_fn in MachineClass Babu Moger
2020-01-28 16:29 ` Igor Mammedov
2020-01-28 19:45 ` Babu Moger
2020-01-28 20:12 ` Eduardo Habkost
2020-01-29 9:14 ` Igor Mammedov
2020-01-29 16:17 ` Babu Moger
2020-02-03 15:17 ` Igor Mammedov
2020-02-03 21:49 ` Babu Moger
2020-02-04 7:38 ` Igor Mammedov
2020-01-29 16:32 ` Babu Moger
2020-01-29 16:51 ` Eduardo Habkost
2020-01-29 17:05 ` Babu Moger
2019-12-04 0:37 ` [PATCH v3 08/18] hw/i386: Update structures for nodes_per_pkg Babu Moger
2019-12-04 0:37 ` [PATCH v3 09/18] i386: Add CPUX86Family type in CPUX86State Babu Moger
2019-12-04 0:38 ` [PATCH v3 10/18] hw/386: Add EPYC mode topology decoding functions Babu Moger
2019-12-04 0:38 ` [PATCH v3 11/18] i386: Cleanup and use the EPYC mode topology functions Babu Moger
2019-12-04 0:38 ` [PATCH v3 12/18] numa: Split the numa initialization Babu Moger
2019-12-04 0:38 ` [PATCH v3 13/18] hw/i386: Introduce apicid_from_cpu_idx in PCMachineState Babu Moger
2019-12-04 0:38 ` [PATCH v3 14/18] hw/i386: Introduce topo_ids_from_apicid handler PCMachineState Babu Moger
2019-12-04 0:38 ` [PATCH v3 15/18] hw/i386: Introduce apic_id_from_topo_ids handler in PCMachineState Babu Moger
2019-12-04 0:38 ` [PATCH v3 16/18] hw/i386: Introduce EPYC mode function handlers Babu Moger
2020-01-28 20:04 ` Eduardo Habkost [this message]
2020-01-28 21:48 ` Babu Moger
2020-01-29 16:41 ` Eduardo Habkost
2019-12-04 0:38 ` [PATCH v3 17/18] i386: Fix pkg_id offset for epyc mode Babu Moger
2019-12-04 0:39 ` [PATCH v3 18/18] tests: Update the Unit tests Babu Moger
2020-02-03 14:59 ` [PATCH v3 00/18] APIC ID fixes for AMD EPYC CPU models Igor Mammedov
2020-02-03 19:31 ` Babu Moger
2020-02-04 8:02 ` Igor Mammedov
2020-02-04 19:08 ` Babu Moger
2020-02-05 9:38 ` Igor Mammedov
2020-02-05 16:10 ` Babu Moger
2020-02-05 16:56 ` Igor Mammedov
2020-02-05 19:07 ` Babu Moger
2020-02-06 13:08 ` Igor Mammedov
2020-02-06 15:32 ` Babu Moger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200128200438.GJ18770@habkost.net \
--to=ehabkost@redhat.com \
--cc=armbru@redhat.com \
--cc=babu.moger@amd.com \
--cc=imammedo@redhat.com \
--cc=mst@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=rth@twiddle.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).