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

[Jan Beulich <jbeulich@suse.com>]

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@suse.com>

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