LinuxLists.cc - [PATCH] x86/PAT: have pat_enabled() properly reflect state when running on e.g. Xen

2022-04-30 10:31:47

Subject: [PATCH] x86/PAT: have pat_enabled() properly reflect state when running on e.g. Xen

The latest with commit bdd8b6c98239 ("drm/i915: replace X86_FEATURE_PAT
with pat_enabled()") pat_enabled() returning false (because of PAT
initialization being suppressed in the absence of MTRRs being announced
to be available) has become a problem: The i915 driver now fails to
initialize when running PV on Xen (i915_gem_object_pin_map() is where I
located the induced failure), and its error handling is flaky enough to
(at least sometimes) result in a hung system.

Yet even beyond that problem the keying of the use of WC mappings to
pat_enabled() (see arch_can_pci_mmap_wc()) means that in particular
graphics frame buffer accesses would have been quite a bit less
performant than possible.

Arrange for the function to return true in such environments, without
undermining the rest of PAT MSR management logic considering PAT to be
disabled: Specifically, no writes to the PAT MSR should occur.

For the new boolean to live in .init.data, init_cache_modes() also needs
moving to .init.text (where it could/should have lived already before).

Signed-off-by: Jan Beulich <[email protected]>
---
On the system where I observed the issue, a knock-on effect of driver
initialization failing was that the SATA-controller also started to
report failures.

--- a/arch/x86/mm/pat/memtype.c
+++ b/arch/x86/mm/pat/memtype.c
@@ -62,6 +62,7 @@

static bool __read_mostly pat_bp_initialized;
static bool __read_mostly pat_disabled = !IS_ENABLED(CONFIG_X86_PAT);
+static bool __initdata pat_force_disabled = !IS_ENABLED(CONFIG_X86_PAT);
static bool __read_mostly pat_bp_enabled;
static bool __read_mostly pat_cm_initialized;

@@ -86,6 +87,7 @@ void pat_disable(const char *msg_reason)
static int __init nopat(char *str)
{
pat_disable("PAT support disabled via boot option.");
+ pat_force_disabled = true;
return 0;
}
early_param("nopat", nopat);
@@ -272,7 +274,7 @@ static void pat_ap_init(u64 pat)
wrmsrl(MSR_IA32_CR_PAT, pat);
}

-void init_cache_modes(void)
+void __init init_cache_modes(void)
{
u64 pat = 0;

@@ -313,6 +315,13 @@ void init_cache_modes(void)
*/
pat = PAT(0, WB) | PAT(1, WT) | PAT(2, UC_MINUS) | PAT(3, UC) |
PAT(4, WB) | PAT(5, WT) | PAT(6, UC_MINUS) | PAT(7, UC);
+ } else if (!pat_force_disabled &&
+ boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
+ /*
+ * Clearly PAT is enabled underneath. Allow pat_enabled() to
+ * reflect this.
+ */
+ pat_bp_enabled = true;
}

__init_cache_modes(pat);

2022-05-03 15:07:04

by Jürgen Groß

[permalink] [raw]

Subject: Re: [PATCH] x86/PAT: have pat_enabled() properly reflect state when running on e.g. Xen

On 28.04.22 16:50, Jan Beulich wrote:
> The latest with commit bdd8b6c98239 ("drm/i915: replace X86_FEATURE_PAT
> with pat_enabled()") pat_enabled() returning false (because of PAT
> initialization being suppressed in the absence of MTRRs being announced
> to be available) has become a problem: The i915 driver now fails to
> initialize when running PV on Xen (i915_gem_object_pin_map() is where I
> located the induced failure), and its error handling is flaky enough to
> (at least sometimes) result in a hung system.
>
> Yet even beyond that problem the keying of the use of WC mappings to
> pat_enabled() (see arch_can_pci_mmap_wc()) means that in particular
> graphics frame buffer accesses would have been quite a bit less
> performant than possible.
>
> Arrange for the function to return true in such environments, without
> undermining the rest of PAT MSR management logic considering PAT to be
> disabled: Specifically, no writes to the PAT MSR should occur.
>
> For the new boolean to live in .init.data, init_cache_modes() also needs
> moving to .init.text (where it could/should have lived already before).
>
> Signed-off-by: Jan Beulich <[email protected]>

I think this approach isn't the best way to tackle the issue.

It can be solved rather easily by not deriving the supported caching
modes via pat_enabled(), but by adding specific functions to query
the needed caching mode from the PAT translation tables, and to use
those functions instead of pat_enabled().

