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

[Toshi Kani]

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