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

["Luis R. Rodriguez" <mcgrof@kernel.org>]

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

I take it you meant "but not 2)" ? 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.

I can read the above description to also say:

"Hey you need to implement PAT with the same skeleton code as MTRR"

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

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

 Luis