Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1513823 > unrolled thread

[RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-02 13:30 +0100
    Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory "Charles (Chas) Williams" <ciwillia@brocade.com> - 2016-11-02 23:50 +0100
      Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-03 18:50 +0100
        Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory "Charles (Chas) Williams" <ciwillia@brocade.com> - 2016-11-04 13:30 +0100
          Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-04 19:10 +0100
            Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory "Charles (Chas) Williams" <ciwillia@brocade.com> - 2016-11-04 21:50 +0100
              Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-04 22:00 +0100
      Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate  memory Thomas Gleixner <tglx@linutronix.de> - 2016-11-07 17:30 +0100
        Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory "Charles (Chas) Williams" <ciwillia@brocade.com> - 2016-11-07 18:10 +0100
          Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate  memory Thomas Gleixner <tglx@linutronix.de> - 2016-11-07 21:30 +0100
            Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory "Charles (Chas) Williams" <ciwillia@brocade.com> - 2016-11-08 15:30 +0100
              Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate  memory Thomas Gleixner <tglx@linutronix.de> - 2016-11-08 15:40 +0100
                Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory "Charles (Chas) Williams" <ciwillia@brocade.com> - 2016-11-08 16:00 +0100
                  Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate  memory Thomas Gleixner <tglx@linutronix.de> - 2016-11-08 17:30 +0100
                    [PATCH] x86/cpuid: Deal with broken firmware once more Thomas Gleixner <tglx@linutronix.de> - 2016-11-09 16:40 +0100
                      Re: [PATCH] x86/cpuid: Deal with broken firmware once more Thomas Gleixner <tglx@linutronix.de> - 2016-11-09 16:50 +0100
                      Re: [PATCH] x86/cpuid: Deal with broken firmware once more Peter Zijlstra <peterz@infradead.org> - 2016-11-09 17:10 +0100
                        Re: [PATCH] x86/cpuid: Deal with broken firmware once more "Charles (Chas) Williams" <ciwillia@brocade.com> - 2016-11-09 17:40 +0100
                          Re: [PATCH] x86/cpuid: Deal with broken firmware once more Thomas Gleixner <tglx@linutronix.de> - 2016-11-09 19:50 +0100
                      Re: [PATCH] x86/cpuid: Deal with broken firmware once more "Charles (Chas) Williams" <ciwillia@brocade.com> - 2016-11-09 19:20 +0100
                      [tip:x86/urgent] x86/cpu: Deal with broken firmware (VMWare/XEN) tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2016-11-09 21:30 +0100
                        Re: [tip:x86/urgent] x86/cpu: Deal with broken firmware (VMWare/XEN) Alok Kataria <akataria@vmware.com> - 2016-11-11 07:00 +0100
                      Re: [PATCH] x86/cpuid: Deal with broken firmware once more "M. Vefa Bicakci" <m.v.b@runbox.com> - 2016-11-10 05:00 +0100
                        Re: [PATCH] x86/cpuid: Deal with broken firmware once more "Charles (Chas) Williams" <ciwillia@brocade.com> - 2016-11-10 12:00 +0100
                          Re: [PATCH] x86/cpuid: Deal with broken firmware once more Thomas Gleixner <tglx@linutronix.de> - 2016-11-10 12:20 +0100
                        Re: [PATCH] x86/cpuid: Deal with broken firmware once more Thomas Gleixner <tglx@linutronix.de> - 2016-11-10 12:20 +0100
                          Re: [PATCH] x86/cpuid: Deal with broken firmware once more Peter Zijlstra <peterz@infradead.org> - 2016-11-10 12:50 +0100
                          Re: [PATCH] x86/cpuid: Deal with broken firmware once more Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-11-10 15:10 +0100
                            Re: [PATCH] x86/cpuid: Deal with broken firmware once more "Charles (Chas) Williams" <ciwillia@brocade.com> - 2016-11-10 16:10 +0100
                              Re: [PATCH] x86/cpuid: Deal with broken firmware once more Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-11-10 16:40 +0100
                                Re: [PATCH] x86/cpuid: Deal with broken firmware once more Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-10 17:00 +0100
                                Re: [PATCH] x86/cpuid: Deal with broken firmware once more Thomas Gleixner <tglx@linutronix.de> - 2016-11-10 18:20 +0100
                            Re: [PATCH] x86/cpuid: Deal with broken firmware once more Thomas Gleixner <tglx@linutronix.de> - 2016-11-10 16:20 +0100
                              Re: [PATCH] x86/cpuid: Deal with broken firmware once more Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-11-10 16:40 +0100
                                Re: [PATCH] x86/cpuid: Deal with broken firmware once more Thomas Gleixner <tglx@linutronix.de> - 2016-11-10 18:20 +0100
                                  Re: [PATCH] x86/cpuid: Deal with broken firmware once more Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-11-10 19:10 +0100

Page 1 of 2  [1] 2  Next page →

#1513823 — [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

After the hotplug rework Charles Williams reported that his vmware
virtualized system no longer boots and crashes in rapl_cpu_online().
As it turns out topology_max_packages() reports four while
topology_logical_package_id() for CPU two and three returns 65535. That
means cpu_to_rapl_pmu() for those CPUs is accessing not allocated memory
of rapl_pmus->pmus[].
"M. Vefa Bicakci" reported the same problem on XEN.
This patch ensures we error out in such an invalid situation.

Reported-by: "Charles (Chas) Williams" <ciwillia@brocade.com>
Tested-by: "M. Vefa Bicakci" <m.v.b@runbox.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
I am not sure if this a race with the new hotplug code or something that was
always there. Both (M. Vefa Bicakc and Charles) say that the box boots
sometimes fine (without the patch).  smp_store_boot_cpu_info() should have run
before the notofoert and thus should have set the info properly. However I got
the following bootlog from Charles with this patch:

[    0.017110] smpboot: APIC(0) Converting physical 0 to logical package 0
[    0.017111] smpboot: APIC(1) Converting physical 1 to logical package 1
[    0.017113] smpboot: Max logical packages: 2                                                                                            
…
[    1.995494] RAPL PMU: rapl pmu error: max package: 2 but CPU1 belongs to 65535
[    1.995647] rapl pmu error: max package: 2 but CPU1 belongs to 65535
 
So it seems that the information got overwritten. I am not sure how to proceed
here. That memory corruption should be found and fixed and a boot crash might
motivate one to do so… I can't reproduce this on barematal.

Thread starts at
  d40f8e3c-b332-c331-38b9-11eb4f4aaaa7@brocade.com

 arch/x86/events/intel/rapl.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
index 0a535cea8ff3..f5d85f2853d7 100644
--- a/arch/x86/events/intel/rapl.c
+++ b/arch/x86/events/intel/rapl.c
@@ -682,6 +682,15 @@ static int __init init_rapl_pmus(void)
 {
 	int maxpkg = topology_max_packages();
 	size_t size;
+	unsigned int cpu;
+
+	for_each_possible_cpu(cpu) {
+		if (topology_logical_package_id(cpu) >= maxpkg) {
+			pr_err("rapl pmu error: max package: %u but CPU%d belongs to %u\n",
+			       maxpkg, cpu, topology_logical_package_id(cpu));
+			return -EINVAL;
+		}
+	}
 
 	size = sizeof(*rapl_pmus) + maxpkg * sizeof(struct rapl_pmu *);
 	rapl_pmus = kzalloc(size, GFP_KERNEL);
-- 
2.10.2

[toc] | [next] | [standalone]

#1514195

On 11/02/2016 08:25 AM, Sebastian Andrzej Siewior wrote:
> I am not sure if this a race with the new hotplug code or something that was
> always there. Both (M. Vefa Bicakc and Charles) say that the box boots
> sometimes fine (without the patch).  smp_store_boot_cpu_info() should have run
> before the notofoert and thus should have set the info properly. However I got
> the following bootlog from Charles with this patch:

I don't this this is a race.  Here is some debugging from the two CPU VM
(2 sockets, 1 core per socket).  In identify_cpu() we have:

         /* The boot/hotplug time assigment got cleared, restore it */
         c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);

The values just after this:

	[    0.228306] identify_cpu: c ffff88023fd0a040  logical_proc_id 65535  c->phys_proc_id 2

So what's interesting here, is the phys_proc_id of 2 for CPU1:

         int topology_phys_to_logical_pkg(unsigned int phys_pkg)
         {
                 if (phys_pkg >= max_physical_pkg_id)
                         return -1;
                 return physical_to_logical_pkg[phys_pkg];
         }

And we happen to know the max_physical_pkg_id is 2 in this case.
So apparently, topology_phys_to_logical_pkg() returns -1 and it gets
assigned to the logical_proc_id.

I don't know why the CPU's phys_proc_id is 2.

[toc] | [prev] | [next] | [standalone]

#1514748

On 2016-11-02 18:47:49 [-0400], Charles (Chas) Williams wrote:
> I don't this this is a race.  Here is some debugging from the two CPU VM
> (2 sockets, 1 core per socket).  In identify_cpu() we have:
> 
>         /* The boot/hotplug time assigment got cleared, restore it */
>         c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);
> 
> The values just after this:
> 
> 	[    0.228306] identify_cpu: c ffff88023fd0a040  logical_proc_id 65535  c->phys_proc_id 2
> 
> So what's interesting here, is the phys_proc_id of 2 for CPU1:
> 
>         int topology_phys_to_logical_pkg(unsigned int phys_pkg)
>         {
>                 if (phys_pkg >= max_physical_pkg_id)
>                         return -1;
>                 return physical_to_logical_pkg[phys_pkg];
>         }
> 
> And we happen to know the max_physical_pkg_id is 2 in this case.
> So apparently, topology_phys_to_logical_pkg() returns -1 and it gets
> assigned to the logical_proc_id.
> 
> I don't know why the CPU's phys_proc_id is 2.

