[v2,07/10] misc: fastrpc: Add support for audiopd

In order to be able to start the adsp listener for audiopd using adsprpcd,
we need to add the corresponding ioctl for creating a static process.
On that ioctl call we need to allocate the heap. Allocating the heap needs
to be happening only once and needs to be kept between different device
open calls, so attach it to the channel context to make sure that remains
until the RPMSG driver is removed. Then, if there are any VMIDs associated
with the static ADSP process, do a call to SCM to assign it.
And then, send all the necessary info related to heap to the DSP.

Co-developed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/misc/fastrpc.c      | 126 ++++++++++++++++++++++++++++++++++++
 include/uapi/misc/fastrpc.h |   7 ++
 2 files changed, 133 insertions(+)

Hi Abel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on robh/for-next linus/master v6.0-rc3 next-20220901]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Abel-Vesa/misc-fastrpc-Add-audiopd-support/20220902-235842
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 4ec7ac90ff399b7d9af81cc8afd430a22786c61b
config: mips-randconfig-r035-20220902 (https://download.01.org/0day-ci/archive/20220903/202209031027.5XvVp3U8-lkp@intel.com/config)
compiler: mipsel-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/7036272dbe486564afd86e31ca0abf48aa1535ed
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Abel-Vesa/misc-fastrpc-Add-audiopd-support/20220902-235842
        git checkout 7036272dbe486564afd86e31ca0abf48aa1535ed
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/misc/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:573,
                    from include/asm-generic/bug.h:22,
                    from arch/mips/include/asm/bug.h:42,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:13,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/mips/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:78,
                    from include/linux/spinlock.h:55,
                    from include/linux/swait.h:7,
                    from include/linux/completion.h:12,
                    from drivers/misc/fastrpc.c:5:
   drivers/misc/fastrpc.c: In function 'fastrpc_init_create_static_process':
>> drivers/misc/fastrpc.c:1249:48: warning: format '%x' expects argument of type 'unsigned int', but argument 7 has type 'struct qcom_scm_vmperm *' [-Wformat=]
    1249 |                         dev_dbg(fl->sctx->dev, "Assinging memory with phys 0x%llx size 0x%llx perms 0x%x, vmperms %x, vmcount %x\n",
         |                                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:134:29: note: in definition of macro '__dynamic_func_call'
     134 |                 func(&id, ##__VA_ARGS__);               \
         |                             ^~~~~~~~~~~
   include/linux/dynamic_debug.h:166:9: note: in expansion of macro '_dynamic_func_call'
     166 |         _dynamic_func_call(fmt,__dynamic_dev_dbg,               \
         |         ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:155:9: note: in expansion of macro 'dynamic_dev_dbg'
     155 |         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~
   include/linux/dev_printk.h:155:30: note: in expansion of macro 'dev_fmt'
     155 |         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                              ^~~~~~~
   drivers/misc/fastrpc.c:1249:25: note: in expansion of macro 'dev_dbg'
    1249 |                         dev_dbg(fl->sctx->dev, "Assinging memory with phys 0x%llx size 0x%llx perms 0x%x, vmperms %x, vmcount %x\n",
         |                         ^~~~~~~
   drivers/misc/fastrpc.c:1249:116: note: format string is defined here
    1249 |                         dev_dbg(fl->sctx->dev, "Assinging memory with phys 0x%llx size 0x%llx perms 0x%x, vmperms %x, vmcount %x\n",
         |                                                                                                                   ~^
         |                                                                                                                    |
         |                                                                                                                    unsigned int


vim +1249 drivers/misc/fastrpc.c

  1198	
  1199	static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
  1200						      char __user *argp)
  1201	{
  1202		struct fastrpc_init_create_static init;
  1203		struct fastrpc_invoke_args *args;
  1204		struct fastrpc_phy_page pages[1];
  1205		char *name;
  1206		int err;
  1207		struct {
  1208			int pgid;
  1209			u32 namelen;
  1210			u32 pageslen;
  1211		} inbuf;
  1212		u32 sc;
  1213	
  1214		args = kcalloc(FASTRPC_CREATE_STATIC_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
  1215		if (!args)
  1216			return -ENOMEM;
  1217	
  1218		if (copy_from_user(&init, argp, sizeof(init))) {
  1219			err = -EFAULT;
  1220			goto err;
  1221		}
  1222	
  1223		if (init.namelen > INIT_FILE_NAMELEN_MAX) {
  1224			err = -EINVAL;
  1225			goto err;
  1226		}
  1227	
  1228		name = kzalloc(init.namelen, GFP_KERNEL);
  1229		if (!name) {
  1230			err = -ENOMEM;
  1231			goto err;
  1232		}
  1233	
  1234		if (copy_from_user(name, (void __user *)(uintptr_t)init.name, init.namelen)) {
  1235			err = -EFAULT;
  1236			goto err_name;
  1237		}
  1238	
  1239		if (!fl->cctx->remote_heap) {
  1240			err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
  1241							&fl->cctx->remote_heap);
  1242			if (err)
  1243				goto err_name;
  1244	
  1245			/* Map if we have any heap VMIDs associated with this ADSP Static Process. */
  1246			if (fl->cctx->vmcount) {
  1247				unsigned int perms = BIT(QCOM_SCM_VMID_HLOS);
  1248	
> 1249				dev_dbg(fl->sctx->dev, "Assinging memory with phys 0x%llx size 0x%llx perms 0x%x, vmperms %x, vmcount %x\n",
  1250					fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size,
  1251					perms, fl->cctx->vmperms, fl->cctx->vmcount);
  1252				err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
  1253								(u64)fl->cctx->remote_heap->size, &perms,
  1254								fl->cctx->vmperms, fl->cctx->vmcount);
  1255				if (err) {
  1256					dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d",
  1257						fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
  1258					goto err_map;
  1259				}
  1260			}
  1261		}
  1262	
  1263		inbuf.pgid = fl->tgid;
  1264		inbuf.namelen = init.namelen;
  1265		inbuf.pageslen = 0;
  1266		fl->pd = USER_PD;
  1267	
  1268		args[0].ptr = (u64)(uintptr_t)&inbuf;
  1269		args[0].length = sizeof(inbuf);
  1270		args[0].fd = -1;
  1271	
  1272		args[1].ptr = (u64)(uintptr_t)name;
  1273		args[1].length = inbuf.namelen;
  1274		args[1].fd = -1;
  1275	
  1276		pages[0].addr = fl->cctx->remote_heap->phys;
  1277		pages[0].size = fl->cctx->remote_heap->size;
  1278	
  1279		args[2].ptr = (u64)(uintptr_t) pages;
  1280		args[2].length = sizeof(*pages);
  1281		args[2].fd = -1;
  1282	
  1283		sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_STATIC, 3, 0);
  1284	
  1285		err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
  1286					      sc, args);
  1287		if (err)
  1288			goto err_invoke;
  1289	
  1290		kfree(args);
  1291	
  1292		return 0;
  1293	err_invoke:
  1294	err_map:
  1295		fastrpc_buf_free(fl->cctx->remote_heap);
  1296	err_name:
  1297		kfree(name);
  1298	err:
  1299		kfree(args);
  1300	
  1301		return err;
  1302	}
  1303

Thanks Abel,
Some minor comments.

On 02/09/2022 16:48, Abel Vesa wrote:
> In order to be able to start the adsp listener for audiopd using adsprpcd,
> we need to add the corresponding ioctl for creating a static process.
> On that ioctl call we need to allocate the heap. Allocating the heap needs
> to be happening only once and needs to be kept between different device
> open calls, so attach it to the channel context to make sure that remains
> until the RPMSG driver is removed. Then, if there are any VMIDs associated
> with the static ADSP process, do a call to SCM to assign it.
> And then, send all the necessary info related to heap to the DSP.
> 
> Co-developed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>   drivers/misc/fastrpc.c      | 126 ++++++++++++++++++++++++++++++++++++
>   include/uapi/misc/fastrpc.h |   7 ++
>   2 files changed, 133 insertions(+)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 7c364c58e379..2c656da4ca5e 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -37,8 +37,20 @@
>   #define FASTRPC_DSP_UTILITIES_HANDLE	2
>   #define FASTRPC_CTXID_MASK (0xFF0)
>   #define INIT_FILELEN_MAX (2 * 1024 * 1024)
> +#define INIT_FILE_NAMELEN_MAX (128)
>   #define FASTRPC_DEVICE_NAME	"fastrpc"
> +
> +/* Add memory to static PD pool, protection thru XPU */
> +#define ADSP_MMAP_HEAP_ADDR  4
> +/* MAP static DMA buffer on DSP User PD */
> +#define ADSP_MMAP_DMA_BUFFER  6
> +/* Add memory to static PD pool protection thru hypervisor */
> +#define ADSP_MMAP_REMOTE_HEAP_ADDR  8
> +/* Add memory to userPD pool, for user heap */
>   #define ADSP_MMAP_ADD_PAGES 0x1000
> +/* Add memory to userPD pool, for LLC heap */
> +#define ADSP_MMAP_ADD_PAGES_LLC 0x3000,
> +
>   #define DSP_UNSUPPORTED_API (0x80000414)
>   /* MAX NUMBER of DSP ATTRIBUTES SUPPORTED */
>   #define FASTRPC_MAX_DSP_ATTRIBUTES (256)
> @@ -72,6 +84,7 @@
>   		FASTRPC_BUILD_SCALARS(0, method, in, out, 0, 0)
>   
>   #define FASTRPC_CREATE_PROCESS_NARGS	6
> +#define FASTRPC_CREATE_STATIC_PROCESS_NARGS	3
>   /* Remote Method id table */
>   #define FASTRPC_RMID_INIT_ATTACH	0
>   #define FASTRPC_RMID_INIT_RELEASE	1
> @@ -261,6 +274,7 @@ struct fastrpc_channel_ctx {
>   	u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
>   	struct fastrpc_device *secure_fdevice;
>   	struct fastrpc_device *fdevice;
> +	struct fastrpc_buf *remote_heap;
>   	bool secure;
>   	bool unsigned_support;
>   };
> @@ -1167,6 +1181,7 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
>   		spin_unlock(&fl->lock);
>   		fastrpc_context_put(ctx);
>   	}
> +

new line not relevant to this patch.

>   	if (err)
>   		dev_dbg(fl->sctx->dev, "Error: Invoke Failed %d\n", err);
>   
> @@ -1191,6 +1206,111 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
>   	return false;
>   }
>   
> +static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> +					      char __user *argp)
> +{
> +	struct fastrpc_init_create_static init;
> +	struct fastrpc_invoke_args *args;
> +	struct fastrpc_phy_page pages[1];
> +	char *name;
> +	int err;
> +	struct {
> +		int pgid;
> +		u32 namelen;
> +		u32 pageslen;
> +	} inbuf;
> +	u32 sc;
> +
> +	args = kcalloc(FASTRPC_CREATE_STATIC_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
> +	if (!args)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(&init, argp, sizeof(init))) {
> +		err = -EFAULT;
> +		goto err;
> +	}
> +
> +	if (init.namelen > INIT_FILE_NAMELEN_MAX) {
> +		err = -EINVAL;
> +		goto err;
> +	}
> +
> +	name = kzalloc(init.namelen, GFP_KERNEL);
> +	if (!name) {
> +		err = -ENOMEM;
> +		goto err;
> +	}
> +
> +	if (copy_from_user(name, (void __user *)(uintptr_t)init.name, init.namelen)) {
> +		err = -EFAULT;
> +		goto err_name;
> +	}
> +
> +	if (!fl->cctx->remote_heap) {
> +		err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
> +						&fl->cctx->remote_heap);
> +		if (err)
> +			goto err_name;
> +
> +		/* Map if we have any heap VMIDs associated with this ADSP Static Process. */
> +		if (fl->cctx->vmcount) {
> +			unsigned int perms = BIT(QCOM_SCM_VMID_HLOS);
> +
> +			dev_dbg(fl->sctx->dev, "Assinging memory with phys 0x%llx size 0x%llx perms 0x%x, vmperms %x, vmcount %x\n",
> +				fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size,
> +				perms, fl->cctx->vmperms, fl->cctx->vmcount);

Please remove this debug, do we really need this?

> +			err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
> +							(u64)fl->cctx->remote_heap->size, &perms,
> +							fl->cctx->vmperms, fl->cctx->vmcount);
> +			if (err) {
> +				dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d",
> +					fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
> +				goto err_map;
> +			}
> +		}
> +	}
> +
> +	inbuf.pgid = fl->tgid;
> +	inbuf.namelen = init.namelen;
> +	inbuf.pageslen = 0;
> +	fl->pd = USER_PD;
> +
> +	args[0].ptr = (u64)(uintptr_t)&inbuf;
> +	args[0].length = sizeof(inbuf);
> +	args[0].fd = -1;
> +
> +	args[1].ptr = (u64)(uintptr_t)name;
> +	args[1].length = inbuf.namelen;
> +	args[1].fd = -1;
> +
> +	pages[0].addr = fl->cctx->remote_heap->phys;
> +	pages[0].size = fl->cctx->remote_heap->size;
> +
> +	args[2].ptr = (u64)(uintptr_t) pages;
> +	args[2].length = sizeof(*pages);
> +	args[2].fd = -1;
> +
> +	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_STATIC, 3, 0);
> +
> +	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
> +				      sc, args);
> +	if (err)
> +		goto err_invoke;
> +
> +	kfree(args);
> +
> +	return 0;
> +err_invoke:
What happens to the secure mappings here, if invoke failed should we not 
cleanup that part?
Or may be a not saying that we will be reusing this would be nice.

