Re: [PATCH] Fix CPUID_Fn8000001E_EBX for AMD

From: Jade Cheng <chengjiayao@bytedance.com>
To: Eduardo Habkost <ehabkost@redhat.com>
Cc: pbonzini <pbonzini@redhat.com>,
	qemu-devel <qemu-devel@nongnu.org>,
	pizhenwei@bytedance.com, Babu Moger <babu.moger@amd.com>
Subject: Re: [PATCH] Fix CPUID_Fn8000001E_EBX for AMD
Date: Mon, 22 Nov 2021 06:47:25 -0600	[thread overview]
Message-ID: <CAACWsD_21mjmu2uJB=ZLjE6sN7c-31q6OrrMbrrN7bO0Q4AyCw@mail.gmail.com> (raw)
In-Reply-To: <20210726210018.bfpd7rpnatw5377z@habkost.net>

[-- Attachment #1: Type: text/plain, Size: 2436 bytes --]

Sorry for this really late response due to some personal stuff.
Thanks to Eduardo for clarifying, and yes, this fix is to make the bits 7:0
in Fn8000_001E_EBX be physical core_id per package/socket, instead for what
used to be physical core_id per die.
When topo for vm is like: sockets=1, dies=8, cores=8
Before this fix, the bits 7:0 in Fn8000_001E_EBX are:
Socket_0 { die_0 {0-7}, die_1 {0-7}, ..., die_7 {0-7}}
With this patch, the last 8 bits becomes:
Socket_0 {die_0 {0-7}, die_1 {8-15}, ..., die_7 {56-63}}
Now, the new CPUID is in the same logical state as what it be in a physical
machine, which makes me believe this fix is necessary.
On Tue, Jul 27, 2021, 05:00 <ehabkost@redhat.com> wrote:
CCing the original author of that code (Babu Moger). On Wed, Jun 30, 2021
at 04:25:51PM +0800, Jade Cheng wrote: > According to AMD64 Arch
Programmer's Manual Appendix D, > bits 7:0 in Fn8000_001E_EBX should be
physical core(s) per logical processor, not per die. Do you mean physical
cores per package/socket? > > Signed-off-by: Jade Cheng Do you have a
pointer to the specific paragraph of the documentation that states that?
https://www.amd.com/system/files/TechDocs/24594.pdf page 634 says: CPUID
Fn8000_001E_EBX Compute Unit Identifiers [...] 7:0 ComputeUnitId Compute
unit ID. Identifies a Compute Unit, which may be one or more physical cores
that each implement one or more logical processors I don't see any content
referencing physical cores per logical processor, or physical cores per
package/socket. Which problem are you trying to fix here? > --- >
target/i386/cpu.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1
deletion(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index
a9fe1662d3..417f5ba81f 100644 > --- a/target/i386/cpu.c > +++
b/target/i386/cpu.c > @@ -381,7 +381,13 @@ static void
encode_topo_cpuid8000001e(X86CPU *cpu, X86CPUTopoInfo *topo_info, > * NOTE:
CoreId is already part of apic_id. Just use it. We can > * use all the 8
bits to represent the core_id here. > */ > - *ebx =
((topo_info->threads_per_core - 1) << 8) | (topo_ids.core_id & 0xFF); > +
uint32_t core_id = topo_ids.core_id; > + > + if (IS_AMD_CPU(&cpu->env)) { >
+ core_id = topo_ids.core_id + topo_ids.die_id * topo_info->cores_per_die;
> + } > + > + *ebx = ((topo_info->threads_per_core - 1) << 8) | (core_id &
0xFF); > > /* > * CPUID_Fn8000001E_ECX [Node Identifiers] (NodeId) > -- >
2.24.3 (Apple Git-128) > -- Eduardo

[-- Attachment #2: Type: text/html, Size: 4783 bytes --]