Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability

[Chuck Zmudzinski <brchuckz@netscape.net>]

On 5/20/22 12:43 AM, Chuck Zmudzinski wrote:
> On 5/4/22 5:14 AM, Juergen Gross wrote:
>> On 04.05.22 10:31, Jan Beulich wrote:
>>> On 03.05.2022 15:22, Juergen Gross wrote:
>>>> Some drivers are using pat_enabled() in order to test availability of
>>>> special caching modes (WC and UC-). This will lead to false negatives
>>>> in case the system was booted e.g. with the "nopat" variant and the
>>>> BIOS did setup the PAT MSR supporting the queried mode, or if the
>>>> system is running as a Xen PV guest.
>>> ...
>>>> Add test functions for those caching modes instead and use them at the
>>>> appropriate places.
>>>>
>>>> Fixes: bdd8b6c98239 ("drm/i915: replace X86_FEATURE_PAT with 
>>>> pat_enabled()")
>>>> Fixes: ae749c7ab475 ("PCI: Add arch_can_pci_mmap_wc() macro")
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ...
>>>
>>>
>>> ... these uses there are several more. You say nothing on why those 
>>> want
>>> leaving unaltered. When preparing my earlier patch I did inspect them
>>> and came to the conclusion that these all would also better observe the
>>> adjusted behavior (or else I couldn't have left pat_enabled() as the 
>>> only
>>> predicate). In fact, as said in the description of my earlier patch, in
>>> my debugging I did find the use in i915_gem_object_pin_map() to be the
>>> problematic one, which you leave alone.
>>
>> Oh, I missed that one, sorry.
>
> That is why your patch would not fix my Haswell unless
> it also touches i915_gem_object_pin_map() in
> drivers/gpu/drm/i915/gem/i915_gem_pages.c
>
>>
>> I wanted to be rather defensive in my changes, but I agree at least the
>> case in arch_phys_wc_add() might want to be changed, too.
>
> I think your approach needs to be more aggressive so it will fix
> all the known false negatives introduced by bdd8b6c98239
> such as the one in i915_gem_object_pin_map().
>
> I looked at Jan's approach and I think it would fix the issue
> with my Haswell as long as I don't use the nopat option. I
> really don't have a strong opinion on that question, but I
> think the nopat option as a Linux kernel option, as opposed
> to a hypervisor option, should only affect the kernel, and
> if the hypervisor provides the pat feature, then the kernel
> should not override that, but because of the confusion,

The confusion is: does "nopat" only mean the kernel does not
provide pat to device drivers, or does it mean kernel drivers
are not to use pat even if the hypervisor provides it?
I think the original purpose of bdd8b6c98239 was to
enable "nopat" to disable the use or pat in the i915 driver
even if the feature is present from either the kernel or the
hypervisor. This interpretation of the meaning of "nopat"
would favor Jan's approach, I think.

Chuck