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

[Luis R. Rodriguez]

On Fri, Mar 18, 2016 at 2:35 PM, Toshi Kani <toshi.kani@xxxxxxx> wrote:
> On Thu, 2016-03-17 at 17:06 -0700, Luis R. Rodriguez wrote:
>> On Mar 17, 2016 2:04 PM, "Toshi Kani" <toshi.kani@xxxxxxx> wrote:
>> >
>> > On Wed, 2016-03-16 at 00:29 +0100, Luis R. Rodriguez wrote:
>> > > On Tue, Mar 15, 2016 at 05:48:44PM -0600, Toshi Kani wrote:
>> > > > On Tue, 2016-03-15 at 01:15 +0100, Luis R. Rodriguez wrote:
>> > > > > On Fri, Mar 11, 2016 at 06:16:36PM -0700, Toshi Kani wrote:
>> > > > > >
>> >  :
>> > > > MTRR code does not have to be dead for the virtual machines with no
>> > > > MTRR support.  The code just needs to handle the disabled case
>> > > > properly.  I consider this is part of 1) that the kernel keeps the
>> > > > MTRR state as disabled.
>> > >
>> > > For Xen it was possible to use PAT without any of the MTRR code
>> > > running, I don't see why its needed then and why other virtual
>> > > machines that only need PAT need it.
>> >
>> > Virtual BIOS does not need MTRRs since it does not manage the platform.
>>
>> Unless if in dom0 and if some of this purposely wants to be punted
>> there to leverage existing kernel code. On the Xen thread I'm asking
>> about the implications of that and how/if Xen should be doing. We can
>> follow up on this there as its Xen specific.
>
> I do not see any issue for Xen, but sure, we can discuss about Xen in a
> separate thread.

While on point -- I'll just wanted to clarify that a while ago you had
hinted we needed to have Xen return a valid type with
mtrr_type_lookup(), I then also explained how Xen disables MTRR [0],
it was however unclear if you still believe mtrr_type_lookup() is
needed on the guest side. Jan had pointed out that the Xen Hypervisor
implements the XENPF_read_memtype hypercall. On the recent thread I
posted [1] I got into my review of the prospects of implementing
support for using this hypercall on Linux xen guests and issues and
concerns with it. Please feel free to follow up there and we can take
up the other items below here as they relate to bare metal.

[0] http://lkml.kernel.org/r/20150903235429.GZ8051@xxxxxxxxxxxxx
[1] http://lkml.kernel.org/r/CAB=NE6UTp0T=rbOcAg88iPsfBJneY7O5-3c11VfFgAPiepoTNg@xxxxxxxxxxxxxx

>> > > On x86 Linux code we now have ioremap_uc() that can't use MTRR behind
>> > > the scenes, why would something like this on the BIOS not be
>> > > possible? That ultimately uses set_pte_at(). What limitations are
>> > > there on the BIOS that prevent us from just using strong UC for PAT
>> > > on the BIOS?
>> >
>> > Because it requires to run in virtual mode with page tables.
>>
>> Ah... interesting... is UC really needed, what is the default? If the
>> default is used would there be an issue ? Can such work be deferred to
>> a later time ? It seems like a high burden to require on large piece
>> of legacy architecture to just blow a fan.
>
> The default cache attribute (i.e. ranges not covered by MTRRs) is specified
> by the MTRR default type MSR.

Do we really need UC for the fan? What is the default for PAT? Can't
the same be used so that we way by default all ranges match what is
also the default by PAT? Would that really break fan control ? If we
have a match should't we be able to not have to worry about MTRRs at
all in-kernel even on bare metal?

Another option, which I've alluded to on the Xen thread is skipping
over the MTRR space from the e820 map. Is that not possible ? This
could be last resort... but which I'm hinting more for the Xen side of
things if we *really* need get_mtrr() on the Xen guest side of
things...

>> > > > > >
>> > > MTRR has a bunch of junk that is outside of the scope of the generic
>> > > procedure which I'd hope we can skip.
>> >
>> > We can skip the part that modifies MTRR setup. I think that is the part
>> > you think is a junk.
>>
>> Sure, but the more we can avoid any of that code the better. For
>> example consider the cleanup patch to increase the chunk size, do we
>> even need the cleanup anymore ?
>
> No, I do not think we need it now.  I think this cleanup was needed to
> allocate more free slots to MTRRs.  We do not need to care about the number
> of free slots as long as the kernel does not insert any new entry to MTRRs.

Beautiful, thanks.

 Luis