Re: Fwd: [PATCH] x86: Use larger chunks in mtrr_cleanup

Re: Fwd: [PATCH] x86: Use larger chunks in mtrr_cleanup

[Luis R. Rodriguez]

On Thu, Sep 03, 2015 at 06:48:46PM -0600, Toshi Kani wrote:
> On Fri, 2015-09-04 at 01:54 +0200, Luis R. Rodriguez wrote:
> > On Thu, Sep 03, 2015 at 05:21:14PM -0600, Toshi Kani wrote:
> > > On Fri, 2015-09-04 at 00:45 +0200, Luis R. Rodriguez wrote:
> > > > On Thu, Sep 03, 2015 at 04:25:31PM -0600, Toshi Kani wrote:
>  :
> > > > > On Xen,
> > > > 
> > > > When Xen is used a platform firmware may still set up MTRR, even if the
> > > > hypervisor doesn't set up MTRR right ? So same issue and question here.
> > > 
> > > Right, I meant to say Xen guests.
> > 
> > Ah but its import complicated than that.
> > 
> > > In case of the Xen hypervisor,
> > > mtrr_type_lookup() returns a valid type as it runs on a platform.
> > 
> > I am not sure if this happens today, I know MTRR is simply disabled by
> > the Xen Hypervisor on the CPU explicitly, it disable it so guests reading
> > the MTRR capabilities sees it as disabled when queried.
> 
> Oh, I would not let the hypervisor to disable MTRRs...

Commit 586ab6a055376ec3f3e1e8 ("x86/pvh: disable MTRR feature on cpuid for Dom0")
by Roger Pau Monné disables MTRR for PVH dom0, so that cpuid returns that
MTRR is disabled to guests. Then later on Linux as of commit 47591df50512
("xen: Support Xen pv-domains using PAT") added by Juergen as of v3.19
Linux guests can end up booting without MTRR but with PAT now enabled.

> > Then since the Xen Linux guests cannot speak MTRR through the hypervisor
> > (for instance Xen guests cannot ask Xen hypervisor to mtrr_type_lookup() for
> > it) if PCI passthrough is used it could mean a guest might set up / use
> > incorrect info as well.
> > 
> > If I undestand this correctly then I think we're in a pickle with Xen unless
> > we add hypervisor support and hypercall support for mtrr_type_lookup().
> 
> I was under assumption that MTRRs are emulated and disabled on guests.

Some "special" flavor Linux guests (with non-upstream code) have guest
MTRR hypercall support, for vanilla Xen and Linux they just never get MTRR
support. After Juergen's Linux changes though Xen guests can now get
shiny PAT support. Since MTRR hypercall support is not upstream and MTRR is
ancient I decided instead of adding MTRR hypercall support upstream to go with
converting all drivers to PAT interfaces, with the assumption there would be no
issues.

> Isn't guest physical address virtualized?

It is, there is a xen iotlb and stuff but that should ensure dom0 gets
to get proper access to devices, and if you use PCI passthrough you want
the best experience as well.

> I know other proprietary VMMs on IA64,
> but know nothing about Xen...  So, please disregard my comments to Xen. :-)

No worries, no one knows all the answers, we work together to remove
cob webs off of these odd corners no one cares about :)

  Luis

Re: Fwd: [PATCH] x86: Use larger chunks in mtrr_cleanup

[Toshi Kani]

On Fri, 2015-09-04 at 03:40 +0200, Luis R. Rodriguez wrote:
> On Thu, Sep 03, 2015 at 06:48:46PM -0600, Toshi Kani wrote:
> > On Fri, 2015-09-04 at 01:54 +0200, Luis R. Rodriguez wrote:
> > > On Thu, Sep 03, 2015 at 05:21:14PM -0600, Toshi Kani wrote:
> > > > On Fri, 2015-09-04 at 00:45 +0200, Luis R. Rodriguez wrote:
> > > > > On Thu, Sep 03, 2015 at 04:25:31PM -0600, Toshi Kani wrote:
> >  :
> > > > > > On Xen,
> > > > > 
> > > > > When Xen is used a platform firmware may still set up MTRR, even if 
> > > > > the hypervisor doesn't set up MTRR right ? So same issue and 
> > > > > question here.
> > > > 
> > > > Right, I meant to say Xen guests.
> > > 
> > > Ah but its import complicated than that.
> > > 
> > > > In case of the Xen hypervisor,
> > > > mtrr_type_lookup() returns a valid type as it runs on a platform.
> > > 
> > > I am not sure if this happens today, I know MTRR is simply disabled by
> > > the Xen Hypervisor on the CPU explicitly, it disable it so guests 
> > > reading the MTRR capabilities sees it as disabled when queried.
> > 
> > Oh, I would not let the hypervisor to disable MTRRs...
> 
> Commit 586ab6a055376ec3f3e1e8 ("x86/pvh: disable MTRR feature on cpuid for 
> Dom0") by Roger Pau Monné disables MTRR for PVH dom0, so that cpuid returns 
> that MTRR is disabled to guests. 

Oh, I see.  It just clears the capability bit so that the kernel thinks MTRRs
are disabled.  That makes sense.

> Then later on Linux as of commit 47591df50512 ("xen: Support Xen pv-domains
> using PAT") added by Juergen as of v3.19 Linux guests can end up booting
> without MTRR but with PAT now enabled.

Nice!

> > > Then since the Xen Linux guests cannot speak MTRR through the hypervisor
> > > (for instance Xen guests cannot ask Xen hypervisor to mtrr_type_lookup() 
> > > for it) if PCI passthrough is used it could mean a guest might set up / 
> > > use incorrect info as well.
> > > 
> > > If I undestand this correctly then I think we're in a pickle with Xen 
> > > unless we add hypervisor support and hypercall support for
> > > mtrr_type_lookup().
> > 
> > I was under assumption that MTRRs are emulated and disabled on guests.
> 
> Some "special" flavor Linux guests (with non-upstream code) have guest
> MTRR hypercall support, for vanilla Xen and Linux they just never get MTRR
> support. After Juergen's Linux changes though Xen guests can now get
> shiny PAT support. Since MTRR hypercall support is not upstream and MTRR is
> ancient I decided instead of adding MTRR hypercall support upstream to go 
> with converting all drivers to PAT interfaces, with the assumption there 
> would be no issues.
> 
> > Isn't guest physical address virtualized?
> 
> It is, there is a xen iotlb and stuff but that should ensure dom0 gets
> to get proper access to devices, and if you use PCI passthrough you want
> the best experience as well.
> 
> > I know other proprietary VMMs on IA64, but know nothing about Xen...  So, 
> > please disregard my comments to Xen. :-)
> 
> No worries, no one knows all the answers, we work together to remove
> cob webs off of these odd corners no one cares about :)

Thanks for all the info!  That helps.
-Toshi

Re: Fwd: [PATCH] x86: Use larger chunks in mtrr_cleanup

[Luis R. Rodriguez]

On Wed, Mar 16, 2016 at 1:20 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Thu, Nov 05, 2015 at 11:43:59AM -0800, Luis R. Rodriguez wrote:
>> On Thu, Nov 5, 2015 at 11:14 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> > On Mon, Sep 14, 2015 at 7:46 AM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>> >>
>> >> Booting with 'disable_mtrr_cleanup' works, but the system I am working with
>> >> isn't actually failing--it just gets ugly error messages.  And the BIOS on the
>> >> system I am working with had set up the MTRRs correctly.
>> >
>> > Please post boot log and /proc/mtrr for:
>> > 1. without your patch
>> > 2. without your patch and with disable_mtrr_cleanup in boot command line.
>> > 3. with your patch.
>>
>> Stuart,
>>
>> to provide some context -- I reached out to Yinghai as he wrote the
>> original mtrr cleanup code. The commit logs seem to read that a crash
>> was possible on systems with > 4 GiB RAM with some types of BIOSes...
>> The cleanup code seems to trigger when variable MTRRs do not exist
>> that are UC, or when all varible MTRRs that exist are just UC + WB
>> (Yinghai correct me if I'm wrong). The commit log in question
>> (95ffa2438d0e9 "x86: mtrr cleanup for converting continuous to
>> discrete layout, v8") was not very clear about the cause of the crash
>> -- but suppose the issue here was the BIOS on some systems might want
>> to create some UC variable MTRRs early on and there was no UC MTRRs
>> available, and I can only guess the cleanup exists as hack for those
>> BIOSes. Even if that was the case -- its still not clear *why* the
>> crash would happen but I suppose a driver mishap can happen without UC
>> guarantees for some devices the BIOS may want to enable UC MTRR on.
>>
>> To be able to determine what we do upstream we need to understand the
>> above first. We also need to understand if the cleanup might also be
>> implicated by userspace drivers using /proc/mtrr, or if a proprietary
>> driver exists that does use mtrr_add() directly even though PAT has
>> been available for ages and all drivers are now properly converted.
>>
>> With clear answers to the above we'll be able to determine what the
>> right course of action should be for this patch. For instance I'm
>> inclined to strive to disable the complex cleanup code if we don't
>> need it anymore, but if we do need it your patch makes sense. If the
>> patch makes sense then though are we going to have to keep updating
>> the segment size *every time* as systems grow? That seems rather
>> silly. And if PAT is prevalent why are vendors adding MTRRs still? The
>> cleanup seems complex and a major hack for a fix for some BIOSes, I'd
>> much rather identify the exact issue and only have a fix to address
>> that case.
>
> I never heard back... so let's take this up on the other thread I just
> raised.

Although we never got an answer, it seems now its clear, at least
Toshi has provided a confirmation that the cleanup is no longer needed
given that "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" [0]. So instead of
enhancing the cleanup code for larger systems we should now just
remove this cleanup code completely now.

[0] http://lkml.kernel.org/r/1458336958.6393.544.camel@hpe.com

 Luis