lkml.org 
[lkml]   [2022]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] x86/PAT: have pat_enabled() properly reflect state when running on e.g. Xen
On 12.07.22 15:22, Chuck Zmudzinski wrote:
> 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.

Chuck, could you please send out a proper patch with your initial fix
(setting pat_bp_enabled) and the fix above?

I've chatted with Boris Petkov on IRC and he is fine with that.

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

Boris and I agreed to pursue my approach further by removing the
dependency between PAT and MTRR and to make this whole mess more
clear.

I will start to work on this as soon as possible, which will
probably be some time in September.

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

In principle I agree, but you should be aware of my refactoring plans.


Juergen

[unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2022-07-12 15:33    [W:0.069 / U:0.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site