Re: [PATCH 12/23] drm/xe/xe3p: Flush userptr/shrinker bo cachelines manually

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

Hi,

On Mon, 2025-10-13 at 20:24 -0700, Lucas De Marchi wrote:
> From: Tejas Upadhyay <tejas.upadhyay@intel.com>
> 
> Starting with Xe3p, HW will flush cachelines marked with XA only when
> media is off. We have few cases where kernel will have non-XA
> cachelines
> which needs manual flush as we postpone the invalidation.

is XA- and non-XA cachelines described somewhere in the code?

> 
> Flush asap from correctness POV to ensure non accelerated CPU copy to
> swap/shmem file will see coherent view of memory, but also from
> security
> POV where later flush can't corrupt the next user of those pages.
> 
> Signed-off-by: Tejas Upadhyay <tejas.upadhyay@intel.com>
> [ TODO: xe_device_needs_cache_flush() seems a bad name that doesn't
>   really review the context - it may need to be renamed/localized ]
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

This worries me. Any dma-fence *must*, when signaled, have flushed all
caches from data resulting from the operation it marks the completion
of. The code below indicates that's not the case?

Note also that the wait for idle in xe_bo_trigger_rebind does not
guarantee that the bo is idle, just that there are no pending bind
operations.

In fault mode we have a slightly different approach to ensure data is
flushed before pages are released.

Could you elaborate a bit on how this is intendend to work.

Thanks,
Thomas


> ---
>  drivers/gpu/drm/xe/xe_bo.c      |  3 ++-
>  drivers/gpu/drm/xe/xe_device.c  | 20 ++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_device.h  |  1 +
>  drivers/gpu/drm/xe/xe_userptr.c |  3 ++-
>  4 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 7b65020818738..05bc61d9e37cf 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -673,7 +673,8 @@ static int xe_bo_trigger_rebind(struct xe_device
> *xe, struct xe_bo *bo,
>  
>  		if (!xe_vm_in_fault_mode(vm)) {
>  			drm_gpuvm_bo_evict(vm_bo, true);
> -			continue;
> +			if (!xe_device_needs_cache_flush(xe))
> +				continue;
>  		}
>  
>  		if (!idle) {
> diff --git a/drivers/gpu/drm/xe/xe_device.c
> b/drivers/gpu/drm/xe/xe_device.c
> index 7efa8da9e1069..168a45fe36838 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -1081,6 +1081,26 @@ void xe_device_l2_flush(struct xe_device *xe)
>  	xe_force_wake_put(gt_to_fw(gt), fw_ref);
>  }
>  
> +/**
> + * xe_device_needs_cache_flush - Whether the cache needs to be
> flushed
> + * @xe: The device to check.
> + *
> + * Return: true if the device needs cache flush, false otherwise.
> + */
> +bool xe_device_needs_cache_flush(struct xe_device *xe)
> +{
> +	/*
> +	 * Xe3p will flush cachelines marked with XA only when media
> is off. We
> +	 * have few cases where kernel will have non-XA cachelines
> which needs
> +	 * manual flush and this is one of them as we postpone the
> +	 * invalidation. Flush asap from correctness POV to ensure
> non
> +	 * accelerated CPU copy to swap/shmem file will see coherent
> view of
> +	 * memory, but also from security POV where later flush
> can't corrupt
> +	 * the next user of those pages.
> +	 */
> +	return GRAPHICS_VER(xe) >= 35 && !IS_DGFX(xe);
> +}
> +
>  /**
>   * xe_device_td_flush() - Flush transient L3 cache entries
>   * @xe: The device
> diff --git a/drivers/gpu/drm/xe/xe_device.h
> b/drivers/gpu/drm/xe/xe_device.h
> index 32cc6323b7f64..15e67db44b56c 100644
> --- a/drivers/gpu/drm/xe/xe_device.h
> +++ b/drivers/gpu/drm/xe/xe_device.h
> @@ -179,6 +179,7 @@ void xe_device_snapshot_print(struct xe_device
> *xe, struct drm_printer *p);
>  u64 xe_device_canonicalize_addr(struct xe_device *xe, u64 address);
>  u64 xe_device_uncanonicalize_addr(struct xe_device *xe, u64
> address);
>  
> +bool xe_device_needs_cache_flush(struct xe_device *xe);
>  void xe_device_td_flush(struct xe_device *xe);
>  void xe_device_l2_flush(struct xe_device *xe);
>  
> diff --git a/drivers/gpu/drm/xe/xe_userptr.c
> b/drivers/gpu/drm/xe/xe_userptr.c
> index f16e92cd80904..86ce1c3ef41aa 100644
> --- a/drivers/gpu/drm/xe/xe_userptr.c
> +++ b/drivers/gpu/drm/xe/xe_userptr.c
> @@ -112,7 +112,8 @@ static void __vma_userptr_invalidate(struct xe_vm
> *vm, struct xe_userptr_vma *uv
>  				    false, MAX_SCHEDULE_TIMEOUT);
>  	XE_WARN_ON(err <= 0);
>  
> -	if (xe_vm_in_fault_mode(vm) && userptr->initial_bind) {
> +	if ((xe_vm_in_fault_mode(vm) ||
> xe_device_needs_cache_flush(vm->xe)) &&
> +	    userptr->initial_bind) {
>  		err = xe_vm_invalidate_vma(vma);
>  		XE_WARN_ON(err);
>  	}
>