2016-12-08 09:07:15

by Thomas Gleixner

[permalink] [raw]
Subject: [PATCH] x86/smpboot: Make logical package management more robust

The logical package management has several issues:

- The APIC ids provided by ACPI are not required to be the same as the
initial APIC id which can be retrieved by CPUID. The APIC ids provided
by ACPI are those which are written by the BIOS into the APIC. The
initial id is set by hardware and can not be changed. The hardware
provided ids contain the real hardware package information.

Especially AMD sets the effective APIC id different from the hardware id
as they need to reserve space for the IOAPIC ids starting at id 0.

As a consequence those machines trigger the currently active firmware
bug printouts in dmesg, These are obviously wrong.

- Virtual machines have their own interesting way of enumerating APICs and
packages which are not reliably covered by the current implementation.

The sizing of the mapping array has been tweaked to be generously large to
handle systems which provide a wrong core count when HT is disabled so the
whole magic which checks for space in the physical hotplug case is not
needed anymore.

Simplify the whole machinery and do the mapping when the CPU starts and the
CPUID derived physical package information is available. This solves the
observed problems on AMD machines and works for the virtualization issues
as well.

Signed-off-by: Thomas Gleixner <[email protected]>
---
arch/x86/kernel/apic/apic.c | 15 ------------
arch/x86/kernel/cpu/common.c | 24 ++++++--------------
arch/x86/kernel/smpboot.c | 51 ++++++++++++++++---------------------------
3 files changed, 27 insertions(+), 63 deletions(-)

--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2159,21 +2159,6 @@ int __generic_processor_info(int apicid,
}