I'm preparing a patch for that purpose.

Juergen

Attachments:

OpenPGP_0xB0DE9DD628BF132F.asc (3.08 kB)
OpenPGP public key OpenPGP_signature (505.00 B)
OpenPGP digital signature Download all attachments

2022-05-11 16:24:37

On 7/5/22 12:14 PM, Borislav Petkov wrote:
> On Tue, Jul 05, 2022 at 05:56:36PM +0200, Jan Beulich wrote:
> > Re-using pat_disabled like you do in your suggestion below won't
> > work, because mtrr_bp_init() calls pat_disable() when MTRRs
> > appear to be disabled (from the kernel's view). The goal is to
> > honor "nopat" without honoring any other calls to pat_disable().
>
> Actually, the current goal is to adjust Xen dom0 because:
>
> 1. it uses the PAT code
>
> 2. but then it does something special and hides the MTRRs
>
> which is not something real hardware does.
>
> So this one-off thing should be prominent, visible and not get in the
> way.

Hello,

I have spent a fair amount of time this past weekend
investigating this regression and what might have caused
it and I also have done several tests on my Xen workstation
that exhibits the regression to verify my understanding
of the problem and also raise other problematic points.

I think, in addition to immediately fixing the regression by
fixing the now five-year-old commit that is the root cause
of the recently exposed regression, as discussed in my
earlier message which proposes a simple patch to fix that
five-year-old broken commit,

https://lore.kernel.org/lkml/[email protected]/

there needs to be a decision about how best to deal with
this very aptly described "one-off Xen thing" in the future.

One problem in x86/mm/pat/memtype.c is the fact that there
exist two functions, pat_init(), and init_cache_modes(), that
do more or less the same thing, so that when one of those
functions needs to be updated, the other also needs to
be updated. In the past, when changes to the pat_enable
and pat_disable functions and variables were made, all
too often, the change was only applied to pat_init() and not
to init_cache_modes() and the one-off Xen case was simply
not addressed and dealt with properly.

So I propose the following:

1) Identify those things that always need to be done in
either pat_init() or init_cache_modes() and try to combine
those things into a single function so that changes will
be applied for both cases. We sort of have that now with
__init_cache_modes(), but it is not really good enough to
prevent the regressions we sometimes get in Xen PV
domains such as the current regression we have with
Xen Dom0 and certain particular Intel graphics devices.

2) If it is not possible to do what is proposed in 1), at least
re-factor the code to make it very clear that whenever
either pat_init() needs to be adjusted or init_cache_modes()
needs to be adjusted, care must be taken to ensure that
all cases are properly dealt with, including the
one-off Xen case of enabling PAT with MTRR disabled,
Currently, AIUI, all one really needs to know is that
init_cached_modes() handles two special cases:
First, when PAT is disabled for any reason, including when
the "nopat" boot option is set, and second, the one-off
Xen case of MTRR being disabled but PAT being enabled.

I am trying to do number 2 with a patch series I am
working on. It is up to the experts to decide if it
is possible or even desirable to improve the code so
that any changes to the caching configuration are more
likely to be properly implemented for all supported platforms
by successfully combining the two functions pat_init() and
init_cache_modes() into a single function. The new function
would probably need to be renamed and include bits from
mtrr_bp_enabled, etc. for it to function properly.

If anyone wants to argue that it is not desirable to try
combine pat_init() and init_cache_modes() into a single
function, I think the burden of proof rests on that
person to demonstrate why it is good and clean
coding practice to continue to have them separate
and independent from each other in the code when
they both essentially do the same thing in the end, which
is call __init_cache_modes() and determine the PAT
state, and also probably the MTRR state.

Best Regards,

Chuck

2022-07-11 14:53:05

