Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1360196 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-17 22:10 +0100
    Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-03-18 01:10 +0100
      Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-18 21:50 +0100
        Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-03-29 19:20 +0200
          Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-29 23:00 +0200
            Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-03-30 00:20 +0200
              Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-30 01:30 +0200
                Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-03-30 01:50 +0200
                  Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-30 02:20 +0200
                    Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-03-30 02:40 +0200

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

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

> >
 :
> > We do not need subarch for this case since presence of the MTRR feature
> > can be tested with CPUID and MSR.
> 
> But in this case its about two different cases:
> 
> No MTRR - but you need to emulate PAT
> No MTRR - but you can just read what the hypervisor already set up
> 
> So it a given MTRR is disabled for both, what we need then is semantics
> to distinguish between qemu32 and Xen PV. Curious to see what you use,
> in your current new patch it was not clear what you did.

X86_FEATURE_PAT tells us if CPU supports PAT. So, the kernel can
distinguish the two cases above without knowing qemu32 or Xen.


> > > 
 :
> > AFAIK, MTRR is the only way to specify UC attribute in physical mode on
> > x86.  On ia64, one can simply set the UC bit to a physical address to
> > specify UC attribute.  I wish we had something similar.
> 
> Whoa, you mean on the BIOS? This is BIG deal if true. Why haven't you
> just said this a long time ago?

Yes, BIOS.  I think I've told you before when I mentioned that BIOS might
need to manage fan speed.  Virt BIOS does not need to do such thing.

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

> > > > The kernel can be ignorant of the MTRR setup as long as it does
> > > > not modify it.
> > > 
> > > Sure, we're already there. The kernel no longer modifies the
> > > MTRR setup unless of course you boot without PAT enabled. I think
> > > we need to move beyond that to ACPI if we can to let regular
> > > Linux boots never have to deal with MTRR at all. The code is
> > > complex and nasty why not put let folks put a nail on the coffin for
> > > good?
> > 
> > I think we are good as long as we do not modify it.  The complexity
> > comes with the modification.
> 
> Ew. I look at that code and cannot comprehend why we'd ever want to keep
> it always running.

The code is basically no-op on Xen.

> > > > > 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.
> > > 
> > > I'm not sure if that's really needed. Doesn't PAT just require
> > > setting the wrmsrl(MSR_IA32_CR_PAT, pat) for each AP?
> > 
> > No, it requires the same procedure as MTRR.
> 
> 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.

> > > > > 
 :
> > > >  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.
> > > 
> > > I checked through code inspection and indeed, it seems it would break
> > > Xen's PAT setup.
> > > 
> > > For the record: the issue here was code that should not run ran, that
> > > is dead code ran. I'm working towards a generic solution for this.
> > 
> > Please review the next version.
> 
> Sure..

Thanks!
-Toshi

[toc] | [next] | [standalone]

#1360262

On Mar 17, 2016 2:04 PM, "Toshi Kani" <toshi.kani@hpe.com> 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:
> > > > > 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:
> > >  :
>  :
> > > 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.

> > > We do not need subarch for this case since presence of the MTRR feature
> > > can be tested with CPUID and MSR.
> >
> > But in this case its about two different cases:
> >
> > No MTRR - but you need to emulate PAT
> > No MTRR - but you can just read what the hypervisor already set up
> >
> > So it a given MTRR is disabled for both, what we need then is semantics
> > to distinguish between qemu32 and Xen PV. Curious to see what you use,
> > in your current new patch it was not clear what you did.
>
> X86_FEATURE_PAT tells us if CPU supports PAT. So, the kernel can
> distinguish the two cases above without knowing qemu32 or Xen.

Ok indeed, neat.

> > > AFAIK, MTRR is the only way to specify UC attribute in physical mode on
> > > x86.  On ia64, one can simply set the UC bit to a physical address to
> > > specify UC attribute.  I wish we had something similar.
> >
> > Whoa, you mean on the BIOS? This is BIG deal if true. Why haven't you
> > just said this a long time ago?
>
> Yes, BIOS.  I think I've told you before when I mentioned that BIOS might
> need to manage fan speed.  Virt BIOS does not need to do such thing.

