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

[Juergen Gross <jgross@xxxxxxxx>]

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