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 17:09, Chuck Zmudzinski wrote:
> On 7/12/2022 9:32 AM, Juergen Gross wrote:
>> 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.
>
> That's great, I will submit a formal patch later today.
>
>>
>>> 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.
>
> Good, I will look for your patches and try them out.
>
>>
>>> 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.
>
> I will defer to you and stop working on this re-factoring effort, but I
> will prepare a formal patch to fix the regression later today.
>
> I do think Jan's point about respecting the administrator's "nopat" setting
> should also be considered. AFAICT, the "nopat" option in current code

Yes, please add that, too. This was what I meant with "the fix above".

> is also not being respected on the bare metal, and the patch to
> init_cache_modes with a force_no_pat variable is also needed to
> ensure "nopat" is respected on the bare metal, AFAICT.

Hmm, I don't see how the PAT MSR will be written on bare metal when "nopat"
has been specified.

I just tried it with a 5.19 kernel and it booted with the correct PAT
settings:

[ 0.000000] x86/PAT: PAT support disabled via boot option.
[ 0.000986] x86/PAT: Configuration [0-7]: WB WT UC- UC WB WT UC- UC


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