You had mentioned befor BIOS uses MTRR, not that *the BIOS can only
MTRR for UC*. These are two very different things, hence my surprise.

> > 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 kernel can be ignorant of the MTRR setup as long as it does
> > > > > not modify it.
> > > >
> > > > Sure, we're already there. The kernel no longer modifies the
> > > > MTRR setup unless of course you boot without PAT enabled. I think
> > > > we need to move beyond that to ACPI if we can to let regular
> > > > Linux boots never have to deal with MTRR at all. The code is
> > > > complex and nasty why not put let folks put a nail on the coffin for
> > > > good?
> > >
> > > I think we are good as long as we do not modify it.  The complexity
> > > comes with the modification.
> >
> > Ew. I look at that code and cannot comprehend why we'd ever want to keep
> > it always running.
>
> The code is basically no-op on Xen.

Right, but if we can strive towards similar goals with more up to date
BIOSes as an option on bare metal why not.

> > > > > > 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.
> > > >
> > > > I'm not sure if that's really needed. Doesn't PAT just require
> > > > setting the wrmsrl(MSR_IA32_CR_PAT, pat) for each AP?
> > >
> > > No, it requires the same procedure as MTRR.
> >
> > 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 ?

  Luis

[toc] | [prev] | [next] | [standalone]

#1360917

On Thu, 2016-03-17 at 17:06 -0700, Luis R. Rodriguez wrote:
> On Mar 17, 2016 2:04 PM, "Toshi Kani" <toshi.kani@hpe.com> 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.


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

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

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1366486

On Fri, Mar 18, 2016 at 2:35 PM, Toshi Kani <toshi.kani@hpe.com> 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@hpe.com> 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@wotan.suse.de
[1] http://lkml.kernel.org/r/CAB=NE6UTp0T=rbOcAg88iPsfBJneY7O5-3c11VfFgAPiepoTNg@mail.gmail.com

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

[toc] | [prev] | [next] | [standalone]

#1366706

On Tue, 2016-03-29 at 10:14 -0700, Luis R. Rodriguez wrote:
> On Fri, Mar 18, 2016 at 2:35 PM, Toshi Kani <toshi.kani@hpe.com> 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@hpe.com> wrote:
> > > > 
 :
> > 
> > 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.

What I said in the email [0] was that "When MTRRs are enabled, the kernel
needs to check through mtrr_type_lookup()".

Since Xen guests have MTRRs disabled, this statement does not apply. It
returns MTRR_TYPE_INVALID when disabled, and this is fine for the guests.

> [0] http://lkml.kernel.org/r/20150903235429.GZ8051@wotan.suse.de
> [1] http://lkml.kernel.org/r/CAB=NE6UTp0T=rbOcAg88iPsfBJneY7O5-3c11VfFgAP
> iepoTNg@mail.gmail.com
> 
> > > > > 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? 

When you say "we", are you referring Xen guests?  Xen guests do not need to
control the fan, so they do not need UC set in MTRRs.

In general, yes, MMIO registers need UC when they need to be accessed.

> What is the default for PAT? 

There is no such thing as 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?

We do not need to know about BIOS impl, such as fan control, etc.  The
point is that if BIOS sets MTRRs, then the kernel keeps their setup.  If
(virtual) BIOS does not enable MTRRs, the kernel keeps them disabled.  We
just need not to mess with the setup.

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

There is no MTRR space in the e820 map since they are MSRs.  Since Xen
guests disable MTRRs, I do not think you have any issue here...

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1366748

