Re: [PATCH v1 3/4] drm/xe/vram: Add initial support for PF-mem regions

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] `xe_vram_region_init_pfmem` does not enforce page alignment for the caller-provided `offset` and `size`.
- [Medium] Incorrect physical address calculation for PFMEM resources in `xe_ttm_vram_mgr_alloc_sgt` due to hardcoded parent VRAM region.
- [Low] Duplicate function declarations for `xe_map_resource_to_region` and `xe_vram_probe`.

Pre-existing issues:
- [High] If the LMEM BAR mapping fails, `vram_region_init` creates a bogus, non-NULL pointer for `vram->mapping`, leading to a kernel panic.
--

--- Patch [3]: [PATCH v1 3/4] drm/xe/vram: Add initial support for PF-mem regions ---
commit 6301fdec9ffb8a28243957040dadf342236bc7f7
Author: Piotr Piórkowski <[email protected]>

drm/xe/vram: Add initial support for PF-mem regions

This commit adds helpers to allocate and initialize PF-mem VRAM regions from
a caller-provided parent, offset, and size. It also assigns the matching TTM
placement. A separate TTM VRAM manager is initialized when a caller installs
the region as the tile kernel_vram.

> diff --git a/drivers/gpu/drm/xe/xe_vram.c b/drivers/gpu/drm/xe/xe_vram.c
> index e5f83df988263..29c93e9aa5a72 100644
> --- a/drivers/gpu/drm/xe/xe_vram.c
> +++ b/drivers/gpu/drm/xe/xe_vram.c
[ ... ]
> @@ -276,6 +278,45 @@ static int vram_region_init(struct xe_device *xe, struct xe_vram_region *vram,
[ ... ]
> +int xe_vram_region_init_pfmem(struct xe_vram_region *vram,
> +			      struct xe_vram_region *parent,
> +			      u64 offset, u64 size)
> +{
> +	struct xe_device *xe = parent->xe;
> +	resource_size_t remain_io_size;
> +
> +	if (!size || offset > parent->usable_size ||
> +	    size > parent->usable_size - offset)
> +		return -EINVAL;

[Severity: Medium]
Does this validation block need to enforce page alignment for the caller-provided
offset and size? Unaligned physical base addresses could propagate to the TTM
buddy allocator and SGL mappings, which might lead to unaligned PTE programming
in the GPU.

Also, when a buffer object allocated in kernel_vram is exported, will
xe_ttm_vram_mgr_alloc_sgt() calculate the correct physical address?

Currently, it calculates the address using the parent tile's VRAM:

drivers/gpu/drm/xe/xe_ttm_vram_mgr.c:xe_ttm_vram_mgr_alloc_sgt() {
    ...
    phys_addr_t phys = cursor.start + xe_vram_region_io_start(tile->mem.vram);
    ...
}

Should it dynamically resolve the region using xe_map_resource_to_region(res)
to ensure the PFMEM offset is included?

> +
> +	remain_io_size = offset < parent->io_size ? parent->io_size - offset : 0;
> +	if (!remain_io_size)
> +		return -EINVAL;
> +
> +	return vram_region_init(xe, vram, parent, offset, size, size, remain_io_size);

[Severity: High]
This is a pre-existing issue, but if the parent LMEM BAR mapping fails, could
this propagate a bogus, non-NULL pointer to the PFMEM region?

In vram_region_init(), if lmem_bar->mapping is NULL, calculating:

    vram->mapping = lmem_bar->mapping + offset;

with a non-zero offset yields a poisoned pointer that bypasses safety checks like
if (vram->mapping).

> +}
[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_vram.h b/drivers/gpu/drm/xe/xe_vram.h
> index 87088ffbfd5e5..4c77d128e3293 100644
> --- a/drivers/gpu/drm/xe/xe_vram.h
> +++ b/drivers/gpu/drm/xe/xe_vram.h
> @@ -16,7 +16,14 @@ struct ttm_resource;
>  struct xe_vram_region *xe_map_resource_to_region(struct ttm_resource *res);
>  int xe_vram_probe(struct xe_device *xe);
>  
> +struct xe_vram_region *xe_map_resource_to_region(struct ttm_resource *res);
> +int xe_vram_probe(struct xe_device *xe);

[Severity: Low]
This isn't a bug, but these function declarations were already present right
above. Were they duplicated by accident?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3