Re: [PATCH v3 i-g-t] lib/intel_pat: Cache PAT config for unprivileged processes

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Thu, 05 Mar 2026 08:02:46 -0800, <himanshu.girotra@intel.com> wrote:
>
> From: Himanshu Girotra <himanshu.girotra@intel.com>
>
> Commit 4e59b8e7779b ("lib/intel_pat: use kernel debugfs as
> authoritative PAT source for Xe") changed the Xe PAT lookup to
> read from debugfs, which requires root access. Some xe_oa subtests
> fork a child process that drops root privileges before performing
> GPU operations that internally need the PAT write-back index.
> After dropping root, the debugfs read fails.
>
> Fix this by caching the PAT configuration in struct xe_device at
> xe_device_get() time. Since xe_device_get() is called while still
> root, the cache is available to forked children even after
> igt_drop_root(). Tests that open a new fd after forking must call
> xe_device_get() explicitly before dropping root to populate the
> cache for that fd.
>
> v2: Cache PAT in struct xe_device instead of a global static;
>     drop the intel_pat_precache() helper — xe_device_get() already
>     populates the cache, so call it directly in xe_oa before
>     igt_drop_root() (Ashutosh Dixit)
>
> v3: Use pat_cache pointer, NULL indicates unpopulated cache;
>     remove redundant xe_oa.c change (Ashutosh Dixit)
>
>     Add FIXME for multi-GPU (Zbigniew Kempczyński)
>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Xin Wang <x.wang@intel.com>
> Cc: Nishit Sharma <nishit.sharma@intel.com>
> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
>
> Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Signed-off-by: Himanshu Girotra <himanshu.girotra@intel.com>
> ---
>  lib/intel_pat.c   | 18 ++++++++++--------
>  lib/xe/xe_query.c | 20 ++++++++++++++++++++
>  lib/xe/xe_query.h |  5 +++++
>  3 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/lib/intel_pat.c b/lib/intel_pat.c
> index 8660a2515..e758ce9be 100644
> --- a/lib/intel_pat.c
> +++ b/lib/intel_pat.c
> @@ -6,6 +6,7 @@
>  #include <fcntl.h>
>  #include "igt.h"
>  #include "intel_pat.h"
> +#include "xe/xe_query.h"
>
>  /**
>   * xe_get_pat_sw_config - Helper to read PAT (Page Attribute Table) software configuration
> @@ -99,17 +100,18 @@ static void intel_get_pat_idx(int fd, struct intel_pat_cache *pat)
>	uint16_t dev_id;
>
>	/*
> -	 * For Xe driver, query the kernel's PAT software configuration
> -	 * via debugfs. The kernel is the authoritative source for PAT
> -	 * indices, accounting for platform-specific workarounds
> -	 * (e.g. Wa_16023588340) at runtime.
> +	 * For Xe, use the PAT cache stored in struct xe_device.
> +	 * xe_device_get() populates the cache while still root; forked
> +	 * children that inherit the xe_device can use it post-drop_root().
>	 */
>	if (is_xe_device(fd)) {
> -		int32_t parsed = xe_get_pat_sw_config(fd, pat);
> +		struct xe_device *xe_dev = xe_device_get(fd);
>
> -		igt_assert_f(parsed > 0,
> -			     "Failed to get PAT sw_config from debugfs (parsed=%d)\n",
> -			     parsed);
> +		igt_assert_f(xe_dev->pat_cache,
> +			     "PAT sw_config not available for fd %d -- "
> +			     "debugfs not accessible (missing root or not mounted?)\n",
> +			     fd);

Why print fd here? What information is it providing? Do we need it?


> +		*pat = *xe_dev->pat_cache;
>		return;
>	}
>
> diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
> index 981d76948..ea8889f3b 100644
> --- a/lib/xe/xe_query.c
> +++ b/lib/xe/xe_query.c
> @@ -22,6 +22,7 @@
>  #include "ioctl_wrappers.h"
>  #include "igt_map.h"
>
> +#include "intel_pat.h"

nit but let's move this just below igt_map.h (i.e. move up by one line
without a blank line) so that the xe includes below stay separate.

>  #include "xe_query.h"
>  #include "xe_ioctl.h"
>
> @@ -235,6 +236,7 @@ static void xe_device_free(struct xe_device *xe_dev)
>	free(xe_dev->mem_regions);
>	free(xe_dev->vram_size);
>	free(xe_dev->eu_stall);
> +	free(xe_dev->pat_cache);
>	free(xe_dev);
>  }
>
> @@ -299,6 +301,24 @@ struct xe_device *xe_device_get(int fd)
>	xe_dev->default_alignment = __mem_default_alignment(xe_dev->mem_regions);
>	xe_dev->has_vram = __mem_has_vram(xe_dev->mem_regions);
>
> +	/*
> +	 * Populate the PAT cache while we still have sufficient privileges
> +	 * to read debugfs.  Forked children that inherit this xe_device
> +	 * (via fork()) will be able to use the cached values even after
> +	 * dropping root with igt_drop_root(). pat_cache is left NULL if
> +	 * debugfs is not accessible.
> +	 *
> +	 * FIXME: the cache is keyed by fd; for multi-GPU support this
> +	 * should be extended to cache PAT entries by platform version/
> +	 * revision instead.
> +	 */
> +	xe_dev->pat_cache = calloc(1, sizeof(*xe_dev->pat_cache));
> +	igt_assert(xe_dev->pat_cache);
> +	if (xe_get_pat_sw_config(xe_dev->fd, xe_dev->pat_cache) <= 0) {
> +		free(xe_dev->pat_cache);
> +		xe_dev->pat_cache = NULL;
> +	}
> +
>	/* We may get here from multiple threads, use first cached xe_dev */
>	pthread_mutex_lock(&cache.cache_mutex);
>	prev = find_in_cache_unlocked(fd);
> diff --git a/lib/xe/xe_query.h b/lib/xe/xe_query.h
> index d7a9f95f9..53a32e4b1 100644
> --- a/lib/xe/xe_query.h
> +++ b/lib/xe/xe_query.h
> @@ -9,6 +9,8 @@
>  #ifndef XE_QUERY_H
>  #define XE_QUERY_H
>
> +#include "intel_pat.h"
> +

Why include this before the <> includes below. Please include this if it is
needed below with the "" includes in alphabetical order.

You likely don't even need the include, you can just use the following
forward struct declaration:

struct intel_pat_cache;

That's it. Because we have a struct pointer. Basically the idea for a
forward declaration is to not create a potential circular dependency in the
include files.

Put the forward declaration below all the includes.

>  #include <stdint.h>
>  #include <xe_drm.h>
>
> @@ -74,6 +76,9 @@ struct xe_device {
>
>	/** @dev_id: Device id of xe device */
>	uint16_t dev_id;
> +
> +	/** @pat_cache: cached PAT index configuration, NULL if not yet populated */
> +	struct intel_pat_cache *pat_cache;
>  };
>
>  #define xe_for_each_engine(__fd, __hwe) \
> --
> 2.50.1
>

I'll take another look at the next version and R-b it everything is ok.

Thanks.
--
Ashutosh