Re: [RFC PATCH v3 25/37] kvx: Add system call support

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

On Mon, Jul 22, 2024, at 09:41, ysionneau@kalrayinc.com wrote:

> +/**
> + * access_ok: - Checks if a user space pointer is valid
> + * @addr: User space pointer to start of block to check
> + * @size: Size of block to check
> + *
> + * Context: User context only.  This function may sleep.
> + *
> + * Checks if a pointer to a block of memory in user space is valid.
> + *
> + * Returns true (nonzero) if the memory block may be valid, false 
> (zero)
> + * if it is definitely invalid.
> + *
> + * Note that, depending on architecture, this function probably just
> + * checks that the pointer is in the user space range - after calling
> + * this function, memory access functions may still return -EFAULT.
> + */
> +#define access_ok(addr, size) ({					\
> +	__chk_user_ptr(addr);						\
> +	likely(__access_ok((addr), (size)));				\
> +})
> +
> +#include <asm-generic/access_ok.h>

You should not need a custom access_ok() function here, juse use
the asm-generic version directly.

> + * Copyright (C) 2017-2023 Kalray Inc.
> + * Author(s): Clement Leger
> + */
> +
> +#define __ARCH_WANT_SYS_CLONE

I previously commented that you should remove
__ARCH_WANT_NEW_STAT to match what we did for loongarch.

After long discussion, we now put that back though, so you
should probably do the same here. The way you do this is now
different with the move to the common syscall.tbl format,
and I think in the case of NEW_STAT this will change again
since we now should't even need that conditional any more.

> +
> +#define __ARCH_WANT_SYS_CLONE3

__ARCH_WANT_SYS_CLONE3 is now mandatory, and you can drop the
select in 6.11.

> +/* Additional KVX specific syscalls */
> +#define __NR_cachectl (__NR_arch_specific_syscall)
> +__SYSCALL(__NR_cachectl, sys_cachectl)

This one should now go into scripts/syscall.tbl instead.

> +SYSCALL_DEFINE4(cachectl, unsigned long, addr, unsigned long, len,
> +		unsigned long, cache, unsigned long, flags)
> +{
> +	bool wb = !!(flags & CACHECTL_FLAG_OP_WB);
> +	bool inval = !!(flags & CACHECTL_FLAG_OP_INVAL);
> +
> +	if (len == 0)
> +		return 0;
> +
> +	/* Check for overflow */
> +	if (addr + len < addr)
> +		return -EFAULT;
> +
> +	if (cache != CACHECTL_CACHE_DCACHE)
> +		return -EINVAL;
> +
> +	if ((flags & CACHECTL_FLAG_OP_MASK) == 0)
> +		return -EINVAL;
> +
> +	if (flags & CACHECTL_FLAG_ADDR_PHYS) {
> +		if (!IS_ENABLED(CONFIG_CACHECTL_UNSAFE_PHYS_OPERATIONS))
> +			return -EINVAL;
> +
> +		if (!capable(CAP_SYS_ADMIN))
> +			return -EPERM;
> +
> +		dcache_wb_inval_phys_range(addr, len, wb, inval);
> +		return 0;
> +	}
> +
> +	return dcache_wb_inval_virt_range(addr, len, wb, inval);
> +}

This syscall is different from apparently all other architectures
that have a cache management call, in two ways:

- The CONFIG_CACHECTL_UNSAFE_PHYS_OPERATIONS flag and the operation
  behind it seems like a badly defined interface, I assume this is
  a performance hack for a particular device driver, but it should
  not really be done at the system call level. Ideally I think this
  should be redesigned so it's never needed. If you do need it for
  something, please make that a separate patch with a long explanation
  about how it's used.

- The other architectures tend to use sys_cacheflush() with
  more or less standardized calling conventions. Could you
  use the callong conventions from arch/{csky,parisc} instead?

> +#include <linux/syscalls.h>
> +
> +#include <asm/syscalls.h>
> +
> +#undef __SYSCALL
> +#define __SYSCALL(nr, call)[nr] = (call),
> +
> +void *sys_call_table[__NR_syscalls] = {
> +	[0 ... __NR_syscalls - 1] = sys_ni_syscall,
> +#include <asm/unistd.h>
> +};

This file will have to change in 6.11 as I replaced the
uapi/asm-generic/unistd.h file with a generated version
taking scripts/syscall.tbl as its input. Please copy the
code from arch/loongarch.

       Arnd