On Tue, Mar 29, 2016 at 2:46 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> On Tue, 2016-03-29 at 10:14 -0700, Luis R. Rodriguez wrote:
>> On Fri, Mar 18, 2016 at 2:35 PM, Toshi Kani <toshi.kani@hpe.com> 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@hpe.com> wrote:
>> > > >
>  :
>> >
>> > 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.
>
> What I said in the email [0] was that "When MTRRs are enabled, the kernel
> needs to check through mtrr_type_lookup()".
>
> Since Xen guests have MTRRs disabled, this statement does not apply. It
> returns MTRR_TYPE_INVALID when disabled, and this is fine for the guests.

Ah, I see thanks for the clarification!

>> [0] http://lkml.kernel.org/r/20150903235429.GZ8051@wotan.suse.de
>> [1] http://lkml.kernel.org/r/CAB=NE6UTp0T=rbOcAg88iPsfBJneY7O5-3c11VfFgAP
>> iepoTNg@mail.gmail.com
>>
>> > > > > 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?
>
> When you say "we", are you referring Xen guests?  Xen guests do not need to
> control the fan, so they do not need UC set in MTRRs.
>
> In general, yes, MMIO registers need UC when they need to be accessed.

Curious, what does a BIOS do for fan control when MTRRs are disabled?

Also what if a BIOS just set MSR_MTRRdefType to uncachable only ?
Wouldn't that help simplify the BIOS when systems are known as not
wanting to deal with reading MTRRs on the kernel front, even if its
just to read the setup ?

I'm trying to determine exactly why a BIOS cannot simply enable use an
alternative for what it needs for fan control and let the kernel live
without any MTRR code at run time as an option. Although the
documentation says that the same "procedure" is needed for PAT setup,
I see it possible to split the skeleton of the code and have each
peace of code live separately and compartmentalized, they'd just have
respective calls on the skeleton of the procedure.

>> What is the default for PAT?
>
> There is no such thing as 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?
>
> We do not need to know about BIOS impl, such as fan control, etc.  The
> point is that if BIOS sets MTRRs, then the kernel keeps their setup.

Right, if the kernel no longer uses it directly it seems like an
aweful lot of code to keep updating simply for a BIOS requirement, I'm
trying to see if we can have the option to live without this
requirement.

> If (virtual) BIOS does not enable MTRRs, the kernel keeps them disabled.  We
> just need not to mess with the setup.

Sure, thanks! I'm trying to see if we can have a similar option 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...
>
> There is no MTRR space in the e820 map since they are MSRs.  Since Xen
> guests disable MTRRs, I do not think you have any issue here...

Xen seems to clip the e820 map given to a guest in certain MTRR
conditions, see init_e820(), this calls
machine_specific_memory_setup() which later clips MTRR if
mtrr_top_of_ram(). This is an Intel check that trims the e820 map if
MTRRs were found to be enabled and the default MTRR is not write-back.
If returns the address of the first non write-back variable MTRR, it
uses clip_to_limit() to limit the exposed memory [0], notice how
clip_to_limit() is also used to generally limit exposed memory through
the opt_mem boot parameter as well. Its not exactly clear why that's
done, but this looks very similar to the Linux MTRR cleanup -- see
x86_get_mtrr_mem_range().

[0] http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/x86/e820.c

  Luis

[toc] | [prev] | [next] | [standalone]

#1366783

On Tue, 2016-03-29 at 15:12 -0700, Luis R. Rodriguez wrote:
> On Tue, Mar 29, 2016 at 2:46 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> > On Tue, 2016-03-29 at 10:14 -0700, Luis R. Rodriguez wrote:
> > > On Fri, Mar 18, 2016 at 2:35 PM, Toshi Kani <toshi.kani@hpe.com>
> > > wrote:
 :
> > > 
> > > Do we really need UC for the fan?
> > 
> > When you say "we", are you referring Xen guests?  Xen guests do not
> > need to control the fan, so they do not need UC set in MTRRs.
> > 
> > In general, yes, MMIO registers need UC when they need to be accessed.
> 
> Curious, what does a BIOS do for fan control when MTRRs are disabled?

You mean, when the kernel modified the MTRR setup and disabled them.  BIOS
would assume the original setup and still access the registers.  This may
lead to undefined behavior and may result in a system crash.

