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.
Signed-off-by: Jade Cheng <chengjiayao@bytedance.com>
---
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)
Hello, I am Jade Cheng working for ByteDance, sending this email is aimed at ping you guys to check the patch I submitted earlier, link is attached below: https://patchew.org/QEMU/20210630082551.12956-1-chengjiayao@bytedance.com/ Please do me a favor to give it a review, and let me know if you have any concerns. Stay safe and have a nice day = ) Best, Jiayao (Jade) Cheng On Wed, Jun 30, 2021, 16:26 <chengjiayao@bytedance.com> 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. Signed-off-by: Jade Cheng <chengjiayao@bytedance.com> --- 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)
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 <chengjiayao@bytedance.com> 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
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
© 2016 - 2024 Red Hat, Inc.