Re: x86/PV: (lack of) MTRR exposure

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Apr 29, 2022 at 12:00:21PM +0200, Jan Beulich wrote:
> On 29.04.2022 10:00, Roger Pau Monné wrote:
> > On Thu, Apr 28, 2022 at 05:53:17PM +0200, Jan Beulich wrote:
> >> Hello,
> >>
> >> in the course of analyzing the i915 driver causing boot to fail in
> >> Linux 5.18 I found that Linux, for all the years, has been running
> >> in PV mode as if PAT was (mostly) disabled. This is a result of
> >> them tying PAT initialization to MTRR initialization, while we
> >> offer PAT but not MTRR in CPUID output. This was different before
> >> our moving to CPU featuresets, and as such one could view this
> >> behavior as a regression from that change.
> >>
> >> The first question here is whether not exposing MTRR as a feature
> >> was really intended, in particular also for PV Dom0. The XenoLinux
> >> kernel and its forward ports did make use of XENPF_*_memtype to
> >> deal with MTRRs. That's functionality which (maybe for a good
> >> reason) never made it into the pvops kernel. Note that PVH Dom0
> >> does have access to the original settings, as the host values are
> >> used as initial state there.
> >>
> >> The next question would be how we could go about improving the
> >> situation. For the particular issue in 5.18 I've found a relatively
> >> simple solution [1] (which also looks to help graphics performance
> >> on other systems, according to my initial observations with using
> >> the change), albeit its simplicity likely means it either is wrong
> >> in some way, or might not be liked for looking hacky and/or abusive.
> > 
> > I wonder whether the patch needs to be limited to the CPUID Hypervisor
> > bit being present.  If the PAT MSR is readable and returns a value !=
> > 0 then PAT should be available?
> 
> I simply didn't want to be too "aggressive". There may be reasons why
> they want things to be the way they are on native. All I really care
> about is that things are broken on PV Xen; as such I wouldn't even
> mind tightening the check to xen_pv_domain(). But first I need
> feedback from the maintainers there anyway.

Right, I did also consider suggesting to limit this to PV at first,
but I was unsure why native wouldn't also want such behavior.  Maybe
there's no hardware that has PAT but not MTRR, and hence this was
never considered.

> > I guess we should instead check that the current PAT value matches
> > what Linux expects, before declaring PAT enabled?
> 
> I don't think such a check is needed, the code ...
> 
> > Note there's already a comment in init_cache_modes that refers to Xen
> > in the check for PAT CPUID bit.
> 
> ... in __init_cache_modes() already does it the other way around:
> Adapt behavior to what is found in PAT.
> 
> >  We might want to expand that comment
> > (or add one to the new check you are adding).
> 
> I don't see what further information you would want to put there.

It would depend on how the check ends up looking I think.  If the
enabling is limited to also having the Hypervisor CPUID bit set then
the comment should likely mention why setting it on native is not
safe.

Anyway, let's see what maintainers think.

The other option would be to set some kind of hooks for Xen PV to be
able to report PAT as enabled (maybe a Xen PV implementation of
mtrr_ops?), but that seems like over-engineering it.  My main concern
with setting pat_bp_enabled to true is whether in the future such
setting could be used to gate PAT MSR writes.  It's already like this
for APs (in pat_init() -> pat_ap_init()), but we avoid that path
because we don't report MTRR support AFAICT.

Thanks, Roger.