On 11.07.22 19:41, Chuck Zmudzinski wrote:
> On 7/11/2022 10:31 AM, Juergen Gross wrote:
>> On 11.07.22 16:18, Chuck Zmudzinski wrote:
>>> On 7/5/22 12:14 PM, Borislav Petkov wrote:
>>>> On Tue, Jul 05, 2022 at 05:56:36PM +0200, Jan Beulich wrote:
>>>>> Re-using pat_disabled like you do in your suggestion below won't
>>>>> work, because mtrr_bp_init() calls pat_disable() when MTRRs
>>>>> appear to be disabled (from the kernel's view). The goal is to
>>>>> honor "nopat" without honoring any other calls to pat_disable().
>>>>
>>>> Actually, the current goal is to adjust Xen dom0 because:
>>>>
>>>> 1. it uses the PAT code
>>>>
>>>> 2. but then it does something special and hides the MTRRs
>>>>
>>>> which is not something real hardware does.
>>>>
>>>> So this one-off thing should be prominent, visible and not get in the
>>>> way.
>>>
>>> Hello,
>>>
>>> I have spent a fair amount of time this past weekend
>>> investigating this regression and what might have caused
>>> it and I also have done several tests on my Xen workstation
>>> that exhibits the regression to verify my understanding
>>> of the problem and also raise other problematic points.
>>>
>>> I think, in addition to immediately fixing the regression by
>>> fixing the now five-year-old commit that is the root cause
>>> of the recently exposed regression, as discussed in my
>>> earlier message which proposes a simple patch to fix that
>>> five-year-old broken commit,
>>>
>>> https://lore.kernel.org/lkml/[email protected]/
>>>
>>> there needs to be a decision about how best to deal with
>>> this very aptly described "one-off Xen thing" in the future.
>>>
>>> One problem in x86/mm/pat/memtype.c is the fact that there
>>> exist two functions, pat_init(), and init_cache_modes(), that
>>> do more or less the same thing, so that when one of those
>>> functions needs to be updated, the other also needs to
>>> be updated. In the past, when changes to the pat_enable
>>> and pat_disable functions and variables were made, all
>>> too often, the change was only applied to pat_init() and not
>>> to init_cache_modes() and the one-off Xen case was simply
>>> not addressed and dealt with properly.
>>>
>>> So I propose the following:
>>>
>>> 1) Identify those things that always need to be done in
>>> either pat_init() or init_cache_modes() and try to combine
>>> those things into a single function so that changes will
>>> be applied for both cases. We sort of have that now with
>>> __init_cache_modes(), but it is not really good enough to
>>> prevent the regressions we sometimes get in Xen PV
>>> domains such as the current regression we have with
>>> Xen Dom0 and certain particular Intel graphics devices.
>>>
>>> 2) If it is not possible to do what is proposed in 1), at least
>>> re-factor the code to make it very clear that whenever
>>> either pat_init() needs to be adjusted or init_cache_modes()
>>> needs to be adjusted, care must be taken to ensure that
>>> all cases are properly dealt with, including the
>>> one-off Xen case of enabling PAT with MTRR disabled,
>>> Currently, AIUI, all one really needs to know is that
>>> init_cached_modes() handles two special cases:
>>> First, when PAT is disabled for any reason, including when
>>> the "nopat" boot option is set, and second, the one-off
>>> Xen case of MTRR being disabled but PAT being enabled.
>>>
>>> I am trying to do number 2 with a patch series I am
>>> working on. It is up to the experts to decide if it
>>> is possible or even desirable to improve the code so
>>> that any changes to the caching configuration are more
>>> likely to be properly implemented for all supported platforms
>>> by successfully combining the two functions pat_init() and
>>> init_cache_modes() into a single function. The new function
>>> would probably need to be renamed and include bits from
>>> mtrr_bp_enabled, etc. for it to function properly.
>>>
>>> If anyone wants to argue that it is not desirable to try
>>> combine pat_init() and init_cache_modes() into a single
>>> function, I think the burden of proof rests on that
>>> person to demonstrate why it is good and clean
>>> coding practice to continue to have them separate
>>> and independent from each other in the code when
>>> they both essentially do the same thing in the end, which
>>> is call __init_cache_modes() and determine the PAT
>>> state, and also probably the MTRR state.
>>
>> I think the proper solution would be to make PAT and MTRR independent
>> from each other with some additional restructuring on the PAT side:
>>
>> Today PAT can't be used without MTRR being available, unless MTRR is at
>> least configured via CONFIG_MTRR and the system is running as Xen PV
>> guest. In this case PAT is automatically available via the hypervisor,
>> but the PAT MSR can't be modified by the kernel and MTRR is disabled.
>>
>> As an additional complexity the availability of PAT can't be queried
>> via pat_enabled() in the Xen PV case, as the lack of MTRR will set PAT
>> to be disabled.
>
> My testing indicates your presumption is not correct, or at least my
> testing on my Xen workstation makes me doubt that your assertion that
> PAT can't be queried vi pat_enabled() in the Xen PV case is true.

I was referring to the current (regressed) state.

