QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Eduardo Habkost <ehabkost@redhat.com>
To: "Moger, Babu" <Babu.Moger@amd.com>
Cc: "mst@redhat.com" <mst@redhat.com>,
	"armbru@redhat.com" <armbru@redhat.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"imammedo@redhat.com" <imammedo@redhat.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"ssg.sos.staff" <ssg.sos.staff@amd.com>,
	"rth@twiddle.net" <rth@twiddle.net>
Subject: Re: [Qemu-devel] [RFC 2 PATCH 08/16] i386: Cleanup and use the new epyc mode topology functions
Date: Fri, 11 Oct 2019 00:51:44 -0300
Message-ID: <20191011035144.GE29387@habkost.net> (raw)
In-Reply-To: <156779715031.21957.17374671669134234845.stgit@localhost.localdomain>

On Fri, Sep 06, 2019 at 07:12:33PM +0000, Moger, Babu wrote:
> Use the new epyc mode functions and delete the unused code.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  target/i386/cpu.c |  171 +++++++++++++++--------------------------------------
>  1 file changed, 48 insertions(+), 123 deletions(-)
> 
[...]
>  /* Encode cache info for CPUID[8000001E] */
> -static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
> -                                       uint32_t *eax, uint32_t *ebx,
> -                                       uint32_t *ecx, uint32_t *edx)
> +static void encode_topo_cpuid8000001e(CPUX86State *env,
> +                                      uint32_t *eax, uint32_t *ebx,
> +                                      uint32_t *ecx, uint32_t *edx)
>  {
> -    struct core_topology topo = {0};
> -    unsigned long nodes;
> -    int shift;
> +    X86CPUTopoIDs topo_ids = { 0 };
> +    unsigned long nodes, shift;
> +    X86CPU *cpu = env_archcpu(env);
> +    CPUState *cs = env_cpu(env);
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    X86CPUTopoInfo topo_info = {
> +        .numa_nodes = nb_numa_nodes,
> +        .nr_sockets = ms->smp.sockets,
> +        .nr_cores = ms->smp.cores,
> +        .nr_threads = ms->smp.threads,
> +    };
> +
> +    nodes = nodes_in_pkg(&topo_info);
> +    x86_topo_ids_from_idx_epyc(&topo_info, cs->cpu_index, &topo_ids);

You probably want to use x86_topo_ids_from_apicid() instead.
cpu_index is a deprecated identifier for a CPU, and may not match
the actual CPU topology if using CPU hotplug.

>  
> -    build_core_topology(cs->nr_cores, cpu->core_id, &topo);
>      *eax = cpu->apic_id;
> +
>      /*
>       * CPUID_Fn8000001E_EBX
>       * 31:16 Reserved
> @@ -496,11 +421,12 @@ static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
>       *             3 Core complex id
>       *           1:0 Core id
>       */
> -    if (cs->nr_threads - 1) {
> -        *ebx = ((cs->nr_threads - 1) << 8) | (topo.node_id << 3) |
> -                (topo.ccx_id << 2) | topo.core_id;
> +    if (topo_info.nr_threads - 1) {
> +        *ebx = ((topo_info.nr_threads - 1) << 8) | (topo_ids.node_id << 3) |
> +                (topo_ids.ccx_id << 2) | topo_ids.core_id;
>      } else {
> -        *ebx = (topo.node_id << 4) | (topo.ccx_id << 3) | topo.core_id;
> +        *ebx = (topo_ids.node_id << 4) | (topo_ids.ccx_id << 3) |
> +                topo_ids.core_id;

It's nice to see we are deleting old code and reusing the same
topology functions.  However, I'm worried that the relationship
between the topology IDs in APIC ID (EAX) and the topology IDs in
EBX and ECX isn't clear.

I'm also worried there's no code enforcing a limit for
node_id/ccx_id/core_id to ensure they fit in the fields defined
above.  These hardcoded limits will become painful to maintain
once we have new CPUs with different constraints.  We need to
make the limits explicit in the code, and also be prepared for
them to change in the future depending on the CPU model.

>      }
>      /*
>       * CPUID_Fn8000001E_ECX
> @@ -510,9 +436,8 @@ static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
>       *         2  Socket id
>       *       1:0  Node id
>       */
> -    if (topo.num_nodes <= 4) {
> -        *ecx = ((topo.num_nodes - 1) << 8) | (cpu->socket_id << 2) |
> -                topo.node_id;
> +    if (nodes <= 4) {
> +        *ecx = ((nodes - 1) << 8) | (topo_ids.pkg_id << 2) | topo_ids.node_id;
>      } else {
>          /*
>           * Node id fix up. Actual hardware supports up to 4 nodes. But with
> @@ -527,12 +452,12 @@ static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
>           * number of nodes. find_last_bit returns last set bit(0 based). Left
>           * shift(+1) the socket id to represent all the nodes.
>           */
> -        nodes = topo.num_nodes - 1;
> +        nodes = nodes - 1;
>          shift = find_last_bit(&nodes, 8);
> -        *ecx = ((topo.num_nodes - 1) << 8) | (cpu->socket_id << (shift + 1)) |
> -                topo.node_id;
> +        *ecx = (nodes << 8) | (topo_ids.pkg_id << (shift + 1)) | topo_ids.node_id;

Like above, we also need to ensure pkg_id and node_id are withing
reasonable limits, somewhere in the code, and that the limits can
change in the future depending on the CPU model.

>      }
>      *edx = 0;
> +
>  }
>  
>  /*
> @@ -4580,19 +4505,19 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          }
>          switch (count) {
>          case 0: /* L1 dcache info */
> -            encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache, cs,
> +            encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache,
>                                         eax, ebx, ecx, edx);
>              break;
>          case 1: /* L1 icache info */
> -            encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache, cs,
> +            encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache,
>                                         eax, ebx, ecx, edx);
>              break;
>          case 2: /* L2 cache info */
> -            encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache, cs,
> +            encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache,
>                                         eax, ebx, ecx, edx);
>              break;
>          case 3: /* L3 cache info */
> -            encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache, cs,
> +            encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache,
>                                         eax, ebx, ecx, edx);
>              break;
>          default: /* end of info */
> @@ -4602,7 +4527,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          break;
>      case 0x8000001E:
>          assert(cpu->core_id <= 255);
> -        encode_topo_cpuid8000001e(cs, cpu,
> +        encode_topo_cpuid8000001e(env,
>                                    eax, ebx, ecx, edx);
>          break;
>      case 0xC0000000:
> 

-- 
Eduardo


  reply index

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-06 19:11 [Qemu-devel] [RFC 2 PATCH 00/16] APIC ID fixes for AMD EPYC CPU models Moger, Babu
2019-09-06 19:11 ` [Qemu-devel] [RFC 2 PATCH 01/16] numa: Split the numa functionality Moger, Babu
2019-10-10  3:25   ` Eduardo Habkost
2019-12-02 20:18     ` Babu Moger
2019-09-06 19:11 ` [Qemu-devel] [RFC 2 PATCH 02/16] hw/i386: Rename X86CPUTopoInfo structure to X86CPUTopoIDs Moger, Babu
2019-10-10  3:26   ` Eduardo Habkost
2019-09-06 19:11 ` [Qemu-devel] [RFC 2 PATCH 03/16] hw/i386: Introduce X86CPUTopoInfo to contain topology info Moger, Babu
2019-10-11  2:29   ` Eduardo Habkost
2019-12-02 20:23     ` Babu Moger
2019-10-11  3:54   ` Eduardo Habkost
2019-12-02 20:25     ` Babu Moger
2019-09-06 19:12 ` [Qemu-devel] [RFC 2 PATCH 04/16] machine: Add SMP Sockets in CpuTopology Moger, Babu
2019-10-11  2:31   ` Eduardo Habkost
2019-09-06 19:12 ` [Qemu-devel] [RFC 2 PATCH 05/16] hw/i386: Simplify topology Offset/width Calculation Moger, Babu
2019-10-11  2:32   ` Eduardo Habkost
2019-12-02 20:29     ` Babu Moger
2019-09-06 19:12 ` [Qemu-devel] [RFC 2 PATCH 06/16] hw/core: Add core complex id in X86CPU topology Moger, Babu
2019-09-06 19:20   ` Eric Blake
2019-09-06 19:34     ` Moger, Babu
2019-09-22 12:48   ` Michael S. Tsirkin
2019-09-23 14:38     ` Moger, Babu
2019-09-06 19:12 ` [Qemu-devel] [RFC 2 PATCH 07/16] hw/386: Add new epyc mode topology decoding functions Moger, Babu
2019-10-11  3:29   ` Eduardo Habkost
2019-12-02 20:38     ` Babu Moger
2019-09-06 19:12 ` [Qemu-devel] [RFC 2 PATCH 08/16] i386: Cleanup and use the new epyc mode topology functions Moger, Babu
2019-10-11  3:51   ` Eduardo Habkost [this message]
2019-09-06 19:12 ` [Qemu-devel] [RFC 2 PATCH 09/16] hw/i386: Introduce initialize_topo_info function Moger, Babu
2019-10-11  3:54   ` Eduardo Habkost
2019-09-06 19:12 ` [Qemu-devel] [RFC 2 PATCH 10/16] hw/i386: Introduce apicid_from_cpu_idx in PCMachineState Moger, Babu
2019-09-06 19:12 ` [Qemu-devel] [RFC 2 PATCH 11/16] Introduce-topo_ids_from_apicid-handler Moger, Babu
2019-09-06 19:13 ` [Qemu-devel] [RFC 2 PATCH 12/16] hw/i386: Introduce apic_id_from_topo_ids handler in PCMachineState Moger, Babu
2019-09-06 19:13 ` [Qemu-devel] [RFC 2 PATCH 13/16] machine: Add new epyc property " Moger, Babu
2019-10-11  3:59   ` Eduardo Habkost
2019-10-11 16:23     ` Moger, Babu
2019-10-11 16:59     ` Moger, Babu
2019-10-11 19:03       ` Eduardo Habkost
2019-09-06 19:13 ` [Qemu-devel] [RFC 2 PATCH 14/16] hw/i386: Introduce epyc mode function handlers Moger, Babu
2019-09-06 19:13 ` [Qemu-devel] [RFC 2 PATCH 15/16] i386: Fix pkg_id offset for epyc mode Moger, Babu
2019-10-11  4:03   ` Eduardo Habkost
2019-09-06 19:13 ` [Qemu-devel] [RFC 2 PATCH 16/16] hw/core: Fix up the machine_set_cpu_numa_node for epyc Moger, Babu
2019-10-11  4:10   ` Eduardo Habkost
2019-12-02 20:44     ` Babu Moger
2019-09-20 22:44 ` [RFC 2 PATCH 00/16] APIC ID fixes for AMD EPYC CPU models Moger, Babu

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=20191011035144.GE29387@habkost.net \
    --to=ehabkost@redhat.com \
    --cc=Babu.Moger@amd.com \
    --cc=armbru@redhat.com \
    --cc=imammedo@redhat.com \
    --cc=mst@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=rth@twiddle.net \
    --cc=ssg.sos.staff@amd.com \
    /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

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git