Date:   Tue, 12 Jul 2022 09:22:24 -0400
From:   Chuck Zmudzinski <brchuckz@...scape.net>
To:     Jan Beulich <jbeulich@...e.com>
Cc:     Andrew Lutomirski <luto@...nel.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        lkml <linux-kernel@...r.kernel.org>,
        "xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
        Juergen Gross <jgross@...e.com>, Borislav Petkov <bp@...en8.de>
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

Powered by blists - more mailing lists