[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Juergen Gross <jgross@xxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 4 May 2022 10:31:47 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=uQfWNe1RxVIVhGw370YZzsDPbGmRCJ6APtQ7SkS5ucE=; b=O/MdNjN/vdEiSXOOv5aLaCtKv24zDcfASUE87VxaXEgPQKkbLQvP3Vv8mzBcP+UPSkGYiMSfawR2oP9hdRouktrHCaJjbMoFw7NLD301yyAi6491YzglTUj7Vs9f3UUu8Vu07gMAm6AAIroem+1EURPb63bd+afJDuf4ah/XP0KhZCJD0cCxBfIE9R87T5Or7YUVtAJeZZxxbYnWTRHqNQK0o87njqmybuWh3Tb1fe3fbY1WdOd3ue6N8NLs/39mRmVoR2tMRB3cra0mdkwohQlau0ZBp4Vq1bo/RLlVJkMkGNk4Ut2SqS+x26oFZATs8JPye6R+ii69xpG+p78r4w==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=J/TcqT3FmKHLvjBoB2dTAf6qSEpSlXxnWt72lZgDZiAkI9tYIVrN/TMZ97c4y/V8yI78LutDPzRWxcmg1UJrgz9KA0VRaTkZFLFW0RNv83UrNA/oW59EGQRvDoy16Mdyh1VlRZeeijbPKMW3LAfX2jXocIGzzFWIaF8yrVWhgNdEUkP1Hjm7xtuQyASavG+hMlPcSml8DTlPrUWgZBEdWPF4WhzKdnRK679jX3rpCDgOy3LDFefGuhONH43yATAH2+3s0WMz83mvZRpn3j9U6HPs2J+1OK2Vht86oeQTX3STNcvMm7lTncPA3eB7iROCONua4/pv6WEC0CUv2l2ALQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>, Ingo Molnar <mingo@xxxxxxxxxx>, Borislav Petkov <bp@xxxxxxxxx>, Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>, "H. Peter Anvin" <hpa@xxxxxxxxx>, Andy Lutomirski <luto@xxxxxxxxxx>, Peter Zijlstra <peterz@xxxxxxxxxxxxx>, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>, Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>, David Airlie <airlied@xxxxxxxx>, Daniel Vetter <daniel@xxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, x86@xxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, intel-gfx@xxxxxxxxxxxxxxxxxxxxx, dri-devel@xxxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Wed, 04 May 2022 08:32:04 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.