All of lore.kernel.org
 help / color / mirror / Atom feed
From: Juergen Gross <jgross@suse.com>
To: Chuck Zmudzinski <brchuckz@netscape.net>,
	Jan Beulich <jbeulich@suse.com>
Cc: Andrew Lutomirski <luto@kernel.org>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	lkml <linux-kernel@vger.kernel.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Borislav Petkov <bp@alien8.de>
Subject: Re: [PATCH] x86/PAT: have pat_enabled() properly reflect state when running on e.g. Xen
Date: Tue, 12 Jul 2022 15:32:37 +0200	[thread overview]
Message-ID: <45fd7ade-61fe-18fa-aacc-ed80fdf3fb8d@suse.com> (raw)
In-Reply-To: <8011dff1-6551-898f-7e37-38ede106e2f4@netscape.net>


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

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


[-- 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 --]

  reply	other threads:[~2022-07-12 13:32 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-28 14:50 [PATCH] x86/PAT: have pat_enabled() properly reflect state when running on e.g. Xen Jan Beulich
2022-05-03 12:54 ` Juergen Gross
2022-05-11 13:32   ` Juergen Gross
2022-05-21 13:56 ` Chuck Zmudzinski
2022-05-25  8:55 ` Ping: " Jan Beulich
2022-07-04 11:58   ` Thorsten Leemhuis
2022-07-04 12:26     ` Jan Beulich
2022-07-05 10:57       ` Thorsten Leemhuis
2022-07-05 11:02         ` Jan Beulich
2022-07-05 13:36         ` Borislav Petkov
2022-07-05 13:38           ` Juergen Gross
2022-07-14 17:17         ` Chuck Zmudzinski
2022-07-14 22:33           ` Chuck Zmudzinski
2022-07-14 22:45             ` Chuck Zmudzinski
2022-07-19 14:26               ` Chuck Zmudzinski
2022-07-05 15:04 ` Borislav Petkov
2022-07-05 15:56   ` Jan Beulich
2022-07-05 16:14     ` Borislav Petkov
2022-07-06  6:17       ` Jan Beulich
2022-07-06 17:01         ` Borislav Petkov
2022-07-07  6:38           ` Jan Beulich
2022-07-11 10:40             ` Borislav Petkov
2022-07-11 11:38       ` Chuck Zmudzinski
2022-07-11 12:28       ` [PATCH] x86/PAT: have pat_enabled() properly reflect state when running on e.g. Xen, with corrected patch Chuck Zmudzinski
2022-07-11 14:18       ` [PATCH] x86/PAT: have pat_enabled() properly reflect state when running on e.g. Xen Chuck Zmudzinski
2022-07-11 14:31         ` Juergen Gross
2022-07-11 17:41           ` Chuck Zmudzinski
2022-07-12  5:49             ` Juergen Gross
2022-07-12  6:04             ` Jan Beulich
2022-07-12 13:22               ` Chuck Zmudzinski
2022-07-12 13:32                 ` Juergen Gross [this message]
2022-07-12 15:09                   ` Chuck Zmudzinski
2022-07-12 15:30                     ` Juergen Gross
2022-07-12 16:34                       ` Chuck Zmudzinski
2022-08-15 10:20 ` [tip: x86/urgent] x86/PAT: Have pat_enabled() properly reflect state when running on Xen tip-bot2 for Jan Beulich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=45fd7ade-61fe-18fa-aacc-ed80fdf3fb8d@suse.com \
    --to=jgross@suse.com \
    --cc=bp@alien8.de \
    --cc=brchuckz@netscape.net \
    --cc=dave.hansen@linux.intel.com \
    --cc=jbeulich@suse.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=peterz@infradead.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.