x86/PV: (lack of) MTRR exposure

x86/PV: (lack of) MTRR exposure

[Jan Beulich]

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.
We can't, for example, simply turn on the MTRR bit in CPUID, as that
would implicitly lead to the kernel trying to write the PAT MSR (if,
see below, it didn't itself zap the bit). We also can't simply
ignore PAT MSR writes, as the kernel won't check whether writes
actually took effect. (All of that did work on top of old Xen
apparently only because xen_init_capabilities() itself also forces
the MTRR feature to off.)

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2022-04/msg02392.html

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

[Boris Ostrovsky]

On 4/28/22 11:53 AM, 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.


Initially MTRR was supposed to be supported but it was shot down by x86 maintainers who strongly suggested to use PAT.


https://lists.xen.org/archives/html/xen-devel/2010-09/msg01634.html


-boris


>
> 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.
> We can't, for example, simply turn on the MTRR bit in CPUID, as that
> would implicitly lead to the kernel trying to write the PAT MSR (if,
> see below, it didn't itself zap the bit). We also can't simply
> ignore PAT MSR writes, as the kernel won't check whether writes
> actually took effect. (All of that did work on top of old Xen
> apparently only because xen_init_capabilities() itself also forces
> the MTRR feature to off.)
>
> Jan
>
> [1] https://lists.xen.org/archives/html/xen-devel/2022-04/msg02392.html
>

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

[Roger Pau Monné]

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 guess we should instead check that the current PAT value matches
what Linux expects, before declaring PAT enabled?

Note there's already a comment in init_cache_modes that refers to Xen
in the check for PAT CPUID bit.  We might want to expand that comment
(or add one to the new check you are adding).

Thanks, Roger.

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

[Jan Beulich]

On 28.04.2022 23:30, Boris Ostrovsky wrote:
> 
> On 4/28/22 11:53 AM, 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.
> 
> 
> Initially MTRR was supposed to be supported but it was shot down by x86 maintainers who strongly suggested to use PAT.
> 
> 
> https://lists.xen.org/archives/html/xen-devel/2010-09/msg01634.html

I recall Ingo's dislike, yes. But them suggesting to use PAT when
PAT depends on MTRR internally is, well, odd. Plus there continues
to be the question why PVH Dom0 should see (and be able to play
with) MTRRs, when PV Dom0 can't even learn their values.

Jan

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

[Jan Beulich]

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

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

[Roger Pau Monné]

On Fri, Apr 29, 2022 at 11:50:39AM +0200, Jan Beulich wrote:
> On 28.04.2022 23:30, Boris Ostrovsky wrote:
> > 
> > On 4/28/22 11:53 AM, 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.
> > 
> > 
> > Initially MTRR was supposed to be supported but it was shot down by x86 maintainers who strongly suggested to use PAT.
> > 
> > 
> > https://lists.xen.org/archives/html/xen-devel/2010-09/msg01634.html
> 
> I recall Ingo's dislike, yes. But them suggesting to use PAT when
> PAT depends on MTRR internally is, well, odd. Plus there continues
> to be the question why PVH Dom0 should see (and be able to play
> with) MTRRs, when PV Dom0 can't even learn their values.

Oh, I didn't realize the handling of MTRR in PVH dom0 was a question,
sorry.

I don't think it makes sense to limit PVH dom0 access to MTRR if that
then implies changes to Linux when running in PVH mode so it can use
PAT, and likely changes to Xen in order to avoid using MTRR when
calculating the effective cache types (ie: in epte_get_entry_emt).

I also don't think there's a need to have this kind of feature parity
between PVH and PV dom0s.  IMO we should expose whatever is required
or makes the implementation of guests easier.  For PVH we could always
stop reporting the CPUID MTRR bit and thus stop exposing MTRRs and
guests should cope.

Thanks, Roger.

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

[Roger Pau Monné]

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.

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 4228 bytes --]

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 #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Jan Beulich]

On 02.05.2022 16:25, Juergen Gross wrote:
> PAT MSR writes can be handled by special casing in xen_write_msr_safe().

You can squash the write attempt there, but that'll still confuse the caller
assuming the write actually took effect.

Jan

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 426 bytes --]

On 02.05.22 16:50, Jan Beulich wrote:
> On 02.05.2022 16:25, Juergen Gross wrote:
>> PAT MSR writes can be handled by special casing in xen_write_msr_safe().
> 
> You can squash the write attempt there, but that'll still confuse the caller
> assuming the write actually took effect.

With the list of cpus supported by Xen I don't see a big challenge here.
PAT virtualization handles everything we need.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1997 bytes --]

On 28.04.22 17:53, 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.
> We can't, for example, simply turn on the MTRR bit in CPUID, as that
> would implicitly lead to the kernel trying to write the PAT MSR (if,
> see below, it didn't itself zap the bit). We also can't simply
> ignore PAT MSR writes, as the kernel won't check whether writes
> actually took effect. (All of that did work on top of old Xen
> apparently only because xen_init_capabilities() itself also forces
> the MTRR feature to off.)

I've sent an alternative patch addressing this problem:

https://lore.kernel.org/lkml/20220503132207.17234-3-jgross@suse.com/T/#u

Lets see whether it is accepted.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]