> Also what if a BIOS just set MSR_MTRRdefType to uncachable only ?

Many BIOSes actually set the default type to UC.  MTRRs then cover regular
memory with WB.

> Wouldn't that help simplify the BIOS when systems are known as not
> wanting to deal with reading MTRRs on the kernel front, even if its
> just to read the setup ?

Nope.

> I'm trying to determine exactly why a BIOS cannot simply enable use an
> alternative for what it needs for fan control and let the kernel live
> without any MTRR code at run time as an option. Although the
> documentation says that the same "procedure" is needed for PAT setup,
> I see it possible to split the skeleton of the code and have each
> peace of code live separately and compartmentalized, they'd just have
> respective calls on the skeleton of the procedure.

I agree that the MTRR rendezvous handler can be improved for PAT, but I do
not see a compelling reason to make such change now.  With my fix, I think
the code works reasonably for Xen.

> > > What is the default for PAT?
> > 
> > There is no such thing as 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?
> > 
> > We do not need to know about BIOS impl, such as fan control, etc.  The
> > point is that if BIOS sets MTRRs, then the kernel keeps their setup.
> 
> Right, if the kernel no longer uses it directly it seems like an
> aweful lot of code to keep updating simply for a BIOS requirement, I'm
> trying to see if we can have the option to live without this
> requirement.

Please be aware of the hibernation case. I think this procedure involves
setting MTRRs back to the original setup.

> > If (virtual) BIOS does not enable MTRRs, the kernel keeps them
> > disabled.  We just need not to mess with the setup.
> 
> Sure, thanks! I'm trying to see if we can have a similar option 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...
> > 
> > There is no MTRR space in the e820 map since they are MSRs.  Since Xen
> > guests disable MTRRs, I do not think you have any issue here...
> 
> Xen seems to clip the e820 map given to a guest in certain MTRR
> conditions, see init_e820(), this calls
> machine_specific_memory_setup() which later clips MTRR if
> mtrr_top_of_ram(). This is an Intel check that trims the e820 map if
> MTRRs were found to be enabled and the default MTRR is not write-back.
> If returns the address of the first non write-back variable MTRR, it
> uses clip_to_limit() to limit the exposed memory [0], notice how
> clip_to_limit() is also used to generally limit exposed memory through
> the opt_mem boot parameter as well. Its not exactly clear why that's
> done, but this looks very similar to the Linux MTRR cleanup -- see
> x86_get_mtrr_mem_range().
> 
> [0] http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/x86/e820.c

It looks to me that the code makes sure all E820_RAM ranges in the e820
table are covered by WB entries of MTRRs.  If not, it trims the e820 table.

I suppose it tries to react on a case when someone modified MTRRs and
resulted in mismatch with the e820 table.  I'd think you do not need this
code as long as you do not modify the MTRR setup.

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1366800

On Tue, Mar 29, 2016 at 5:16 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> On Tue, 2016-03-29 at 15:12 -0700, Luis R. Rodriguez wrote:
>> On Tue, Mar 29, 2016 at 2:46 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
>> > On Tue, 2016-03-29 at 10:14 -0700, Luis R. Rodriguez wrote:
>> > > On Fri, Mar 18, 2016 at 2:35 PM, Toshi Kani <toshi.kani@hpe.com>
>> > > wrote:
>  :
>> > >
>> > > Do we really need UC for the fan?
>> >
>> > When you say "we", are you referring Xen guests?  Xen guests do not
>> > need to control the fan, so they do not need UC set in MTRRs.
>> >
>> > In general, yes, MMIO registers need UC when they need to be accessed.
>>
>> Curious, what does a BIOS do for fan control when MTRRs are disabled?
>
> You mean, when the kernel modified the MTRR setup and disabled them.

Nope, but the below is good to know!

I meant to ask about the case where the option the lets a user go in a
muck with BIOS settings to disable MTRR e xists and the user disables
MTRR. What would happen for fan control in such situations? I'd
imagine such cases allow for a system to exist with proper fan
control, and allow the kernel to boot without having to deal with the
pesky MTRRs at all, while PAT lives on, no?

