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
