Date: Wed, 9 Nov 2016 19:37:40 +0100 (CET)
From: Thomas Gleixner <tglx@linutronix.de>
To: "Charles (Chas) Williams" <ciwillia@brocade.com>
cc: Peter Zijlstra <peterz@infradead.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>, x86@kernel.org,
        LKML <linux-kernel@vger.kernel.org>,
        "M. Vefa Bicakci" <m.v.b@runbox.com>, Borislav Petkov <bp@alien8.de>
Subject: Re: [PATCH] x86/cpuid: Deal with broken firmware once more
In-Reply-To: <9badae12-f07e-b2e3-c1ce-c2017cd5e510@brocade.com>
Message-ID: <alpine.DEB.2.20.1611091936480.3501@nanos>
References: <20161102122557.qs4rl6mb7n7l7j7p@linutronix.de> <24e69019-60d0-29e7-e31f-c6f00f9ed98a@brocade.com> <alpine.DEB.2.20.1611071717000.3709@nanos> <58e229e2-91f4-a97f-1b9f-089f48ef994a@brocade.com> <alpine.DEB.2.20.1611072117040.3501@nanos>
 <e3a6f9cb-ee46-6e5c-7b2e-9a6ed2fc949e@brocade.com> <alpine.DEB.2.20.1611081525160.3501@nanos> <86609338-2b45-ed7e-fb07-99421e43a2f1@brocade.com> <alpine.DEB.2.20.1611081721330.3501@nanos> <alpine.DEB.2.20.1611091613540.3501@nanos>
 <20161109160324.GR3142@twins.programming.kicks-ass.net> <9badae12-f07e-b2e3-c1ce-c2017cd5e510@brocade.com>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1793
Lines: 49

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.
>