This is the physical ID. You have two logical IDs (on your two sockets
machine). What is max_physical_pkg_id? In order to get that -1 you would
have to max_physical_pkg_id of 1 but code does
	max_physical_pkg_id = DIV_ROUND_UP(MAX_LOCAL_APIC, ncpus);

and I would be a little surprised if this is 1.

Sebastian

[toc] | [prev] | [next] | [standalone]

#1515173

On 11/03/2016 01:47 PM, Sebastian Andrzej Siewior wrote:
> On 2016-11-02 18:47:49 [-0400], Charles (Chas) Williams wrote:
>> I don't this this is a race.  Here is some debugging from the two CPU VM
>> (2 sockets, 1 core per socket).  In identify_cpu() we have:
>>
>>         /* The boot/hotplug time assigment got cleared, restore it */
>>         c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);
>>
>> The values just after this:
>>
>> 	[    0.228306] identify_cpu: c ffff88023fd0a040  logical_proc_id 65535  c->phys_proc_id 2
>>
>> So what's interesting here, is the phys_proc_id of 2 for CPU1:
>>
>>         int topology_phys_to_logical_pkg(unsigned int phys_pkg)
>>         {
>>                 if (phys_pkg >= max_physical_pkg_id)
>>                         return -1;
>>                 return physical_to_logical_pkg[phys_pkg];
>>         }
>>
>> And we happen to know the max_physical_pkg_id is 2 in this case.
>> So apparently, topology_phys_to_logical_pkg() returns -1 and it gets
>> assigned to the logical_proc_id.
>>
>> I don't know why the CPU's phys_proc_id is 2.
>
> This is the physical ID. You have two logical IDs (on your two sockets
> machine). What is max_physical_pkg_id? In order to get that -1 you would
> have to max_physical_pkg_id of 1 but code does
> 	max_physical_pkg_id = DIV_ROUND_UP(MAX_LOCAL_APIC, ncpus);
>
> and I would be a little surprised if this is 1.
>
> Sebastian

The initial CPU boots and is identified:

[    0.009018] identify_boot_cpu
[    0.009174] generic_identify: phys_proc_id is now 0
...
[    0.009427] identify_cpu: before c ffffffff81ae2680  logical_proc_id 0  c->phys_proc_id 0
[    0.009506] identify_cpu: after c ffffffff81ae2680  logical_proc_id 65535  c->phys_proc_id 0

So, this is fine because the APIC hasn't been scanned yet.  APIC
now gets scanned:

[    0.015789] smpboot: APIC(0) Converting physical 0 to logical package 0, cpu 0 (ffff88023fc0a040)
[    0.015794] smpboot: APIC(1) Converting physical 1 to logical package 1, cpu 1 (ffff88023fd0a040)
[    0.015797] smpboot: Max logical packages: 2

So, at this point, I think everything is correct.  But now the secondary
CPU's "boot":

[    0.236569] identify_secondary_cpu
[    0.236620] generic_identify: phys_proc_id is now 2

[    0.236745] identify_cpu: before c ffff88023fd0a040  logical_proc_id 65535  c->phys_proc_id 2
[    0.236747] identify_cpu: after c ffff88023fd0a040  logical_proc_id 65535  c->phys_proc_id 2

So, APIC discovered I have a cpu 0 and 1 but generic_identify() is called
my second CPU, 2.  This is >= max_physical_pkg_id, so it is going to get
set to -1.

The comment at the end of identfy_cpu() says:

         /* The boot/hotplug time assigment got cleared, restore it */

So, logical_proc_id being wrong here before restoration doesn't bother
me since I assume something in booting the secondary CPU's clears any
existing cpu data.

I know detect_extended_topology() is likely being called for both CPU's
and getting the right values (checking this now).  I don't know why
generic_identify() is resetting this value.

