Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <200f2cc9-1747-95a3-82b1-600650cff1df@linaro.org>
Date:   Tue, 6 Sep 2022 15:10:41 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH v2 07/10] misc: fastrpc: Add support for audiopd
Content-Language: en-US
To:     Abel Vesa <abel.vesa@linaro.org>,
        Amol Maheshwari <amahesh@qti.qualcomm.com>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        Ekansh Gupta <quic_ekagupt@quicinc.com>
Cc:     Rob Herring <robh@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-arm-msm@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        devicetree@vger.kernel.org
References: <20220902154900.3404524-1-abel.vesa@linaro.org>
 <20220902154900.3404524-8-abel.vesa@linaro.org>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
In-Reply-To: <20220902154900.3404524-8-abel.vesa@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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 */