> +err_map:
> +	fastrpc_buf_free(fl->cctx->remote_heap);
> +err_name:
> +	kfree(name);
> +err:
> +	kfree(args);
> +
> +	return err;
> +}
> +
>   static int fastrpc_init_create_process(struct fastrpc_user *fl,
>   					char __user *argp)
>   {
> @@ -1918,6 +2038,9 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
>   	case FASTRPC_IOCTL_INIT_ATTACH_SNS:
>   		err = fastrpc_init_attach(fl, SENSORS_PD);
>   		break;
> +	case FASTRPC_IOCTL_INIT_CREATE_STATIC:
> +		err = fastrpc_init_create_static_process(fl, argp);
> +		break;
>   	case FASTRPC_IOCTL_INIT_CREATE:
>   		err = fastrpc_init_create_process(fl, argp);
>   		break;
> @@ -2183,6 +2306,9 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>   	if (cctx->secure_fdevice)
>   		misc_deregister(&cctx->secure_fdevice->miscdev);
>   
> +	if (cctx->remote_heap)
> +		fastrpc_buf_free(cctx->remote_heap);
> +
>   	of_platform_depopulate(&rpdev->dev);
>   
>   	cctx->rpdev = NULL;
> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
> index 5e29f2cfa42d..2cdf2f137d33 100644
> --- a/include/uapi/misc/fastrpc.h
> +++ b/include/uapi/misc/fastrpc.h
> @@ -16,6 +16,7 @@
>   #define FASTRPC_IOCTL_MEM_MAP		_IOWR('R', 10, struct fastrpc_mem_map)
>   #define FASTRPC_IOCTL_MEM_UNMAP		_IOWR('R', 11, struct fastrpc_mem_unmap)
>   #define FASTRPC_IOCTL_GET_DSP_INFO	_IOWR('R', 13, struct fastrpc_ioctl_capability)
> +#define FASTRPC_IOCTL_INIT_CREATE_STATIC _IOWR('R', 15, struct fastrpc_init_create_static)
>   
>   /**
>    * enum fastrpc_map_flags - control flags for mapping memory on DSP user process
> @@ -87,6 +88,12 @@ struct fastrpc_init_create {
>   	__u64 file;	/* pointer to elf file */
>   };
>   
> +struct fastrpc_init_create_static {
> +	__u32 namelen;	/* length of pd process name */
> +	__u32 memlen;
> +	__u64 name;	/* pd process name */
> +};
> +
>   struct fastrpc_alloc_dma_buf {
>   	__s32 fd;	/* fd */
>   	__u32 flags;	/* flags to map with */