Re: [RFC v2 13/21] drm/xe: Add xe_guc_access_counter layer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect address computation in `xe_guc_access_counter_page_va` causes middle bits of the virtual address to be lost and shifts the tracking window improperly.
- [High] Handling a G2H access counter notification when the USM feature is disabled causes a kernel crash due to an uninitialized spinlock and a NULL pointer dereference.
--

commit 94395d3f68cfb99d219c20819de2d7f8aae519a8
Author: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>

    drm/xe: Add xe_guc_access_counter layer
    
    Add GuC to host (G2H) access counter notification handler to parse
    GuC firmware messages into struct xe_access_counter and forward to
    xe_access_counter_handler for processing.

> diff --git a/drivers/gpu/drm/xe/xe_guc_access_counter.c b/drivers/gpu/drm/xe/xe_guc_access_counter.c
> new file mode 100644
> index 0000000000000..7e9b835e1ed67
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_guc_access_counter.c
> @@ -0,0 +1,74 @@
[ ... ]
> +static u64 xe_guc_access_counter_page_va(struct xe_device *xe, u64 addr, u8 gran)
> +{
> +	u64 gran_size = xe_access_counter_granularity_to_size(gran);
> +	u64 sub_gran_size = gran_size / 32;
> +	u64 region_base = ALIGN_DOWN(addr, gran_size);
> +	u64 offset_in_subchunk = addr & (sub_gran_size - 1);
> +	u64 page_va = region_base + offset_in_subchunk;

[Severity: High]
Does this logic compute the correct virtual address? 

By adding offset_in_subchunk (which only keeps the lowest log2(sub_gran_size)
bits) to region_base (which clears the lower log2(gran_size) bits), any bits
between sub_gran_size and gran_size are completely discarded.

> +
> +	xe_dbg(xe, "gran_size = %llx, addr = = %llx, region_base = %llx, page_va=%llx\n",
> +	       gran_size, addr, region_base, page_va);
> +	return page_va;
> +}
> +
> +/**
> + * xe_guc_access_counter_handler() - G2H access counter handler
[ ... ]
> +	addr = ((u64)(msg[3] & ACC_VIRTUAL_ADDR_RANGE_HI) << 32) |
> +		(msg[2] & ACC_VIRTUAL_ADDR_RANGE_LO);
> +	ac.consumer.page_va = xe_guc_access_counter_page_va(xe, addr, ac.consumer.granularity);
> +	ac.gran_end = ac.consumer.page_va +
> +		      xe_access_counter_granularity_to_size(ac.consumer.granularity);

[Severity: High]
If the page_va computation is missing the middle bits of the address, does this
also cause gran_end to be misaligned? This could improperly shift the tracking
window and cause incorrect VMA lookups.

> +	ac.consumer.asid = FIELD_GET(ACC_ASID, msg[1]);
> +	ac.consumer.engine_class = FIELD_GET(ACC_ENG_CLASS, msg[1]);
> +	ac.consumer.engine_instance = FIELD_GET(ACC_ENG_INSTANCE, msg[1]);
> +	ac.consumer.vfid = FIELD_GET(ACC_VFID, msg[2]);
> +#undef GUC_ACC_MSG_LEN_DW
> +
> +	return xe_access_counter_handler(xe, &ac);

[Severity: High]
Is it safe to unconditionally forward this message to the access counter backend?

If the system is initialized with has_usm == false, the access counter queue
is not fully initialized. Receiving an unexpected access counter notification
message from the GuC could lead to:

drivers/gpu/drm/xe/xe_access_counter.c:xe_access_counter_handler() {
    ...
    spin_lock_irqsave(&ac_queue->lock, flags);
    ...
    xe_usm_queue_push(ac_queue, ac);
    ...
}

Attempting to acquire an uninitialized spinlock, followed by a NULL pointer
dereference of ac_queue->data in xe_usm_queue_push(). Should we verify if the
feature is supported before processing the notification?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909124457.1610890-23-himal.prasad.ghimiray@intel.com?part=13