Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1d86d8ff-6878-5488-e8c4-cbe8a5e8f624@suse.com>
Date:   Wed, 4 May 2022 10:31:47 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Subject: Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode
 availability
Content-Language: en-US
To:     Juergen Gross <jgross@suse.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Andy Lutomirski <luto@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        xen-devel@lists.xenproject.org, x86@kernel.org,
        linux-kernel@vger.kernel.org, intel-gfx@lists.freedesktop.org,
        dri-devel@lists.freedesktop.org
References: <20220503132207.17234-1-jgross@suse.com>
 <20220503132207.17234-3-jgross@suse.com>
From:   Jan Beulich <jbeulich@suse.com>
In-Reply-To: <20220503132207.17234-3-jgross@suse.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?c1Z6S2RLYmZscWpIYTV3SzlMZ0RDVTl1R3VKTlA3cEUvQ2pYQkJuS3k3SzBm?=
 =?utf-8?B?endydmJ2Zm5sRnNSbnhYL282dU5ZV200czdBVjJBcUJkY29vd1ZQSG5TMUE0?=
 =?utf-8?B?T3M2OWx5ZXpEREtrTEtqSkFhc0pyK1NNcktVZ0JLbTBqalI2enYvQSszNkNm?=
 =?utf-8?B?UzNwd00wM0xJQ0FQQXhhOFowZmljTXE4MjBaRTJ2WW1XV29NUTN4c3IwcWd6?=
 =?utf-8?B?MXJYdEFtZTN3UExRTHFzRnl5TUtIcnYvUjd3czYvejVvTFhRay9xK3I3QkM3?=
 =?utf-8?B?Q042WWlSc2toUUJNSmp0YTRpZ2NNVFdVc0tXQ0YyaFdSNkNLSHdpUzFDTVdC?=
 =?utf-8?B?RDdGZjA5TG1hK2JBRWd2NFBXVWJjc1dxVStBRDkwcVlnQzE2YzZvSVZvTTY4?=
 =?utf-8?B?QWFmYTdVbHM4UzBSenFiTjBQSnBJZFZyMW14WDdkMkdoUDFtS3JINFZWN3lP?=
 =?utf-8?B?ODhVc2ltLzlwaHhQTDRDa01LTzNPYUNtK3NFZG9TWEtMcjRRK0hkV1h0Tkhr?=
 =?utf-8?B?THN4NUYrM3EyYmt3RFRicExRRVRlQzNjR29zbC94VHRXZGFIaUlLZzBKb0lk?=
 =?utf-8?B?ZGw2akpJRFB2YlZsajd6M0pjM3ZSdmJtWkt6RExGMi9adjh5SHNIU3NkbkNG?=
 =?utf-8?B?c09SZ3g0ZGdwdEI0YlRHbG9hNnJhM1VveHpvVjFXT05YSFJWc1BhZkNCaUFV?=
 =?utf-8?B?TUNtc3dTS2FBMkU0OEtHdnVGbHdIRTBVOWpoeVN6QjEvZlgwSzhJTnp6NEdk?=
 =?utf-8?B?T3k0bVh0aW5TVzZ3OFVsZWt2VXIxejROQWo5cXpVVEQ4bU5tNHhPcDBOcmk5?=
 =?utf-8?B?ZlJKamJoUlpmb0psSEZmTmY2ekdhYzV0NG5qMmJwY3R6M0pnWUIwM0tGVVJl?=
 =?utf-8?B?RU5mYmROeXp1dnlXUVpCeGppWTcxQWpGYUdWd1dhczJQSllSNTR4ZlU1RmlN?=
 =?utf-8?B?Rjh0ZklkZTJXT0hNUUFPWXVZWDRJR1lBb2x2YWlKZUNZRTF6dkt1emlCcmcy?=
 =?utf-8?B?bmltenQyMXFTQ29rc09vZnRsdFRvWG11U1lWZDBoNU1RUVBBMC9BMmlhRWh0?=
 =?utf-8?B?M0lQU2k5b0U2RmF4bEx1Z3pxVGd0WnQwTmJ3OXFFbEdML0V2akQ4dUs0cW05?=
 =?utf-8?B?WWtGcXE0NGdCbjVtV3NUNHBEZG80ZlZUeFQ5dC9odXo2bmZ3K0ZobUhFeEN1?=
 =?utf-8?B?Vjl0cnVyL0xNRFVudTFWbkNUY2c0REUydVF0SS8wTFN5cXAwVGlMazRBUyt3?=
 =?utf-8?B?Z05lUDVxRU14dGZoZDJ3RG5sdmpLWUYremlWeFNNZ2IwRWtsRFlRREs0WmJD?=
 =?utf-8?B?a3NMSkM4elIxSjMzdlBpb3lBU3VmazVDc3pLQTU2SkZCaGhUYXEzWjYrdUFr?=
 =?utf-8?B?ei9yaUFpc0ErKzR3L0FGUERyVlNybzRrQ3RFNWpkVFdYSXhScjJxWURpZ3Jv?=
 =?utf-8?B?U1lxWC80azJpU1E3KzQ0L1BSazVCWDgrWlpTM01rMDV5R3Eyd3JBQmlTazVm?=
 =?utf-8?B?S3p6TzluYlhETTFRN1ZDU0JqcTc3L3NtYktNbW9jZmNFUDJPN3BtY1YxVkNW?=
 =?utf-8?B?Tkh5aW1yM1JaWGp4cUVLanFvMERhd1NHL2NhSHhTWmlJMzIwUCs3clBWUlRa?=
 =?utf-8?B?aEorUlNqbkZ1S3JCZ0xOc0haK0hPSitmRzhHTVpBQ3RlcE9wYU5UWnQ5Wkha?=
 =?utf-8?B?K05WRys3VzhRQUxHU1RTaVFzWkRPSElzSmZLOGlrYXVuWWtVNXExL1BVVks1?=
 =?utf-8?B?emZSdG1jUHlMeUFyeHBzUHlDcllHV3loMzJiS0tzOWZKbmRLVjZYNU5uUVkr?=
 =?utf-8?B?V2hTQ0c3T1loVzRGbFlJa3Z4L2ZlMWRnQVhpc1VrYmNkMm15Z08vYjYrREFy?=
 =?utf-8?B?cGU5U0lacTJlbElxSXM5K3JjSnQwdzk4Ri9HbGJDYTJlbmI3clI0Y3N4UVQr?=
 =?utf-8?B?MS9waUJpRTQ3ZVBWcWhqcld0UDJ2bmtlOGYvK2lEcmhzUEtIM091RzZzWHd6?=
 =?utf-8?B?a1BTNDZWemdNTU1jZnNDMzVncmgyU3l2MGRUN1YyM1Y1NGZNK2I1VkU5YkxI?=
 =?utf-8?B?c29DRWdYZXUwYmpTWlA3V0pqL081bHJlbGZDSXVGUDFlajZZL2VJSjNtcWY0?=
 =?utf-8?B?UHM3SDFPUHVCMlllcmdBMXFUMDlVTFc2dml6S2l4RmpZQVpzL3VoZE9lNHBO?=
 =?utf-8?B?RFRGNHF0YmJhclZSWUlISmdXQno5QmNpSWVNNUpzcWNNM3c3bHY3QVVhUlJj?=
 =?utf-8?B?WGhSeitqYTBiREJmWUFJaThMSkFIcVFBdzdqQ0lEWStEUmpKTVpVT0tobEIr?=
 =?utf-8?B?b1JHd1JiMktNVEhVdkJtRW9ZQjA3RklUeGdqUmpkWEJDZlpXTUxRUT09?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d493a38e-8de6-4ab7-a9ef-08da2da8884e
X-MS-Exchange-CrossTenant-AuthSource: DU2PR04MB8616.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 May 2022 08:31:49.2208
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: AhtRaIIfvZAc7TwDIZ0GnLdy5KNKjJI1rFZ3z2ZgAfSMQzvSpvcQBWpZmqGIssObKPMWhzvHPKsiXLbV90Ay3g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB5437
X-Spam-Status: No, score=-5.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.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@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