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