>
> Let me begin a discussion by posing a question about the following
> code in init_cache_modes(), a function implemented in
> arch/x86/mm/pat/memtypes.c, and which is called from
> arch/x86/kernel/setup.c but only effective if pat_cm_initialized
> has not already been set by pat_init(), as is obvious from the
> immediate return if pat_cm_initialized has already been set.
>
> Now in the Xen PV case, pat_cm_initialized is not set because
> pat_init(), where it is normally set, was skipped due to the fact
> that MTRRs are disabled in Xen PV domains. I verified that
> init_cache_modes() is activated in the Xen PV Dom0 case by my
> testing. So, I ask, why cannot the code that is present here in
> init_cache_modes() be used to test for PAT in the Xen PV Dom0
> environment and if the test is positive, why cannot pat_bp_enabled
> be set which will cause pat_enabled() to return true in the Xen
> PV environment? In fact, I used the additional 'else if' block to
> set pat_bp_enabled to true in the Xen PV environment for the
> case when boot_cpu_has(X86_FEATURE_PAT) is true and PAT
> MSR does not return 0, that, is, when pat !=0 after calling
> rdmsrl(MSR_IA32_CR_PAT, pat) for the boot CPU. All the existing
> comments in this function indicate this is a valid way to test
> for the existence of PAT in the Xen PV Dom0 environment.

I agree this is a possibility to fix the regression.

In the long run I still think that my idea is the better one, as it
allows to run without CONFIG_MTRR even on bare metal without losing
the PAT functionality.

>
> Moreover... (please move to the bottom of the code snippet
> for more information about my tests in the Xen PV environment...)
>
> void init_cache_modes(void)
> {
>    u64 pat = 0;
>
>    if (pat_cm_initialized)
>         return;
>
>    if (boot_cpu_has(X86_FEATURE_PAT)) {
>         /*
>         * CPU supports PAT. Set PAT table to be consistent with
>         * PAT MSR. This case supports "nopat" boot option, and
>         * virtual machine environments which support PAT without
>         * MTRRs. In specific, Xen has unique setup to PAT MSR.
>         *
>         * If PAT MSR returns 0, it is considered invalid and emulates
>         * as No PAT.
>         */
>         rdmsrl(MSR_IA32_CR_PAT, pat);
>    }
>
>    if (!pat) {
>         /*
>         * No PAT. Emulate the PAT table that corresponds to the two
>         * cache bits, PWT (Write Through) and PCD (Cache Disable).
>         * This setup is also the same as the BIOS default setup.
>         *
>         * PTE encoding:
>         *
>         *       PCD
>         *       |PWT PAT
>         *       ||    slot
>         *       00    0    WB : _PAGE_CACHE_MODE_WB
>         *       01    1    WT : _PAGE_CACHE_MODE_WT
>         *       10    2    UC-: _PAGE_CACHE_MODE_UC_MINUS
>         *       11    3    UC : _PAGE_CACHE_MODE_UC
>         *
>         * NOTE: When WC or WP is used, it is redirected to UC- per
>         * the default setup in __cachemode2pte_tbl[].
>         */
>         pat = PAT(0, WB) | PAT(1, WT) | PAT(2, UC_MINUS) | PAT(3, UC) |
>               PAT(4, WB) | PAT(5, WT) | PAT(6, UC_MINUS) | PAT(7, UC);
>    }
>
>    else if (!pat_bp_enabled) {
>         /*
>         * In some environments, specifically Xen PV, PAT
>         * initialization is skipped because MTRRs are
>         * disabled even though PAT is available. In such
>         * environments, set PAT to initialized and enabled to
>         * correctly indicate to callers of pat_enabled() that
>         * PAT is available and prevent PAT from being disabled.
>         */
>         pat_bp_enabled = true;
>         pr_info("x86/PAT: PAT enabled by init_cache_modes\n");
>    }
>
>    __init_cache_modes(pat);
> }
>
> This function, patched with the extra 'else if' block, fixes the
> regression on my Xen worksatation, and the pr_info message
> "x86/PAT: PAT enabled by init_cache_modes" appears in the logs
> when running this patched kernel in my Xen Dom0. This means
> that in the Xen PV environment on my Xen Dom0 workstation,
> rdmsrl(MSR_IA32_CR_PAT, pat) successfully tested for the presence
> of PAT on the virtual CPU that Xen exposed to the Linux kernel on my
> Xen Dom0 workstation. At least that is what I think my tests prove.

Yes, your fix is correct AFAICS.

