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

[Juergen Gross <jgross@xxxxxxxx>]

On 29.04.22 12:53, Roger Pau Monné wrote:
> 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.
The clean way to handle this mess would be to support PAT in the kernel
without requiring MTRR.

The only reason for PAT to requite MTRR seems to be the common usage of
mtrr_rendezvous_handler(). I need to look into that a little bit further,
but I think this should be rather easy to solve by using a generic
rendezvous handler and proper callbacks, which will use common backend
functions.

PAT MSR writes can be handled by special casing in xen_write_msr_safe().


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature