Re: [CI-only] drm/xe: Isolate permanently wedged devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/xe: CPU mmaps to VRAM are not invalidated during PCI error recovery
- [High] drm/xe: MMIO write to disabled PCI device during AER recovery
- [Medium] drm/xe: Leak of drm_exec lockdep context when xe_device_io_get() fails in preempt_rebind_work_func()
--

commit 757d33b97cc3a56989d49ecaab9b08dca7015ebb
Author: Arvind Yadav <arvind.yadav@intel.com>

drm/xe: Isolate permanently wedged devices

This commit isolates permanently wedged devices by shutting down display,
suspending IRQs, invalidating CPU mmaps, and blocking IOCTLs. It also
introduces AER error handling that temporarily blocks device access during
PCI reset.

> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 9ef2ce743770f..5a17cfe8f883f 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c

[ ... ]

> @@ -496,6 +497,27 @@ static void ggtt_node_remove(struct xe_ggtt_node *node)
>  	ggtt_node_fini(node);
>  }
>  
> +static void ggtt_node_remove(struct xe_ggtt_node *node)
> +{
> +	struct xe_device *xe = tile_to_xe(node->ggtt->tile);
> +	int idx;
> +
> +	/*
> +	 * GGTT updates write page table entries through the BAR and request a
> +	 * TLB invalidation. Skip them after unplug, where the BAR is gone.
> +	 * Keep them during PCI error recovery so a recovered device does not
> +	 * inherit stale GGTT entries.
> +	 */
> +	if (!drm_dev_enter(&xe->drm, &idx)) {
> +		__ggtt_node_remove(node, false);
> +		return;
> +	}
> +
> +	__ggtt_node_remove(node, !xe_device_wedged(xe));

[Severity: High]
Does this code allow MMIO writes to a disabled PCI device during AER
recovery?

During AER recovery, prepare_device_for_reset() disables the PCI device
using pci_disable_device(). However, ggtt_node_remove() does not use
xe_device_io_get() to check if IO is blocked. Instead, it checks
!xe_device_wedged(xe), which evaluates to true because the device is in
reset, not wedged.

Consequently, concurrent BO destruction or the delayed removal workqueue
can call __ggtt_node_remove(node, true) asynchronously, which performs MMIO
writes to the GGTT and TLB registers while the PCI device memory decoding is
disabled. This could cause PCIe Unsupported Requests (UR), leading to system
crashes or secondary AER cascades.

> +
> +	drm_dev_exit(idx);
> +}
> +
>  static void ggtt_node_remove_work_func(struct work_struct *work)
>  {

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_pci_error.c b/drivers/gpu/drm/xe/xe_pci_error.c
> index 79ce0c671549a..3dfcef03850e4 100644
> --- a/drivers/gpu/drm/xe/xe_pci_error.c
> +++ b/drivers/gpu/drm/xe/xe_pci_error.c

[ ... ]

> @@ -20,19 +19,19 @@ static void prepare_device_for_reset(struct pci_dev *pdev)
>  	struct xe_gt *gt;
>  	u8 id;
>  
> -	/*
> -	 * Wedge the device to prevent userspace access but do not send the uevent.
> -	 * xe_device_wedged_fini() releases runtime pm if wedged flag is set, so acquire a runtime
> -	 * pm reference to avoid underflow.
> -	 */
> -	if (!atomic_xchg(&xe->wedged.flag, 1))
> -		xe_pm_runtime_get_noresume(xe);
> -
> +	/* Block device access until PCI error recovery completes. */
>  	xe_device_set_in_reset(xe);
> +	wake_up_all(&xe->ufence_wq);
>  
>  	for_each_gt(gt, xe, id)
>  		xe_gt_declare_wedged(gt);
>  
> +	/*
> +	 * GT wedging signals pending fences. Existing hardware users can now
> +	 * finish without blocking reset.
> +	 */
> +	xe_device_io_drain(xe);
> +
>  	pci_disable_device(pdev);

[Severity: High]
Are CPU mmaps to VRAM invalidated during the PCI error recovery path?

Unlike the permanent wedge path, which explicitly calls
xe_bo_wedged_invalidate_mmaps(), the PCI reset path does not tear down
existing userspace CPU mappings pointing to VRAM before calling
pci_disable_device().

Any userspace process actively reading or writing to these mappings will
generate PCIe memory transactions to a disabled BAR. This causes Unsupported
Request (UR) or Master Abort errors, which might trigger secondary AER
faults and interfere with the ongoing PCI reset.

>  }
>  
>  static pci_ers_result_t xe_pci_error_slot_reset(struct pci_dev *pdev)

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index b98f2e56bf6f4..c003ba90f27c9 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c

[ ... ]

> @@ -546,6 +547,12 @@ static void preempt_rebind_work_func(struct work_struct *w)
>  	if (err)
>  		goto out_unlock_outer;
>  
> +	if (xe_device_io_get(vm->xe, &io_idx)) {
> +		xe_validation_ctx_fini(&ctx);
> +		err = 0;
> +		goto out_unlock_outer;
> +	}
> +

[Severity: Medium]
Can this early exit leak the drm_exec lockdep context?

xe_validation_ctx_init() initializes the validation context along with the
provided drm_exec structure, which internally establishes a ww_acquire_ctx
for lockdep.

Jumping directly to out_unlock_outer bypasses the
drm_exec_until_all_locked() macro loop entirely. Because the loop is skipped,
drm_exec_fini() is never called by the macro's cleanup condition.

Since xe_validation_ctx_fini() does not finalize the drm_exec context, the
ww_acquire_ctx on the stack remains registered as held in lockdep, which
would cause a lockdep splat when the function returns.

>  	drm_exec_until_all_locked(&exec) {
>  		bool done = false;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016093237.340525-1-arvind.yadav@intel.com?part=1