Re: [PATCH v5 1/4] drm/xe/forcewake: synchronize engine-cycle access with unplug

On 8/12/2026 5:21 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Incomplete device unplug synchronization in query ioctls leading to potential MMIO access after unplug.
> - [Medium] Missing array_index_nospec() on user-provided indices creates Spectre v1 gadgets.
> --
>
> --- Patch [1]: [PATCH v5 1/4] drm/xe/forcewake: synchronize engine-cycle access with unplug ---
> commit cd6f1d67df5ef0b65b3a0c38b2fc6c7bd6e42c60
> Author: Zongyao Bai <[email protected]>
>
> drm/xe/forcewake: synchronize engine-cycle access with unplug
>
> This patch adds drm_dev_enter() and drm_dev_exit() to query_engine_cycles()
> to protect forcewake and MMIO accesses against device unplug operations. It
> also updates the probe failure path to use drm_dev_unplug() to ensure proper
> synchronization.
>
>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> index 71111ad324658..81b31325e5817 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
> [ ... ]
>
>> @@ -1166,7 +1166,7 @@ int xe_device_probe(struct xe_device *xe)
>>   
>>   err_unregister_display:
>>   	xe_display_unregister(xe);
>> -	drm_dev_unregister(&xe->drm);
>> +	drm_dev_unplug(&xe->drm);
>>   
>>   	return err;
>>   }
>> diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
>> index dc975f5953683..3c356bc48e2ab 100644
>> --- a/drivers/gpu/drm/xe/xe_query.c
>> +++ b/drivers/gpu/drm/xe/xe_query.c
> [ ... ]
>
>> @@ -143,28 +145,44 @@ query_engine_cycles(struct xe_device *xe,
>>   		return -EINVAL;
>>   
>>   	eci = &resp.eci;
>> +	if (!drm_dev_enter(&xe->drm, &dev_idx))
>> +		return -ENODEV;
>> +
> [Severity: High]
> This is a pre-existing issue, but since this patch introduces device unplug
> synchronization for query_engine_cycles(), should similar protection be added
> to other queries like query_pxp_status() and query_oa_units()?
>
> In xe_query.c, query_pxp_status() accesses hardware status without SRCU
> protection:
>
> query_pxp_status()
>      xe_pxp_get_readiness_status(xe->pxp);
>
> Similarly, query_oa_units() reads MMIO indirectly:
>
> query_oa_units()
>      xe_oa_timestamp_frequency()
>          xe_mmio_read32()
>
> Without drm_dev_enter() protection, can these queries cause page faults if a
> userspace process calls DRM_IOCTL_XE_DEVICE_QUERY concurrently with a device
> unplug event?

it is a pre-existing issue.
Yes, I agree with these similar protections should be added. But I want 
to focus this delay-release forcewake function for query_engine_cycles() 
first.
For others,  they are not introduced in this patch. I prefer follow up 
in other patches, but not in this patch.

>
>>   	gt = xe_device_get_gt(xe, eci->gt_id);
>> -	if (!gt)
>> -		return -EINVAL;
>> +	if (!gt) {
>> +		err = -EINVAL;
>> +		goto out;
>> +	}
>>   
>> -	if (eci->engine_class >= ARRAY_SIZE(user_to_xe_engine_class))
>> -		return -EINVAL;
>> +	if (eci->engine_class >= ARRAY_SIZE(user_to_xe_engine_class)) {
>> +		err = -EINVAL;
>> +		goto out;
>> +	}
>>   
>>   	hwe = xe_gt_hw_engine(gt, user_to_xe_engine_class[eci->engine_class],
>>   			      eci->engine_instance, true);
> [Severity: Medium]
> This is a pre-existing issue, but does the engine_class array access need
> mitigation against speculative execution?
>
> The eci->engine_class index comes directly from userspace and is used
> to access the user_to_xe_engine_class array after a bounds check.
> Could this be used to bypass bounds checks speculatively and leak
> information via cache side channels?
>
> Additionally, earlier in this function xe_device_get_gt() is called with
> eci->gt_id, which also originates from userspace:
>
> xe_device_get_gt()
>      if (gt_id >= xe->info.tile_count * xe->info.max_gt_per_tile)
>          return NULL;
>      tile = &xe->tiles[gt_id / xe->info.max_gt_per_tile];
>
> Should array_index_nospec() be used for these userspace-provided indices?

Also pre-existing. This patch only converts the existing return -EINVAL;
sites to err = -EINVAL; goto out; for the drm_dev_exit() cleanup; it 
does not change
the bounds checks or the array indexing, so it neither introduces nor 
changes these gadgets.