> BIOS
> would assume the original setup and still access the registers.  This may
> lead to undefined behavior and may result in a system crash.
>
>> Also what if a BIOS just set MSR_MTRRdefType to uncachable only ?
>
> Many BIOSes actually set the default type to UC.

Thanks, I asked as I saw my BIOS uses write-back by default. Good to
know there are different strategies.

> MTRRs then cover regular memory with WB.

When you say regular memory you mean everything else we see as RAM? I
was under the impression we'd only need MTRR for a special range of
memory, and its up to implementation how they are used. If you can use
MTRR to change the cache attribute for regular RAM and if this is
actually a requirement if the default MTRR is UC then one way or
another a BIOS seems to always require MTRR, either for UC setting for
fan control or WB for regular RAM, is that right?

>> Wouldn't that help simplify the BIOS when systems are known as not
>> wanting to deal with reading MTRRs on the kernel front, even if its
>> just to read the setup ?
>
> Nope.
>
>> I'm trying to determine exactly why a BIOS cannot simply enable use an
>> alternative for what it needs for fan control and let the kernel live
>> without any MTRR code at run time as an option. Although the
>> documentation says that the same "procedure" is needed for PAT setup,
>> I see it possible to split the skeleton of the code and have each
>> peace of code live separately and compartmentalized, they'd just have
>> respective calls on the skeleton of the procedure.
>
> I agree that the MTRR rendezvous handler can be improved for PAT, but I do
> not see a compelling reason to make such change now.  With my fix, I think
> the code works reasonably for Xen.

Agreed, don't think its needed now, my questions are for future optimizations.

>> > > What is the default for PAT?
>> >
>> > There is no such thing as 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?
>> >
>> > We do not need to know about BIOS impl, such as fan control, etc.  The
>> > point is that if BIOS sets MTRRs, then the kernel keeps their setup.
>>
>> Right, if the kernel no longer uses it directly it seems like an
>> aweful lot of code to keep updating simply for a BIOS requirement, I'm
>> trying to see if we can have the option to live without this
>> requirement.
>
> Please be aware of the hibernation case. I think this procedure involves
> setting MTRRs back to the original setup.

Eek, right, so best just disable them if we can.

>> > If (virtual) BIOS does not enable MTRRs, the kernel keeps them
>> > disabled.  We just need not to mess with the setup.
>>
>> Sure, thanks! I'm trying to see if we can have a similar option 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...
>> >
>> > There is no MTRR space in the e820 map since they are MSRs.  Since Xen
>> > guests disable MTRRs, I do not think you have any issue here...
>>
>> Xen seems to clip the e820 map given to a guest in certain MTRR
>> conditions, see init_e820(), this calls
>> machine_specific_memory_setup() which later clips MTRR if
>> mtrr_top_of_ram(). This is an Intel check that trims the e820 map if
>> MTRRs were found to be enabled and the default MTRR is not write-back.
>> If returns the address of the first non write-back variable MTRR, it
>> uses clip_to_limit() to limit the exposed memory [0], notice how
>> clip_to_limit() is also used to generally limit exposed memory through
>> the opt_mem boot parameter as well. Its not exactly clear why that's
>> done, but this looks very similar to the Linux MTRR cleanup -- see
>> x86_get_mtrr_mem_range().
>>
>> [0] http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/x86/e820.c
>
> It looks to me that the code makes sure all E820_RAM ranges in the e820
> table are covered by WB entries of MTRRs.  If not, it trims the e820 table.
>
> I suppose it tries to react on a case when someone modified MTRRs and
> resulted in mismatch with the e820 table.  I'd think you do not need this
> code as long as you do not modify the MTRR setup.

Great thanks for that -- another optimization possible.

  Luis

[toc] | [prev] | [next] | [standalone]

#1366815

