| Message ID | 20240227103231.1556302-7-zhao1.liu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce smp.modules for x86 in QEMU | expand |
On 2/27/2024 6:32 PM, Zhao Liu wrote: > From: Zhao Liu <zhao1.liu@intel.com> > > Refer to the fixes of cache_info_passthrough ([1], [2]) and SDM, the > CPUID.04H:EAX[bits 25:14] and CPUID.04H:EAX[bits 31:26] should use the > nearest power-of-2 integer. > > The nearest power-of-2 integer can be calculated by pow2ceil() or by > using APIC ID offset/width (like L3 topology using 1 << die_offset [3]). > > But in fact, CPUID.04H:EAX[bits 25:14] and CPUID.04H:EAX[bits 31:26] > are associated with APIC ID. For example, in linux kernel, the field > "num_threads_sharing" (Bits 25 - 14) is parsed with APIC ID. And for > another example, on Alder Lake P, the CPUID.04H:EAX[bits 31:26] is not > matched with actual core numbers and it's calculated by: > "(1 << (pkg_offset - core_offset)) - 1". > > Therefore the topology information of APIC ID should be preferred to > calculate nearest power-of-2 integer for CPUID.04H:EAX[bits 25:14] and > CPUID.04H:EAX[bits 31:26]: > 1. d/i cache is shared in a core, 1 << core_offset should be used > instead of "cs->nr_threads" in encode_cache_cpuid4() for > CPUID.04H.00H:EAX[bits 25:14] and CPUID.04H.01H:EAX[bits 25:14]. > 2. L2 cache is supposed to be shared in a core as for now, thereby > 1 << core_offset should also be used instead of "cs->nr_threads" in > encode_cache_cpuid4() for CPUID.04H.02H:EAX[bits 25:14]. > 3. Similarly, the value for CPUID.04H:EAX[bits 31:26] should also be > calculated with the bit width between the package and SMT levels in > the APIC ID (1 << (pkg_offset - core_offset) - 1). > > In addition, use APIC ID bits calculations to replace "pow2ceil()" for > cache_info_passthrough case. > > [1]: efb3934adf9e ("x86: cpu: make sure number of addressable IDs for processor cores meets the spec") > [2]: d7caf13b5fcf ("x86: cpu: fixup number of addressable IDs for logical processors sharing cache") > [3]: d65af288a84d ("i386: Update new x86_apicid parsing rules with die_offset support") > > Fixes: 7e3482f82480 ("i386: Helpers to encode cache information consistently") > Suggested-by: Robert Hoo <robert.hu@linux.intel.com> > Tested-by: Yongwei Ma <yongwei.ma@intel.com> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > --- > Changes since v7: > * Fixed calculations in cache_info_passthrough case. (Xiaoyao) > * Renamed variables as *_width. (Xiaoyao) > * Unified variable names for encoding cache_info_passthrough case and > non-cache_info_passthrough case as addressable_cores_width and > addressable_threads_width. > * Fixed typos in commit message. (Xiaoyao) > * Dropped Michael/Babu's ACKed/Tested tags since the code change. > * Re-added Yongwei's Tested tag For his re-testing. > > Changes since v3: > * Fixed compile warnings. (Babu) > * Fixed spelling typo. > > Changes since v1: > * Used APIC ID offset to replace "pow2ceil()" for cache_info_passthrough > case. (Yanan) > * Split the L1 cache fix into a separate patch. > * Renamed the title of this patch (the original is "i386/cpu: Fix number > of addressable IDs in CPUID.04H"). > --- > target/i386/cpu.c | 37 ++++++++++++++++++++++++++++--------- > 1 file changed, 28 insertions(+), 9 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 81d9046167e8..c77bcbc44d59 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -6014,7 +6014,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > { > X86CPU *cpu = env_archcpu(env); > CPUState *cs = env_cpu(env); > - uint32_t die_offset; > uint32_t limit; > uint32_t signature[3]; > X86CPUTopoInfo topo_info; > @@ -6086,7 +6085,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > (cpuid2_cache_descriptor(env->cache_info_cpuid2.l1i_cache) << 8) | > (cpuid2_cache_descriptor(env->cache_info_cpuid2.l2_cache)); > break; > - case 4: > + case 4: { > + int addressable_cores_width; > + int addressable_threads_width; > + > /* cache info: needed for Core compatibility */ > if (cpu->cache_info_passthrough) { > x86_cpu_get_cache_cpuid(index, count, eax, ebx, ecx, edx); > @@ -6098,39 +6100,55 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14); > int vcpus_per_socket = cs->nr_cores * cs->nr_threads; > if (cs->nr_cores > 1) { > + addressable_cores_width = apicid_pkg_offset(&topo_info) - > + apicid_core_offset(&topo_info); > + > *eax &= ~0xFC000000; > - *eax |= (pow2ceil(cs->nr_cores) - 1) << 26; > + *eax |= ((1 << addressable_cores_width) - 1) << 26; > } > if (host_vcpus_per_cache > vcpus_per_socket) { > + /* Share the cache at package level. */ > + addressable_threads_width = apicid_pkg_offset(&topo_info); > + > *eax &= ~0x3FFC000; > - *eax |= (pow2ceil(vcpus_per_socket) - 1) << 14; > + *eax |= ((1 << addressable_threads_width) - 1) << 14; > } > } > } else if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) { > *eax = *ebx = *ecx = *edx = 0; > } else { > *eax = 0; > + addressable_cores_width = apicid_pkg_offset(&topo_info) - > + apicid_core_offset(&topo_info); > + > switch (count) { > case 0: /* L1 dcache info */ > + addressable_threads_width = apicid_core_offset(&topo_info); > encode_cache_cpuid4(env->cache_info_cpuid4.l1d_cache, > - cs->nr_threads, cs->nr_cores, > + (1 << addressable_threads_width), > + (1 << addressable_cores_width), > eax, ebx, ecx, edx); > break; > case 1: /* L1 icache info */ > + addressable_threads_width = apicid_core_offset(&topo_info); > encode_cache_cpuid4(env->cache_info_cpuid4.l1i_cache, > - cs->nr_threads, cs->nr_cores, > + (1 << addressable_threads_width), > + (1 << addressable_cores_width), > eax, ebx, ecx, edx); > break; > case 2: /* L2 cache info */ > + addressable_threads_width = apicid_core_offset(&topo_info); > encode_cache_cpuid4(env->cache_info_cpuid4.l2_cache, > - cs->nr_threads, cs->nr_cores, > + (1 << addressable_threads_width), > + (1 << addressable_cores_width), > eax, ebx, ecx, edx); > break; > case 3: /* L3 cache info */ > - die_offset = apicid_die_offset(&topo_info); > if (cpu->enable_l3_cache) { > + addressable_threads_width = apicid_die_offset(&topo_info); Please get rid of the local variable @addressable_threads_width. It is truly confusing. In this patch, it is assigned to - apicid_pkg_offset(&topo_info); - apicid_core_offset(&topo_info); - apicid_die_offset(&topo_info); in different cases. I only suggested the name of addressable_core_width in v7, which is apicid_pkg_offset(&topo_info) - apicid_core_offset(&topo_info); And it is straightforward that it means the number of bits in x2APICID to encode different addressable cores. But it is not similar to addressable_threads_width, the semantic changes per different cache level. In fact, you want something like bit_width_of_addressable_threads_sharing_this_level_of_cache. So I suggest stop using the variable of "address_therads_width". Instead just apicid_*_offset() directly. With above fixed, feel free to add my Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com> > encode_cache_cpuid4(env->cache_info_cpuid4.l3_cache, > - (1 << die_offset), cs->nr_cores, > + (1 << addressable_threads_width), > + (1 << addressable_cores_width), > eax, ebx, ecx, edx); > break; > } > @@ -6141,6 +6159,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > } > } > break; > + } > case 5: > /* MONITOR/MWAIT Leaf */ > *eax = cpu->mwait.eax; /* Smallest monitor-line size in bytes */
Hi Xiaoyao, > > case 3: /* L3 cache info */ > > - die_offset = apicid_die_offset(&topo_info); > > if (cpu->enable_l3_cache) { > > + addressable_threads_width = apicid_die_offset(&topo_info); > > Please get rid of the local variable @addressable_threads_width. > > It is truly confusing. There're several reasons for this: 1. This commit is trying to use APIC ID topology layout to decode 2 cache topology fields in CPUID[4], CPUID.04H:EAX[bits 25:14] and CPUID.04H:EAX[bits 31:26]. When there's a addressable_cores_width to map to CPUID.04H:EAX[bits 31:26], it's more clear to also map CPUID.04H:EAX[bits 25:14] to another variable. 2. All these 2 variables are temporary in this commit, and they will be replaed by 2 helpers in follow-up cleanup of this series. 3. Similarly, to make it easier to clean up later with the helper and for more people to review, it's neater to explicitly indicate the CPUID.04H:EAX[bits 25:14] with a variable here. 4. I call this field as addressable_threads_width since it's "Maximum number of addressable IDs for logical processors sharing this cache". Its own name is long, but given the length, only individual words could be selected as names. TBH, "addressable" deserves more emphasis than "sharing". The former emphasizes the fact that the number of threads chosen here is based on the APIC ID layout and does not necessarily correspond to actual threads. Therefore, this variable is needed here. Thanks, Zhao
Hi Xiaoyao, Did the following reason convince you? Could I take your r/b tag with current code? ;-) Thanks, Zhao On Sun, Mar 10, 2024 at 09:38:19PM +0800, Zhao Liu wrote: > Date: Sun, 10 Mar 2024 21:38:19 +0800 > From: Zhao Liu <zhao1.liu@linux.intel.com> > Subject: Re: [PATCH v9 06/21] i386/cpu: Use APIC ID info to encode cache > topo in CPUID[4] > > Hi Xiaoyao, > > > > case 3: /* L3 cache info */ > > > - die_offset = apicid_die_offset(&topo_info); > > > if (cpu->enable_l3_cache) { > > > + addressable_threads_width = apicid_die_offset(&topo_info); > > > > Please get rid of the local variable @addressable_threads_width. > > > > It is truly confusing. > > There're several reasons for this: > > 1. This commit is trying to use APIC ID topology layout to decode 2 > cache topology fields in CPUID[4], CPUID.04H:EAX[bits 25:14] and > CPUID.04H:EAX[bits 31:26]. When there's a addressable_cores_width to map > to CPUID.04H:EAX[bits 31:26], it's more clear to also map > CPUID.04H:EAX[bits 25:14] to another variable. > > 2. All these 2 variables are temporary in this commit, and they will be > replaed by 2 helpers in follow-up cleanup of this series. > > 3. Similarly, to make it easier to clean up later with the helper and > for more people to review, it's neater to explicitly indicate the > CPUID.04H:EAX[bits 25:14] with a variable here. > > 4. I call this field as addressable_threads_width since it's "Maximum > number of addressable IDs for logical processors sharing this cache". > > Its own name is long, but given the length, only individual words could > be selected as names. > > TBH, "addressable" deserves more emphasis than "sharing". The former > emphasizes the fact that the number of threads chosen here is based on > the APIC ID layout and does not necessarily correspond to actual threads. > > Therefore, this variable is needed here. > > Thanks, > Zhao >
On 3/10/2024 9:38 PM, Zhao Liu wrote: > Hi Xiaoyao, > >>> case 3: /* L3 cache info */ >>> - die_offset = apicid_die_offset(&topo_info); >>> if (cpu->enable_l3_cache) { >>> + addressable_threads_width = apicid_die_offset(&topo_info); >> >> Please get rid of the local variable @addressable_threads_width. >> >> It is truly confusing. > > There're several reasons for this: > > 1. This commit is trying to use APIC ID topology layout to decode 2 > cache topology fields in CPUID[4], CPUID.04H:EAX[bits 25:14] and > CPUID.04H:EAX[bits 31:26]. When there's a addressable_cores_width to map > to CPUID.04H:EAX[bits 31:26], it's more clear to also map > CPUID.04H:EAX[bits 25:14] to another variable. I don't dislike using a variable. I dislike the name of that variable since it's misleading > 2. All these 2 variables are temporary in this commit, and they will be > replaed by 2 helpers in follow-up cleanup of this series. you mean patch 20? I don't see how removing the local variable @addressable_threads_width conflicts with patch 20. As a con, it introduces code churn. > 3. Similarly, to make it easier to clean up later with the helper and > for more people to review, it's neater to explicitly indicate the > CPUID.04H:EAX[bits 25:14] with a variable here. If you do want keeping the variable. Please add a comment above it to explain the meaning. > 4. I call this field as addressable_threads_width since it's "Maximum > number of addressable IDs for logical processors sharing this cache". > > Its own name is long, but given the length, only individual words could > be selected as names. > > TBH, "addressable" deserves more emphasis than "sharing". The former > emphasizes the fact that the number of threads chosen here is based on > the APIC ID layout and does not necessarily correspond to actual threads. > > Therefore, this variable is needed here. > > Thanks, > Zhao >
On Mon, Mar 11, 2024 at 05:03:02PM +0800, Xiaoyao Li wrote: > Date: Mon, 11 Mar 2024 17:03:02 +0800 > From: Xiaoyao Li <xiaoyao.li@intel.com> > Subject: Re: [PATCH v9 06/21] i386/cpu: Use APIC ID info to encode cache > topo in CPUID[4] > > On 3/10/2024 9:38 PM, Zhao Liu wrote: > > Hi Xiaoyao, > > > > > > case 3: /* L3 cache info */ > > > > - die_offset = apicid_die_offset(&topo_info); > > > > if (cpu->enable_l3_cache) { > > > > + addressable_threads_width = apicid_die_offset(&topo_info); > > > > > > Please get rid of the local variable @addressable_threads_width. > > > > > > It is truly confusing. > > > > There're several reasons for this: > > > > 1. This commit is trying to use APIC ID topology layout to decode 2 > > cache topology fields in CPUID[4], CPUID.04H:EAX[bits 25:14] and > > CPUID.04H:EAX[bits 31:26]. When there's a addressable_cores_width to map > > to CPUID.04H:EAX[bits 31:26], it's more clear to also map > > CPUID.04H:EAX[bits 25:14] to another variable. > > I don't dislike using a variable. I dislike the name of that variable since > it's misleading Names are hard to choose... > > > 2. All these 2 variables are temporary in this commit, and they will be > > replaed by 2 helpers in follow-up cleanup of this series. > > you mean patch 20? > > I don't see how removing the local variable @addressable_threads_width > conflicts with patch 20. As a con, it introduces code churn. Yes...I prefer to wrap it in variables in advance, then the meaning of the fields is clearer I think. > > 3. Similarly, to make it easier to clean up later with the helper and > > for more people to review, it's neater to explicitly indicate the > > CPUID.04H:EAX[bits 25:14] with a variable here. > > If you do want keeping the variable. Please add a comment above it to > explain the meaning. > OK, I'll add comments for both 2 variables. Thanks!
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 81d9046167e8..c77bcbc44d59 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -6014,7 +6014,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, { X86CPU *cpu = env_archcpu(env); CPUState *cs = env_cpu(env); - uint32_t die_offset; uint32_t limit; uint32_t signature[3]; X86CPUTopoInfo topo_info; @@ -6086,7 +6085,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, (cpuid2_cache_descriptor(env->cache_info_cpuid2.l1i_cache) << 8) | (cpuid2_cache_descriptor(env->cache_info_cpuid2.l2_cache)); break; - case 4: + case 4: { + int addressable_cores_width; + int addressable_threads_width; + /* cache info: needed for Core compatibility */ if (cpu->cache_info_passthrough) { x86_cpu_get_cache_cpuid(index, count, eax, ebx, ecx, edx); @@ -6098,39 +6100,55 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14); int vcpus_per_socket = cs->nr_cores * cs->nr_threads; if (cs->nr_cores > 1) { + addressable_cores_width = apicid_pkg_offset(&topo_info) - + apicid_core_offset(&topo_info); + *eax &= ~0xFC000000; - *eax |= (pow2ceil(cs->nr_cores) - 1) << 26; + *eax |= ((1 << addressable_cores_width) - 1) << 26; } if (host_vcpus_per_cache > vcpus_per_socket) { + /* Share the cache at package level. */ + addressable_threads_width = apicid_pkg_offset(&topo_info); + *eax &= ~0x3FFC000; - *eax |= (pow2ceil(vcpus_per_socket) - 1) << 14; + *eax |= ((1 << addressable_threads_width) - 1) << 14; } } } else if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) { *eax = *ebx = *ecx = *edx = 0; } else { *eax = 0; + addressable_cores_width = apicid_pkg_offset(&topo_info) - + apicid_core_offset(&topo_info); + switch (count) { case 0: /* L1 dcache info */ + addressable_threads_width = apicid_core_offset(&topo_info); encode_cache_cpuid4(env->cache_info_cpuid4.l1d_cache, - cs->nr_threads, cs->nr_cores, + (1 << addressable_threads_width), + (1 << addressable_cores_width), eax, ebx, ecx, edx); break; case 1: /* L1 icache info */ + addressable_threads_width = apicid_core_offset(&topo_info); encode_cache_cpuid4(env->cache_info_cpuid4.l1i_cache, - cs->nr_threads, cs->nr_cores, + (1 << addressable_threads_width), + (1 << addressable_cores_width), eax, ebx, ecx, edx); break; case 2: /* L2 cache info */ + addressable_threads_width = apicid_core_offset(&topo_info); encode_cache_cpuid4(env->cache_info_cpuid4.l2_cache, - cs->nr_threads, cs->nr_cores, + (1 << addressable_threads_width), + (1 << addressable_cores_width), eax, ebx, ecx, edx); break; case 3: /* L3 cache info */ - die_offset = apicid_die_offset(&topo_info); if (cpu->enable_l3_cache) { + addressable_threads_width = apicid_die_offset(&topo_info); encode_cache_cpuid4(env->cache_info_cpuid4.l3_cache, - (1 << die_offset), cs->nr_cores, + (1 << addressable_threads_width), + (1 << addressable_cores_width), eax, ebx, ecx, edx); break; } @@ -6141,6 +6159,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, } } break; + } case 5: /* MONITOR/MWAIT Leaf */ *eax = cpu->mwait.eax; /* Smallest monitor-line size in bytes */