Re: [RFC PATCH v1 8/8] misc/arm-cla: Add userspace interface

["Arnd Bergmann" <arnd@arndb.de>]

On Fri, Jul 17, 2026, at 12:47, Ryan Roberts wrote:
> Expose CLA devices through a character device so userspace can enumerate
> the available hardware and map accelerator register frames.
>
> Define version 1 of the CLA UAPI with a GET_PARAM ioctl. Report device
> topology, CPU affinity, domain membership, mmap offsets, architecture
> version and attached accelerator masks, together with the IIDR, DEVARCH
> and REVIDR of each accelerator.
>
> CLA registers can only be read from the CPU local to the device, while
> enumeration may occur on any CPU. Validate the supported CLA
> architecture version during device setup and cache the CLA and
> accelerator identification registers for later ioctl queries.

This interface looks very raw at the moment, I expect this will have
one or more larger redesigns.

Most importantly, a single character device to expose an arbitrary
number of underlying hardware features is an inherently flawed security
model. If any specific accelerator is ever found to have a
major vulnerability, that would mean administrators will have to
disable all of them by default.

> Support shared read-write mmap of one or more CLA register pages. Create
> a context for every domain covered by the mapping and resolve faults
> only while that context owns the domain. Queue unassigned contexts with
> the domain scheduler, drop mmap_lock while waiting for assignment and
> retry the fault after the context is woken.

I still need some time to better understand what this means.
Does a CPU have multiple concurrently running contexts? Is a
user process able to starve the allocation of other processes
by just requesting a lot of them?

> +static long cla_ioctl_get_param(unsigned long arg)
> +{
> +	struct arm_cla_param __user *uparam = (void __user *)arg;
> +	struct arm_cla_param param;
> +	int accel_id;
> +	int dev_id;
> +	int ret;
> +
> +	if (copy_from_user(&param, uparam, sizeof(param)))
> +		return -EFAULT;
> +
> +	ret = cla_ioctl_validate_param(&param);
> +	if (ret)
> +		return ret;
> +
> +	dev_id = dev_nospec(ARM_CLA_PARAM_INDEX_DEV(param.index));
> +	accel_id = accel_nospec(ARM_CLA_PARAM_INDEX_ACCEL(param.index));

Why is the dev_id/accel_id not a property of the device node itself?

> +	switch (param.param) {
> +	case ARM_CLA_PARAM_UABI_VERSION:
> +		param.value = ARM_CLA_UABI_VERSION;
> +		break;

UABI definitions are not versioned, you have to stay compatible
indefinitely. If you need something else, add a new command.

> +	wait_event_interruptible(ctx->waitq,
> +				 READ_ONCE(domain->assigned_ctx) == ctx ||
> +				 cla_ctx_is_dying(ctx) ||
> +				 READ_ONCE(domain->broken));

If you call wait_event_interruptible(), you have to check the return
code and deal with it being interrupted.

> +static const struct file_operations cla_fops = {
> +	.owner = THIS_MODULE,
> +	.mmap = cla_file_mmap,
> +	.unlocked_ioctl = cla_file_ioctl,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl = cla_file_ioctl,
> +#endif

No need for the #ifdef here. Technically setting .compat_ioctl=compat_ptr_ioctl
is the correct way here, though that may change in the future now that
s390 compat mode is gone.

      Arnd