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

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

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.
> > > > 
> > > > When CPU does not support MTRR, ex. qemu32 virtual CPU, MTRR
> > > > simply skips PAT init, which causes PAT left enabled without
> > > > initialization.  Also, get_mtrr_state() calls pat_init() on
> > > > BSP even if MTRR is disabled by its MSR.  This causes pat_init()
> > > > be called on BSP only.
> > > 
> > > So I don't understand what all this hoopla is all about: why can't
> > > you
> > > simply call pat_disable() in mtrr_ap_init() and be done with it?
> > > 
> > > void mtrr_ap_init(void)
> > > {
> > >         if (!mtrr_enabled()) {
> > > 		pat_disable();
> > >                 return;
> > > 	}
> > > 
> > > ?
> > 
> > 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).  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? 

Thanks,
-Toshi