Xen project Mailing List

Re: [Xen-devel] [PATCH v2 04/23] x86: Don't use potentially incorrect CPUID values for topology information

From: Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>

Date: Mon, 7 Aug 2023 14:39:28 +0200

Cc: Wei Liu <wei.liu2@xxxxxxxxxx>, KarimAllah Ahmed <karahmed@xxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Jan H. Schönherr <jschoenh@xxxxxxxxx>, Matt Wilson <msw@xxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>, Anthony Liguori <aliguori@xxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx

Delivery-date: Mon, 07 Aug 2023 12:40:00 +0000

Feedback-id: idc5945a3:Fastmail

List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

Jan Beulich: > On 07.08.2023 11:58, Simon Gaiser wrote: >> Jan Beulich: >>> On 07.08.2023 10:18, Simon Gaiser wrote: >>>> Anthony Liguori: >>>>> From: Jan H. Schönherr <jschoenh@xxxxxxxxx> >>>>> >>>>> Intel says for CPUID leaf 0Bh: >>>>> >>>>> "Software must not use EBX[15:0] to enumerate processor >>>>> topology of the system. This value in this field >>>>> (EBX[15:0]) is only intended for display/diagnostic >>>>> purposes. The actual number of logical processors >>>>> available to BIOS/OS/Applications may be different from >>>>> the value of EBX[15:0], depending on software and platform >>>>> hardware configurations." >>>>> >>>>> And yet, we're using them to derive the number cores in a package >>>>> and the number of siblings in a core. >>>>> >>>>> Derive the number of siblings and cores from EAX instead, which is >>>>> intended for that. >>>>> >>>>> Signed-off-by: Jan H. Schönherr <jschoenh@xxxxxxxxx> >>>>> --- >>>>> xen/arch/x86/cpu/common.c | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c >>>>> index e9588b3..22f392f 100644 >>>>> --- a/xen/arch/x86/cpu/common.c >>>>> +++ b/xen/arch/x86/cpu/common.c >>>>> @@ -479,8 +479,8 @@ void detect_extended_topology(struct cpuinfo_x86 *c) >>>>> initial_apicid = edx; >>>>> >>>>> /* Populate HT related information from sub-leaf level 0 */ >>>>> - core_level_siblings = c->x86_num_siblings = LEVEL_MAX_SIBLINGS(ebx); >>>>> core_plus_mask_width = ht_mask_width = BITS_SHIFT_NEXT_LEVEL(eax); >>>>> + core_level_siblings = c->x86_num_siblings = 1 << ht_mask_width; >>>>> >>>>> sub_index = 1; >>>>> do { >>>>> @@ -488,8 +488,8 @@ void detect_extended_topology(struct cpuinfo_x86 *c) >>>>> >>>>> /* Check for the Core type in the implemented sub leaves */ >>>>> if ( LEAFB_SUBTYPE(ecx) == CORE_TYPE ) { >>>>> - core_level_siblings = LEVEL_MAX_SIBLINGS(ebx); >>>>> core_plus_mask_width = BITS_SHIFT_NEXT_LEVEL(eax); >>>>> + core_level_siblings = 1 << core_plus_mask_width; >>>> >>>> >>>> On the i5-1135G7 (4 cores with each 2 threads) I'm currently testing on >>>> I noticed that this changes leads to core_level_siblings == 16 and >>>> therefore x86_max_cores == 8. If read from ebx like before this change >>>> and what Linux is still doing [1] it reads core_level_siblings == 8 (as >>>> expected?). >>>> >>>> What's the expected semantic here? If it's to derive the actual number >>>> of siblings (and therefore cores) in a package as the commit message >>>> suggest, the new code can't work even ignoring the example from my test >>>> system. It will always produce powers of 2, so can't get it right on a >>>> system with, say, 6 cores. >>> >>> The only use of the variable in question is in this statement: >>> >>> c->x86_max_cores = (core_level_siblings / c->x86_num_siblings); >>> >>> Note the "max" in the name. This is how many _could_ be there, not how >>> many _are_ there, aiui. >> >> I'm indeed not quite sure on the intended semantic, hence the question >> (although it's not clear to me what case that "could" would cover here). > > "Could" covers for a number of reasons why APIC IDs may not be contiguous. > Consider a 6-code system: The APIC IDs need to cover for at least 8 there. > >> It doesn't have to be identical but Linux says for it's version of the >> variable: >> >> The number of cores in a package. This information is retrieved via >> CPUID. >> >> And if I look at it's usage in set_nr_sockets in Xen: >> >> nr_sockets = last_physid(phys_cpu_present_map) >> / boot_cpu_data.x86_max_cores >> / boot_cpu_data.x86_num_siblings + 1; > > This validly uses the field in the "max" sense, not in the "actual" one. I see, both cases cover APIC IDs not just numbers of logical CPUs. Thanks for the explanation and the pointer from Andrew in his response. I just had noticed the, to me, unexpected value, while debugging something. But based on this explanation things are actually working as intended here. (The actual problem I was looking for it turned out to be [1].) [1]: https://lore.kernel.org/xen-devel/7f158a54548456daba9f2e105d099d2e5e2c2f38.1691399031.git.simon@xxxxxxxxxxxxxxxxxxxxxx/ >> It seems to be also be used in this meaning. At least on my test system >> I get last_physid == 7 (as I would have expected for 8 logical cpus). So >> dividing this by the 4 (number of cores) and 2 (threads per core) is >> what I think was intended here. > > Would you mind providing raw data from your system: Both the raw CPUID > output for the leaf/leaves of interest here and the APIC IDs of all > threads? Sure: From attached patch: (XEN) dbg smp_processor_id() = 0 (XEN) dbg get_apic_id() = 0 (XEN) dbg cpuid_count(0xb, SMT_LEVEL) = 0x1, 0x2, 0x100, 0 (XEN) dbg cpuid_count(0xb, 0x1) = 0x4, 0x8, 0x201, 0 (XEN) dbg smp_processor_id() = 1 (XEN) dbg get_apic_id() = 1 (XEN) dbg cpuid_count(0xb, SMT_LEVEL) = 0x1, 0x2, 0x100, 0x1 (XEN) dbg cpuid_count(0xb, 0x1) = 0x4, 0x8, 0x201, 0x1 (XEN) dbg smp_processor_id() = 2 (XEN) dbg get_apic_id() = 2 (XEN) dbg cpuid_count(0xb, SMT_LEVEL) = 0x1, 0x2, 0x100, 0x2 (XEN) dbg cpuid_count(0xb, 0x1) = 0x4, 0x8, 0x201, 0x2 (XEN) dbg smp_processor_id() = 3 (XEN) dbg get_apic_id() = 3 (XEN) dbg cpuid_count(0xb, SMT_LEVEL) = 0x1, 0x2, 0x100, 0x3 (XEN) dbg cpuid_count(0xb, 0x1) = 0x4, 0x8, 0x201, 0x3 (XEN) dbg smp_processor_id() = 4 (XEN) dbg get_apic_id() = 4 (XEN) dbg cpuid_count(0xb, SMT_LEVEL) = 0x1, 0x2, 0x100, 0x4 (XEN) dbg cpuid_count(0xb, 0x1) = 0x4, 0x8, 0x201, 0x4 (XEN) dbg smp_processor_id() = 5 (XEN) dbg get_apic_id() = 5 (XEN) dbg cpuid_count(0xb, SMT_LEVEL) = 0x1, 0x2, 0x100, 0x5 (XEN) dbg cpuid_count(0xb, 0x1) = 0x4, 0x8, 0x201, 0x5 (XEN) dbg smp_processor_id() = 6 (XEN) dbg get_apic_id() = 6 (XEN) dbg cpuid_count(0xb, SMT_LEVEL) = 0x1, 0x2, 0x100, 0x6 (XEN) dbg cpuid_count(0xb, 0x1) = 0x4, 0x8, 0x201, 0x6 (XEN) dbg smp_processor_id() = 7 (XEN) dbg get_apic_id() = 7 (XEN) dbg cpuid_count(0xb, SMT_LEVEL) = 0x1, 0x2, 0x100, 0x7 (XEN) dbg cpuid_count(0xb, 0x1) = 0x4, 0x8, 0x201, 0x7 From the MADT table: [02Ch 0044 1] Subtable Type : 00 [Processor Local APIC] [02Dh 0045 1] Length : 08 [02Eh 0046 1] Processor ID : 00 [02Fh 0047 1] Local Apic ID : 00 [030h 0048 4] Flags (decoded below) : 00000001 Processor Enabled : 1 Runtime Online Capable : 0 [034h 0052 1] Subtable Type : 00 [Processor Local APIC] [035h 0053 1] Length : 08 [036h 0054 1] Processor ID : 01 [037h 0055 1] Local Apic ID : 02 [038h 0056 4] Flags (decoded below) : 00000001 Processor Enabled : 1 Runtime Online Capable : 0 [03Ch 0060 1] Subtable Type : 00 [Processor Local APIC] [03Dh 0061 1] Length : 08 [03Eh 0062 1] Processor ID : 02 [03Fh 0063 1] Local Apic ID : 04 [040h 0064 4] Flags (decoded below) : 00000001 Processor Enabled : 1 Runtime Online Capable : 0 [044h 0068 1] Subtable Type : 00 [Processor Local APIC] [045h 0069 1] Length : 08 [046h 0070 1] Processor ID : 03 [047h 0071 1] Local Apic ID : 06 [048h 0072 4] Flags (decoded below) : 00000001 Processor Enabled : 1 Runtime Online Capable : 0 [04Ch 0076 1] Subtable Type : 00 [Processor Local APIC] [04Dh 0077 1] Length : 08 [04Eh 0078 1] Processor ID : 04 [04Fh 0079 1] Local Apic ID : 01 [050h 0080 4] Flags (decoded below) : 00000001 Processor Enabled : 1 Runtime Online Capable : 0 [054h 0084 1] Subtable Type : 00 [Processor Local APIC] [055h 0085 1] Length : 08 [056h 0086 1] Processor ID : 05 [057h 0087 1] Local Apic ID : 03 [058h 0088 4] Flags (decoded below) : 00000001 Processor Enabled : 1 Runtime Online Capable : 0 [05Ch 0092 1] Subtable Type : 00 [Processor Local APIC] [05Dh 0093 1] Length : 08 [05Eh 0094 1] Processor ID : 06 [05Fh 0095 1] Local Apic ID : 05 [060h 0096 4] Flags (decoded below) : 00000001 Processor Enabled : 1 Runtime Online Capable : 0 [064h 0100 1] Subtable Type : 00 [Processor Local APIC] [065h 0101 1] Length : 08 [066h 0102 1] Processor ID : 07 [067h 0103 1] Local Apic ID : 07 [068h 0104 4] Flags (decoded below) : 00000001 Processor Enabled : 1 Runtime Online Capable : 0 [06Ch 0108 1] Subtable Type : 00 [Processor Local APIC] [06Dh 0109 1] Length : 08 [06Eh 0110 1] Processor ID : 08 [06Fh 0111 1] Local Apic ID : FF [070h 0112 4] Flags (decoded below) : 00000000 Processor Enabled : 0 Runtime Online Capable : 0 [074h 0116 1] Subtable Type : 00 [Processor Local APIC] [075h 0117 1] Length : 08 [076h 0118 1] Processor ID : 09 [077h 0119 1] Local Apic ID : FF [078h 0120 4] Flags (decoded below) : 00000000 Processor Enabled : 0 Runtime Online Capable : 0 [07Ch 0124 1] Subtable Type : 00 [Processor Local APIC] [07Dh 0125 1] Length : 08 [07Eh 0126 1] Processor ID : 0A [07Fh 0127 1] Local Apic ID : FF [080h 0128 4] Flags (decoded below) : 00000000 Processor Enabled : 0 Runtime Online Capable : 0 [084h 0132 1] Subtable Type : 00 [Processor Local APIC] [085h 0133 1] Length : 08 [086h 0134 1] Processor ID : 0B [087h 0135 1] Local Apic ID : FF [088h 0136 4] Flags (decoded below) : 00000000 Processor Enabled : 0 Runtime Online Capable : 0 [08Ch 0140 1] Subtable Type : 00 [Processor Local APIC] [08Dh 0141 1] Length : 08 [08Eh 0142 1] Processor ID : 0C [08Fh 0143 1] Local Apic ID : FF [090h 0144 4] Flags (decoded below) : 00000000 Processor Enabled : 0 Runtime Online Capable : 0 [094h 0148 1] Subtable Type : 00 [Processor Local APIC] [095h 0149 1] Length : 08 [096h 0150 1] Processor ID : 0D [097h 0151 1] Local Apic ID : FF [098h 0152 4] Flags (decoded below) : 00000000 Processor Enabled : 0 Runtime Online Capable : 0 [09Ch 0156 1] Subtable Type : 00 [Processor Local APIC] [09Dh 0157 1] Length : 08 [09Eh 0158 1] Processor ID : 0E [09Fh 0159 1] Local Apic ID : FF [0A0h 0160 4] Flags (decoded below) : 00000000 Processor Enabled : 0 Runtime Online Capable : 0 [0A4h 0164 1] Subtable Type : 00 [Processor Local APIC] [0A5h 0165 1] Length : 08 [0A6h 0166 1] Processor ID : 0F [0A7h 0167 1] Local Apic ID : FF [0A8h 0168 4] Flags (decoded below) : 00000000 Processor Enabled : 0 Runtime Online Capable : 0 If you wanted something else or need more, just let me know. Simon

Attachment: OpenPGP_signature
Description: OpenPGP digital signature

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.