Message-ID: <1d06203b-97ff-e7eb-28ae-4cdbc7569218@suse.com>
Date:   Wed, 13 Jul 2022 15:34:16 +0200
From:   Jan Beulich <jbeulich@...e.com>
To:     Chuck Zmudzinski <brchuckz@...scape.net>
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>,
        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>,
        xen-devel@...ts.xenproject.org, stable@...r.kernel.org,
        linux-kernel@...r.kernel.org, Juergen Gross <jgross@...e.com>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Subject: Re: [PATCH v2] Subject: x86/PAT: Report PAT on CPUs that support PAT
 without MTRR

On 13.07.2022 13:10, Chuck Zmudzinski wrote:
> On 7/13/2022 6:36 AM, Chuck Zmudzinski wrote:
>> On 7/13/2022 5:09 AM, Jan Beulich wrote:
>>> On 13.07.2022 10:51, Chuck Zmudzinski wrote:
>>>> On 7/13/22 2:18 AM, Jan Beulich wrote:
>>>>> 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:
>>>>
>>>> Well, I hope we can come to an agreement so I have
>>>> your S-o-b. But that would probably require me to remove
>>>> Juergen's R-b.
>>>>
>>>>>> @@ -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
>>>>
>>>> I thought about checking for the administrator's "nopat"
>>>> setting where you suggest which would limit the effect
>>>> of "nopat" to not reporting PAT as enabled to device
>>>> drivers who query for PAT availability using pat_enabled().
>>>> The main reason I did not do that is that due to the fact
>>>> that we cannot write to the PAT MSR, we cannot really
>>>> disable PAT. But we come closer to respecting the wishes
>>>> of the administrator by configuring the caching modes as
>>>> if PAT is actually disabled by the hardware or firmware
>>>> when in fact it is not.
>>>>
>>>> What would you propose logging as a message when
>>>> we report PAT as disabled via pat_enabled()? The main
>>>> reason I did not choose to check the new variable in the
>>>> new 'else if' block is that I could not figure out what to
>>>> tell the administrator in that case. I think we would have
>>>> to log something like, "nopat is set, but we cannot disable
>>>> PAT, doing our best to disable PAT by not reporting PAT
>>>> as enabled via pat_enabled(), but that does not guarantee
>>>> that kernel drivers and components cannot use PAT if they
>>>> query for PAT support using boot_cpu_has(X86_FEATURE_PAT)
>>>> instead of pat_enabled()." However, I acknowledge WC mappings
>>>> would still be disabled because arch_can_pci_mmap_wc() will
>>>> be false if pat_enabled() is false.
>>>>
>>>> Perhaps we also need to log something if we keep the
>>>> check for "nopat" where I placed it. We could say something
>>>> like: "nopat is set, but we cannot disable hardware/firmware
>>>> PAT support, so we are emulating as if there is no PAT support
>>>> which puts in place a software view that does not match
>>>> hardware."
>>>>
>>>> No matter what, because we cannot write to PAT MSR in
>>>> the Xen PV case, we probably need to log something to
>>>> explain the problems associated with trying to honor the
>>>> administrator's request. Also, what log level should it be.
>>>> Should it be a pr_warn instead of a pr_info?
>>>
>>> I'm afraid I'm the wrong one to answer logging questions. As you
>>> can see from my original patch, I didn't add any new logging (and
>>> no addition was requested in the comments that I have got). I also
>>> don't think "nopat" has ever meant "disable PAT", as the feature
>>> is either there or not. Instead I think it was always seen as
>>> "disable fiddling with PAT", which by implication means using
>>> whatever is there (if the feature / MSR itself is available).
>>
>> IIRC, I do think I mentioned in the comments on your patch that
>> it would be preferable to mention in the commit message that
>> your patch would change the current behavior of "nopat" on
>> Xen. The question is, how much do we want to change the
>> current behavior of "nopat" on Xen. I think if we have to change
>> the current behavior of "nopat" on Xen and if we are going
>> to propagate that change to all current stable branches all
>> the way back to 4.9.y,, we better make a lot of noise about
>> what we are doing here.
>>
>> Chuck
> 
> And in addition, if we are going to backport this patch to
> all current stable branches, we better have a really, really,
> good reason for changing the behavior of "nopat" on Xen.
> 
> Does such a reason exist?

Well, the simple reason is: It doesn't work the same way under Xen
and non-Xen (in turn because, before my patch or whatever equivalent
work, things don't work properly anyway, PAT-wise). Yet it definitely
ought to behave the same everywhere, imo.

Jan

> Or perhaps, Juergen, could you
> accept a v3 of my patch that does not need to decide
> how to backport the change to "nopat" to the stable branches
> that are affected by the current behavior of "nopat" on Xen?
> 
> To do such a v3, I would just have to fix the style problems
> with my original patch and not come to an agreement with
> Jan about how to deal with the "nopat" problem.
> 
> Chuck
> 
> Chuck

Powered by blists - more mailing lists