Date: Thu, 1 Jun 2017 14:29:11 -0300
From: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
To: Peter Zijlstra <peterz@infradead.org>
Cc: tglx@linutronix.de, x86@kernel.org, linux-kernel@vger.kernel.org,
        kevin.b.stanton@intel.com, bp@alien8.de
Subject: Re: [PATCH 2/3] x86/apic: Add TSC_DEADLINE quirk due to errata
Message-ID: <20170601172911.GB4766@khazad-dum.debian.net>
References: <20170531155201.218077283@infradead.org>
 <20170531155306.050849877@infradead.org>
 <20170531215855.GA5382@khazad-dum.debian.net>
 <20170601093549.iz5kzjju73har3qk@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170601093549.iz5kzjju73har3qk@hirez.programming.kicks-ass.net>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3858
Lines: 83

On Thu, 01 Jun 2017, Peter Zijlstra wrote:
> On Wed, May 31, 2017 at 06:58:55PM -0300, Henrique de Moraes Holschuh wrote:
> > On Wed, 31 May 2017, Peter Zijlstra wrote:
> > > +	DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_SKYLAKE_X,	0x02000014),
> > ...
> > > +	DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_SKYLAKE_MOBILE,	0xb2),
> > > +	DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_SKYLAKE_DESKTOP,	0xb2),
> > ...
> > > +	DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_KABYLAKE_MOBILE,	0x52),
> > > +	DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_KABYLAKE_DESKTOP,	0x52),
> > 
> > Maybe these revisions need to be decreased by one, so that the SGX
> > behavior of messing with the microcode revision is taken into account?
> 
> If anything, we should fix whatever sets our boot_cpu_data.microcode
> number, not muck with this table. However..

That would work, yes.  But that would breed confusion, it is visible to
userspace, and it and might create unforeseen issues in the future.  I
feel it is best to not go that way.

> > When the processor microcode is auto-updated from the (signed) FIT, and
> > SGX's PRMRR feature is supported, the processor will report its
> > microcode revision as one less.  Therefore, a microcode update with
> > revision 0xb2 auto-loaded from FIT would be reported by RDMSR(0x8b) as
> > revision 0xb1.
> > 
> > I know about this SGX-related behavior from a coreboot commit from ~two
> > years ago (link below).  As far as I can tell, the Intel 64/IA32 SDM is
> > *still* missing any mention about this behavior in vol 3A section 9.11
> > (where it describes microcode updates).
> > 
> > https://review.coreboot.org/cgit/coreboot.git/commit/?id=5042aad4ded1651638ae9b60e34114b65e4f211e
> 
> That commit also states that is avoids a superfluous microcode load. And

In the case of your patch, it would avoid issuing an incorrect warning
to the user and needlessly triggering the errata workaround if the
system UEFI has exactly the revision of the microcode that first fixed
the issue, and it was loaded from FIT.

OTOH, it will add a further (if rather small) nail to the coffin of any
attempts to depend on the fact that microcode was loaded from FIT -- and
anything that does so would get in the way of users and distros fixing
systems with an operating system microcode update.

On those grounds, I retract my suggestion to handle the microcode
revision oddity related to SGX.

Please keep the patch as is.

> we've verified on affected systems (both Thomas and I have a SKL system
> with the PRMRR bit set in MTRRCAP) that when we manually load the
> microcode image the reported revision number matches the one from the
> image.

Yes, it works.  That is the reason why I never proposed a fix for our
microcode loader in these two years, actually.  It works just fine right
now, and if Intel really wanted a different behavior, they should have
requested such a change or sent in a patch in the first place.  Instead,
the changed behavior wasn't even documented in the Intel SDM, since it
is backwards-compatible.

In fact, as far as I am concerned, loading the same microcode update
over itself just to destroy the "loaded from FIT" marker IS the lesser
evil, at least as things currently stand.

> The microcode people (Cc'ed) might want to further look into this is
> they care about avoiding the superfluous reload, but for the purpose of
> this patch all is well.

I think we want to keep our current behavior in the microcode loader as
well.  It is not there by mistake: it is exactly what the Intel SDM
recommends.

I believe it was no coincidence that this microcode-from-FIT marker was
designed by Intel in a way that non-modified operating systems (and
advanced boot loaders capable of microcode updates) in the field would
by default always attempt to override the FIT microcode and reset that
flag.  Just like we're doing.

-- 
  Henrique Holschuh