> So why is this not a valid way to test for the existence of
> PAT in the Xen PV environment? Are the existing comments
> in init_cache_modes() about supporting both the case when
> the "nopat" boot option is set and the specific case of Xen and
> MTRR disabled wrong? My testing confirms those comments are
> correct.
>
>> This leads to some drivers believing that not all cache
>> modes are available, resulting in failures or degraded functionality
>> (the current regression).
>
> I also did some tests to try to verify that fast WC caching mode
> was used as requested by the i915 Intel graphics driver on my
> Xen Dom0 workstation and so far I cannot prove that the WC caching
> mode was not used, and I have some custom logs from the
> Linux kernel i915 driver that indicates my Linux Xen Dom0 is able to
> use the fast WC caching mode, so long as the bug that causes
> pat_enabled() to return a false negative is fixed.
>
> So my testing seems to contradict rather than confirm your claim
> that "the availability of PAT can't be queried via pat_enabled() in
> the Xen PV case." I think my patch successfully does what you
> claim cannot be done, and I also think the existing comments in
> init_cache_modes() indicates that my patch is successfully testing
> for PAT in the Xen PV case.
>
>>
>> The same applies to a kernel built with no MTRR support: it won't
>> allow to use the PAT MSR, even if there is no technical reason for
>> that, other than setting up PAT on all cpus the same way (which is a
>> requirement of the processor's cache management) is relying on some
>> MTRR specific code.
>
> After reading the code extensively, I am curious about testing
> and even successfully compiling a kernel with options like
> CONFIG_MTRR and CONFIG_PAT disabled. But my testing indicates
> such problems could be debugged and fixed if they manifest
> using the current code which, AFAICT, appears to be mostly
> designed and tested for the case when both PAT and MTRR
> are enabled on modern x86 hardware. I would expect many
> bugs for configurations that deviate too far from that paradigm
> on Linux. This is the problem with Xen's unique configuration
> that enables PAT but disables MTRR. I think the problem can
> be solved, it is just that there is not enough care taken to
> ensure that these outlier cases will work as well as the common
> configuration that Linux supports well, which is with both
> MTRR and PAT enabled on x86.

On bare metal PAT can't work without CONFIG_MTRR, as setting the PAT
MSR uniformly across cpus can't be done (the code is not available
without CONFIG_MTRR).

>
>>
>> All of that should be fixable by:
>>
>> - moving the function needed by PAT from MTRR specific code one level
>> up
>> - adding a PAT indirection layer supporting the 3 cases "no or disabled
>> PAT", "PAT under kernel control", and "PAT under Xen control"
>
> I am not sure the right way to divide the cases is "PAT under
> kernel control" and "PAT under Xen control." I also looked at
> the code in the Xen hypervisor and it does allow for Dom0
> to use a Xen hypercall provided by the xenctrl library to enable
> PAT WC caching mode, and I ran some tests for evidence that

I guess you mean XEN_DMOP_pin_memory_cacheattr. This is meant to
be used for device emulation.

> my Xen Dom0 was making such hypercalls to support the WC
> caching mode, but I have not yet been able to verify that the
> WC caching mode is enabled by a hypercall from Dom0 to Xen,
> although by reading the code in the hypervisor and xenctrl it
> seems to be possible. I just don't think the i915 Linux kernel
> driver uses such hypercalls, and I am not sure that my Xen
> Dom0 could be using the fast WC caching mode because
> "PAT is under Xen control" instead of "under kernel control"
> unless the kernel is relying on Xen to provide PAT via Xen
> hypercalls. The successful call of my Xen Dom0 to rdmsrl

The PAT MSR is setup by the hypervisor to support all possible
caching modes. Dom0 just needs to set the correct bits in a
page table entry to select the wanted caching mode.

The reason why there is no hypercall to modify the PAT MSR is,
that it is required that the MSR has the same value across all
cpus on the system. This precludes the possibility that any
guest could change it, as this would change it for all guests.
This all is true only for PV guests, of course, as HVM or PVH
guests get a virtualized PAT MSR via the hardware virtualization
feature.

> which obtained a valid configuration of PAT for the virtual
> boot CPU exposed by Xen to the kernel indicates at least that
> the kernel can detect, if not control, what the supported PAT
> caching modes are. Of course you are absolutely correct that
> the kernel cannot directly control the PAT caching modes in the
> Xen PV environment, at least not without possibly resorting to
> some Xen hypercalls that are most likely not currently in use
> by the kernel.

There is such hypercall. The hypercall you are thinking of is
only changing the caching mode for a specific set of pages by
modifying the related EPT/NPT entries.

