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

["Luis R. Rodriguez" <mcgrof@xxxxxxxxxx>]

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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel