Re: [PATCH v3 06/16] accel/thames: Create a context per client on device open

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] accel/thames: mapping overlap in thames_context_alloc_user_pages() on >4K pages
- [Medium] accel/thames: select IOMMU_IO_PGTABLE_LPAE without Kconfig dependencies
--

commit aad1ae439ba5b7b0cdec19071014a9f8abea740d
Author: Tomeu Vizoso <tomeu.vizoso@gmail.com>

accel/thames: Create a context per client on device open

This commit creates a per-client context on device open and manages an
ARM_64_LPAE_S1 page table for each context using IOMMU_IO_PGTABLE_LPAE. It
allocates cyclic ASIDs, maps the core windows and per-context user stack and
save area pages, and populates the IPC context table entries.

> diff --git a/drivers/accel/thames/Kconfig b/drivers/accel/thames/Kconfig
> index c621002d8efaa..9362b92740751 100644
> --- a/drivers/accel/thames/Kconfig
> +++ b/drivers/accel/thames/Kconfig
> @@ -6,6 +6,7 @@ config DRM_ACCEL_THAMES
>  	depends on TI_K3_DSP_REMOTEPROC
>  	depends on REMOTEPROC
>  	depends on MAILBOX
> +	select IOMMU_IO_PGTABLE_LPAE

[Severity: Medium]
Can selecting IOMMU_IO_PGTABLE_LPAE here without its dependencies cause
unmet direct dependency warnings and build failures?

In drivers/iommu/Kconfig, IOMMU_IO_PGTABLE_LPAE is inside an if IOMMU_SUPPORT
block (which depends on MMU) and has:

config IOMMU_IO_PGTABLE_LPAE
	bool "ARMv7/v8 Long Descriptor Format"
	select IOMMU_IO_PGTABLE
	depends on ARM || ARM64 || COMPILE_TEST
	depends on !GENERIC_ATOMIC64	# for cmpxchg64()

Because TI_K3_DSP_REMOTEPROC depends on ARCH_K3 || COMPILE_TEST,
DRM_ACCEL_THAMES can be enabled when IOMMU_SUPPORT is disabled, or under
COMPILE_TEST on architectures with GENERIC_ATOMIC64=y or MMU=n.

When IOMMU_SUPPORT=n, drivers/iommu/iommu-pages.o is not built, causing
undefined references to iommu_alloc_pages_node_sz() and iommu_free_pages() in
drivers/iommu/io-pgtable-arm.c, and GENERIC_ATOMIC64=y fails to compile due
to cmpxchg64_relaxed().

Should DRM_ACCEL_THAMES also depend on MMU and !GENERIC_ATOMIC64, and select
IOMMU_SUPPORT (similar to DRM_PANTHOR and DRM_PANFROST)?

>  	help
>  	  Choose this option if you have a Texas Instruments J722S and want to
>  	  use its C7x DSP cores as compute accelerators.
> diff --git a/drivers/accel/thames/thames_gem.c b/drivers/accel/thames/thames_gem.c
> new file mode 100644
> index 0000000000000..b98e0fe203b08
> --- /dev/null
> +++ b/drivers/accel/thames/thames_gem.c

[ ... ]

> +static struct drm_gem_dma_object *thames_gem_create(struct drm_device *ddev, size_t size)
> +{
> +	struct device *dev = drm_dev_dma_dev(ddev);
> +	struct thames_gem_object *bo;
> +	struct drm_gem_object *obj;
> +	size_t offset = 0;
> +	int ret;
> +
> +	size = PAGE_ALIGN(size);
> +
> +	bo = kzalloc_obj(*bo, GFP_KERNEL);
> +	if (!bo)
> +		return ERR_PTR(-ENOMEM);
> +
> +	obj = &bo->base.base;
> +	obj->funcs = &thames_gem_funcs;
> +	bo->base.map_noncoherent = true;
> +
> +	ret = drm_gem_object_init(ddev, obj, size);

[ ... ]

> +static struct drm_gem_dma_object *thames_context_alloc_user_pages(struct thames_file_priv *priv,
> +								  u64 dsp_va, size_t size)
> +{
> +	int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_CACHE;
> +	struct drm_gem_dma_object *dma_obj;
> +	int ret;
> +
> +	dma_obj = thames_gem_create(&priv->tdev->ddev, size);
> +	if (IS_ERR(dma_obj))
> +		return dma_obj;
> +
> +	ret = thames_context_map(priv, dsp_va, prot, dma_obj->dma_addr, dma_obj->base.size);
> +	if (ret) {
> +		drm_gem_object_put(&dma_obj->base);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return dma_obj;
> +}
> +
> +static int thames_context_map_user_pages(struct thames_file_priv *priv)
> +{
> +	struct drm_gem_dma_object *dma_obj;
> +
> +	dma_obj = thames_context_alloc_user_pages(priv, THAMES_USER_STACK_BASE,
> +						  THAMES_USER_STACK_SIZE);
> +	if (IS_ERR(dma_obj))
> +		return PTR_ERR(dma_obj);
> +	priv->user_stack = dma_obj;
> +
> +	dma_obj = thames_context_alloc_user_pages(priv, THAMES_USER_SAVE_AREA_BASE,
> +						  THAMES_USER_SAVE_AREA_SIZE);
> +	if (IS_ERR(dma_obj))
> +		return PTR_ERR(dma_obj);
> +	priv->save_area = dma_obj;
> +
> +	return 0;
> +}

[Severity: High]
What happens on arm64 kernels configured with 16 KiB or 64 KiB pages when
thames_open() calls thames_context_create() -> thames_context_map_user_pages()?

In drivers/accel/thames/thames_abi.h, the user region addresses are fixed at
4 KiB granularity:

#define THAMES_USER_REGION_BASE		0x10000000000ull
#define THAMES_USER_REGION_SIZE		0x200000ull
#define THAMES_USER_SAVE_AREA_BASE	(THAMES_USER_REGION_BASE + 0x2000ull)
#define THAMES_USER_SAVE_AREA_SIZE	0x2000ull
#define THAMES_USER_STACK_BASE		(THAMES_USER_REGION_BASE + 0x5000ull)
#define THAMES_USER_STACK_SIZE		0x1000ull

In thames_gem_create(), size is rounded up to the host PAGE_SIZE via
PAGE_ALIGN(size) and stored in dma_obj->base.size. Then
thames_context_alloc_user_pages() maps dma_obj->base.size bytes into the DSP
page table rather than the requested size.

When PAGE_SIZE is 16 KiB (0x4000) or 64 KiB (0x10000),
thames_context_map_user_pages() first maps user_stack starting at
THAMES_USER_REGION_BASE + 0x5000, and then maps save_area for PAGE_SIZE bytes
starting at THAMES_USER_REGION_BASE + 0x2000 (covering at least +0x2000 to
+0x6000).

Won't the save_area mapping cover the guard page at +0x4000 and collide with
the already-mapped user_stack PTEs at +0x5000, triggering a WARN_ON in
arm_lpae_init_pte() and failing context creation with -EEXIST?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-thames-v3-0-3ab40aa17157@tomeuvizoso.net?part=6