Message-ID: <1309c3f5-86c7-e4f8-9f35-e0d430951d49@netscape.net>
Date:   Fri, 15 Jul 2022 15:53:04 -0400
From:   Chuck Zmudzinski <brchuckz@...scape.net>
To:     Jan Beulich <jbeulich@...e.com>
Cc:     Dave Hansen <dave.hansen@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
        Dan Williams <dan.j.williams@...el.com>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Tom Lendacky <thomas.lendacky@....com>,
        Jane Chu <jane.chu@...cle.com>,
        Tianyu Lan <Tianyu.Lan@...rosoft.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Sean Christopherson <seanjc@...gle.com>,
        Juergen Gross <jgross@...e.com>,
        xen-devel@...ts.xenproject.org, stable@...r.kernel.org,
        Thorsten Leemhuis <regressions@...mhuis.info>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] Subject: x86/PAT: Report PAT on CPUs that support PAT
 without MTRR

On 7/15/2022 6:05 AM, Jan Beulich wrote:
> On 15.07.2022 04:07, Chuck Zmudzinski wrote:
> > On 7/14/2022 1:30 AM, Thorsten Leemhuis wrote:
> >> On 13.07.22 03:36, Chuck Zmudzinski wrote:
> >>> The commit 99c13b8c8896d7bcb92753bf
> >>> ("x86/mm/pat: Don't report PAT on CPUs that don't support it")
> >>> incorrectly failed to account for the case in init_cache_modes() when
> >>> CPUs do support PAT and falsely reported PAT to be disabled when in
> >>> fact PAT is enabled. In some environments, notably in Xen PV domains,
> >>> MTRR is disabled but PAT is still enabled, and that is the case
> >>> that the aforementioned commit failed to account for.
> >>>
> >>> As an unfortunate consequnce, the pat_enabled() function currently does
> >>> not correctly report that PAT is enabled in such environments. The fix
> >>> is implemented in init_cache_modes() by setting pat_bp_enabled to true
> >>> in init_cache_modes() for the case that commit 99c13b8c8896d7bcb92753bf
> >>> ("x86/mm/pat: Don't report PAT on CPUs that don't support it") failed
> >>> to account for.
> >>>
> >>> This approach arranges for pat_enabled() to return true in the Xen PV
> >>> environment without undermining the rest of PAT MSR management logic
> >>> that considers PAT to be disabled: Specifically, no writes to the PAT
> >>> MSR should occur.
> >>>
> >>> This patch fixes a regression that some users are experiencing with
> >>> Linux as a Xen Dom0 driving particular Intel graphics devices by
> >>> correctly reporting to the Intel i915 driver that PAT is enabled where
> >>> previously it was falsely reporting that PAT is disabled. Some users
> >>> are experiencing system hangs in Xen PV Dom0 and all users on Xen PV
> >>> Dom0 are experiencing reduced graphics performance because 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 are quite a bit
> >>> less performant than possible without this patch.
> >>>
> >>> Also, with the current code, in the Xen PV environment, PAT will not be
> >>> disabled if the administrator sets the "nopat" boot option. Introduce
> >>> a new boolean variable, pat_force_disable, to forcibly disable PAT
> >>> when the administrator sets the "nopat" option to override the default
> >>> behavior of using the PAT configuration that Xen has provided.
> >>>
> >>> 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).
> >>>
> >>> Fixes: 99c13b8c8896d7bcb92753bf ("x86/mm/pat: Don't report PAT on CPUs that don't support it")
> >>
> >> BTW, "submitting-patches.rst" says it should just be "the first 12
> >> characters of the SHA-1 ID"
> > 
> > Actually it says "at least", so more that 12 is It is probably safest
> > to put the entire SHA-1 ID in because of the possibility of
> > a collision. At least that's how I understand what
> > submitting-patches.rst.
> > 
> >>
> >>> Co-developed-by: Jan Beulich <jbeulich@...e.com>
> >>> Cc: stable@...r.kernel.org
> >>> Signed-off-by: Chuck Zmudzinski <brchuckz@....com>
> >>
> >> Sorry, have to ask: is this supposed to finally fix this regression?
> >> https://lore.kernel.org/regressions/YnHK1Z3o99eMXsVK@mail-itl/
> > 
> > Yes that's the first report I saw to lkml about this isssue. So if I submit
> > a v3 I will include that. But my patch does not have a sign-off from the
> > Co-developer as I mentioned in a message earlier today, and the
> > discussion that has ensued leads me to think a better solution is to just
> > revert the commit in the i915 driver instead, and leave the bigger questions
> > for Juergen Gross and his plans to re-work the x86/PAT code in September,
> > as he said on this thread in the last couple of days. This patch is dead
> > now,
> > as far as I can tell, because the Co-developer is not cooperating.
>
> ???
>
> Jan

I think I recall you said my patch proves I don't want your S-o-b. I
also want
to add some useful logs with your approach, probably a pr_warn, which you
are not interested in doing. I think it is probably necessary, under the
current
situation, to warn all users of Xen/Linux that Linux on Xen is not
guaranteed
to be secure and full-featured anymore. That is also why I think the Linux
maintainers are ignoring your patch. They are basically saying Xen is just
some weird one-off thing, I can't remember who said it, but I did read
it here
in some of the comments on your patch, and you do not seem to be listening
to and responding to what the Linux developers are asking you to do.

Two things I see here in my efforts to get a patch to fix this regression:

1. Does Xen have plans to give Linux running in Dom0 write-access to the
PAT MSR?

2. Does Xen have plans to expose MTRRs to Linux running in Dom0?

These don't have to be the defaults for Dom0. But can Xen at least make
these as supported configurations for Linux? Then, the problem of Xen
being some weird one-off thing goes away. At least that's how I see
the situation. Maybe Xen can provide these for Linux in Dom0, but
from what I've read here, it seems Xen is not willing to do that for
Linux. Do I understand that correctly?

Chuck

Powered by blists - more mailing lists