> I would have to do much more testing to really determine
> what is happening and how my Xen Dom0 is really
> configuring the PAT caching modes and specifically, if
> it is actually using the fast WC caching mode as requested
> by the i915 Intel graphics driver.

A PV dom0 can only adapt itself to the PAT setting the Xen
hypervisor has done.

>
>> - removing the dependency of PAT on MTRR
>>
>> I'd be up for trying this, but right now I haven't got the time to do
>> it. I might be able to start working on that in September.
>
> Please let me know if/when you start on it, I would be interested
> to try out what you come up with on my Xen Dom0.

Will do that.

>
>>
>> Meanwhile I think either Jan's patch or the simple one of Chuck
>> should be applied.
>
> I agree.

Happy to hear that. :-)

Juergen

Attachments:

OpenPGP_0xB0DE9DD628BF132F.asc (3.08 kB)
OpenPGP public key OpenPGP_signature (501.00 B)
OpenPGP digital signature Download all attachments

2022-07-12 06:39:50

by Jan Beulich

[permalink] [raw]

Subject: Re: [PATCH] x86/PAT: have pat_enabled() properly reflect state when running on e.g. Xen

On 11.07.2022 19:41, Chuck Zmudzinski wrote:
> Moreover... (please move to the bottom of the code snippet
> for more information about my tests in the Xen PV environment...)
>
> void init_cache_modes(void)
> {
>    u64 pat = 0;
>
>    if (pat_cm_initialized)
>         return;
>
>    if (boot_cpu_has(X86_FEATURE_PAT)) {
>         /*
>         * CPU supports PAT. Set PAT table to be consistent with
>         * PAT MSR. This case supports "nopat" boot option, and
>         * virtual machine environments which support PAT without
>         * MTRRs. In specific, Xen has unique setup to PAT MSR.
>         *
>         * If PAT MSR returns 0, it is considered invalid and emulates
>         * as No PAT.
>         */
>         rdmsrl(MSR_IA32_CR_PAT, pat);
>    }
>
>    if (!pat) {
>         /*
>         * No PAT. Emulate the PAT table that corresponds to the two
>         * cache bits, PWT (Write Through) and PCD (Cache Disable).
>         * This setup is also the same as the BIOS default setup.
>         *
>         * PTE encoding:
>         *
>         *       PCD
>         *       |PWT PAT
>         *       ||    slot
>         *       00    0    WB : _PAGE_CACHE_MODE_WB
>         *       01    1    WT : _PAGE_CACHE_MODE_WT
>         *       10    2    UC-: _PAGE_CACHE_MODE_UC_MINUS
>         *       11    3    UC : _PAGE_CACHE_MODE_UC
>         *
>         * NOTE: When WC or WP is used, it is redirected to UC- per
>         * the default setup in __cachemode2pte_tbl[].
>         */
>         pat = PAT(0, WB) | PAT(1, WT) | PAT(2, UC_MINUS) | PAT(3, UC) |
>               PAT(4, WB) | PAT(5, WT) | PAT(6, UC_MINUS) | PAT(7, UC);
>    }
>
>    else if (!pat_bp_enabled) {
>         /*
>         * In some environments, specifically Xen PV, PAT
>         * initialization is skipped because MTRRs are
>         * disabled even though PAT is available. In such
>         * environments, set PAT to initialized and enabled to
>         * correctly indicate to callers of pat_enabled() that
>         * PAT is available and prevent PAT from being disabled.
>         */
>         pat_bp_enabled = true;
>         pr_info("x86/PAT: PAT enabled by init_cache_modes\n");
>    }
>
>    __init_cache_modes(pat);
> }
>
> This function, patched with the extra 'else if' block, fixes the
> regression on my Xen worksatation, and the pr_info message
> "x86/PAT: PAT enabled by init_cache_modes" appears in the logs
> when running this patched kernel in my Xen Dom0. This means
> that in the Xen PV environment on my Xen Dom0 workstation,
> rdmsrl(MSR_IA32_CR_PAT, pat) successfully tested for the presence
> of PAT on the virtual CPU that Xen exposed to the Linux kernel on my
> Xen Dom0 workstation. At least that is what I think my tests prove.
>
> So why is this not a valid way to test for the existence of
> PAT in the Xen PV environment? Are the existing comments
> in init_cache_modes() about supporting both the case when
> the "nopat" boot option is set and the specific case of Xen and
> MTRR disabled wrong? My testing confirms those comments are
> correct.

At the very least this ignores the possible "nopat" an admin may
have passed to the kernel.

Jan

2022-07-12 13:32:24

