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

From: Juergen Gross
Date: Wed May 04 2022 - 05:14:13 EST

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.

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.

Depends on what the wanted semantics for "nopat" are.

Right now "nopat" will result in the PAT MSR left unchanged and the
cache mode translation tables be initialized accordingly.

So does "nopat" mean that the PAT MSR shouldn't be changed, or that
PAGE_BIT_PAT will never be set?

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.

Okay.


--- 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.

Oh, I missed that one, sorry.

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.

kvm_is_mmio_pfn() should not really matter at least for the Xen case.

With the other use cases in memtype.c I'm rather on the edge.

In case the x86 maintainers think those should be changed, too, I agree
that your approach might be the better one.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature