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. While, as per my earlier patch, I agree with the Xen PV case, I'm not convinced "nopat" is supposed to honor firmware-provided settings. In fact in my patch I did arrange for "nopat" to also take effect under Xen PV. > Add test functions for those caching modes instead and use them at the > appropriate places. > > For symmetry reasons export the already existing x86_has_pat_wp() for > modules, too. > > 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@xxxxxxxx> I think this wants a Reported-by as well. > --- a/arch/x86/include/asm/pci.h > +++ b/arch/x86/include/asm/pci.h > @@ -94,7 +94,7 @@ int pcibios_set_irq_routing(struct pci_dev *dev, int pin, int irq); > > > #define HAVE_PCI_MMAP > -#define arch_can_pci_mmap_wc() pat_enabled() > +#define arch_can_pci_mmap_wc() x86_has_pat_wc() Besides this and ... > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > @@ -76,7 +76,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > if (args->flags & ~(I915_MMAP_WC)) > return -EINVAL; > > - if (args->flags & I915_MMAP_WC && !pat_enabled()) > + if (args->flags & I915_MMAP_WC && !x86_has_pat_wc()) > return -ENODEV; > > obj = i915_gem_object_lookup(file, args->handle); > @@ -757,7 +757,7 @@ i915_gem_dumb_mmap_offset(struct drm_file *file, > > if (HAS_LMEM(to_i915(dev))) > mmap_type = I915_MMAP_TYPE_FIXED; > - else if (pat_enabled()) > + else if (x86_has_pat_wc()) > mmap_type = I915_MMAP_TYPE_WC; > else if (!i915_ggtt_has_aperture(to_gt(i915)->ggtt)) > return -ENODEV; > @@ -813,7 +813,7 @@ i915_gem_mmap_offset_ioctl(struct drm_device *dev, void *data, > break; > > case I915_MMAP_OFFSET_WC: > - if (!pat_enabled()) > + if (!x86_has_pat_wc()) > return -ENODEV; > type = I915_MMAP_TYPE_WC; > break; > @@ -823,7 +823,7 @@ i915_gem_mmap_offset_ioctl(struct drm_device *dev, void *data, > break; > > case I915_MMAP_OFFSET_UC: > - if (!pat_enabled()) > + if (!x86_has_pat_uc_minus()) > return -ENODEV; > type = I915_MMAP_TYPE_UC; > break; ... 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. Jan