Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code

[Toshi Kani <toshi.kani@hpe.com>]

On Fri, 2016-03-11 at 15:34 -0800, Luis R. Rodriguez wrote:
> On Fri, Mar 11, 2016 at 3:56 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> > On Fri, 2016-03-11 at 23:17 +0100, Luis R. Rodriguez wrote:
> > > On Fri, Mar 11, 2016 at 11:57:12AM -0700, Toshi Kani wrote:
> > > > On Fri, 2016-03-11 at 10:24 +0100, Borislav Petkov wrote:
> > > > > On Thu, Mar 10, 2016 at 09:45:46PM -0700, Toshi Kani wrote:
> > > > > > MTRR manages PAT initialization as it implements a rendezvous
> > > > > > handler that initializes PAT as part of MTRR initialization.
 :
> > > > 
> > > > No, it does not fix it. The problem in this particular case, i.e.
> > > > MTRR disabled by its MSR, is that mtrr_bp_init() calls pat_init()
> > > > (as PAT enabled) and initializes PAT on BSP. After APs are
> > > > launched, we need the MTRR's rendezvous handler to initialize PAT
> > > > on APs to be consistent with BSP. However, MTRR rendezvous handler
> > > > is no-op since MTRR is disabled.
> > > 
> > > This seems like a hack on enabling PAT through MTRR code, can we have
> > > a PAT rendezvous handler on its own, or provide a generic rendezvous
> > > handler that lets you deal with whatever interfaces need setup. Then
> > > conflicts can just be negotiated early.
> > 
> > The MTRR code can be enhanced so that the rendezvous handler can handle
> > MTRR and PAT state independently.  I noted this case as (*) in the
> > table of this patch description.  This is a separate item, however.
> > 
> > MTRR calling PAT was not a hack (as I suppose we did not have VMs at
> > that time), although this can surely be improved.  As Intel SDM state
> > below, both MTRR and PAT require the same procedure, and the PAT
> > initialization sequence is defined in the MTRR section.
> > 
> > ===
> > 11.12.4 Programming the PAT
> >  :
> > The operating system is responsible for insuring that changes to a PAT
> > entry occur in a manner that maintains the consistency of the processor
> > caches and translation lookaside buffers (TLB). This is accomplished by
> > following the procedure as specified in Section 11.11.8, “MTRR
> > Considerations in MP Systems,” for changing the value of an MTRR in a
> > multiple processor system. It requires a specific sequence of
> > operations that includes flushing the processors caches and TLBs.
> > ===
> > 
> > > What I'm after is seeing if we can ultimately disable MTRR on kernel
> > > code but still have PAT enabled. I realize you've mentioned BIOS code
> > > may use some MTRR setup code but this is only true for some systems.
> > > I know for a fact Xen cannot use MTRR, it seems qemu32 does not
> > > enable
> > > it either. So why not have the ability to skip through its set up ?
> > 
> > MTRR support has two meanings:
> >  1) The kernel keeps the MTRR setup by BIOS.
> >  2) The kernel modifies the MTRR setup.
> > 
> > I am in a position that we need 1) but 2).
> 
> I take it you meant "but not 2)" ? 

Yes. :)

> There *are folks however who do
> more as I noted earlier. Perhaps now now, but in the future I'd
> encourage folks to rip MTRR out of their own BIOS, and enable a new
> ACPI legacy flag to say "MTRR required". That'd eventually can help
> bury MTRR for good while remaining backward compatible.

Well, BIOS using MTRR is better than BIOS setting page tables in the SMI
handler.  The kernel can be ignorant of the MTRR setup as long as it does
not modify it.

> I can read the above description to also say:
> 
> "Hey you need to implement PAT with the same skeleton code as MTRR"

No, I did not say that.  MTRR's rendezvous handler can be generalized to
work with both MTRR and PAT.  We do not need two separate handlers.  In
fact, it needs to be a single handler so that both can be initialized
together.

> If we do that, we can pave the way to deprecate MTRR as legacy for
> good first on Linux.

I do not think such change will deprecate MTRR.  It just means that Linux
can enable PAT on virtual CPUs with PAT & !MTRR capability.

> > In fact, the kernel disabling MTRRs is the same as 2).
> > 
> > > I'll also note Xen managed to enable PAT only without enabling MTRR,
> > > this was done through pat_init_cache_modes() -- not sure if this can
> > > be leveraged for qemu32...
> > 
> > I am interested to know how Xen managed this.  Is this done by the Xen
> > hypervisor initializes guest's PAT on behalf of the guest kernel?
> 
> Yup. And the cache read thingy was reading back its own setup, which
> was different than what Linux used by default IIRC. Juergen can
> elaborate more.

Yeah, I'd like to make sure that my changes won't break it.

Thanks,
-Toshi