[toc] | [prev] | [next] | [standalone]

#1515347

On 2016-11-04 08:20:37 [-0400], Charles (Chas) Williams wrote:
> The initial CPU boots and is identified:
> 
> [    0.009018] identify_boot_cpu
> [    0.009174] generic_identify: phys_proc_id is now 0
> ...
> [    0.009427] identify_cpu: before c ffffffff81ae2680  logical_proc_id 0  c->phys_proc_id 0
> [    0.009506] identify_cpu: after c ffffffff81ae2680  logical_proc_id 65535  c->phys_proc_id 0
> 
> So, this is fine because the APIC hasn't been scanned yet.  APIC
> now gets scanned:
> 
> [    0.015789] smpboot: APIC(0) Converting physical 0 to logical package 0, cpu 0 (ffff88023fc0a040)
> [    0.015794] smpboot: APIC(1) Converting physical 1 to logical package 1, cpu 1 (ffff88023fd0a040)
> [    0.015797] smpboot: Max logical packages: 2

where is the APICID here is comming from?

> So, at this point, I think everything is correct.  But now the secondary
> CPU's "boot":
> 
> [    0.236569] identify_secondary_cpu
> [    0.236620] generic_identify: phys_proc_id is now 2

so here is where fun starts. Xen has also
arch/x86/xen/smp.c::cpu_bringup() where the phys_proc_id is changed. But
isn't done for vmware but it might a place where they duct tape things.

How is this APIC id different from the earlier? I guess based on your
output that generic_identify() changes the content of phys_proc_id.

> [    0.236745] identify_cpu: before c ffff88023fd0a040  logical_proc_id 65535  c->phys_proc_id 2
> [    0.236747] identify_cpu: after c ffff88023fd0a040  logical_proc_id 65535  c->phys_proc_id 2
> 
> So, APIC discovered I have a cpu 0 and 1 but generic_identify() is called
> my second CPU, 2.  This is >= max_physical_pkg_id, so it is going to get
> set to -1.

Now. max_physical_pkg_id is huge. The physical_to_logical_pkg array is
set to -1 on init so slot two has the value -1. That is what you see -
not the -1 because of ">= max_physical_pkg_id". 

> The comment at the end of identfy_cpu() says:
> 
>         /* The boot/hotplug time assigment got cleared, restore it */
> 
> So, logical_proc_id being wrong here before restoration doesn't bother
> me since I assume something in booting the secondary CPU's clears any
> existing cpu data.
> 
> I know detect_extended_topology() is likely being called for both CPU's
> and getting the right values (checking this now).  I don't know why
> generic_identify() is resetting this value.

I don't know either. But it is clearly reading the apic id twice and
second approach is different from the first which leads to different
results. So if you figure out how the first APICID for the second CPU is
retrieved and then you see how it happens for the second time. There
must be a difference.

Sebastian

[toc] | [prev] | [next] | [standalone]

#1515399

On 11/04/2016 02:03 PM, Sebastian Andrzej Siewior wrote:
> On 2016-11-04 08:20:37 [-0400], Charles (Chas) Williams wrote:
>> The initial CPU boots and is identified:
>>
>> [    0.009018] identify_boot_cpu
>> [    0.009174] generic_identify: phys_proc_id is now 0
>> ...
>> [    0.009427] identify_cpu: before c ffffffff81ae2680  logical_proc_id 0  c->phys_proc_id 0
>> [    0.009506] identify_cpu: after c ffffffff81ae2680  logical_proc_id 65535  c->phys_proc_id 0
>>
>> So, this is fine because the APIC hasn't been scanned yet.  APIC
>> now gets scanned:
>>
>> [    0.015789] smpboot: APIC(0) Converting physical 0 to logical package 0, cpu 0 (ffff88023fc0a040)
>> [    0.015794] smpboot: APIC(1) Converting physical 1 to logical package 1, cpu 1 (ffff88023fd0a040)
>> [    0.015797] smpboot: Max logical packages: 2
>
> where is the APICID here is comming from?

This comes from here:

                 unsigned int apicid = apic->cpu_present_to_apicid(cpu);

                 if (apicid == BAD_APICID || !apic->apic_id_valid(apicid))
                         continue;
                 if (!topology_update_package_map(apicid, cpu))

And I think this is the part that is "wrong".  The apicid appears to
be a logical CPU id.  I believe that in most cases this mapping comes
from x86_bios_cpu_apicid (or x86_cpu_to_apicid) which is generated in
generic_processor_info() which maps apicid's to logical cpu indexes.

Note that apic->cpu_present_to_apicid() is using just the cpu_index.

         for_each_present_cpu(cpu) {
                 unsigned int apicid = apic->cpu_present_to_apicid(cpu);

                 if (apicid == BAD_APICID || !apic->apic_id_valid(apicid))
                         continue;
                 if (!topology_update_package_map(apicid, cpu))
                         continue;
                 pr_warn("CPU %u APICId %x disabled\n", cpu, apicid);
                 per_cpu(x86_bios_cpu_apicid, cpu) = BAD_APICID;
                 set_cpu_possible(cpu, false);
                 set_cpu_present(cpu, false);
         }

>> So, at this point, I think everything is correct.  But now the secondary
>> CPU's "boot":
>>
>> [    0.236569] identify_secondary_cpu
>> [    0.236620] generic_identify: phys_proc_id is now 2
>
> so here is where fun starts. Xen has also
> arch/x86/xen/smp.c::cpu_bringup() where the phys_proc_id is changed. But
> isn't done for vmware but it might a place where they duct tape things.
>
> How is this APIC id different from the earlier? I guess based on your
> output that generic_identify() changes the content of phys_proc_id.
>
>> [    0.236745] identify_cpu: before c ffff88023fd0a040  logical_proc_id 65535  c->phys_proc_id 2
>> [    0.236747] identify_cpu: after c ffff88023fd0a040  logical_proc_id 65535  c->phys_proc_id 2
>>
>> So, APIC discovered I have a cpu 0 and 1 but generic_identify() is called
>> my second CPU, 2.  This is >= max_physical_pkg_id, so it is going to get
>> set to -1.
>
> Now. max_physical_pkg_id is huge. The physical_to_logical_pkg array is
> set to -1 on init so slot two has the value -1. That is what you see -
> not the -1 because of ">= max_physical_pkg_id".
>
>> The comment at the end of identfy_cpu() says:
>>
>>         /* The boot/hotplug time assigment got cleared, restore it */
>>
>> So, logical_proc_id being wrong here before restoration doesn't bother
>> me since I assume something in booting the secondary CPU's clears any
>> existing cpu data.
>>
>> I know detect_extended_topology() is likely being called for both CPU's
>> and getting the right values (checking this now).  I don't know why
>> generic_identify() is resetting this value.
>
> I don't know either. But it is clearly reading the apic id twice and
> second approach is different from the first which leads to different
> results. So if you figure out how the first APICID for the second CPU is
> retrieved and then you see how it happens for the second time. There
> must be a difference.