by Chuck Zmudzinski

[permalink] [raw]

Subject: Re: [PATCH] x86/PAT: have pat_enabled() properly reflect state when running on e.g. Xen

On 7/12/2022 2:04 AM, Jan Beulich wrote:
> On 11.07.2022 19:41, Chuck Zmudzinski wrote:
> > Moreover... (please move to the bottom of the code snippet
> > for more information about my tests in the Xen PV environment...)
> >
> > void init_cache_modes(void)
> > {
> >    u64 pat = 0;
> >
> >    if (pat_cm_initialized)
> >         return;
> >
> >    if (boot_cpu_has(X86_FEATURE_PAT)) {
> >         /*
> >         * CPU supports PAT. Set PAT table to be consistent with
> >         * PAT MSR. This case supports "nopat" boot option, and
> >         * virtual machine environments which support PAT without
> >         * MTRRs. In specific, Xen has unique setup to PAT MSR.
> >         *
> >         * If PAT MSR returns 0, it is considered invalid and emulates
> >         * as No PAT.
> >         */
> >         rdmsrl(MSR_IA32_CR_PAT, pat);
> >    }
> >
> >    if (!pat) {
> >         /*
> >         * No PAT. Emulate the PAT table that corresponds to the two
> >         * cache bits, PWT (Write Through) and PCD (Cache Disable).
> >         * This setup is also the same as the BIOS default setup.
> >         *
> >         * PTE encoding:
> >         *
> >         *       PCD
> >         *       |PWT PAT
> >         *       ||    slot
> >         *       00    0    WB : _PAGE_CACHE_MODE_WB
> >         *       01    1    WT : _PAGE_CACHE_MODE_WT
> >         *       10    2    UC-: _PAGE_CACHE_MODE_UC_MINUS
> >         *       11    3    UC : _PAGE_CACHE_MODE_UC
> >         *
> >         * NOTE: When WC or WP is used, it is redirected to UC- per
> >         * the default setup in __cachemode2pte_tbl[].
> >         */
> >         pat = PAT(0, WB) | PAT(1, WT) | PAT(2, UC_MINUS) | PAT(3, UC) |
> >               PAT(4, WB) | PAT(5, WT) | PAT(6, UC_MINUS) | PAT(7, UC);
> >    }
> >
> >    else if (!pat_bp_enabled) {
> >         /*
> >         * In some environments, specifically Xen PV, PAT
> >         * initialization is skipped because MTRRs are
> >         * disabled even though PAT is available. In such
> >         * environments, set PAT to initialized and enabled to
> >         * correctly indicate to callers of pat_enabled() that
> >         * PAT is available and prevent PAT from being disabled.
> >         */
> >         pat_bp_enabled = true;
> >         pr_info("x86/PAT: PAT enabled by init_cache_modes\n");
> >    }
> >
> >    __init_cache_modes(pat);
> > }
> >
> > This function, patched with the extra 'else if' block, fixes the
> > regression on my Xen worksatation, and the pr_info message
> > "x86/PAT: PAT enabled by init_cache_modes" appears in the logs
> > when running this patched kernel in my Xen Dom0. This means
> > that in the Xen PV environment on my Xen Dom0 workstation,
> > rdmsrl(MSR_IA32_CR_PAT, pat) successfully tested for the presence
> > of PAT on the virtual CPU that Xen exposed to the Linux kernel on my
> > Xen Dom0 workstation. At least that is what I think my tests prove.
> >
> > So why is this not a valid way to test for the existence of
> > PAT in the Xen PV environment? Are the existing comments
> > in init_cache_modes() about supporting both the case when
> > the "nopat" boot option is set and the specific case of Xen and
> > MTRR disabled wrong? My testing confirms those comments are
> > correct.
>
> At the very least this ignores the possible "nopat" an admin may
> have passed to the kernel.

