From: Igor Mammedov <imammedo@redhat.com>
To: Babu Moger <babu.moger@amd.com>
Cc: qemu-devel@nongnu.org, pbonzini@redhat.com, ehabkost@redhat.com,
rth@twiddle.net
Subject: Re: [PATCH v2 2/3] hw/i386: Build apic_id from CpuInstanceProperties
Date: Mon, 13 Jul 2020 11:15:27 +0200 [thread overview]
Message-ID: <20200713111527.23bf98a8@redhat.com> (raw)
In-Reply-To: <159362466828.36204.14044362989991188460.stgit@naples-babu.amd.com>
On Wed, 01 Jul 2020 12:31:08 -0500
Babu Moger <babu.moger@amd.com> wrote:
> Build apic_id from CpuInstanceProperties if numa configured.
> Use the node_id from user provided numa information. This
> will avoid conflicts between numa information and apic_id
> generated.
>
> Re-arranged the code little bit to make sure CpuInstanceProperties
> is initialized before calling.
>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> hw/i386/pc.c | 6 +++++-
> hw/i386/x86.c | 19 +++++++++++++------
> include/hw/i386/topology.h | 14 +++++++++++---
> include/hw/i386/x86.h | 6 ++++--
> tests/test-x86-cpuid.c | 39 ++++++++++++++++++++-------------------
> 5 files changed, 53 insertions(+), 31 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index d103b8c0ab..e613b2299f 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -800,13 +800,17 @@ void pc_smp_parse(MachineState *ms, QemuOpts *opts)
> void pc_hot_add_cpu(MachineState *ms, const int64_t id, Error **errp)
> {
> X86MachineState *x86ms = X86_MACHINE(ms);
> - int64_t apic_id = x86_cpu_apic_id_from_index(x86ms, id);
> + CpuInstanceProperties props;
> + int64_t apic_id;
> Error *local_err = NULL;
>
> if (id < 0) {
> error_setg(errp, "Invalid CPU id: %" PRIi64, id);
> return;
> }
> + props = ms->possible_cpus->cpus[id].props;
> +
> + apic_id = x86_cpu_apic_id_from_index(x86ms, id, props);
>
> if (apic_id >= ACPI_CPU_HOTPLUG_ID_LIMIT) {
> error_setg(errp, "Unable to add CPU: %" PRIi64
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 34229b45c7..7554416ae0 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -93,7 +93,8 @@ static void x86_set_epyc_topo_handlers(MachineState *machine)
> * all CPUs up to max_cpus.
> */
> uint32_t x86_cpu_apic_id_from_index(X86MachineState *x86ms,
> - unsigned int cpu_index)
> + unsigned int cpu_index,
> + CpuInstanceProperties props)
> {
> X86MachineClass *x86mc = X86_MACHINE_GET_CLASS(x86ms);
> X86CPUTopoInfo topo_info;
> @@ -102,7 +103,7 @@ uint32_t x86_cpu_apic_id_from_index(X86MachineState *x86ms,
>
> init_topo_info(&topo_info, x86ms);
>
> - correct_id = x86ms->apicid_from_cpu_idx(&topo_info, cpu_index);
> + correct_id = x86ms->apicid_from_cpu_idx(&topo_info, cpu_index, props);
> if (x86mc->compat_apic_id_mode) {
> if (cpu_index != correct_id && !warned && !qtest_enabled()) {
> error_report("APIC IDs set in compatibility mode, "
> @@ -136,6 +137,8 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
> const CPUArchIdList *possible_cpus;
> MachineState *ms = MACHINE(x86ms);
> MachineClass *mc = MACHINE_GET_CLASS(x86ms);
> + CpuInstanceProperties props;
> +
>
> /* Check for apicid encoding */
> if (cpu_x86_use_epyc_apic_id_encoding(ms->cpu_type)) {
> @@ -144,6 +147,8 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
>
> x86_cpu_set_default_version(default_cpu_version);
>
> + possible_cpus = mc->possible_cpu_arch_ids(ms);
> +
> /*
> * Calculates the limit to CPU APIC ID values
> *
> @@ -152,13 +157,15 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
> *
> * This is used for FW_CFG_MAX_CPUS. See comments on fw_cfg_arch_create().
> */
> - x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms,
> - ms->smp.max_cpus - 1) + 1;
> - possible_cpus = mc->possible_cpu_arch_ids(ms);
> + props = ms->possible_cpus->cpus[ms->smp.max_cpus - 1].props;
>
> + x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms,
> + ms->smp.max_cpus - 1,
> + props) + 1;
> for (i = 0; i < ms->possible_cpus->len; i++) {
> + props = ms->possible_cpus->cpus[i].props;
> ms->possible_cpus->cpus[i].arch_id =
> - x86_cpu_apic_id_from_index(x86ms, i);
> + x86_cpu_apic_id_from_index(x86ms, i, props);
> }
>
> for (i = 0; i < ms->smp.cpus; i++) {
> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
> index 7cb21e9c82..a800fc905f 100644
> --- a/include/hw/i386/topology.h
> +++ b/include/hw/i386/topology.h
> @@ -221,10 +221,17 @@ static inline void x86_init_topo_ids(X86CPUTopoInfo *topo_info,
> * 'cpu_index' is a sequential, contiguous ID for the CPU.
> */
> static inline apic_id_t x86_apicid_from_cpu_idx_epyc(X86CPUTopoInfo *topo_info,
> - unsigned cpu_index)
> + unsigned cpu_index,
> + CpuInstanceProperties props)
> {
> X86CPUTopoIDs topo_ids;
> - x86_topo_ids_from_idx_epyc(topo_info, cpu_index, &topo_ids);
> +
> + if (props.has_node_id) {
> + x86_init_topo_ids(topo_info, props, &topo_ids);
> + } else {
> + x86_topo_ids_from_idx_epyc(topo_info, cpu_index, &topo_ids);
why this branch is needed?
> + }
> +
> 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
> @@ -280,7 +287,8 @@ static inline void x86_topo_ids_from_apicid(apic_id_t apicid,
> * 'cpu_index' is a sequential, contiguous ID for the CPU.
> */
> static inline apic_id_t x86_apicid_from_cpu_idx(X86CPUTopoInfo *topo_info,
> - unsigned cpu_index)
> + unsigned cpu_index,
> + CpuInstanceProperties props)
> {
> X86CPUTopoIDs topo_ids;
> x86_topo_ids_from_idx(topo_info, cpu_index, &topo_ids);
> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
> index b79f24e285..3109f39554 100644
> --- a/include/hw/i386/x86.h
> +++ b/include/hw/i386/x86.h
> @@ -65,7 +65,8 @@ typedef struct {
>
> /* Apic id specific handlers */
> uint32_t (*apicid_from_cpu_idx)(X86CPUTopoInfo *topo_info,
> - unsigned cpu_index);
> + unsigned cpu_index,
> + CpuInstanceProperties props);
> void (*topo_ids_from_apicid)(apic_id_t apicid, X86CPUTopoInfo *topo_info,
> X86CPUTopoIDs *topo_ids);
> apic_id_t (*apicid_from_topo_ids)(X86CPUTopoInfo *topo_info,
> @@ -93,7 +94,8 @@ typedef struct {
> void init_topo_info(X86CPUTopoInfo *topo_info, const X86MachineState *x86ms);
>
> uint32_t x86_cpu_apic_id_from_index(X86MachineState *pcms,
> - unsigned int cpu_index);
> + unsigned int cpu_index,
> + CpuInstanceProperties props);
>
> void x86_cpu_new(X86MachineState *pcms, int64_t apic_id, Error **errp);
> void x86_cpus_init(X86MachineState *pcms, int default_cpu_version);
> diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c
> index 049030a50e..a1308e214b 100644
> --- a/tests/test-x86-cpuid.c
> +++ b/tests/test-x86-cpuid.c
> @@ -29,6 +29,7 @@
> static void test_topo_bits(void)
> {
> X86CPUTopoInfo topo_info = {0};
> + CpuInstanceProperties props = {0};
>
> /* simple tests for 1 thread per core, 1 core per die, 1 die per package */
> topo_info = (X86CPUTopoInfo) {0, 1, 1, 1};
> @@ -37,10 +38,10 @@ static void test_topo_bits(void)
> g_assert_cmpuint(apicid_die_width(&topo_info), ==, 0);
>
> topo_info = (X86CPUTopoInfo) {0, 1, 1, 1};
> - g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 0), ==, 0);
> - g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1), ==, 1);
> - g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 2), ==, 2);
> - g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 3), ==, 3);
> + g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 0, props), ==, 0);
> + g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1, props), ==, 1);
> + g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 2, props), ==, 2);
> + g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 3, props), ==, 3);
>
>
> /* Test field width calculation for multiple values
> @@ -92,38 +93,38 @@ static void test_topo_bits(void)
> g_assert_cmpuint(apicid_pkg_offset(&topo_info), ==, 5);
>
> topo_info = (X86CPUTopoInfo) {0, 1, 6, 3};
> - g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 0), ==, 0);
> - g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1), ==, 1);
> - g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 2), ==, 2);
> + g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 0, props), ==, 0);
> + g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1, props), ==, 1);
> + g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 2, props), ==, 2);
>
> topo_info = (X86CPUTopoInfo) {0, 1, 6, 3};
> - g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1 * 3 + 0), ==,
> + g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1 * 3 + 0, props), ==,
> (1 << 2) | 0);
> - g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1 * 3 + 1), ==,
> + g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1 * 3 + 1, props), ==,
> (1 << 2) | 1);
> - g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1 * 3 + 2), ==,
> + g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1 * 3 + 2, props), ==,
> (1 << 2) | 2);
>
> - g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 2 * 3 + 0), ==,
> + g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 2 * 3 + 0, props), ==,
> (2 << 2) | 0);
> - g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 2 * 3 + 1), ==,
> + g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 2 * 3 + 1, props), ==,
> (2 << 2) | 1);
> - g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 2 * 3 + 2), ==,
> + g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 2 * 3 + 2, props), ==,
> (2 << 2) | 2);
>
> - g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 5 * 3 + 0), ==,
> + g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 5 * 3 + 0, props), ==,
> (5 << 2) | 0);
> - g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 5 * 3 + 1), ==,
> + g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 5 * 3 + 1, props), ==,
> (5 << 2) | 1);
> - g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 5 * 3 + 2), ==,
> + g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 5 * 3 + 2, props), ==,
> (5 << 2) | 2);
>
> g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info,
> - 1 * 6 * 3 + 0 * 3 + 0), ==, (1 << 5));
> + 1 * 6 * 3 + 0 * 3 + 0, props), ==, (1 << 5));
> g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info,
> - 1 * 6 * 3 + 1 * 3 + 1), ==, (1 << 5) | (1 << 2) | 1);
> + 1 * 6 * 3 + 1 * 3 + 1, props), ==, (1 << 5) | (1 << 2) | 1);
> g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info,
> - 3 * 6 * 3 + 5 * 3 + 2), ==, (3 << 5) | (5 << 2) | 2);
> + 3 * 6 * 3 + 5 * 3 + 2, props), ==, (3 << 5) | (5 << 2) | 2);
> }
>
> int main(int argc, char **argv)
>
next prev parent reply other threads:[~2020-07-13 9:16 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-01 17:30 [PATCH v2 0/3] Fix couple of issues with AMD topology Babu Moger
2020-07-01 17:31 ` [PATCH v2 1/3] hw/i386: Initialize topo_ids from CpuInstanceProperties Babu Moger
2020-07-13 9:08 ` Igor Mammedov
2020-07-13 15:02 ` Babu Moger
2020-07-13 16:17 ` Igor Mammedov
2020-07-13 16:43 ` Babu Moger
2020-07-13 17:32 ` Igor Mammedov
2020-07-13 19:30 ` Babu Moger
2020-07-14 16:41 ` Igor Mammedov
2020-07-14 17:26 ` Babu Moger
2020-07-14 18:26 ` Igor Mammedov
2020-07-24 17:05 ` Igor Mammedov
2020-07-27 15:49 ` Babu Moger
2020-07-27 17:14 ` Igor Mammedov
2020-07-27 23:59 ` Babu Moger
2020-07-29 14:12 ` Igor Mammedov
2020-07-29 21:22 ` Babu Moger
2020-07-30 11:27 ` Igor Mammedov
2020-07-01 17:31 ` [PATCH v2 2/3] hw/i386: Build apic_id " Babu Moger
2020-07-13 9:15 ` Igor Mammedov [this message]
2020-07-13 15:00 ` Babu Moger
2020-07-01 17:31 ` [PATCH v2 3/3] hw/386: Fix uninitialized memory with -device and CPU hotplug Babu Moger
2020-07-27 16:36 ` Igor Mammedov
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=20200713111527.23bf98a8@redhat.com \
--to=imammedo@redhat.com \
--cc=babu.moger@amd.com \
--cc=ehabkost@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.