/*
- * This can happen on physical hotplug. The sanity check at boot time
- * is done from native_smp_prepare_cpus() after num_possible_cpus() is
- * established.
- */
- if (topology_update_package_map(apicid, cpu) < 0) {
- int thiscpu = max + disabled_cpus;
-
- pr_warning("APIC: Package limit reached. Processor %d/0x%x ignored.\n",
- thiscpu, apicid);
-
- disabled_cpus++;
- return -ENOSPC;
- }
-
- /*
* Validate version
*/
if (version == 0x0) {
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -979,29 +979,21 @@ 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.
+ * Validate that ACPI/mptables have the same information about the
+ * effective APIC id and update the package map.
*/
-static void sanitize_package_id(struct cpuinfo_x86 *c)
+static void validate_apic_and_package_id(struct cpuinfo_x86 *c)
{
#ifdef CONFIG_SMP
- unsigned int pkg, apicid, cpu = smp_processor_id();
+ unsigned int 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",
+ if (apicid != c->apicid) {
+ pr_err(FW_BUG "CPU%u: APIC id mismatch. Firmware: %x APIC: %x\n",
cpu, apicid, c->initial_apicid);
- c->initial_apicid = 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);
+ BUG_ON(topology_update_package_map(c->phys_proc_id, cpu));
#else
c->logical_proc_id = 0;
#endif
@@ -1132,7 +1124,6 @@ static void identify_cpu(struct cpuinfo_
#ifdef CONFIG_NUMA
numa_add_cpu(smp_processor_id());
#endif
- sanitize_package_id(c);
}

/*
@@ -1188,6 +1179,7 @@ void identify_secondary_cpu(struct cpuin
enable_sep_cpu();
#endif
mtrr_ap_init();
+ validate_apic_and_package_id(c);
}

static __init int setup_noclflush(char *arg)
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -104,7 +104,6 @@ static unsigned int max_physical_pkg_id
unsigned int __max_logical_packages __read_mostly;
EXPORT_SYMBOL(__max_logical_packages);
static unsigned int logical_packages __read_mostly;
-static bool logical_packages_frozen __read_mostly;

/* Maximum number of SMT threads on any online core */
int __max_smt_threads __read_mostly;
@@ -274,9 +273,14 @@ static void notrace start_secondary(void
cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
}

-int topology_update_package_map(unsigned int apicid, unsigned int cpu)
+/**
+ * topology_update_package_map - Update the physical to logical package map
+ * @pkg: The physical package id as retrieved via CPUID
+ * @cpu: The cpu for which this is updated
+ */
+int topology_update_package_map(unsigned int pkg, unsigned int cpu)
{
- unsigned int new, pkg = apicid >> boot_cpu_data.x86_coreid_bits;
+ unsigned int new;

/* Called from early boot ? */
if (!physical_package_map)
@@ -289,16 +293,17 @@ int topology_update_package_map(unsigned
if (test_and_set_bit(pkg, physical_package_map))
goto found;

- if (logical_packages_frozen) {
- physical_to_logical_pkg[pkg] = -1;
- pr_warn("APIC(%x) Package %u exceeds logical package max\n",
- apicid, pkg);
+ if (logical_packages >= __max_logical_packages) {
+ pr_warn("Package %u of CPU %u exceeds BIOS package data %u.\n",
+ logical_packages, cpu, __max_logical_packages);
return -ENOSPC;
}

new = logical_packages++;
- pr_info("APIC(%x) Converting physical %u to logical package %u\n",
- apicid, pkg, new);
+ if (new != pkg) {
+ pr_info("CPU %u Converting physical %u to logical package %u\n",
+ cpu, pkg, new);
+ }
physical_to_logical_pkg[pkg] = new;

found:
@@ -319,9 +324,9 @@ int topology_phys_to_logical_pkg(unsigne
}
EXPORT_SYMBOL(topology_phys_to_logical_pkg);

-static void __init smp_init_package_map(void)
+static void __init smp_init_package_map(unsigned int cpu, unsigned int pkg)
{
- unsigned int ncpus, cpu;
+ unsigned int ncpus;
size_t size;

/*
@@ -366,27 +371,9 @@ static void __init smp_init_package_map(
size = BITS_TO_LONGS(max_physical_pkg_id) * sizeof(unsigned long);
physical_package_map = kzalloc(size, GFP_KERNEL);

- 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);
- }
-
- if (logical_packages > __max_logical_packages) {
- pr_warn("Detected more packages (%u), then computed by BIOS data (%u).\n",
- logical_packages, __max_logical_packages);
- logical_packages_frozen = true;
- __max_logical_packages = logical_packages;
- }
-
pr_info("Max logical packages: %u\n", __max_logical_packages);
+
+ topology_update_package_map(pkg, cpu);
}

void __init smp_store_boot_cpu_info(void)
@@ -396,7 +383,7 @@ void __init smp_store_boot_cpu_info(void

*c = boot_cpu_data;
c->cpu_index = id;
- smp_init_package_map();
+ smp_init_package_map(id, c->phys_proc_id);
}

/*


2016-12-08 09:14:00

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH] x86/smpboot: Make logical package management more robust

On Thu, Dec 08, 2016 at 10:04:25AM +0100, Thomas Gleixner wrote:
> The logical package management has several issues:
>
> - The APIC ids provided by ACPI are not required to be the same as the
> initial APIC id which can be retrieved by CPUID. The APIC ids provided
> by ACPI are those which are written by the BIOS into the APIC. The
> initial id is set by hardware and can not be changed. The hardware
> provided ids contain the real hardware package information.
>
> Especially AMD sets the effective APIC id different from the hardware id
> as they need to reserve space for the IOAPIC ids starting at id 0.
>
> As a consequence those machines trigger the currently active firmware
> bug printouts in dmesg, These are obviously wrong.
>
> - Virtual machines have their own interesting way of enumerating APICs and
> packages which are not reliably covered by the current implementation.
>
> The sizing of the mapping array has been tweaked to be generously large to
> handle systems which provide a wrong core count when HT is disabled so the
> whole magic which checks for space in the physical hotplug case is not
> needed anymore.
>
> Simplify the whole machinery and do the mapping when the CPU starts and the
> CPUID derived physical package information is available. This solves the
> observed problems on AMD machines and works for the virtualization issues
> as well.
>
> Signed-off-by: Thomas Gleixner <[email protected]>

Already

Tested-by: Borislav Petkov <[email protected]>

Thanks!

--
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

2016-12-08 12:08:44

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [PATCH] x86/smpboot: Make logical package management more robust

On Thu, Dec 08, 2016 at 10:04:25AM +0100, Thomas Gleixner wrote:
> @@ -289,16 +293,17 @@ int topology_update_package_map(unsigned
> if (test_and_set_bit(pkg, physical_package_map))
> goto found;
>
> - if (logical_packages_frozen) {
> - physical_to_logical_pkg[pkg] = -1;
> - pr_warn("APIC(%x) Package %u exceeds logical package max\n",
> - apicid, pkg);
> + if (logical_packages >= __max_logical_packages) {
> + pr_warn("Package %u of CPU %u exceeds BIOS package data %u.\n",
> + logical_packages, cpu, __max_logical_packages);
> return -ENOSPC;
> }
>
> new = logical_packages++;
> - pr_info("APIC(%x) Converting physical %u to logical package %u\n",
> - apicid, pkg, new);
> + if (new != pkg) {
> + pr_info("CPU %u Converting physical %u to logical package %u\n",
> + cpu, pkg, new);
> + }

This makes the print conditional on the phy<->logical mapping not
matching; I thought it was a concious decision to print everything in
the initial version.

This way, if we have a 4 node system and nodes 1,2 are crossed we'll
only see:

"Converting physical 2 to logical package 1"
"Converting physical 1 to logical package 2"

And nothing on the other two nodes, which could be slightly confusing.

> physical_to_logical_pkg[pkg] = new;
>
> found:



> @@ -366,27 +371,9 @@ static void __init smp_init_package_map(
> size = BITS_TO_LONGS(max_physical_pkg_id) * sizeof(unsigned long);
> physical_package_map = kzalloc(size, GFP_KERNEL);
>
> - 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);
> - }
> -
> - if (logical_packages > __max_logical_packages) {
> - pr_warn("Detected more packages (%u), then computed by BIOS data (%u).\n",
> - logical_packages, __max_logical_packages);
> - logical_packages_frozen = true;
> - __max_logical_packages = logical_packages;

So we'll never 'shrink' the initially computed max; which could result
in using more memory than strictly needed, otoh it makes physical
hotplug happier.

> - }
> -
> pr_info("Max logical packages: %u\n", __max_logical_packages);
> +
> + topology_update_package_map(pkg, cpu);
> }


2016-12-08 12:52:13

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] x86/smpboot: Make logical package management more robust

On Thu, 8 Dec 2016, Peter Zijlstra wrote:
> On Thu, Dec 08, 2016 at 10:04:25AM +0100, Thomas Gleixner wrote:
> > @@ -289,16 +293,17 @@ int topology_update_package_map(unsigned
> > if (test_and_set_bit(pkg, physical_package_map))
> > goto found;
> >
> > - if (logical_packages_frozen) {
> > - physical_to_logical_pkg[pkg] = -1;
> > - pr_warn("APIC(%x) Package %u exceeds logical package max\n",
> > - apicid, pkg);
> > + if (logical_packages >= __max_logical_packages) {
> > + pr_warn("Package %u of CPU %u exceeds BIOS package data %u.\n",
> > + logical_packages, cpu, __max_logical_packages);
> > return -ENOSPC;
> > }
> >
> > new = logical_packages++;
> > - pr_info("APIC(%x) Converting physical %u to logical package %u\n",
> > - apicid, pkg, new);
> > + if (new != pkg) {
> > + pr_info("CPU %u Converting physical %u to logical package %u\n",
> > + cpu, pkg, new);
> > + }
>
> This makes the print conditional on the phy<->logical mapping not
> matching; I thought it was a concious decision to print everything in
> the initial version.
>
> This way, if we have a 4 node system and nodes 1,2 are crossed we'll
> only see:
>
> "Converting physical 2 to logical package 1"
> "Converting physical 1 to logical package 2"
>
> And nothing on the other two nodes, which could be slightly confusing.

Fair enough. I make it unconditional again.

> > - if (logical_packages > __max_logical_packages) {
> > - pr_warn("Detected more packages (%u), then computed by BIOS data (%u).\n",
> > - logical_packages, __max_logical_packages);
> > - logical_packages_frozen = true;
> > - __max_logical_packages = logical_packages;
>
> So we'll never 'shrink' the initially computed max; which could result
> in using more memory than strictly needed, otoh it makes physical
> hotplug happier.

Yes. I was debating that back and forth and at the end decided that making
it simple and robust is a good tradeoff vs. the slightly higher memory
consumption. Though on most systems that's a non issue as number of
possible cpus/packages is the same as the actual available ones. The insane
setups have to suffer - rightfully so.

Thanks,

tglx

2016-12-08 13:04:47

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [PATCH] x86/smpboot: Make logical package management more robust

On Thu, Dec 08, 2016 at 01:49:28PM +0100, Thomas Gleixner wrote:
>
> > > - if (logical_packages > __max_logical_packages) {
> > > - pr_warn("Detected more packages (%u), then computed by BIOS data (%u).\n",
> > > - logical_packages, __max_logical_packages);
> > > - logical_packages_frozen = true;
> > > - __max_logical_packages = logical_packages;
> >
> > So we'll never 'shrink' the initially computed max; which could result
> > in using more memory than strictly needed, otoh it makes physical
> > hotplug happier.
>
> Yes. I was debating that back and forth and at the end decided that making
> it simple and robust is a good tradeoff vs. the slightly higher memory
> consumption. Though on most systems that's a non issue as number of
> possible cpus/packages is the same as the actual available ones. The insane
> setups have to suffer - rightfully so.

Don't we overestimate by a factor of 2 due to HT? That is, every single
socket Intel box will have a max_packages of 2.

Not that I care too deeply, and arguably the HT case _is_ insane because
its impossible to tell etc..

2016-12-08 13:12:01

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] x86/smpboot: Make logical package management more robust

On Thu, 8 Dec 2016, Peter Zijlstra wrote:
> On Thu, Dec 08, 2016 at 01:49:28PM +0100, Thomas Gleixner wrote:
> >
> > > > - if (logical_packages > __max_logical_packages) {
> > > > - pr_warn("Detected more packages (%u), then computed by BIOS data (%u).\n",
> > > > - logical_packages, __max_logical_packages);
> > > > - logical_packages_frozen = true;
> > > > - __max_logical_packages = logical_packages;
> > >
> > > So we'll never 'shrink' the initially computed max; which could result
> > > in using more memory than strictly needed, otoh it makes physical
> > > hotplug happier.
> >
> > Yes. I was debating that back and forth and at the end decided that making
> > it simple and robust is a good tradeoff vs. the slightly higher memory
> > consumption. Though on most systems that's a non issue as number of
> > possible cpus/packages is the same as the actual available ones. The insane
> > setups have to suffer - rightfully so.
>
> Don't we overestimate by a factor of 2 due to HT? That is, every single
> socket Intel box will have a max_packages of 2.
>
> Not that I care too deeply, and arguably the HT case _is_ insane because
> its impossible to tell etc..

It is insane.

And we can be smart about it for the normal, non physical hotplug case when
all available CPUs are brought up in smp_init() which is _before_ any of
the package users is initialized.

At that point we know exactly how many packages are available and we can
limit max packages to that value. Hmm?

Thanks,

tglx

2016-12-09 22:09:28

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] x86/smpboot: Make logical package management more robust

On Thu, 8 Dec 2016, Thomas Gleixner wrote:

Boris, can you please verify if that makes the
topology_update_package_map() call which you placed into the Xen cpu
starting code obsolete ?

Thanks,

tglx

2016-12-09 23:01:57

by Boris Ostrovsky

[permalink] [raw]
Subject: Re: [PATCH] x86/smpboot: Make logical package management more robust

On 12/09/2016 05:06 PM, Thomas Gleixner wrote:
> On Thu, 8 Dec 2016, Thomas Gleixner wrote:
>
> Boris, can you please verify if that makes the
> topology_update_package_map() call which you placed into the Xen cpu
> starting code obsolete ?

Will do. I did test your patch but without removing
topology_update_package_map() call. It complained about package IDs
being wrong, but that's expected until I fix Xen part.

-boris

2016-12-09 23:03:16

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] x86/smpboot: Make logical package management more robust

On Fri, 9 Dec 2016, Boris Ostrovsky wrote:
> On 12/09/2016 05:06 PM, Thomas Gleixner wrote:
> > On Thu, 8 Dec 2016, Thomas Gleixner wrote:
> >
> > Boris, can you please verify if that makes the
> > topology_update_package_map() call which you placed into the Xen cpu
> > starting code obsolete ?
>
> Will do. I did test your patch but without removing
> topology_update_package_map() call. It complained about package IDs
> being wrong, but that's expected until I fix Xen part.

That should not longer be the case as I changed the approach to that
management thing.

Thanks,

tglx

2016-12-10 03:29:30

by Boris Ostrovsky

[permalink] [raw]
Subject: Re: [PATCH] x86/smpboot: Make logical package management more robust



On 12/09/2016 06:02 PM, Boris Ostrovsky wrote:
> On 12/09/2016 05:06 PM, Thomas Gleixner wrote:
>> On Thu, 8 Dec 2016, Thomas Gleixner wrote:
>>
>> Boris, can you please verify if that makes the
>> topology_update_package_map() call which you placed into the Xen cpu
>> starting code obsolete ?
>
> Will do. I did test your patch but without removing
> topology_update_package_map() call. It complained about package IDs
> being wrong, but that's expected until I fix Xen part.

Ignore my statement about earlier testing --- it was all on single-node
machines.

Something is broken with multi-node on Intel, but failure modes are
different. Prior to this patch build_sched_domain() reports an error and
pretty soon we crash in scheduler (don't remember off the top of my
head). With patch applied I crash mush later, when one of the drivers
does kmalloc_node(.., cpu_to_node(cpu)) and cpu_to_node() returns 1,
which should never happen ("x86: Booted up 1 node, 32 CPUs" is reported,
for example).

2-node AMD box doesn't have these problems.

I haven't upgraded the Intel machine for about a month but this all must
have happened in 4.9 timeframe.

So I can't answer your question since we clearly have other problems on
Xen. I will be looking into this.

-boris

2016-12-10 03:38:57

by Boris Ostrovsky

[permalink] [raw]
Subject: Re: [PATCH] x86/smpboot: Make logical package management more robust



On 12/09/2016 06:00 PM, Thomas Gleixner wrote:
> On Fri, 9 Dec 2016, Boris Ostrovsky wrote:
>> On 12/09/2016 05:06 PM, Thomas Gleixner wrote:
>>> On Thu, 8 Dec 2016, Thomas Gleixner wrote:
>>>
>>> Boris, can you please verify if that makes the
>>> topology_update_package_map() call which you placed into the Xen cpu
>>> starting code obsolete ?
>>
>> Will do. I did test your patch but without removing
>> topology_update_package_map() call. It complained about package IDs
>> being wrong, but that's expected until I fix Xen part.
>
> That should not longer be the case as I changed the approach to that
> management thing.


I didn't notice this email before I sent the earlier message.

Is these anything else besides this patch that I should use? I applied
it to Linus tree and it didn't apply cleanly (there was some fuzz and
such) so I wonder whether I am missing something.

-boris

2016-12-10 19:06:46

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] x86/smpboot: Make logical package management more robust

On Fri, 9 Dec 2016, Boris Ostrovsky wrote:
> On 12/09/2016 06:00 PM, Thomas Gleixner wrote:
> > On Fri, 9 Dec 2016, Boris Ostrovsky wrote:
> > > On 12/09/2016 05:06 PM, Thomas Gleixner wrote:
> > > > On Thu, 8 Dec 2016, Thomas Gleixner wrote:
> > > >
> > > > Boris, can you please verify if that makes the
> > > > topology_update_package_map() call which you placed into the Xen cpu
> > > > starting code obsolete ?
> > >
> > > Will do. I did test your patch but without removing
> > > topology_update_package_map() call. It complained about package IDs
> > > being wrong, but that's expected until I fix Xen part.
> >
> > That should not longer be the case as I changed the approach to that
> > management thing.
>
>
> I didn't notice this email before I sent the earlier message.
>
> Is these anything else besides this patch that I should use? I applied it to
> Linus tree and it didn't apply cleanly (there was some fuzz and such) so I
> wonder whether I am missing something.

No. I did it against tip, but there is nothing which it depends on.

Thanks,

tglx

2016-12-10 19:11:53

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] x86/smpboot: Make logical package management more robust

On Fri, 9 Dec 2016, Boris Ostrovsky wrote:
> On 12/09/2016 06:02 PM, Boris Ostrovsky wrote:
> > On 12/09/2016 05:06 PM, Thomas Gleixner wrote:
> > > On Thu, 8 Dec 2016, Thomas Gleixner wrote:
> > >
> > > Boris, can you please verify if that makes the
> > > topology_update_package_map() call which you placed into the Xen cpu
> > > starting code obsolete ?
> >
> > Will do. I did test your patch but without removing
> > topology_update_package_map() call. It complained about package IDs
> > being wrong, but that's expected until I fix Xen part.
>
> Ignore my statement about earlier testing --- it was all on single-node
> machines.
>
> Something is broken with multi-node on Intel, but failure modes are different.
> Prior to this patch build_sched_domain() reports an error and pretty soon we
> crash in scheduler (don't remember off the top of my head). With patch applied
> I crash mush later, when one of the drivers does kmalloc_node(..,
> cpu_to_node(cpu)) and cpu_to_node() returns 1, which should never happen
> ("x86: Booted up 1 node, 32 CPUs" is reported, for example).

Hmm. But the cpu_to_node() association is unrelated to the logical package
management.

> 2-node AMD box doesn't have these problems.
>
> I haven't upgraded the Intel machine for about a month but this all must have
> happened in 4.9 timeframe.
>
> So I can't answer your question since we clearly have other problems on Xen. I
> will be looking into this.

Fair enough. What you could do though with this patch applied and the extra
XEN call to topology_update_package_map() removed is to watchout for the
following messages:

pr_info("Max logical packages: %u\n", __max_logical_packages);

and

pr_warn(CPU %u Converting physical %u to logical package %u\n", ...)

Ideally the latter wont show.

Thanks,

tglx

2016-12-10 19:16:16

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] x86/smpboot: Make logical package management more robust

On Sat, 10 Dec 2016, Thomas Gleixner wrote:
> On Fri, 9 Dec 2016, Boris Ostrovsky wrote:
> > On 12/09/2016 06:02 PM, Boris Ostrovsky wrote:
> > > On 12/09/2016 05:06 PM, Thomas Gleixner wrote:
> > > > On Thu, 8 Dec 2016, Thomas Gleixner wrote:
> > > >
> > > > Boris, can you please verify if that makes the
> > > > topology_update_package_map() call which you placed into the Xen cpu
> > > > starting code obsolete ?
> > >
> > > Will do. I did test your patch but without removing
> > > topology_update_package_map() call. It complained about package IDs
> > > being wrong, but that's expected until I fix Xen part.
> >
> > Ignore my statement about earlier testing --- it was all on single-node
> > machines.
> >
> > Something is broken with multi-node on Intel, but failure modes are different.
> > Prior to this patch build_sched_domain() reports an error and pretty soon we
> > crash in scheduler (don't remember off the top of my head). With patch applied
> > I crash mush later, when one of the drivers does kmalloc_node(..,
> > cpu_to_node(cpu)) and cpu_to_node() returns 1, which should never happen
> > ("x86: Booted up 1 node, 32 CPUs" is reported, for example).
>
> Hmm. But the cpu_to_node() association is unrelated to the logical package
> management.

Just came to my mind after hitting send. We had the whole persistent cpuid
to nodeid association work merged in 4.9. So that might be related.

Thanks,

tglx

2016-12-11 03:25:45

by Boris Ostrovsky

[permalink] [raw]
Subject: Re: [PATCH] x86/smpboot: Make logical package management more robust



On 12/10/2016 02:13 PM, Thomas Gleixner wrote:
> On Sat, 10 Dec 2016, Thomas Gleixner wrote:
>> On Fri, 9 Dec 2016, Boris Ostrovsky wrote:
>>> On 12/09/2016 06:02 PM, Boris Ostrovsky wrote:
>>>> On 12/09/2016 05:06 PM, Thomas Gleixner wrote:
>>>>> On Thu, 8 Dec 2016, Thomas Gleixner wrote:
>>>>>
>>>>> Boris, can you please verify if that makes the
>>>>> topology_update_package_map() call which you placed into the Xen cpu
>>>>> starting code obsolete ?
>>>>
>>>> Will do. I did test your patch but without removing
>>>> topology_update_package_map() call. It complained about package IDs
>>>> being wrong, but that's expected until I fix Xen part.
>>>
>>> Ignore my statement about earlier testing --- it was all on single-node
>>> machines.
>>>
>>> Something is broken with multi-node on Intel, but failure modes are different.
>>> Prior to this patch build_sched_domain() reports an error and pretty soon we
>>> crash in scheduler (don't remember off the top of my head). With patch applied
>>> I crash mush later, when one of the drivers does kmalloc_node(..,
>>> cpu_to_node(cpu)) and cpu_to_node() returns 1, which should never happen
>>> ("x86: Booted up 1 node, 32 CPUs" is reported, for example).
>>
>> Hmm. But the cpu_to_node() association is unrelated to the logical package
>> management.
>
> Just came to my mind after hitting send. We had the whole persistent cpuid
> to nodeid association work merged in 4.9. So that might be related.


Yes, that's exactly the reason.

It uses _PXM to set nodeID and _PXM is exposed to dom0 (which is a
privileged PV guest).

Re: you previous message: after I "fix" the problem above, I see
pr_info("Max logical packages: %u\n", __max_logical_packages);
but no
pr_warn(CPU %u Converting physical %u to logical package %u\n", ...)

with or without topology_update_package_map() in
arch/x86/xen/smp.c:cpu_bringup()


-boris


2016-12-12 10:07:47

by Thomas Gleixner

[permalink] [raw]
Subject: [PATCH v2] x86/smpboot: Make logical package management more robust

The logical package management has several issues:

- The APIC ids provided by ACPI are not required to be the same as the
initial APIC id which can be retrieved by CPUID. The APIC ids provided
by ACPI are those which are written by the BIOS into the APIC. The
initial id is set by hardware and can not be changed. The hardware
provided ids contain the real hardware package information.

Especially AMD sets the effective APIC id different from the hardware id
as they need to reserve space for the IOAPIC ids starting at id 0.

As a consequence those machines trigger the currently active firmware
bug printouts in dmesg, These are obviously wrong.

- Virtual machines have their own interesting of enumerating APICs and
packages which are not reliably covered by the current implementation.

The sizing of the mapping array has been tweaked to be generously large to
handle systems which provide a wrong core count when HT is disabled so the
whole magic which checks for space in the physical hotplug case is not
needed anymore.

Simplify the whole machinery and do the mapping when the CPU starts and the
CPUID derived physical package information is available. This solves the
observed problems on AMD machines and works for the virtualization issues
as well.

Remove the extra call from XEN cpu bringup code as it is not longer
required.

Fixes: d49597fd3bc7 ("x86/cpu: Deal with broken firmware (VMWare/XEN)")
Reported-and-tested-by: Borislav Petkov <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: [email protected]
---
arch/x86/kernel/apic/apic.c | 15 ------------
arch/x86/kernel/cpu/common.c | 24 ++++++--------------
arch/x86/kernel/smpboot.c | 51 ++++++++++++++++---------------------------
arch/x86/xen/smp.c | 6 -----
4 files changed, 27 insertions(+), 69 deletions(-)

--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2159,21 +2159,6 @@ int __generic_processor_info(int apicid,
}

/*
- * This can happen on physical hotplug. The sanity check at boot time
- * is done from native_smp_prepare_cpus() after num_possible_cpus() is
- * established.
- */
- if (topology_update_package_map(apicid, cpu) < 0) {
- int thiscpu = max + disabled_cpus;
-
- pr_warning("APIC: Package limit reached. Processor %d/0x%x ignored.\n",
- thiscpu, apicid);
-
- disabled_cpus++;
- return -ENOSPC;
- }
-
- /*
* Validate version
*/
if (version == 0x0) {
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -979,29 +979,21 @@ 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.
+ * Validate that ACPI/mptables have the same information about the
+ * effective APIC id and update the package map.
*/
-static void sanitize_package_id(struct cpuinfo_x86 *c)
+static void validate_apic_and_package_id(struct cpuinfo_x86 *c)
{
#ifdef CONFIG_SMP
- unsigned int pkg, apicid, cpu = smp_processor_id();
+ unsigned int 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",
+ if (apicid != c->apicid) {
+ pr_err(FW_BUG "CPU%u: APIC id mismatch. Firmware: %x APIC: %x\n",
cpu, apicid, c->initial_apicid);
- c->initial_apicid = 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);
+ BUG_ON(topology_update_package_map(c->phys_proc_id, cpu));
#else
c->logical_proc_id = 0;
#endif
@@ -1132,7 +1124,6 @@ static void identify_cpu(struct cpuinfo_
#ifdef CONFIG_NUMA
numa_add_cpu(smp_processor_id());
#endif
- sanitize_package_id(c);
}

/*
@@ -1188,6 +1179,7 @@ void identify_secondary_cpu(struct cpuin
enable_sep_cpu();
#endif
mtrr_ap_init();
+ validate_apic_and_package_id(c);
}

struct msr_range {
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -104,7 +104,6 @@ static unsigned int max_physical_pkg_id
unsigned int __max_logical_packages __read_mostly;
EXPORT_SYMBOL(__max_logical_packages);
static unsigned int logical_packages __read_mostly;
-static bool logical_packages_frozen __read_mostly;

/* Maximum number of SMT threads on any online core */
int __max_smt_threads __read_mostly;
@@ -263,9 +262,14 @@ static void notrace start_secondary(void
cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
}

-int topology_update_package_map(unsigned int apicid, unsigned int cpu)
+/**
+ * topology_update_package_map - Update the physical to logical package map
+ * @pkg: The physical package id as retrieved via CPUID
+ * @cpu: The cpu for which this is updated
+ */
+int topology_update_package_map(unsigned int pkg, unsigned int cpu)
{
- unsigned int new, pkg = apicid >> boot_cpu_data.x86_coreid_bits;
+ unsigned int new;

/* Called from early boot ? */
if (!physical_package_map)
@@ -278,16 +282,17 @@ int topology_update_package_map(unsigned
if (test_and_set_bit(pkg, physical_package_map))
goto found;

- if (logical_packages_frozen) {
- physical_to_logical_pkg[pkg] = -1;
- pr_warn("APIC(%x) Package %u exceeds logical package max\n",
- apicid, pkg);
+ if (logical_packages >= __max_logical_packages) {
+ pr_warn("Package %u of CPU %u exceeds BIOS package data %u.\n",
+ logical_packages, cpu, __max_logical_packages);
return -ENOSPC;
}

new = logical_packages++;
- pr_info("APIC(%x) Converting physical %u to logical package %u\n",
- apicid, pkg, new);
+ if (new != pkg) {
+ pr_info("CPU %u Converting physical %u to logical package %u\n",
+ cpu, pkg, new);
+ }
physical_to_logical_pkg[pkg] = new;

found:
@@ -308,9 +313,9 @@ int topology_phys_to_logical_pkg(unsigne
}
EXPORT_SYMBOL(topology_phys_to_logical_pkg);

-static void __init smp_init_package_map(void)
+static void __init smp_init_package_map(struct cpuinfo_x86 *c, unsigned int cpu)
{
- unsigned int ncpus, cpu;
+ unsigned int ncpus;
size_t size;

/*
@@ -355,27 +360,9 @@ static void __init smp_init_package_map(
size = BITS_TO_LONGS(max_physical_pkg_id) * sizeof(unsigned long);
physical_package_map = kzalloc(size, GFP_KERNEL);

- 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);
- }
-
- if (logical_packages > __max_logical_packages) {
- pr_warn("Detected more packages (%u), then computed by BIOS data (%u).\n",
- logical_packages, __max_logical_packages);
- logical_packages_frozen = true;
- __max_logical_packages = logical_packages;
- }
-
pr_info("Max logical packages: %u\n", __max_logical_packages);
+
+ topology_update_package_map(c->phys_proc_id, cpu);
}

void __init smp_store_boot_cpu_info(void)
@@ -385,7 +372,7 @@ void __init smp_store_boot_cpu_info(void

*c = boot_cpu_data;
c->cpu_index = id;
- smp_init_package_map();
+ smp_init_package_map(c, id);
}

/*
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -87,12 +87,6 @@ static void cpu_bringup(void)
cpu_data(cpu).x86_max_cores = 1;
set_cpu_sibling_map(cpu);

- /*
- * identify_cpu() may have set logical_pkg_id to -1 due
- * to incorrect phys_proc_id. Let's re-comupte it.
- */
- topology_update_package_map(apic->cpu_present_to_apicid(cpu), cpu);
-
xen_setup_cpu_clockevents();

notify_cpu_starting(cpu);

2016-12-12 19:06:45

by Boris Ostrovsky

[permalink] [raw]
Subject: Re: [PATCH v2] x86/smpboot: Make logical package management more robust

On 12/12/2016 05:04 AM, Thomas Gleixner wrote:
> The logical package management has several issues:
>
> - The APIC ids provided by ACPI are not required to be the same as the
> initial APIC id which can be retrieved by CPUID. The APIC ids provided
> by ACPI are those which are written by the BIOS into the APIC. The
> initial id is set by hardware and can not be changed. The hardware
> provided ids contain the real hardware package information.
>
> Especially AMD sets the effective APIC id different from the hardware id
> as they need to reserve space for the IOAPIC ids starting at id 0.
>
> As a consequence those machines trigger the currently active firmware
> bug printouts in dmesg, These are obviously wrong.
>
> - Virtual machines have their own interesting of enumerating APICs and
> packages which are not reliably covered by the current implementation.
>
> The sizing of the mapping array has been tweaked to be generously large to
> handle systems which provide a wrong core count when HT is disabled so the
> whole magic which checks for space in the physical hotplug case is not
> needed anymore.
>
> Simplify the whole machinery and do the mapping when the CPU starts and the
> CPUID derived physical package information is available. This solves the
> observed problems on AMD machines and works for the virtualization issues
> as well.
>
> Remove the extra call from XEN cpu bringup code as it is not longer
> required.
>
> Fixes: d49597fd3bc7 ("x86/cpu: Deal with broken firmware (VMWare/XEN)")
> Reported-and-tested-by: Borislav Petkov <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Cc: [email protected]

For Xen:

Tested-by: Boris Ostrovsky <[email protected]>

(Note that we still have
[Firmware Bug]: CPU13: APIC id mismatch. Firmware: 0 APIC: d
but that will be fixed in Xen code.)

-boris


Subject: [tip:x86/urgent] x86/smpboot: Make logical package management more robust

Commit-ID: 9d85eb9119f4eeeb48e87adfcd71f752655700e9
Gitweb: http://git.kernel.org/tip/9d85eb9119f4eeeb48e87adfcd71f752655700e9
Author: Thomas Gleixner <[email protected]>
AuthorDate: Mon, 12 Dec 2016 11:04:53 +0100
Committer: Thomas Gleixner <[email protected]>
CommitDate: Tue, 13 Dec 2016 10:22:39 +0100

x86/smpboot: Make logical package management more robust

The logical package management has several issues:

- The APIC ids provided by ACPI are not required to be the same as the
initial APIC id which can be retrieved by CPUID. The APIC ids provided
by ACPI are those which are written by the BIOS into the APIC. The
initial id is set by hardware and can not be changed. The hardware
provided ids contain the real hardware package information.

Especially AMD sets the effective APIC id different from the hardware id
as they need to reserve space for the IOAPIC ids starting at id 0.

As a consequence those machines trigger the currently active firmware
bug printouts in dmesg, These are obviously wrong.

- Virtual machines have their own interesting of enumerating APICs and
packages which are not reliably covered by the current implementation.

The sizing of the mapping array has been tweaked to be generously large to
handle systems which provide a wrong core count when HT is disabled so the
whole magic which checks for space in the physical hotplug case is not
needed anymore.

Simplify the whole machinery and do the mapping when the CPU starts and the
CPUID derived physical package information is available. This solves the
observed problems on AMD machines and works for the virtualization issues
as well.

Remove the extra call from XEN cpu bringup code as it is not longer
required.

Fixes: d49597fd3bc7 ("x86/cpu: Deal with broken firmware (VMWare/XEN)")
Reported-and-tested-by: Borislav Petkov <[email protected]>
Tested-by: Boris Ostrovsky <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: M. Vefa Bicakci <[email protected]>
Cc: xen-devel <[email protected]>
Cc: Charles (Chas) Williams <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Alok Kataria <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/alpine.DEB.2.20.1612121102260.3429@nanos
Signed-off-by: Thomas Gleixner <[email protected]>

---
arch/x86/kernel/apic/apic.c | 15 -------------
arch/x86/kernel/cpu/common.c | 24 +++++++--------------
arch/x86/kernel/smpboot.c | 51 +++++++++++++++++---------------------------
arch/x86/xen/smp.c | 6 ------
4 files changed, 27 insertions(+), 69 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index bb47e5e..5b7e43e 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2160,21 +2160,6 @@ int __generic_processor_info(int apicid, int version, bool enabled)
}

/*
- * This can happen on physical hotplug. The sanity check at boot time
- * is done from native_smp_prepare_cpus() after num_possible_cpus() is
- * established.
- */
- if (topology_update_package_map(apicid, cpu) < 0) {
- int thiscpu = max + disabled_cpus;
-
- pr_warning("APIC: Package limit reached. Processor %d/0x%x ignored.\n",
- thiscpu, apicid);
-
- disabled_cpus++;
- return -ENOSPC;
- }
-
- /*
* Validate version
*/
if (version == 0x0) {
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 729f92b..1f6b50a 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -979,29 +979,21 @@ static void x86_init_cache_qos(struct cpuinfo_x86 *c)
}

/*
- * The physical to logical package id mapping is initialized from the
- * acpi/mptables information. Make sure that CPUID actually agrees with
- * that.
+ * Validate that ACPI/mptables have the same information about the
+ * effective APIC id and update the package map.
*/
-static void sanitize_package_id(struct cpuinfo_x86 *c)
+static void validate_apic_and_package_id(struct cpuinfo_x86 *c)
{
#ifdef CONFIG_SMP
- unsigned int pkg, apicid, cpu = smp_processor_id();
+ unsigned int 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",
+ if (apicid != c->apicid) {
+ pr_err(FW_BUG "CPU%u: APIC id mismatch. Firmware: %x APIC: %x\n",
cpu, apicid, c->initial_apicid);
- c->initial_apicid = 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);
+ BUG_ON(topology_update_package_map(c->phys_proc_id, cpu));
#else
c->logical_proc_id = 0;
#endif
@@ -1132,7 +1124,6 @@ static void identify_cpu(struct cpuinfo_x86 *c)
#ifdef CONFIG_NUMA
numa_add_cpu(smp_processor_id());
#endif
- sanitize_package_id(c);
}

/*
@@ -1187,6 +1178,7 @@ void identify_secondary_cpu(struct cpuinfo_x86 *c)
enable_sep_cpu();
#endif
mtrr_ap_init();
+ validate_apic_and_package_id(c);
}

static __init int setup_noclflush(char *arg)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 0c37d4f..e09aa58 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -103,7 +103,6 @@ static unsigned int max_physical_pkg_id __read_mostly;
unsigned int __max_logical_packages __read_mostly;
EXPORT_SYMBOL(__max_logical_packages);
static unsigned int logical_packages __read_mostly;
-static bool logical_packages_frozen __read_mostly;

/* Maximum number of SMT threads on any online core */
int __max_smt_threads __read_mostly;
@@ -273,9 +272,14 @@ static void notrace start_secondary(void *unused)
cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
}

-int topology_update_package_map(unsigned int apicid, unsigned int cpu)
+/**
+ * topology_update_package_map - Update the physical to logical package map
+ * @pkg: The physical package id as retrieved via CPUID
+ * @cpu: The cpu for which this is updated
+ */
+int topology_update_package_map(unsigned int pkg, unsigned int cpu)
{
- unsigned int new, pkg = apicid >> boot_cpu_data.x86_coreid_bits;
+ unsigned int new;

/* Called from early boot ? */
if (!physical_package_map)
@@ -288,16 +292,17 @@ int topology_update_package_map(unsigned int apicid, unsigned int cpu)
if (test_and_set_bit(pkg, physical_package_map))
goto found;

- if (logical_packages_frozen) {
- physical_to_logical_pkg[pkg] = -1;
- pr_warn("APIC(%x) Package %u exceeds logical package max\n",
- apicid, pkg);
+ if (logical_packages >= __max_logical_packages) {
+ pr_warn("Package %u of CPU %u exceeds BIOS package data %u.\n",
+ logical_packages, cpu, __max_logical_packages);
return -ENOSPC;
}

new = logical_packages++;
- pr_info("APIC(%x) Converting physical %u to logical package %u\n",
- apicid, pkg, new);
+ if (new != pkg) {
+ pr_info("CPU %u Converting physical %u to logical package %u\n",
+ cpu, pkg, new);
+ }
physical_to_logical_pkg[pkg] = new;

found:
@@ -318,9 +323,9 @@ int topology_phys_to_logical_pkg(unsigned int phys_pkg)
}
EXPORT_SYMBOL(topology_phys_to_logical_pkg);

-static void __init smp_init_package_map(void)
+static void __init smp_init_package_map(struct cpuinfo_x86 *c, unsigned int cpu)
{
- unsigned int ncpus, cpu;
+ unsigned int ncpus;
size_t size;

/*
@@ -365,27 +370,9 @@ static void __init smp_init_package_map(void)
size = BITS_TO_LONGS(max_physical_pkg_id) * sizeof(unsigned long);
physical_package_map = kzalloc(size, GFP_KERNEL);

- 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);
- }
-
- if (logical_packages > __max_logical_packages) {
- pr_warn("Detected more packages (%u), then computed by BIOS data (%u).\n",
- logical_packages, __max_logical_packages);
- logical_packages_frozen = true;
- __max_logical_packages = logical_packages;
- }
-
pr_info("Max logical packages: %u\n", __max_logical_packages);
+
+ topology_update_package_map(c->phys_proc_id, cpu);
}

void __init smp_store_boot_cpu_info(void)
@@ -395,7 +382,7 @@ void __init smp_store_boot_cpu_info(void)

*c = boot_cpu_data;
c->cpu_index = id;
- smp_init_package_map();
+ smp_init_package_map(c, id);
}

/*
diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index 9fa27ce..311acad 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -87,12 +87,6 @@ static void cpu_bringup(void)
cpu_data(cpu).x86_max_cores = 1;
set_cpu_sibling_map(cpu);

- /*
- * identify_cpu() may have set logical_pkg_id to -1 due
- * to incorrect phys_proc_id. Let's re-comupte it.
- */
- topology_update_package_map(apic->cpu_present_to_apicid(cpu), cpu);
-
xen_setup_cpu_clockevents();

notify_cpu_starting(cpu);

2017-06-06 13:48:29

by Max Vozeler

[permalink] [raw]
Subject: Re: [PATCH v2] x86/smpboot: Make logical package management more robust

Hi Thomas,

there is a problem booting recent kernels on some Xen domUs hosted by
provider JiffyBox.

The kernel seems to crash just after logging
[ 0.038700] SMP alternatives: switching to SMP code

We started seeing this with 4.9.2 and bisecting the 4.9 stable kernels
determined that this commit introduced the problem. Reverting it from 4.9.2
makes the kernel boot again.

Older kernels (starting from 3.16 up to and including 4.9.1) were running
fine in this setup. But recent mainline (tested 4.12-rc3) and 4.9.x both
fail to boot there.

Unfortunately we have no detailed information about the hypervisor or
setup and the provider is not very forthcoming with details. I'm attaching
dmesg of a successful boot (4.9.2 with this commit reverted).

It shows a fairly old XEN version:

[ 0.000000] Xen version: 3.1.2-416.el5 (preserve-AD)

Any ideas?

Thanks and kind regards,

Max


Attachments:
(No filename) (923.00 B)
bisect.log (1.96 kB)
dmesg-ok.txt (17.33 kB)
Download all attachments

2017-06-07 01:51:09

by Boris Ostrovsky

[permalink] [raw]
Subject: Re: [PATCH v2] x86/smpboot: Make logical package management more robust



On 06/06/2017 09:39 AM, Max Vozeler wrote:
> Hi Thomas,
>
> there is a problem booting recent kernels on some Xen domUs hosted by
> provider JiffyBox.
>
> The kernel seems to crash just after logging
> [ 0.038700] SMP alternatives: switching to SMP code


Do you have the crash splat? Stack trace and such.

In fact, full boot log might be useful.

>
> We started seeing this with 4.9.2 and bisecting the 4.9 stable kernels
> determined that this commit introduced the problem. Reverting it from 4.9.2
> makes the kernel boot again.
>
> Older kernels (starting from 3.16 up to and including 4.9.1) were running
> fine in this setup. But recent mainline (tested 4.12-rc3) and 4.9.x both
> fail to boot there.
>
> Unfortunately we have no detailed information about the hypervisor or
> setup and the provider is not very forthcoming with details. I'm attaching
> dmesg of a successful boot (4.9.2 with this commit reverted).
>
> It shows a fairly old XEN version:
>
> [ 0.000000] Xen version: 3.1.2-416.el5 (preserve-AD)


This is a 10 year old hypervisor so it's not especially surprising that
newer kernels don't work. (If anything, I am surprised that you actually
booted 4.9 at all).

There have been a bunch of problems in this area (topology) on PV guests.


-boris

2017-06-07 12:21:16

by Max Vozeler

[permalink] [raw]
Subject: Re: [PATCH v2] x86/smpboot: Make logical package management more robust

On Tue, Jun 06, 2017 at 09:48:37PM -0400, Boris Ostrovsky wrote:
> On 06/06/2017 09:39 AM, Max Vozeler wrote:
> >there is a problem booting recent kernels on some Xen domUs hosted by
> >provider JiffyBox.
> >
> >The kernel seems to crash just after logging
> >[ 0.038700] SMP alternatives: switching to SMP code
>
> Do you have the crash splat? Stack trace and such.
>
> In fact, full boot log might be useful.

Unfortunately, we don't have much more information.

Just after "switching to SMP code" the console connection is lost and we
get a notification that the VM has crashed. I'm attaching the boot log up
to that point.. just in case.

I have asked the hosting provider if they can provide XEN hypervisor logs.

> >We started seeing this with 4.9.2 and bisecting the 4.9 stable kernels
> >determined that this commit introduced the problem. Reverting it from 4.9.2
> >makes the kernel boot again.
> >
> >Older kernels (starting from 3.16 up to and including 4.9.1) were running
> >fine in this setup. But recent mainline (tested 4.12-rc3) and 4.9.x both
> >fail to boot there.
> >
> >Unfortunately we have no detailed information about the hypervisor or
> >setup and the provider is not very forthcoming with details. I'm attaching
> >dmesg of a successful boot (4.9.2 with this commit reverted).
> >
> >It shows a fairly old XEN version:
> >
> >[ 0.000000] Xen version: 3.1.2-416.el5 (preserve-AD)
>
> This is a 10 year old hypervisor so it's not especially surprising that
> newer kernels don't work. (If anything, I am surprised that you actually
> booted 4.9 at all).
>
> There have been a bunch of problems in this area (topology) on PV guests.

Thanks and kind regards,

Max


Attachments:
(No filename) (1.66 kB)
dmesg-crash.txt (5.85 kB)
Download all attachments