I realize that. The patch I proposed here only fixes the regression. It
would be easy to also modify the patch to also observe the 'nopat"
setting. I think your patch had a force_pat_disable local variable that
is set if pat is disabled by the administrator with "nopat." With that
variable available, modifying the patch so in init_cache_modes we have:

     if (!pat || force_pat_disable) {
         /*
          * No PAT. Emulate the PAT table that corresponds to the two

Instead of:

     if (!pat) {
         /*
          * No PAT. Emulate the PAT table that corresponds to the two

would cause the kernel to respect the "nopat" setting by the administrator
in the Xen PV Dom0 environment.

I agree this needs to be fixed up, because currently the code is very
confusing and the current variable names and function names do not
always accurately describe what they actually do in the code. That is
why I am working on a patch to do some re-factoring, which only consists
of function and variable name changes and comment changes to fix
the places where the comments in the code are misleading or incomplete.

I think perhaps the most misnamed variable here is the local
variable pat_disabled in memtypes.c and the most misnamed function is the
pat_disable function in memtypes.c. They should be named pat_init_disabled
and pat_init_disable, respectively, because they do not really disable
PAT in
the code but only prevent execution of the pat_init function. That
leaves open
the possibility for PAT to be enabled by init_cache_modes, which actually
occurs in the current code in the Xen PV Dom0 environment, but the current
code neglects to set pat_bp_enabled to true in that case. So we need a patch
to fix that in order to fix the regression.

Chuck

2022-07-12 13:36:16

[permalink] [raw]

Subject: [tip: x86/urgent] x86/PAT: Have pat_enabled() properly reflect state when running on Xen

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID: 72cbc8f04fe2fa93443c0fcccb7ad91dfea3d9ce
Gitweb: https://git.kernel.org/tip/72cbc8f04fe2fa93443c0fcccb7ad91dfea3d9ce
Author: Jan Beulich <[email protected]>
AuthorDate: Thu, 28 Apr 2022 16:50:29 +02:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Mon, 15 Aug 2022 10:51:23 +02:00

x86/PAT: Have pat_enabled() properly reflect state when running on Xen

After commit ID in the Fixes: tag, pat_enabled() returns false (because
of PAT initialization being suppressed in the absence of MTRRs being
announced to be available).

This has become a problem: the i915 driver now fails to initialize when
running PV on Xen (i915_gem_object_pin_map() is where I located the
induced failure), and its error handling is flaky enough to (at least
sometimes) result in a hung system.

Yet even beyond that problem the keying of the use of WC mappings to
pat_enabled() (see arch_can_pci_mmap_wc()) means that in particular
graphics frame buffer accesses would have been quite a bit less optimal
than possible.

Arrange for the function to return true in such environments, without
undermining the rest of PAT MSR management logic considering PAT to be
disabled: specifically, no writes to the PAT MSR should occur.

For the new boolean to live in .init.data, init_cache_modes() also needs
moving to .init.text (where it could/should have lived already before).

[ bp: This is the "small fix" variant for stable. It'll get replaced
with a proper PAT and MTRR detection split upstream but that is too
involved for a stable backport.
- additional touchups to commit msg. Use cpu_feature_enabled(). ]

Fixes: bdd8b6c98239 ("drm/i915: replace X86_FEATURE_PAT with pat_enabled()")
Signed-off-by: Jan Beulich <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Acked-by: Ingo Molnar <[email protected]>
Cc: <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Lucas De Marchi <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/mm/pat/memtype.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
index d5ef64d..66a209f 100644
--- a/arch/x86/mm/pat/memtype.c
+++ b/arch/x86/mm/pat/memtype.c
@@ -62,6 +62,7 @@

static bool __read_mostly pat_bp_initialized;
static bool __read_mostly pat_disabled = !IS_ENABLED(CONFIG_X86_PAT);
+static bool __initdata pat_force_disabled = !IS_ENABLED(CONFIG_X86_PAT);
static bool __read_mostly pat_bp_enabled;
static bool __read_mostly pat_cm_initialized;

@@ -86,6 +87,7 @@ void pat_disable(const char *msg_reason)
static int __init nopat(char *str)
{
pat_disable("PAT support disabled via boot option.");
+ pat_force_disabled = true;
return 0;
}
early_param("nopat", nopat);
@@ -272,7 +274,7 @@ static void pat_ap_init(u64 pat)
wrmsrl(MSR_IA32_CR_PAT, pat);
}

-void init_cache_modes(void)
+void __init init_cache_modes(void)
{
u64 pat = 0;

@@ -313,6 +315,12 @@ void init_cache_modes(void)
*/
pat = PAT(0, WB) | PAT(1, WT) | PAT(2, UC_MINUS) | PAT(3, UC) |
PAT(4, WB) | PAT(5, WT) | PAT(6, UC_MINUS) | PAT(7, UC);
+ } else if (!pat_force_disabled && cpu_feature_enabled(X86_FEATURE_HYPERVISOR)) {
+ /*
+ * Clearly PAT is enabled underneath. Allow pat_enabled() to
+ * reflect this.
+ */
+ pat_bp_enabled = true;
}

__init_cache_modes(pat);