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