On Tue, 2016-03-29 at 16:43 -0700, Luis R. Rodriguez wrote:
> On Tue, Mar 29, 2016 at 5:16 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> > On Tue, 2016-03-29 at 15:12 -0700, Luis R. Rodriguez wrote:
> > > On Tue, Mar 29, 2016 at 2:46 PM, Toshi Kani <toshi.kani@hpe.com>
> > > wrote:
> > > > On Tue, 2016-03-29 at 10:14 -0700, Luis R. Rodriguez wrote:
> > > > > On Fri, Mar 18, 2016 at 2:35 PM, Toshi Kani <toshi.kani@hpe.com>
> > > > > wrote:
> >  :
> > > > > 
> > > > > Do we really need UC for the fan?
> > > > 
> > > > When you say "we", are you referring Xen guests?  Xen guests do not
> > > > need to control the fan, so they do not need UC set in MTRRs.
> > > > 
> > > > In general, yes, MMIO registers need UC when they need to be
> > > > accessed.
> > > 
> > > Curious, what does a BIOS do for fan control when MTRRs are disabled?
> > 
> > You mean, when the kernel modified the MTRR setup and disabled them.
> 
> Nope, but the below is good to know!
> 
> I meant to ask about the case where the option the lets a user go in a
> muck with BIOS settings to disable MTRR e xists and the user disables
> MTRR. What would happen for fan control in such situations? I'd
> imagine such cases allow for a system to exist with proper fan
> control, and allow the kernel to boot without having to deal with the
> pesky MTRRs at all, while PAT lives on, no?

You mean user disables MTRRs from BIOS setup menu?  I am not a BIOS guy,
but I do not think it offers such option when the code depends on it...

> > BIOS would assume the original setup and still access the
> > registers.  This may lead to undefined behavior and may result in a
> > system crash.
> > 
> > > Also what if a BIOS just set MSR_MTRRdefType to uncachable only ?
> > 
> > Many BIOSes actually set the default type to UC.
> 
> Thanks, I asked as I saw my BIOS uses write-back by default. Good to
> know there are different strategies.
> 
> > MTRRs then cover regular memory with WB.
> 
> When you say regular memory you mean everything else we see as RAM? I
> was under the impression we'd only need MTRR for a special range of
> memory, and its up to implementation how they are used. If you can use
> MTRR to change the cache attribute for regular RAM and if this is
> actually a requirement if the default MTRR is UC then one way or
> another a BIOS seems to always require MTRR, either for UC setting for
> fan control or WB for regular RAM, is that right?

Right, in one way or the other, MTRRs set WB to RAM and UC to MMIO.  PAT is
overwritten by MTRRs, so RAM must be set to WB.

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1366819

On Tue, Mar 29, 2016 at 6:07 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> On Tue, 2016-03-29 at 16:43 -0700, Luis R. Rodriguez wrote:
>> I meant to ask about the case where the option the lets a user go in a
>> muck with BIOS settings to disable MTRR e xists and the user disables
>> MTRR. What would happen for fan control in such situations? I'd
>> imagine such cases allow for a system to exist with proper fan
>> control, and allow the kernel to boot without having to deal with the
>> pesky MTRRs at all, while PAT lives on, no?
>
> You mean user disables MTRRs from BIOS setup menu?

Yup!

> I am not a BIOS guy,
> but I do not think it offers such option when the code depends on it...

Darn, I'm pretty sure I've seen such option before... can't seem to
find such a toggle now.

>> When you say regular memory you mean everything else we see as RAM? I
>> was under the impression we'd only need MTRR for a special range of
>> memory, and its up to implementation how they are used. If you can use
>> MTRR to change the cache attribute for regular RAM and if this is
>> actually a requirement if the default MTRR is UC then one way or
>> another a BIOS seems to always require MTRR, either for UC setting for
>> fan control or WB for regular RAM, is that right?
>
> Right, in one way or the other, MTRRs set WB to RAM and UC to MMIO.  PAT is
> overwritten by MTRRs, so RAM must be set to WB.

I see... thanks....

  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web