Re: [PATCH v2] Subject: x86/PAT: Report PAT on CPUs that support PAT without MTRR

From: Jan Beulich <jbeulich@suse.com>
To: Chuck Zmudzinski <brchuckz@aol.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>,
	Andy Lutomirski <luto@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	Dan Williams <dan.j.williams@intel.com>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	Jane Chu <jane.chu@oracle.com>,
	Tianyu Lan <Tianyu.Lan@microsoft.com>,
	Randy Dunlap <rdunlap@infradead.org>,
	Sean Christopherson <seanjc@google.com>,
	Juergen Gross <jgross@suse.com>,
	xen-devel@lists.xenproject.org, stable@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] Subject: x86/PAT: Report PAT on CPUs that support PAT without MTRR
Date: Wed, 13 Jul 2022 08:18:05 +0200	[thread overview]
Message-ID: <e0faeb99-6c32-a836-3f6b-269318a6b5a6@suse.com> (raw)
In-Reply-To: <9d5070ae4f3e956a95d3f50e24f1a93488b9ff52.1657671676.git.brchuckz@aol.com>

On 13.07.2022 03:36, Chuck Zmudzinski wrote:
> v2: *Add force_pat_disabled variable to fix "nopat" on Xen PV (Jan Beulich)
>     *Add the necessary code to incorporate the "nopat" fix
>     *void init_cache_modes(void) -> void __init init_cache_modes(void)
>     *Add Jan Beulich as Co-developer (Jan has not signed off yet)
>     *Expand the commit message to include relevant parts of the commit
>      message of Jan Beulich's proposed patch for this problem
>     *Fix 'else if ... {' placement and indentation
>     *Remove indication the backport to stable branches is only back to 5.17.y
> 
> I think these changes address all the comments on the original patch
> 
> I added Jan Beulich as a Co-developer because Juergen Gross asked me to
> include Jan's idea for fixing "nopat" that was missing from the first
> version of the patch.

You've sufficiently altered this change to clearly no longer want my
S-o-b; unfortunately in fact I think you broke things:

> @@ -292,7 +294,7 @@ void init_cache_modes(void)
>  		rdmsrl(MSR_IA32_CR_PAT, pat);
>  	}
>  
> -	if (!pat) {
> +	if (!pat || pat_force_disabled) {

By checking the new variable here ...

>  		/*
>  		 * No PAT. Emulate the PAT table that corresponds to the two
>  		 * cache bits, PWT (Write Through) and PCD (Cache Disable).
> @@ -313,6 +315,16 @@ void init_cache_modes(void)
>  		 */
>  		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);

... you put in place a software view which doesn't match hardware. I
continue to think that ...

> +	} else if (!pat_bp_enabled) {

... the variable wants checking here instead (at which point, yes,
this comes quite close to simply being a v2 of my original patch).

By using !pat_bp_enabled here you actually broaden where the change
would take effect. Iirc Boris had asked to narrow things (besides
voicing opposition to this approach altogether). Even without that
request I wonder whether you aren't going to far with this.

Jan