The phys core id from generic_identify() comes from the CPU's EBX register
so we _know_ this is right.

	       if (c->cpuid_level >= 0x00000001) {
			c->initial_apicid = (cpuid_ebx(1) >> 24) & 0xFF;
	#ifdef CONFIG_X86_32
	# ifdef CONFIG_SMP
			c->apicid = apic->phys_pkg_id(c->initial_apicid, 0);
	# else
			c->apicid = c->initial_apicid;
	# endif
	#endif
			c->phys_proc_id = c->initial_apicid;
		}

The intel docs http://x86.renejeschke.de/html/file_module_x86_id_45.html
claims this is the Local APIC ID.  So it seems likely this is correct
value.  It's not clear it matter if this is the right value or not
though.  Even if this is the correct apicid, nothing knows about it.

An argument could be made that instead of checking the cpuid level, we
could just use the apicid based on the cpu index just like the other code.
It would be consistent at least then.

[toc] | [prev] | [next] | [standalone]

#1515406

On 2016-11-04 16:42:33 [-0400], Charles (Chas) Williams wrote:
> This comes from here:
> 
>                 unsigned int apicid = apic->cpu_present_to_apicid(cpu);
> 
>                 if (apicid == BAD_APICID || !apic->apic_id_valid(apicid))
>                         continue;
>                 if (!topology_update_package_map(apicid, cpu))

It is late here. Can you check what function apic->cpu_present_to_apicid
is? It should do the right thing for your APIC.
 
> The phys core id from generic_identify() comes from the CPU's EBX register
> so we _know_ this is right.

So you are sure cpuid_level is > 1 here.

> 	       if (c->cpuid_level >= 0x00000001) {
> 			c->initial_apicid = (cpuid_ebx(1) >> 24) & 0xFF;
> 	#ifdef CONFIG_X86_32
> 	# ifdef CONFIG_SMP
> 			c->apicid = apic->phys_pkg_id(c->initial_apicid, 0);
> 	# else
> 			c->apicid = c->initial_apicid;
> 	# endif
> 	#endif
> 			c->phys_proc_id = c->initial_apicid;
> 		}
> 
> The intel docs http://x86.renejeschke.de/html/file_module_x86_id_45.html
> claims this is the Local APIC ID.  So it seems likely this is correct
> value.  It's not clear it matter if this is the right value or not
> though.  Even if this is the correct apicid, nothing knows about it.

Need to check that later (and your whole mail) against an official
manual. But as you see in the code, it checks the apicid against
->phys_pkg_id on 32bit so it seems sometimes there are changes required
to what the CPUID returns.

Sebastian

[toc] | [prev] | [next] | [standalone]

#1516290 — Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

On Wed, 2 Nov 2016, Charles (Chas) Williams wrote:

> On 11/02/2016 08:25 AM, Sebastian Andrzej Siewior wrote:
> > I am not sure if this a race with the new hotplug code or something that was
> > always there. Both (M. Vefa Bicakc and Charles) say that the box boots
> > sometimes fine (without the patch).  smp_store_boot_cpu_info() should have
> > run
> > before the notofoert and thus should have set the info properly. However I
> > got
> > the following bootlog from Charles with this patch:
> 
> I don't this this is a race.  Here is some debugging from the two CPU VM
> (2 sockets, 1 core per socket).  In identify_cpu() we have:
> 
>         /* The boot/hotplug time assigment got cleared, restore it */
>         c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);
> 
> The values just after this:
> 
> 	[    0.228306] identify_cpu: c ffff88023fd0a040  logical_proc_id 65535
> c->phys_proc_id 2
> 
> So what's interesting here, is the phys_proc_id of 2 for CPU1:
> 
>         int topology_phys_to_logical_pkg(unsigned int phys_pkg)
>         {
>                 if (phys_pkg >= max_physical_pkg_id)
>                         return -1;
>                 return physical_to_logical_pkg[phys_pkg];
>         }
> 
> And we happen to know the max_physical_pkg_id is 2 in this case.
> So apparently, topology_phys_to_logical_pkg() returns -1 and it gets
> assigned to the logical_proc_id.
> 
> I don't know why the CPU's phys_proc_id is 2.

max_physical_pkg_id gets initialized via:

    cpus = boot_cpu_data.x86_max_cores;
    max_physical_pkg_id = DIV_ROUND_UP(MAX_LOCAL_APIC, ncpus);

What's the value of boot_cpu_data.x86_max_cores and MAX_LOCAL_APIC?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1516345

On 11/07/2016 11:19 AM, Thomas Gleixner wrote:
> On Wed, 2 Nov 2016, Charles (Chas) Williams wrote:
>
>> On 11/02/2016 08:25 AM, Sebastian Andrzej Siewior wrote:
>>> I am not sure if this a race with the new hotplug code or something that was
>>> always there. Both (M. Vefa Bicakc and Charles) say that the box boots
>>> sometimes fine (without the patch).  smp_store_boot_cpu_info() should have
>>> run
>>> before the notofoert and thus should have set the info properly. However I
>>> got
>>> the following bootlog from Charles with this patch:
>>
>> I don't this this is a race.  Here is some debugging from the two CPU VM
>> (2 sockets, 1 core per socket).  In identify_cpu() we have:
>>
>>         /* The boot/hotplug time assigment got cleared, restore it */
>>         c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);
>>
>> The values just after this:
>>
>> 	[    0.228306] identify_cpu: c ffff88023fd0a040  logical_proc_id 65535
>> c->phys_proc_id 2
>>
>> So what's interesting here, is the phys_proc_id of 2 for CPU1:
>>
>>         int topology_phys_to_logical_pkg(unsigned int phys_pkg)
>>         {
>>                 if (phys_pkg >= max_physical_pkg_id)
>>                         return -1;
>>                 return physical_to_logical_pkg[phys_pkg];
>>         }
>>
>> And we happen to know the max_physical_pkg_id is 2 in this case.
>> So apparently, topology_phys_to_logical_pkg() returns -1 and it gets
>> assigned to the logical_proc_id.
>>
>> I don't know why the CPU's phys_proc_id is 2.
>
> max_physical_pkg_id gets initialized via:
>
>     cpus = boot_cpu_data.x86_max_cores;
>     max_physical_pkg_id = DIV_ROUND_UP(MAX_LOCAL_APIC, ncpus);
>
> What's the value of boot_cpu_data.x86_max_cores and MAX_LOCAL_APIC?

