On 20.05.2022 06:43, Chuck Zmudzinski wrote:
On 5/4/22 5:14 AM, Juergen Gross wrote:Hmm, why would the kernel not be allowed to override that? Such
On 04.05.22 10:31, Jan Beulich wrote:That is why your patch would not fix my Haswell unless
On 03.05.2022 15:22, Juergen Gross wrote:Oh, I missed that one, sorry.
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@xxxxxxxx>
--- a/arch/x86/include/asm/pci.hBesides this and ...
+++ 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()
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c... these uses there are several more. You say nothing on why those want
+++ 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;
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.
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 theI think your approach needs to be more aggressive so it will fix
case in arch_phys_wc_add() might want to be changed, too.
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,
an override would affect only the single domain where the
kernel runs; other domains could take their own decisions.
Also, for the sake of completeness: "nopat" used when running on
bare metal has the same bad effect on system boot, so there
pretty clearly is an error cleanup issue in the i915 driver. But
that's orthogonal, and I expect the maintainers may not even care
(but tell us "don't do that then").
Jan
but because of the confusion,