Re: [PATCH 2/3] drm/xe/tlb_inval: add xe_tlb_inval_ggtt_full() GGTT invalidation helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] send_tlb_inval_ggtt_full() converts all transmission errors to -ECANCELED, which are silently ignored by the caller, causing TLB invalidation failures to be treated as success.
--

commit 1e4a83a630897165bf2a8c3138c9949149a7404b
Author: Tangudu Tilak Tirumalesh <tilak.tirumalesh.tangudu@intel.com>

drm/xe/tlb_inval: add xe_tlb_inval_ggtt_full() GGTT invalidation helper

This commit introduces the xe_tlb_inval_ggtt_full() helper and its GuC
backend send_tlb_inval_ggtt_full(). It implements full engine TLB
invalidation within the requesting VF across all engines.

> diff --git a/drivers/gpu/drm/xe/xe_guc_tlb_inval.c b/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> index 046d0655122f1..c2f56be833a29 100644
> --- a/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> +++ b/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
[ ... ]
> @@ -98,6 +98,25 @@ static int send_tlb_inval_ggtt(struct xe_tlb_inval *tlb_inval, u32 seqno)
>  	return -ECANCELED;
>  }
>  
> +/*
> + * Emit INVAL_FULL (intra vf) to invalidate engine TLBs across all engines
> + * within the VF.
> + */
> +static int send_tlb_inval_ggtt_full(struct xe_tlb_inval *tlb_inval, u32 seqno)
> +{
> +	struct xe_guc *guc = tlb_inval->private;
> +	u32 action[] = {
> +		XE_GUC_ACTION_TLB_INVALIDATION,
> +		seqno,
> +		MAKE_INVAL_OP(XE_GUC_TLB_INVAL_FULL),
> +	};
> +
> +	if (send_tlb_inval(guc, action, ARRAY_SIZE(action)))
> +		return -ECANCELED;

[Severity: High]
Does squashing all transmission errors from send_tlb_inval() into -ECANCELED
mask failures?

If send_tlb_inval() fails (e.g., returning -EBUSY when the CT buffer is full
under heavy load), this returns -ECANCELED. The pre-existing macro
xe_tlb_inval_issue() translates -ECANCELED into a synchronous success (0)
and signals the fence, hiding the failure from upper layers:

    __ret = op((__tlb_inval), (__fence)->seqno, ##args);
    ...
    __ret == -ECANCELED ? 0 : __ret;

This could cause the hardware to proceed with stale translations in the TLB,
potentially leading to memory corruption or use-after-free if the memory has
been reallocated.

> +
> +	return 0;
> +}
[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_tlb_inval_types.h b/drivers/gpu/drm/xe/xe_tlb_inval_types.h
> index 3d1797d186fd7..d77be1aedc918 100644
> --- a/drivers/gpu/drm/xe/xe_tlb_inval_types.h
> +++ b/drivers/gpu/drm/xe/xe_tlb_inval_types.h
[ ... ]
> @@ -36,6 +36,16 @@ struct xe_tlb_inval_ops {
>  	 */
>  	int (*ggtt)(struct xe_tlb_inval *tlb_inval, u32 seqno);
>  
> +	/**
> +	 * @ggtt_full: Full engine TLB invalidation within a VF
> +	 * @tlb_inval: TLB invalidation client
> +	 * @seqno: Seqno of TLB invalidation
> +	 *
> +	 * Return 0 on success, -ECANCELED if backend is mid-reset, error on
> +	 * failure
> +	 */
> +	int (*ggtt_full)(struct xe_tlb_inval *tlb_inval, u32 seqno);

[Severity: High]
Could we pass the actual error code back instead? The documentation here
specifies the contract expects "error on failure", but the implementation in
send_tlb_inval_ggtt_full() unconditionally returns -ECANCELED instead of the
real error from send_tlb_inval(), which violates this contract.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826103156.3233208-1-tilak.tirumalesh.tangudu@intel.com?part=2