I have discovered that that is not the problem.  smp_init_package_map()
is calculating the physical core id using the following:

         for_each_present_cpu(cpu) {
                 unsigned int apicid = apic->cpu_present_to_apicid(cpu);

		...
		if (!topology_update_package_map(apicid, cpu))
                         continue;

	...
	int topology_update_package_map(unsigned int apicid, unsigned int cpu)
	{
		unsigned int new, pkg = apicid >> boot_cpu_data.x86_coreid_bits;

But later when the secondary CPU's are identified they use a different
calculation using the local APIC ID from the CPU's registers:

	static void generic_identify(struct cpuinfo_x86 *c)
	...
         if (c->cpuid_level >= 0x00000001) {
                 c->initial_apicid = (cpuid_ebx(1) >> 24) & 0xFF;
	...
		c->phys_proc_id = c->initial_apicid;

So at the end of identify_cpu() when the boot/hotplug assignment is
put back:

         c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);

topology_phys_to_logical_pkg() is returning an invalid logical processor
since one isn't configured.

It's not clear to me what the right thing to do is or which is right.

[toc] | [prev] | [next] | [standalone]

#1516564 — Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

On Mon, 7 Nov 2016, Charles (Chas) Williams wrote:
> On 11/07/2016 11:19 AM, Thomas Gleixner wrote:
> > On Wed, 2 Nov 2016, Charles (Chas) Williams wrote:
> > > I don't know why the CPU's phys_proc_id is 2.
> > 
> > max_physical_pkg_id gets initialized via:
> > 
> >     cpus = boot_cpu_data.x86_max_cores;
> >     max_physical_pkg_id = DIV_ROUND_UP(MAX_LOCAL_APIC, ncpus);
> > 
> > What's the value of boot_cpu_data.x86_max_cores and MAX_LOCAL_APIC?
> 
> I have discovered that that is not the problem.  smp_init_package_map()
> is calculating the physical core id using the following:
> 
>         for_each_present_cpu(cpu) {
>                 unsigned int apicid = apic->cpu_present_to_apicid(cpu);
> 
> 		...
> 		if (!topology_update_package_map(apicid, cpu))
>                         continue;
> 
> 	...
> 	int topology_update_package_map(unsigned int apicid, unsigned int cpu)
> 	{
> 		unsigned int new, pkg = apicid >>
> boot_cpu_data.x86_coreid_bits;
> 
> But later when the secondary CPU's are identified they use a different
> calculation using the local APIC ID from the CPU's registers:
> 
> 	static void generic_identify(struct cpuinfo_x86 *c)
> 	...
>         if (c->cpuid_level >= 0x00000001) {
>                 c->initial_apicid = (cpuid_ebx(1) >> 24) & 0xFF;
> 	...
> 		c->phys_proc_id = c->initial_apicid;
> 
> So at the end of identify_cpu() when the boot/hotplug assignment is
> put back:
> 
>         c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);
> 
> topology_phys_to_logical_pkg() is returning an invalid logical processor
> since one isn't configured.
> 
> It's not clear to me what the right thing to do is or which is right.

Nice detective work! So the issue is that the package mapping code honours
boot_cpu_data.x86_coreid_bit, while generic_identify does
not. boot_cpu_data.x86_coreid_bit is obviously 1 in your case. Tentative
fix below. I still need to gow through that maze and figure out what could
go wrong with that :(

Thanks,

	tglx

8<------------------------
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -905,6 +905,8 @@ static void detect_null_seg_behavior(str
 
 static void generic_identify(struct cpuinfo_x86 *c)
 {
+	unsigned int pkg;
+
 	c->extended_cpuid_level = 0;
 
 	if (!have_cpuid_p())
@@ -929,7 +931,8 @@ static void generic_identify(struct cpui
 		c->apicid = c->initial_apicid;
 # endif
 #endif
-		c->phys_proc_id = c->initial_apicid;
+		pkg = c->initial_apicid >> boot_cpu_data.x86_coreid_bits;
+		c->phys_proc_id = pkg;
 	}
 
 	get_model_name(c); /* Default name */

[toc] | [prev] | [next] | [standalone]

#1517237

On 11/07/2016 03:20 PM, Thomas Gleixner wrote:
> On Mon, 7 Nov 2016, Charles (Chas) Williams wrote:
>> On 11/07/2016 11:19 AM, Thomas Gleixner wrote:
>>> On Wed, 2 Nov 2016, Charles (Chas) Williams wrote:
>>>> I don't know why the CPU's phys_proc_id is 2.
>>>
>>> max_physical_pkg_id gets initialized via:
>>>
>>>     cpus = boot_cpu_data.x86_max_cores;
>>>     max_physical_pkg_id = DIV_ROUND_UP(MAX_LOCAL_APIC, ncpus);
>>>
>>> What's the value of boot_cpu_data.x86_max_cores and MAX_LOCAL_APIC?
>>
>> I have discovered that that is not the problem.  smp_init_package_map()
>> is calculating the physical core id using the following:
>>
>>         for_each_present_cpu(cpu) {
>>                 unsigned int apicid = apic->cpu_present_to_apicid(cpu);
>>
>> 		...
>> 		if (!topology_update_package_map(apicid, cpu))
>>                         continue;
>>
>> 	...
>> 	int topology_update_package_map(unsigned int apicid, unsigned int cpu)
>> 	{
>> 		unsigned int new, pkg = apicid >>
>> boot_cpu_data.x86_coreid_bits;
>>
>> But later when the secondary CPU's are identified they use a different
>> calculation using the local APIC ID from the CPU's registers:
>>
>> 	static void generic_identify(struct cpuinfo_x86 *c)
>> 	...
>>         if (c->cpuid_level >= 0x00000001) {
>>                 c->initial_apicid = (cpuid_ebx(1) >> 24) & 0xFF;
>> 	...
>> 		c->phys_proc_id = c->initial_apicid;
>>
>> So at the end of identify_cpu() when the boot/hotplug assignment is
>> put back:
>>
>>         c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);
>>
>> topology_phys_to_logical_pkg() is returning an invalid logical processor
>> since one isn't configured.
>>
>> It's not clear to me what the right thing to do is or which is right.
>
> Nice detective work! So the issue is that the package mapping code honours
> boot_cpu_data.x86_coreid_bit, while generic_identify does
> not. boot_cpu_data.x86_coreid_bit is obviously 1 in your case. Tentative
> fix below. I still need to gow through that maze and figure out what could
> go wrong with that :(

I don't think that will fix the issue.  My deubugging leads me to believe
that boot_cpu_data.x86_coreid_bits is probably 0:

	[    0.016335] topology_update_package_map:  apicid 0  pkg 0  cpu 0
	[    0.016398] smpboot: APIC(0) Converting physical 0 to logical package 0, cpu 0 (ffff88023fc0a040)
	[    0.016399] topology_update_package_map:  apicid 1  pkg 1  cpu 1
	[    0.016462] smpboot: APIC(1) Converting physical 1 to logical package 1, cpu 1 (ffff88023fd0a040)

So, I don't know where apic->cpu_present_to_apicid(cpu) is getting its
apicid but it certainly doesn't seem to the match the apicid in the
CPU's registers.  For whatever reason, my VMware system is reporting
that the second CPU has a local APIC ID of 2:

	[    0.009115] identify_cpu: cpu_index 0  phys_proc_id is now 0, apicid 0, initial_apicid 0
	...
	[    0.237401] identify_cpu: cpu_index 1  phys_proc_id is now 2, apicid 2, initial_apicid 2

I was thinking it might be better to call topology_update_package_map()
at the bottom of identify_cpu() to setup the secondary CPU's.  The boot
cpu could be setup during smp_init_package_map().

topology_update_package_map() is also poorly named it can create the
assignment, but can't update it (since it doesn't know the previous
mapping).


> 8<------------------------
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -905,6 +905,8 @@ static void detect_null_seg_behavior(str
>
>  static void generic_identify(struct cpuinfo_x86 *c)
>  {
> +	unsigned int pkg;
> +
>  	c->extended_cpuid_level = 0;
>
>  	if (!have_cpuid_p())
> @@ -929,7 +931,8 @@ static void generic_identify(struct cpui
>  		c->apicid = c->initial_apicid;
>  # endif
>  #endif
> -		c->phys_proc_id = c->initial_apicid;
> +		pkg = c->initial_apicid >> boot_cpu_data.x86_coreid_bits;
> +		c->phys_proc_id = pkg;
>  	}
>
>  	get_model_name(c); /* Default name */
>
>
>

[toc] | [prev] | [next] | [standalone]

#1517244 — Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

On Tue, 8 Nov 2016, Charles (Chas) Williams wrote:
> 	[    0.016335] topology_update_package_map:  apicid 0  pkg 0  cpu 0
> 	[    0.016398] smpboot: APIC(0) Converting physical 0 to logical
> package 0, cpu 0 (ffff88023fc0a040)
> 	[    0.016399] topology_update_package_map:  apicid 1  pkg 1  cpu 1
> 	[    0.016462] smpboot: APIC(1) Converting physical 1 to logical
> package 1, cpu 1 (ffff88023fd0a040)
> 
> So, I don't know where apic->cpu_present_to_apicid(cpu) is getting its
> apicid but it certainly doesn't seem to the match the apicid in the
> CPU's registers.  For whatever reason, my VMware system is reporting
> that the second CPU has a local APIC ID of 2:

The initial information comes from MP tables or ACPI.
 
> 	[    0.009115] identify_cpu: cpu_index 0  phys_proc_id is now 0,
> apicid 0, initial_apicid 0
> 	...
> 	[    0.237401] identify_cpu: cpu_index 1  phys_proc_id is now 2,
> apicid 2, initial_apicid 2

And the CPUID emulation tells something different. Sigh!

> I was thinking it might be better to call topology_update_package_map()
> at the bottom of identify_cpu() to setup the secondary CPU's.  The boot
> cpu could be setup during smp_init_package_map().

Perhaps, but that does not make the inconsistencies go away....

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1517258

On 11/08/2016 09:31 AM, Thomas Gleixner wrote:
> On Tue, 8 Nov 2016, Charles (Chas) Williams wrote:
>> 	[    0.016335] topology_update_package_map:  apicid 0  pkg 0  cpu 0
>> 	[    0.016398] smpboot: APIC(0) Converting physical 0 to logical
>> package 0, cpu 0 (ffff88023fc0a040)
>> 	[    0.016399] topology_update_package_map:  apicid 1  pkg 1  cpu 1
>> 	[    0.016462] smpboot: APIC(1) Converting physical 1 to logical
>> package 1, cpu 1 (ffff88023fd0a040)
>>
>> So, I don't know where apic->cpu_present_to_apicid(cpu) is getting its
>> apicid but it certainly doesn't seem to the match the apicid in the
>> CPU's registers.  For whatever reason, my VMware system is reporting
>> that the second CPU has a local APIC ID of 2:
>
> The initial information comes from MP tables or ACPI.
>
>> 	[    0.009115] identify_cpu: cpu_index 0  phys_proc_id is now 0,
>> apicid 0, initial_apicid 0
>> 	...
>> 	[    0.237401] identify_cpu: cpu_index 1  phys_proc_id is now 2,
>> apicid 2, initial_apicid 2
>
> And the CPUID emulation tells something different. Sigh!
>
>> I was thinking it might be better to call topology_update_package_map()
>> at the bottom of identify_cpu() to setup the secondary CPU's.  The boot
>> cpu could be setup during smp_init_package_map().
>
> Perhaps, but that does not make the inconsistencies go away....

By the time I know it's not consistent, there isn't anything I can do
about it.  I can't update the table to remove the bad information.

The other alternative, is to trust the ACPI and just update the
cpu_data's apicid in identify_cpu() to the value from the table.
The earlier kernels didn't seem to rely as much on this information.
But it does appear to be "wrong" in the APIC table.  From acpidump:

[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

[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 : 01
[038h 0056   4]        Flags (decoded below) : 00000001
                            Processor Enabled : 1

[toc] | [prev] | [next] | [standalone]

#1517346 — Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory

On Tue, 8 Nov 2016, Charles (Chas) Williams wrote:
> On 11/08/2016 09:31 AM, Thomas Gleixner wrote:
> > On Tue, 8 Nov 2016, Charles (Chas) Williams wrote:
> > > 	[    0.016335] topology_update_package_map:  apicid 0  pkg 0  cpu 0
> > > 	[    0.016398] smpboot: APIC(0) Converting physical 0 to logical
> > > package 0, cpu 0 (ffff88023fc0a040)
> > > 	[    0.016399] topology_update_package_map:  apicid 1  pkg 1  cpu 1
> > > 	[    0.016462] smpboot: APIC(1) Converting physical 1 to logical
> > > package 1, cpu 1 (ffff88023fd0a040)
> > > 
> > > So, I don't know where apic->cpu_present_to_apicid(cpu) is getting its
> > > apicid but it certainly doesn't seem to the match the apicid in the
> > > CPU's registers.  For whatever reason, my VMware system is reporting
> > > that the second CPU has a local APIC ID of 2:
> > 
> > The initial information comes from MP tables or ACPI.
> > 
> > > 	[    0.009115] identify_cpu: cpu_index 0  phys_proc_id is now 0,
> > > apicid 0, initial_apicid 0
> > > 	...
> > > 	[    0.237401] identify_cpu: cpu_index 1  phys_proc_id is now 2,
> > > apicid 2, initial_apicid 2
> > 
> > And the CPUID emulation tells something different. Sigh!
> > 
> > > I was thinking it might be better to call topology_update_package_map()
> > > at the bottom of identify_cpu() to setup the secondary CPU's.  The boot
> > > cpu could be setup during smp_init_package_map().
> > 
> > Perhaps, but that does not make the inconsistencies go away....
> 
> By the time I know it's not consistent, there isn't anything I can do
> about it.  I can't update the table to remove the bad information.
> 
> The other alternative, is to trust the ACPI and just update the
> cpu_data's apicid in identify_cpu() to the value from the table.
> The earlier kernels didn't seem to rely as much on this information.
> But it does appear to be "wrong" in the APIC table.  From acpidump:
> 
> [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
> 
> [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 : 01
> [038h 0056   4]        Flags (decoded below) : 00000001
>                            Processor Enabled : 1

Well, which one is wrong is hard to tell :)

I'll have a look how to sort that out.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1518267 — [PATCH] x86/cpuid: Deal with broken firmware once more

Both ACPI and MP specifications require that the APIC id in the respective
tables must be the same as the APIC id in CPUID. 

The kernel retrieves the physical package id from the APIC id during the
ACPI/MP table scan and builds the physical to logical package map.

There exist Virtualbox and Xen implementations which violate the spec. As a
result the physical to logical package map, which relies on the ACPI/MP
tables does not work on those systems, because the CPUID initialized
physical package id does not match the firmware id. This causes system
crashes and malfunction due to invalid package mappings.

The only way to cure this is to sanitize the physical package id after the
CPUID enumeration and yell when the APIC ids are different. If the physical
package IDs differ use the package information from the ACPI/MP tables so
the existing logical package map just works.

Reported-by: "Charles (Chas) Williams" <ciwillia@brocade.com>,
Reported-by: M. Vefa Bicakci <m.v.b@runbox.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/common.c |   31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -979,6 +979,34 @@ static void x86_init_cache_qos(struct cp
 }
 
 /*
+ * The physical to logical package id mapping is initialized from the
+ * acpi/mptables information. Make sure that CPUID actually agrees with
+ * that.
+ */
+static void sanitize_package_id(struct cpuinfo_x86 *c)
+{
+#ifdef CONFIG_SMP
+	unsigned int pkg, apicid, cpu = smp_processor_id();
+
+	apicid = apic->cpu_present_to_apicid(cpu);
+	pkg = apicid >> boot_cpu_data.x86_coreid_bits;
+
+	if (apicid != c->initial_apicid) {
+		pr_err(FW_BUG "CPU%u: APIC id mismatch. Firmware: %x CPUID: %x\n",
+		       cpu, apicid, c->initial_apicid);
+	}
+	if (pkg != c->phys_proc_id) {
+		pr_err(FW_BUG "CPU%u: Using firmware package id %u instead of %u\n",
+		       cpu, pkg, c->phys_proc_id);
+		c->phys_proc_id = pkg;
+	}
+	c->logical_proc_id = topology_phys_to_logical_pkg(pkg);
+#else
+	c->locical_proc_id = 0;
+#endif
+}
+
+/*
  * This does the hard work of actually picking apart the CPU stuff...
  */
 static void identify_cpu(struct cpuinfo_x86 *c)
@@ -1103,8 +1131,7 @@ static void identify_cpu(struct cpuinfo_
 #ifdef CONFIG_NUMA
 	numa_add_cpu(smp_processor_id());
 #endif
-	/* The boot/hotplug time assigment got cleared, restore it */
-	c->logical_proc_id = topology_phys_to_logical_pkg(c->phys_proc_id);
+	sanitize_package_id(c);
 }
 
 /*

[toc] | [prev] | [next] | [standalone]

#1518274 — Re: [PATCH] x86/cpuid: Deal with broken firmware once more

On Wed, 9 Nov 2016, Thomas Gleixner wrote:
> +	if (pkg != c->phys_proc_id) {
> +		pr_err(FW_BUG "CPU%u: Using firmware package id %u instead of %u\n",
> +		       cpu, pkg, c->phys_proc_id);
> +		c->phys_proc_id = pkg;
> +	}
> +	c->logical_proc_id = topology_phys_to_logical_pkg(pkg);
> +#else
> +	c->locical_proc_id = 0;

That want's to be logical_proc_id of course ...

Stupid me.

[toc] | [prev] | [next] | [standalone]

#1518279 — Re: [PATCH] x86/cpuid: Deal with broken firmware once more

On Wed, Nov 09, 2016 at 04:35:51PM +0100, Thomas Gleixner wrote:
> Both ACPI and MP specifications require that the APIC id in the respective
> tables must be the same as the APIC id in CPUID. 
> 
> The kernel retrieves the physical package id from the APIC id during the
> ACPI/MP table scan and builds the physical to logical package map.
> 
> There exist Virtualbox and Xen implementations which violate the spec. As a

ISTR it was VMware, not VirtualBox, but whatever.. they're both crazy
virt stuff.

>  /*
> + * The physical to logical package id mapping is initialized from the
> + * acpi/mptables information. Make sure that CPUID actually agrees with
> + * that.
> + */
> +static void sanitize_package_id(struct cpuinfo_x86 *c)
> +{
> +#ifdef CONFIG_SMP
> +	unsigned int pkg, apicid, cpu = smp_processor_id();
> +
> +	apicid = apic->cpu_present_to_apicid(cpu);
> +	pkg = apicid >> boot_cpu_data.x86_coreid_bits;
> +
> +	if (apicid != c->initial_apicid) {
> +		pr_err(FW_BUG "CPU%u: APIC id mismatch. Firmware: %x CPUID: %x\n",
> +		       cpu, apicid, c->initial_apicid);

Should we not also 'fix' c->initial_apicid ?

> +	}
> +	if (pkg != c->phys_proc_id) {
> +		pr_err(FW_BUG "CPU%u: Using firmware package id %u instead of %u\n",
> +		       cpu, pkg, c->phys_proc_id);
> +		c->phys_proc_id = pkg;
> +	}
> +	c->logical_proc_id = topology_phys_to_logical_pkg(pkg);
> +#else
> +	c->locical_proc_id = 0;

UP FTW ;-)

> +#endif
> +}

[toc] | [prev] | [next] | [standalone]

#1518296 — Re: [PATCH] x86/cpuid: Deal with broken firmware once more

On 11/09/2016 11:03 AM, Peter Zijlstra wrote:
> On Wed, Nov 09, 2016 at 04:35:51PM +0100, Thomas Gleixner wrote:
>> Both ACPI and MP specifications require that the APIC id in the respective
>> tables must be the same as the APIC id in CPUID.
>>
>> The kernel retrieves the physical package id from the APIC id during the
>> ACPI/MP table scan and builds the physical to logical package map.
>>
>> There exist Virtualbox and Xen implementations which violate the spec. As a
>
> ISTR it was VMware, not VirtualBox, but whatever.. they're both crazy
> virt stuff.

Yes, this was VMware in particular.  It would be good to get this comment
right so as not to mislead anyone.

>>  /*
>> + * The physical to logical package id mapping is initialized from the
>> + * acpi/mptables information. Make sure that CPUID actually agrees with
>> + * that.
>> + */
>> +static void sanitize_package_id(struct cpuinfo_x86 *c)
>> +{
>> +#ifdef CONFIG_SMP
>> +	unsigned int pkg, apicid, cpu = smp_processor_id();
>> +
>> +	apicid = apic->cpu_present_to_apicid(cpu);
>> +	pkg = apicid >> boot_cpu_data.x86_coreid_bits;
>> +
>> +	if (apicid != c->initial_apicid) {
>> +		pr_err(FW_BUG "CPU%u: APIC id mismatch. Firmware: %x CPUID: %x\n",
>> +		       cpu, apicid, c->initial_apicid);
>
> Should we not also 'fix' c->initial_apicid ?

Since we have c->apicid and c->initial_apicid it seems reasonable to keep one
set to the "correct" value.  I don't think c->initial_apicid is used past
this.

I should have some tests on this patch later today.

[toc] | [prev] | [next] | [standalone]

#1518405 — Re: [PATCH] x86/cpuid: Deal with broken firmware once more

On Wed, 9 Nov 2016, Charles (Chas) Williams wrote:
> On 11/09/2016 11:03 AM, Peter Zijlstra wrote:
> > On Wed, Nov 09, 2016 at 04:35:51PM +0100, Thomas Gleixner wrote:
> > > Both ACPI and MP specifications require that the APIC id in the respective
> > > tables must be the same as the APIC id in CPUID.
> > > 
> > > The kernel retrieves the physical package id from the APIC id during the
> > > ACPI/MP table scan and builds the physical to logical package map.
> > > 
> > > There exist Virtualbox and Xen implementations which violate the spec. As
> > > a
> > 
> > ISTR it was VMware, not VirtualBox, but whatever.. they're both crazy
> > virt stuff.
> 
> Yes, this was VMware in particular.  It would be good to get this comment
> right so as not to mislead anyone.

Sure, will fix.

> 
> > >  /*
> > > + * The physical to logical package id mapping is initialized from the
> > > + * acpi/mptables information. Make sure that CPUID actually agrees with
> > > + * that.
> > > + */
> > > +static void sanitize_package_id(struct cpuinfo_x86 *c)
> > > +{
> > > +#ifdef CONFIG_SMP
> > > +	unsigned int pkg, apicid, cpu = smp_processor_id();
> > > +
> > > +	apicid = apic->cpu_present_to_apicid(cpu);
> > > +	pkg = apicid >> boot_cpu_data.x86_coreid_bits;
> > > +
> > > +	if (apicid != c->initial_apicid) {
> > > +		pr_err(FW_BUG "CPU%u: APIC id mismatch. Firmware: %x CPUID:
> > > %x\n",
> > > +		       cpu, apicid, c->initial_apicid);
> > 
> > Should we not also 'fix' c->initial_apicid ?
> 
> Since we have c->apicid and c->initial_apicid it seems reasonable to keep one
> set to the "correct" value.  I don't think c->initial_apicid is used past
> this.

It is, but just for a printk in the MCE code, so it should not matter at all.

> I should have some tests on this patch later today.
> 

[toc] | [prev] | [next] | [standalone]

#1518362 — Re: [PATCH] x86/cpuid: Deal with broken firmware once more

On 11/09/2016 10:35 AM, Thomas Gleixner wrote:
> Both ACPI and MP specifications require that the APIC id in the respective
> tables must be the same as the APIC id in CPUID.
>
> The kernel retrieves the physical package id from the APIC id during the
> ACPI/MP table scan and builds the physical to logical package map.
>
> There exist Virtualbox and Xen implementations which violate the spec. As a
> result the physical to logical package map, which relies on the ACPI/MP
> tables does not work on those systems, because the CPUID initialized
> physical package id does not match the firmware id. This causes system
> crashes and malfunction due to invalid package mappings.
>
> The only way to cure this is to sanitize the physical package id after the
> CPUID enumeration and yell when the APIC ids are different. If the physical
> package IDs differ use the package information from the ACPI/MP tables so
> the existing logical package map just works.
>
> Reported-by: "Charles (Chas) Williams" <ciwillia@brocade.com>,
> Reported-by: M. Vefa Bicakci <m.v.b@runbox.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>


For 4 virtual sockets, 1 core per socket VM:

[    0.235459] .... node  #0, CPUs:      #1
[    0.238579] Disabled fast string operations
[    0.238620] mce: CPU supports 0 MCE banks
[    0.238864] [Firmware Bug]: CPU1: APIC id mismatch. Firmware: 1 CPUID: 2
[    0.238878] [Firmware Bug]: CPU1: Using firmware package id 1 instead of 2
[    0.239502]  #2
[    0.241298] Disabled fast string operations
[    0.241356] mce: CPU supports 0 MCE banks
[    0.241429] [Firmware Bug]: CPU2: APIC id mismatch. Firmware: 2 CPUID: 4
[    0.241431] [Firmware Bug]: CPU2: Using firmware package id 2 instead of 4
[    0.241631]  #3
[    0.244075] Disabled fast string operations
[    0.244112] mce: CPU supports 0 MCE banks
[    0.244284] [Firmware Bug]: CPU3: APIC id mismatch. Firmware: 3 CPUID: 6
[    0.244293] [Firmware Bug]: CPU3: Using firmware package id 3 instead of 6

For a 2 virtual sockets, 2 cores per socket, VMware seems to get its
APIC table correct as this fixup code isn't triggered.  The mapping looks like:

[    0.028911] topology_update_package_map:  apicid 0  pkg 0  cpu 0
[    0.029068] smpboot: APIC(0) Converting physical 0 to logical package 0, cpu 0
[    0.029072] topology_update_package_map:  apicid 1  pkg 0  cpu 1
[    0.029220] topology_update_package_map:  apicid 2  pkg 1  cpu 2
[    0.029376] smpboot: APIC(2) Converting physical 1 to logical package 1, cpu 2
[    0.029381] topology_update_package_map:  apicid 3  pkg 1  cpu 3
[    0.029525] smpboot: Max logical packages: 2

For a VM with 1 virtual socket and 4 cores, the behavior is again correct.

[    0.016198] topology_update_package_map:  apicid 0  pkg 0  cpu 0
[    0.016271] smpboot: APIC(0) Converting physical 0 to logical package 0, cpu 0 (ffff88023fc0a040)
[    0.016273] topology_update_package_map:  apicid 1  pkg 0  cpu 1
[    0.016336] topology_update_package_map:  apicid 2  pkg 0  cpu 2
[    0.016397] topology_update_package_map:  apicid 3  pkg 0  cpu 3

It looks like VMware might have some assumption about the minimum number
of cores on a virtual socket.  Regardless, the fix solves my problem!

Thanks!

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web