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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan