[v3,2/4] firmware: Add support for Qualcomm Secure Execution Environment SCM interface

  Add support for SCM calls to Secure OS and the Secure Execution
Environment (SEE) residing in the TrustZone (TZ) via the QSEECOM
interface. This allows communication with Secure/TZ applications, for
example 'uefisecapp' managing access to UEFI variables.

The interface is managed by a platform device to ensure correct lifetime
and establish a device link to the Qualcomm SCM device.

While this patch introduces only a very basic interface without the more
advanced features (such as re-entrant and blocking SCM calls and
listeners/callbacks), this is enough to talk to the aforementioned
'uefisecapp'.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---

Changes in v3:
 - Rebase ontop of latest qcom_scm changes (qcom_scm.h moved).
 - Move qcom_qseecom.h in accordance with qcom_scm.

Changes in v2:
 - Bind the interface to a device.
 - Establish a device link to the SCM device to ensure proper ordering.
 - Register client apps as child devices instead of requiring them to be
   specified in the device tree.
 - Rename (qctree -> qseecom) to allow differentiation between old
   (qseecom) and new (smcinvoke) interfaces to the trusted execution
   environment.

---
 MAINTAINERS                                |   7 +
 drivers/firmware/Kconfig                   |  15 +
 drivers/firmware/Makefile                  |   1 +
 drivers/firmware/qcom_qseecom.c            | 314 +++++++++++++++++++++
 include/linux/firmware/qcom/qcom_qseecom.h | 190 +++++++++++++
 5 files changed, 527 insertions(+)
 create mode 100644 drivers/firmware/qcom_qseecom.c
 create mode 100644 include/linux/firmware/qcom/qcom_qseecom.h
  

On 05/03/2023 04:21, Maximilian Luz wrote:
> Add support for SCM calls to Secure OS and the Secure Execution
> Environment (SEE) residing in the TrustZone (TZ) via the QSEECOM
> interface. This allows communication with Secure/TZ applications, for
> example 'uefisecapp' managing access to UEFI variables.
> 
> The interface is managed by a platform device to ensure correct lifetime
> and establish a device link to the Qualcomm SCM device.
> 
> While this patch introduces only a very basic interface without the more
> advanced features (such as re-entrant and blocking SCM calls and
> listeners/callbacks), this is enough to talk to the aforementioned
> 'uefisecapp'.
> 
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> ---
> 
> Changes in v3:
>   - Rebase ontop of latest qcom_scm changes (qcom_scm.h moved).
>   - Move qcom_qseecom.h in accordance with qcom_scm.
> 
> Changes in v2:
>   - Bind the interface to a device.
>   - Establish a device link to the SCM device to ensure proper ordering.
>   - Register client apps as child devices instead of requiring them to be
>     specified in the device tree.
>   - Rename (qctree -> qseecom) to allow differentiation between old
>     (qseecom) and new (smcinvoke) interfaces to the trusted execution
>     environment.
> 
> ---
>   MAINTAINERS                                |   7 +
>   drivers/firmware/Kconfig                   |  15 +
>   drivers/firmware/Makefile                  |   1 +
>   drivers/firmware/qcom_qseecom.c            | 314 +++++++++++++++++++++
>   include/linux/firmware/qcom/qcom_qseecom.h | 190 +++++++++++++
>   5 files changed, 527 insertions(+)
>   create mode 100644 drivers/firmware/qcom_qseecom.c
>   create mode 100644 include/linux/firmware/qcom/qcom_qseecom.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9201967d198d..1545914a592c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17380,6 +17380,13 @@ F:	Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
>   F:	drivers/net/ethernet/qualcomm/rmnet/
>   F:	include/linux/if_rmnet.h
>   
> +QUALCOMM SECURE EXECUTION ENVIRONMENT COMMUNICATION DRIVER
> +M:	Maximilian Luz <luzmaximilian@gmail.com>
> +L:	linux-arm-msm@vger.kernel.org
> +S:	Maintained
> +F:	drivers/firmware/qcom_qseecom.c
> +F:	include/linux/firmware/qcom/qcom_qseecom.h
> +
>   QUALCOMM TSENS THERMAL DRIVER
>   M:	Amit Kucheria <amitk@kernel.org>
>   M:	Thara Gopinath <thara.gopinath@gmail.com>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index b59e3041fd62..22eec0835abf 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -226,6 +226,21 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
>   
>   	  Say Y here to enable "download mode" by default.
>   
> +config QCOM_QSEECOM
> +	tristate "Qualcomm QSEECOM interface driver"
> +	select MFD_CORE
> +	select QCOM_SCM
> +	help
> +	  Various Qualcomm SoCs have a Secure Execution Environment (SEE) running
> +	  in the Trust Zone. This module provides an interface to that via the
> +	  QSEECOM mechanism, using SCM calls.
> +
> +	  The QSEECOM interface allows, among other things, access to applications
> +	  running in the SEE. An example of such an application is 'uefisecapp',
> +	  which is required to access UEFI variables on certain systems.
> +
> +	  Select M or Y here to enable the QSEECOM interface driver.
> +
>   config SYSFB
>   	bool
>   	select BOOT_VESA_SUPPORT
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 28fcddcd688f..aa48e0821b7d 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
>   obj-$(CONFIG_FW_CFG_SYSFS)	+= qemu_fw_cfg.o
>   obj-$(CONFIG_QCOM_SCM)		+= qcom-scm.o
>   qcom-scm-objs += qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
> +obj-$(CONFIG_QCOM_QSEECOM)	+= qcom_qseecom.o
>   obj-$(CONFIG_SYSFB)		+= sysfb.o
>   obj-$(CONFIG_SYSFB_SIMPLEFB)	+= sysfb_simplefb.o
>   obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
> diff --git a/drivers/firmware/qcom_qseecom.c b/drivers/firmware/qcom_qseecom.c
> new file mode 100644
> index 000000000000..efa5b115b2f1
> --- /dev/null
> +++ b/drivers/firmware/qcom_qseecom.c
> @@ -0,0 +1,314 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Interface driver for the Qualcomm Secure Execution Environment (SEE) /
> + * TrustZone OS (TzOS). Manages communication via the QSEECOM interface, using
> + * Secure Channel Manager (SCM) calls.
> + *
> + * Copyright (C) 2023 Maximilian Luz <luzmaximilian@gmail.com>
> + */
> +
> +#include <asm/barrier.h>
> +#include <linux/device.h>
> +#include <linux/firmware/qcom/qcom_scm.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/string.h>
> +
> +#include <linux/firmware/qcom/qcom_qseecom.h>
> +
> +
> +/* -- Secure-OS SCM call interface. ----------------------------------------- */
> +
> +static int __qseecom_scm_call(const struct qcom_scm_desc *desc,
> +			      struct qseecom_scm_resp *res)
> +{
> +	struct qcom_scm_res scm_res = {};
> +	int status;
> +
> +	status = qcom_scm_call(desc, &scm_res);
> +
> +	res->status = scm_res.result[0];
> +	res->resp_type = scm_res.result[1];
> +	res->data = scm_res.result[2];
> +
> +	if (status)
> +		return status;
> +
> +	return 0;
> +}
> +
> +/**
> + * qseecom_scm_call() - Perform a QSEECOM SCM call.
> + * @qsee: The QSEECOM device.
> + * @desc: SCM call descriptor.
> + * @res:  SCM call response (output).
> + *
> + * Performs the QSEECOM SCM call described by @desc, returning the response in
> + * @rsp.
> + *
> + * Return: Returns zero on success, nonzero on failure.
> + */
> +int qseecom_scm_call(struct qseecom_device *qsee, const struct qcom_scm_desc *desc,
> +		     struct qseecom_scm_resp *res)
> +{
> +	int status;
> +
> +	/*
> +	 * Note: Multiple QSEECOM SCM calls should not be executed same time,
> +	 * so lock things here. This needs to be extended to callback/listener
> +	 * handling when support for that is implemented.
> +	 */
> +
> +	mutex_lock(&qsee->scm_call_lock);
> +	status = __qseecom_scm_call(desc, res);
> +	mutex_unlock(&qsee->scm_call_lock);
> +
> +	dev_dbg(qsee->dev, "%s: owner=%x, svc=%x, cmd=%x, status=%lld, type=%llx, data=%llx",
> +		__func__, desc->owner, desc->svc, desc->cmd, res->status,
> +		res->resp_type, res->data);
> +
> +	if (status) {
> +		dev_err(qsee->dev, "qcom_scm_call failed with error %d\n", status);
> +		return status;
> +	}
> +
> +	/*
> +	 * TODO: Handle incomplete and blocked calls:
> +	 *
> +	 * Incomplete and blocked calls are not supported yet. Some devices
> +	 * and/or commands require those, some don't. Let's warn about them
> +	 * prominently in case someone attempts to try these commands with a
> +	 * device/command combination that isn't supported yet.
> +	 */
> +	WARN_ON(res->status == QSEECOM_RESULT_INCOMPLETE);
> +	WARN_ON(res->status == QSEECOM_RESULT_BLOCKED_ON_LISTENER);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(qseecom_scm_call);
> +
> +
> +/* -- Secure App interface. ------------------------------------------------- */
> +
> +/**
> + * qseecom_app_get_id() - Query the app ID for a given SEE app name.
> + * @qsee:     The QSEECOM device.
> + * @app_name: The name of the app.
> + * @app_id:   The returned app ID.
> + *
> + * Query and return the application ID of the SEE app identified by the given
> + * name. This returned ID is the unique identifier of the app required for
> + * subsequent communication.
> + *
> + * Return: Returns zero on success, nonzero on failure. Returns -ENOENT if the
> + * app has not been loaded or could not be found.
> + */
> +int qseecom_app_get_id(struct qseecom_device *qsee, const char *app_name, u32 *app_id)
> +{
> +	unsigned long name_buf_size = QSEECOM_MAX_APP_NAME_SIZE;
> +	unsigned long app_name_len = strlen(app_name);
> +	struct qcom_scm_desc desc = {};
> +	struct qseecom_scm_resp res = {};
> +	dma_addr_t name_buf_phys;
> +	char *name_buf;
> +	int status;
> +
> +	if (app_name_len >= name_buf_size)
> +		return -EINVAL;
> +
> +	name_buf = kzalloc(name_buf_size, GFP_KERNEL);
> +	if (!name_buf)
> +		return -ENOMEM;
> +
> +	memcpy(name_buf, app_name, app_name_len);
> +
> +	name_buf_phys = dma_map_single(qsee->dev, name_buf, name_buf_size, DMA_TO_DEVICE);
> +	if (dma_mapping_error(qsee->dev, name_buf_phys)) {
> +		kfree(name_buf);
> +		dev_err(qsee->dev, "failed to map dma address\n");
> +		return -EFAULT;
> +	}
> +
> +	desc.owner = QSEECOM_TZ_OWNER_QSEE_OS;
> +	desc.svc = QSEECOM_TZ_SVC_APP_MGR;
> +	desc.cmd = 0x03;
> +	desc.arginfo = QCOM_SCM_ARGS(2, QCOM_SCM_RW, QCOM_SCM_VAL);
> +	desc.args[0] = name_buf_phys;
> +	desc.args[1] = app_name_len;
> +
> +	status = qseecom_scm_call(qsee, &desc, &res);
> +	dma_unmap_single(qsee->dev, name_buf_phys, name_buf_size, DMA_TO_DEVICE);
> +	kfree(name_buf);
> +
> +	if (status)
> +		return status;
> +
> +	if (res.status != QSEECOM_RESULT_SUCCESS)
> +		return -ENOENT;
> +
> +	*app_id = res.data;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(qseecom_app_get_id);
> +
> +/**
> + * qseecom_app_send() - Send to and receive data from a given SEE app.
> + * @qsee:   The QSEECOM device.
> + * @app_id: The ID of the app to communicate with.
> + * @req:    DMA region of the request sent to the app.
> + * @rsp:    DMA region of the response returned by the app.
> + *
> + * Sends a request to the SEE app identified by the given ID and read back its
> + * response. The caller must provide two DMA memory regions, one for the
> + * request and one for the response, and fill out the @req region with the
> + * respective (app-specific) request data. The SEE app reads this and returns
> + * its response in the @rsp region.
> + *
> + * Return: Returns zero on success, nonzero on failure.
> + */
> +int qseecom_app_send(struct qseecom_device *qsee, u32 app_id, struct qseecom_dma *req,
> +		     struct qseecom_dma *rsp)
> +{
> +	struct qseecom_scm_resp res = {};
> +	int status;
> +
> +	struct qcom_scm_desc desc = {
> +		.owner = QSEECOM_TZ_OWNER_TZ_APPS,
> +		.svc = QSEECOM_TZ_SVC_APP_ID_PLACEHOLDER,
> +		.cmd = 0x01,
> +		.arginfo = QCOM_SCM_ARGS(5, QCOM_SCM_VAL,
> +					 QCOM_SCM_RW, QCOM_SCM_VAL,
> +					 QCOM_SCM_RW, QCOM_SCM_VAL),
> +		.args[0] = app_id,
> +		.args[1] = req->phys,
> +		.args[2] = req->size,
> +		.args[3] = rsp->phys,
> +		.args[4] = rsp->size,
> +	};
> +
> +	/* Make sure the request is fully written before sending it off. */
> +	dma_wmb();
> +
> +	status = qseecom_scm_call(qsee, &desc, &res);
> +
> +	/* Make sure we don't attempt any reads before the SCM call is done. */
> +	dma_rmb();
> +
> +	if (status)
> +		return status;
> +
> +	if (res.status != QSEECOM_RESULT_SUCCESS)
> +		return -EIO;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(qseecom_app_send);
> +
> +
> +/* -- Platform specific data. ----------------------------------------------- */
> +
> +struct qseecom_data {
> +	const struct mfd_cell *cells;
> +	int num_cells;
> +};
> +
> +static const struct of_device_id qseecom_dt_match[] = {
> +	{ .compatible = "qcom,qseecom-sc8280xp", },
> +	{ .compatible = "qcom,qseecom", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, qseecom_dt_match);
> +
> +
> +/* -- Driver setup. --------------------------------------------------------- */
> +
> +static int qseecom_setup_scm_link(struct platform_device *pdev)
> +{
> +	const u32 flags = DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;
> +	struct platform_device *scm_dev;
> +	struct device_node *scm_node;
> +	struct device_link *link;
> +	int status = 0;
> +
> +	if (!pdev->dev.of_node)
> +		return -ENODEV;
> +
> +	/* Find the SCM device. */
> +	scm_node = of_parse_phandle(pdev->dev.of_node, "qcom,scm", 0);
> +	if (!scm_node)
> +		return -ENOENT;
> +
> +	scm_dev = of_find_device_by_node(scm_node);
> +	if (!scm_dev) {
> +		status = -ENODEV;
> +		goto put;
> +	}
> +
> +	/* Establish the device link. */
> +	link = device_link_add(&pdev->dev, &scm_dev->dev, flags);
> +	if (!link) {
> +		status = -EINVAL;
> +		goto put;
> +	}
> +
> +	/* Make sure SCM has a driver bound, otherwise defer probe. */
> +	if (link->supplier->links.status != DL_DEV_DRIVER_BOUND) {
> +		status = -EPROBE_DEFER;
> +		goto put;
> +	}
> +
> +put:
> +	of_node_put(scm_node);
> +	return status;
> +}
> +
> +static int qseecom_probe(struct platform_device *pdev)
> +{
> +	const struct qseecom_data *data;
> +	struct qseecom_device *qsee;
> +	int status;
> +
> +	/* Get platform data. */
> +	data = of_device_get_match_data(&pdev->dev);
> +
> +	/* Set up device link. */
> +	status = qseecom_setup_scm_link(pdev);
> +	if (status)
> +		return status;
> +
> +	/* Set up QSEECOM device. */
> +	qsee = devm_kzalloc(&pdev->dev, sizeof(*qsee), GFP_KERNEL);
> +	if (!qsee)
> +		return -ENOMEM;
> +
> +	qsee->dev = &pdev->dev;
> +	mutex_init(&qsee->scm_call_lock);
> +
> +	platform_set_drvdata(pdev, qsee);
> +
> +	/* Add child devices. */
> +	if (data) {
> +		status = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, data->cells,
> +					      data->num_cells, NULL, 0, NULL);
> +	}
> +
> +	return status;
> +}
> +
> +static struct platform_driver qseecom_driver = {
> +	.probe = qseecom_probe,
> +	.driver = {
> +		.name = "qcom_qseecom",
> +		.of_match_table = qseecom_dt_match,
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +	},
> +};
> +module_platform_driver(qseecom_driver);
> +
> +MODULE_AUTHOR("Maximilian Luz <luzmaximilian@gmail.com>");
> +MODULE_DESCRIPTION("Driver for Qualcomm QSEECOM secure OS and secure application interface");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/firmware/qcom/qcom_qseecom.h b/include/linux/firmware/qcom/qcom_qseecom.h
> new file mode 100644
> index 000000000000..5bd9371a92f7
> --- /dev/null
> +++ b/include/linux/firmware/qcom/qcom_qseecom.h
> @@ -0,0 +1,190 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Interface driver for the Qualcomm Secure Execution Environment (SEE) /
> + * TrustZone OS (TzOS). Manages communication via the QSEECOM interface, using
> + * Secure Channel Manager (SCM) calls.
> + *
> + * Copyright (C) 2023 Maximilian Luz <luzmaximilian@gmail.com>
> + */
> +
> +#ifndef _LINUX_QCOM_QSEECOM_H
> +#define _LINUX_QCOM_QSEECOM_H
> +
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/firmware/qcom/qcom_scm.h>
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +
> +
> +/* -- DMA helpers. ---------------------------------------------------------- */
> +
> +/* DMA requirements for QSEECOM SCM calls. */
> +#define QSEECOM_DMA_ALIGNMENT		8
> +#define QSEECOM_DMA_ALIGN(ptr)		ALIGN(ptr, QSEECOM_DMA_ALIGNMENT)
> +
> +/**
> + * struct qseecom_dma - DMA memory region.
> + * @size: Size of the memory region, in bytes.
> + * @virt: Pointer / virtual address to the memory, accessible by the kernel.
> + * @phys: Physical address of the memory region.
> + */
> +struct qseecom_dma {
> +	unsigned long size;

size_t ?

> +	void *virt;
> +	dma_addr_t phys;
> +};

Do we really need this wrapper and the wrappers bellow? They look like a 
pure syntax sugar for me, hiding the dma functions from the user.

> +
> +/**
> + * qseecom_dma_alloc() - Allocate a DMA-able memory region suitable for QSEECOM
> + * SCM calls.
> + * @dev:  The device used for DMA memory allocation.
> + * @dma:  Where to write the allocated memory addresses and size to.
> + * @size: Minimum size of the memory to be allocated.
> + * @gfp:  Flags used for allocation.
> + *
> + * Allocate a DMA-able memory region suitable for interaction with SEE
> + * services/applications and the TzOS. The provided size is treated as the
> + * minimum required size and rounded up, if necessary. The actually allocated
> + * memory region will be stored in @dma. Allocated memory must be freed via
> + * qseecom_dma_free().
> + *
> + * Return: Returns zero on success, -ENOMEM on allocation failure.
> + */
> +static inline int qseecom_dma_alloc(struct device *dev, struct qseecom_dma *dma,
> +				    unsigned long size, gfp_t gfp)

size_t size

gfp is not used

> +{
> +	size = PAGE_ALIGN(size);
> +
> +	dma->virt = dma_alloc_coherent(dev, size, &dma->phys, GFP_KERNEL);
> +	if (!dma->virt)
> +		return -ENOMEM;
> +
> +	dma->size = size;
> +	return 0;
> +}
> +
> +/**
> + * qseecom_dma_free() - Free a DMA memory region.
> + * @dev: The device used for allocation.
> + * @dma: The DMA region to be freed.
> + *
> + * Free a DMA region previously allocated via qseecom_dma_alloc(). Note that
> + * freeing sub-regions is not supported.
> + */
> +static inline void qseecom_dma_free(struct device *dev, struct qseecom_dma *dma)
> +{
> +	dma_free_coherent(dev, dma->size, dma->virt, dma->phys);
> +}
> +
> +/**
> + * qseecom_dma_realloc() - Re-allocate DMA memory region with the requested size.
> + * @dev:  The device used for allocation.
> + * @dma:  The region descriptor to be updated.
> + * @size: The new requested size.
> + * @gfp:  Flags used for allocation.
> + *
> + * Re-allocates a DMA memory region suitable for QSEECOM SCM calls to fit the
> + * requested amount of bytes, if necessary. Does nothing if the provided region
> + * already has enough space to store the requested data.
> + *
> + * See qseecom_dma_alloc() for details.
> + *
> + * Return: Returns zero on success, -ENOMEM on allocation failure.
> + */
> +static inline int qseecom_dma_realloc(struct device *dev, struct qseecom_dma *dma,
> +				      unsigned long size, gfp_t gfp)
> +{
> +	if (PAGE_ALIGN(size) <= dma->size)
> +		return 0;
> +
> +	qseecom_dma_free(dev, dma);
> +	return qseecom_dma_alloc(dev, dma, size, gfp);
> +}

I'll comment on this function when commenting patch 4.

> +
> +/**
> + * qseecom_dma_aligned() - Create a aligned DMA memory sub-region suitable for
> + * QSEECOM SCM calls.
> + * @base:   Base DMA memory region, in which the new region will reside.
> + * @out:    Descriptor to store the aligned sub-region in.
> + * @offset: The offset inside base region at which to place the new sub-region.
> + *
> + * Creates an aligned DMA memory region suitable for QSEECOM SCM calls at or
> + * after the given offset. The size of the sub-region will be set to the
> + * remaining size in the base region after alignment, i.e., the end of the
> + * sub-region will be equal the end of the base region.
> + *
> + * Return: Returns zero on success or -EINVAL if the new aligned memory address
> + * would point outside the base region.
> + */
> +static inline int qseecom_dma_aligned(const struct qseecom_dma *base, struct qseecom_dma *out,
> +				      unsigned long offset)
> +{
> +	void *aligned = (void *)QSEECOM_DMA_ALIGN((uintptr_t)base->virt + offset);
> +
> +	if (aligned - base->virt > base->size)
> +		return -EINVAL;
> +
> +	out->virt = aligned;
> +	out->phys = base->phys + (out->virt - base->virt);
> +	out->size = base->size - (out->virt - base->virt);
> +
> +	return 0;
> +}
> +
> +
> +/* -- Common interface. ----------------------------------------------------- */
> +
> +struct qseecom_device {
> +	struct device *dev;
> +	struct mutex scm_call_lock;	/* Guards QSEECOM SCM calls. */

There can be only one instance of the qseecom call infrastructure. Make 
this mutex static in the qcom_scm.c

> +};
> +
> +
> +/* -- Secure-OS SCM call interface. ----------------------------------------- */
> +
> +#define QSEECOM_TZ_OWNER_TZ_APPS		48
> +#define QSEECOM_TZ_OWNER_QSEE_OS		50
> +
> +#define QSEECOM_TZ_SVC_APP_ID_PLACEHOLDER	0
> +#define QSEECOM_TZ_SVC_APP_MGR			1
> +
> +enum qseecom_scm_result {
> +	QSEECOM_RESULT_SUCCESS			= 0,
> +	QSEECOM_RESULT_INCOMPLETE		= 1,
> +	QSEECOM_RESULT_BLOCKED_ON_LISTENER	= 2,
> +	QSEECOM_RESULT_FAILURE			= 0xFFFFFFFF,
> +};
> +
> +enum qseecom_scm_resp_type {
> +	QSEECOM_SCM_RES_APP_ID			= 0xEE01,
> +	QSEECOM_SCM_RES_QSEOS_LISTENER_ID	= 0xEE02,
> +};
> +
> +/**
> + * struct qseecom_scm_resp - QSEECOM SCM call response.
> + * @status:    Status of the SCM call. See &enum qseecom_scm_result.
> + * @resp_type: Type of the response. See &enum qseecom_scm_resp_type.
> + * @data:      Response data. The type of this data is given in @resp_type.
> + */
> +struct qseecom_scm_resp {
> +	u64 status;
> +	u64 resp_type;
> +	u64 data;
> +};
> +
> +int qseecom_scm_call(struct qseecom_device *qsee, const struct qcom_scm_desc *desc,
> +		     struct qseecom_scm_resp *res);
> +
> +
> +/* -- Secure App interface. ------------------------------------------------- */
> +
> +#define QSEECOM_MAX_APP_NAME_SIZE			64
> +
> +int qseecom_app_get_id(struct qseecom_device *qsee, const char *app_name, u32 *app_id);
> +int qseecom_app_send(struct qseecom_device *qsee, u32 app_id, struct qseecom_dma *req,
> +		     struct qseecom_dma *rsp);

I think that only these calls should be made public / available to other 
modules. qseecom_scm_call also is an internal helper.

> +
> +#endif /* _LINUX_QCOM_QSEECOM_H */
  

On 05/03/2023 04:21, Maximilian Luz wrote:
> Add support for SCM calls to Secure OS and the Secure Execution
> Environment (SEE) residing in the TrustZone (TZ) via the QSEECOM
> interface. This allows communication with Secure/TZ applications, for
> example 'uefisecapp' managing access to UEFI variables.
> 
> The interface is managed by a platform device to ensure correct lifetime
> and establish a device link to the Qualcomm SCM device.
> 
> While this patch introduces only a very basic interface without the more
> advanced features (such as re-entrant and blocking SCM calls and
> listeners/callbacks), this is enough to talk to the aforementioned
> 'uefisecapp'.
> 
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> ---
> 
> Changes in v3:
>   - Rebase ontop of latest qcom_scm changes (qcom_scm.h moved).
>   - Move qcom_qseecom.h in accordance with qcom_scm.
> 
> Changes in v2:
>   - Bind the interface to a device.
>   - Establish a device link to the SCM device to ensure proper ordering.
>   - Register client apps as child devices instead of requiring them to be
>     specified in the device tree.
>   - Rename (qctree -> qseecom) to allow differentiation between old
>     (qseecom) and new (smcinvoke) interfaces to the trusted execution
>     environment.
> 
> ---
>   MAINTAINERS                                |   7 +
>   drivers/firmware/Kconfig                   |  15 +
>   drivers/firmware/Makefile                  |   1 +
>   drivers/firmware/qcom_qseecom.c            | 314 +++++++++++++++++++++
>   include/linux/firmware/qcom/qcom_qseecom.h | 190 +++++++++++++
>   5 files changed, 527 insertions(+)
>   create mode 100644 drivers/firmware/qcom_qseecom.c
>   create mode 100644 include/linux/firmware/qcom/qcom_qseecom.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9201967d198d..1545914a592c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17380,6 +17380,13 @@ F:	Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
>   F:	drivers/net/ethernet/qualcomm/rmnet/
>   F:	include/linux/if_rmnet.h
>   
> +QUALCOMM SECURE EXECUTION ENVIRONMENT COMMUNICATION DRIVER
> +M:	Maximilian Luz <luzmaximilian@gmail.com>
> +L:	linux-arm-msm@vger.kernel.org
> +S:	Maintained
> +F:	drivers/firmware/qcom_qseecom.c
> +F:	include/linux/firmware/qcom/qcom_qseecom.h
> +
>   QUALCOMM TSENS THERMAL DRIVER
>   M:	Amit Kucheria <amitk@kernel.org>
>   M:	Thara Gopinath <thara.gopinath@gmail.com>



> +
> +
> +/* -- Platform specific data. ----------------------------------------------- */
> +
> +struct qseecom_data {
> +	const struct mfd_cell *cells;

The child qseecom devices are not platform devices, so MFD should not be 
used here. Please use aux devices instead.

> +	int num_cells;
> +};
> +
> +static const struct of_device_id qseecom_dt_match[] = {
> +	{ .compatible = "qcom,qseecom-sc8280xp", },

Forgot to mention, while doign review. There is no need for this compat 
until you provide the actual data. Please move it to the patch 4.

> +	{ .compatible = "qcom,qseecom", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, qseecom_dt_match);
  

On 3/7/23 16:32, Dmitry Baryshkov wrote:
> On 05/03/2023 04:21, Maximilian Luz wrote:

[...]

>> +/* -- DMA helpers. ---------------------------------------------------------- */
>> +
>> +/* DMA requirements for QSEECOM SCM calls. */
>> +#define QSEECOM_DMA_ALIGNMENT        8
>> +#define QSEECOM_DMA_ALIGN(ptr)        ALIGN(ptr, QSEECOM_DMA_ALIGNMENT)
>> +
>> +/**
>> + * struct qseecom_dma - DMA memory region.
>> + * @size: Size of the memory region, in bytes.
>> + * @virt: Pointer / virtual address to the memory, accessible by the kernel.
>> + * @phys: Physical address of the memory region.
>> + */
>> +struct qseecom_dma {
>> +    unsigned long size;
> 
> size_t ?

Will fix.

>> +    void *virt;
>> +    dma_addr_t phys;
>> +};
> 
> Do we really need this wrapper and the wrappers bellow? They look like a pure syntax sugar for me, hiding the dma functions from the user.

The idea was that they take care of proper allocation. The Windows driver that
I've reverse-engineered this from allocates memory in multiples of PAGE_SIZE,
so I believe that this might be a requirement (at least on some systems). These
functions are there to ensure that and with that prevent potential bugs by
taking that responsibility from the caller.

>> +
>> +/**
>> + * qseecom_dma_alloc() - Allocate a DMA-able memory region suitable for QSEECOM
>> + * SCM calls.
>> + * @dev:  The device used for DMA memory allocation.
>> + * @dma:  Where to write the allocated memory addresses and size to.
>> + * @size: Minimum size of the memory to be allocated.
>> + * @gfp:  Flags used for allocation.
>> + *
>> + * Allocate a DMA-able memory region suitable for interaction with SEE
>> + * services/applications and the TzOS. The provided size is treated as the
>> + * minimum required size and rounded up, if necessary. The actually allocated
>> + * memory region will be stored in @dma. Allocated memory must be freed via
>> + * qseecom_dma_free().
>> + *
>> + * Return: Returns zero on success, -ENOMEM on allocation failure.
>> + */
>> +static inline int qseecom_dma_alloc(struct device *dev, struct qseecom_dma *dma,
>> +                    unsigned long size, gfp_t gfp)
> 
> size_t size
> 
> gfp is not used

Right, that should have been passed to dma_alloc_coherent(). Will fix that.

>> +{
>> +    size = PAGE_ALIGN(size);
>> +
>> +    dma->virt = dma_alloc_coherent(dev, size, &dma->phys, GFP_KERNEL);
>> +    if (!dma->virt)
>> +        return -ENOMEM;
>> +
>> +    dma->size = size;
>> +    return 0;
>> +}
>> +
>> +/**
>> + * qseecom_dma_free() - Free a DMA memory region.
>> + * @dev: The device used for allocation.
>> + * @dma: The DMA region to be freed.
>> + *
>> + * Free a DMA region previously allocated via qseecom_dma_alloc(). Note that
>> + * freeing sub-regions is not supported.
>> + */
>> +static inline void qseecom_dma_free(struct device *dev, struct qseecom_dma *dma)
>> +{
>> +    dma_free_coherent(dev, dma->size, dma->virt, dma->phys);
>> +}
>> +
>> +/**
>> + * qseecom_dma_realloc() - Re-allocate DMA memory region with the requested size.
>> + * @dev:  The device used for allocation.
>> + * @dma:  The region descriptor to be updated.
>> + * @size: The new requested size.
>> + * @gfp:  Flags used for allocation.
>> + *
>> + * Re-allocates a DMA memory region suitable for QSEECOM SCM calls to fit the
>> + * requested amount of bytes, if necessary. Does nothing if the provided region
>> + * already has enough space to store the requested data.
>> + *
>> + * See qseecom_dma_alloc() for details.
>> + *
>> + * Return: Returns zero on success, -ENOMEM on allocation failure.
>> + */
>> +static inline int qseecom_dma_realloc(struct device *dev, struct qseecom_dma *dma,
>> +                      unsigned long size, gfp_t gfp)
>> +{
>> +    if (PAGE_ALIGN(size) <= dma->size)
>> +        return 0;
>> +
>> +    qseecom_dma_free(dev, dma);
>> +    return qseecom_dma_alloc(dev, dma, size, gfp);
>> +}
> 
> I'll comment on this function when commenting patch 4.
> 
>> +
>> +/**
>> + * qseecom_dma_aligned() - Create a aligned DMA memory sub-region suitable for
>> + * QSEECOM SCM calls.
>> + * @base:   Base DMA memory region, in which the new region will reside.
>> + * @out:    Descriptor to store the aligned sub-region in.
>> + * @offset: The offset inside base region at which to place the new sub-region.
>> + *
>> + * Creates an aligned DMA memory region suitable for QSEECOM SCM calls at or
>> + * after the given offset. The size of the sub-region will be set to the
>> + * remaining size in the base region after alignment, i.e., the end of the
>> + * sub-region will be equal the end of the base region.
>> + *
>> + * Return: Returns zero on success or -EINVAL if the new aligned memory address
>> + * would point outside the base region.
>> + */
>> +static inline int qseecom_dma_aligned(const struct qseecom_dma *base, struct qseecom_dma *out,
>> +                      unsigned long offset)
>> +{
>> +    void *aligned = (void *)QSEECOM_DMA_ALIGN((uintptr_t)base->virt + offset);
>> +
>> +    if (aligned - base->virt > base->size)
>> +        return -EINVAL;
>> +
>> +    out->virt = aligned;
>> +    out->phys = base->phys + (out->virt - base->virt);
>> +    out->size = base->size - (out->virt - base->virt);
>> +
>> +    return 0;
>> +}
>> +
>> +
>> +/* -- Common interface. ----------------------------------------------------- */
>> +
>> +struct qseecom_device {
>> +    struct device *dev;
>> +    struct mutex scm_call_lock;    /* Guards QSEECOM SCM calls. */
> 
> There can be only one instance of the qseecom call infrastructure. Make this mutex static in the qcom_scm.c

Right, will do that.

>> +};
>> +
>> +
>> +/* -- Secure-OS SCM call interface. ----------------------------------------- */
>> +
>> +#define QSEECOM_TZ_OWNER_TZ_APPS        48
>> +#define QSEECOM_TZ_OWNER_QSEE_OS        50
>> +
>> +#define QSEECOM_TZ_SVC_APP_ID_PLACEHOLDER    0
>> +#define QSEECOM_TZ_SVC_APP_MGR            1
>> +
>> +enum qseecom_scm_result {
>> +    QSEECOM_RESULT_SUCCESS            = 0,
>> +    QSEECOM_RESULT_INCOMPLETE        = 1,
>> +    QSEECOM_RESULT_BLOCKED_ON_LISTENER    = 2,
>> +    QSEECOM_RESULT_FAILURE            = 0xFFFFFFFF,
>> +};
>> +
>> +enum qseecom_scm_resp_type {
>> +    QSEECOM_SCM_RES_APP_ID            = 0xEE01,
>> +    QSEECOM_SCM_RES_QSEOS_LISTENER_ID    = 0xEE02,
>> +};
>> +
>> +/**
>> + * struct qseecom_scm_resp - QSEECOM SCM call response.
>> + * @status:    Status of the SCM call. See &enum qseecom_scm_result.
>> + * @resp_type: Type of the response. See &enum qseecom_scm_resp_type.
>> + * @data:      Response data. The type of this data is given in @resp_type.
>> + */
>> +struct qseecom_scm_resp {
>> +    u64 status;
>> +    u64 resp_type;
>> +    u64 data;
>> +};
>> +
>> +int qseecom_scm_call(struct qseecom_device *qsee, const struct qcom_scm_desc *desc,
>> +             struct qseecom_scm_resp *res);
>> +
>> +
>> +/* -- Secure App interface. ------------------------------------------------- */
>> +
>> +#define QSEECOM_MAX_APP_NAME_SIZE            64
>> +
>> +int qseecom_app_get_id(struct qseecom_device *qsee, const char *app_name, u32 *app_id);
>> +int qseecom_app_send(struct qseecom_device *qsee, u32 app_id, struct qseecom_dma *req,
>> +             struct qseecom_dma *rsp);
> 
> I think that only these calls should be made public / available to other modules. qseecom_scm_call also is an internal helper.

So move all calls to qcom_scm and only make these two public? Or move only
qseecom_scm_call() to qcom_scm (which would require to make it public there,
however). Or how would you want this to look?

>> +
>> +#endif /* _LINUX_QCOM_QSEECOM_H */
> 

Regards,
Max
  

On 3/7/23 16:36, Dmitry Baryshkov wrote:
> On 05/03/2023 04:21, Maximilian Luz wrote:
>> Add support for SCM calls to Secure OS and the Secure Execution
>> Environment (SEE) residing in the TrustZone (TZ) via the QSEECOM
>> interface. This allows communication with Secure/TZ applications, for
>> example 'uefisecapp' managing access to UEFI variables.
>>
>> The interface is managed by a platform device to ensure correct lifetime
>> and establish a device link to the Qualcomm SCM device.
>>
>> While this patch introduces only a very basic interface without the more
>> advanced features (such as re-entrant and blocking SCM calls and
>> listeners/callbacks), this is enough to talk to the aforementioned
>> 'uefisecapp'.
>>
>> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
>> ---
>>
>> Changes in v3:
>>   - Rebase ontop of latest qcom_scm changes (qcom_scm.h moved).
>>   - Move qcom_qseecom.h in accordance with qcom_scm.
>>
>> Changes in v2:
>>   - Bind the interface to a device.
>>   - Establish a device link to the SCM device to ensure proper ordering.
>>   - Register client apps as child devices instead of requiring them to be
>>     specified in the device tree.
>>   - Rename (qctree -> qseecom) to allow differentiation between old
>>     (qseecom) and new (smcinvoke) interfaces to the trusted execution
>>     environment.
>>
>> ---
>>   MAINTAINERS                                |   7 +
>>   drivers/firmware/Kconfig                   |  15 +
>>   drivers/firmware/Makefile                  |   1 +
>>   drivers/firmware/qcom_qseecom.c            | 314 +++++++++++++++++++++
>>   include/linux/firmware/qcom/qcom_qseecom.h | 190 +++++++++++++
>>   5 files changed, 527 insertions(+)
>>   create mode 100644 drivers/firmware/qcom_qseecom.c
>>   create mode 100644 include/linux/firmware/qcom/qcom_qseecom.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 9201967d198d..1545914a592c 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -17380,6 +17380,13 @@ F:    Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
>>   F:    drivers/net/ethernet/qualcomm/rmnet/
>>   F:    include/linux/if_rmnet.h
>> +QUALCOMM SECURE EXECUTION ENVIRONMENT COMMUNICATION DRIVER
>> +M:    Maximilian Luz <luzmaximilian@gmail.com>
>> +L:    linux-arm-msm@vger.kernel.org
>> +S:    Maintained
>> +F:    drivers/firmware/qcom_qseecom.c
>> +F:    include/linux/firmware/qcom/qcom_qseecom.h
>> +
>>   QUALCOMM TSENS THERMAL DRIVER
>>   M:    Amit Kucheria <amitk@kernel.org>
>>   M:    Thara Gopinath <thara.gopinath@gmail.com>
> 
> 
> 
>> +
>> +
>> +/* -- Platform specific data. ----------------------------------------------- */
>> +
>> +struct qseecom_data {
>> +    const struct mfd_cell *cells;
> 
> The child qseecom devices are not platform devices, so MFD should not be used here. Please use aux devices instead.

Okay, makes sense. Would this still work with your suggestion in patch 4
regarding a custom (?) bus or can the aux bus be used to implement that? From a
quick look, I believe we could use aux bus for this but I haven't worked with
that before, so I don't know if I'm missing something.

>> +    int num_cells;
>> +};
>> +
>> +static const struct of_device_id qseecom_dt_match[] = {
>> +    { .compatible = "qcom,qseecom-sc8280xp", },
> 
> Forgot to mention, while doign review. There is no need for this compat until you provide the actual data. Please move it to the patch 4.

Sure, will do that.

>> +    { .compatible = "qcom,qseecom", },
>> +    { }
>> +};
>> +MODULE_DEVICE_TABLE(of, qseecom_dt_match);
> 
> 

Regards,
Max
  

On 08/03/2023 16:06, Maximilian Luz wrote:
> On 3/7/23 16:36, Dmitry Baryshkov wrote:
>> On 05/03/2023 04:21, Maximilian Luz wrote:
>>> Add support for SCM calls to Secure OS and the Secure Execution
>>> Environment (SEE) residing in the TrustZone (TZ) via the QSEECOM
>>> interface. This allows communication with Secure/TZ applications, for
>>> example 'uefisecapp' managing access to UEFI variables.
>>>
>>> The interface is managed by a platform device to ensure correct lifetime
>>> and establish a device link to the Qualcomm SCM device.
>>>
>>> While this patch introduces only a very basic interface without the more
>>> advanced features (such as re-entrant and blocking SCM calls and
>>> listeners/callbacks), this is enough to talk to the aforementioned
>>> 'uefisecapp'.
>>>
>>> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
>>> ---
>>>
>>> Changes in v3:
>>>   - Rebase ontop of latest qcom_scm changes (qcom_scm.h moved).
>>>   - Move qcom_qseecom.h in accordance with qcom_scm.
>>>
>>> Changes in v2:
>>>   - Bind the interface to a device.
>>>   - Establish a device link to the SCM device to ensure proper ordering.
>>>   - Register client apps as child devices instead of requiring them 
>>> to be
>>>     specified in the device tree.
>>>   - Rename (qctree -> qseecom) to allow differentiation between old
>>>     (qseecom) and new (smcinvoke) interfaces to the trusted execution
>>>     environment.
>>>
>>> ---
>>>   MAINTAINERS                                |   7 +
>>>   drivers/firmware/Kconfig                   |  15 +
>>>   drivers/firmware/Makefile                  |   1 +
>>>   drivers/firmware/qcom_qseecom.c            | 314 +++++++++++++++++++++
>>>   include/linux/firmware/qcom/qcom_qseecom.h | 190 +++++++++++++
>>>   5 files changed, 527 insertions(+)
>>>   create mode 100644 drivers/firmware/qcom_qseecom.c
>>>   create mode 100644 include/linux/firmware/qcom/qcom_qseecom.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 9201967d198d..1545914a592c 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -17380,6 +17380,13 @@ F:    
>>> Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
>>>   F:    drivers/net/ethernet/qualcomm/rmnet/
>>>   F:    include/linux/if_rmnet.h
>>> +QUALCOMM SECURE EXECUTION ENVIRONMENT COMMUNICATION DRIVER
>>> +M:    Maximilian Luz <luzmaximilian@gmail.com>
>>> +L:    linux-arm-msm@vger.kernel.org
>>> +S:    Maintained
>>> +F:    drivers/firmware/qcom_qseecom.c
>>> +F:    include/linux/firmware/qcom/qcom_qseecom.h
>>> +
>>>   QUALCOMM TSENS THERMAL DRIVER
>>>   M:    Amit Kucheria <amitk@kernel.org>
>>>   M:    Thara Gopinath <thara.gopinath@gmail.com>
>>
>>
>>
>>> +
>>> +
>>> +/* -- Platform specific data. 
>>> ----------------------------------------------- */
>>> +
>>> +struct qseecom_data {
>>> +    const struct mfd_cell *cells;
>>
>> The child qseecom devices are not platform devices, so MFD should not 
>> be used here. Please use aux devices instead.
> 
> Okay, makes sense. Would this still work with your suggestion in patch 4
> regarding a custom (?) bus or can the aux bus be used to implement that? 
> From a
> quick look, I believe we could use aux bus for this but I haven't worked 
> with
> that before, so I don't know if I'm missing something.

Initially I thought that a custom bus might be required, to provide 
custom probe function. After giving it a thought, I think you can get 
away with using aux bus. So, embed the struct auxiliary_device into 
qseecom_app_device.

> 
>>> +    int num_cells;
>>> +};
>>> +
>>> +static const struct of_device_id qseecom_dt_match[] = {
>>> +    { .compatible = "qcom,qseecom-sc8280xp", },
>>
>> Forgot to mention, while doign review. There is no need for this 
>> compat until you provide the actual data. Please move it to the patch 4.
> 
> Sure, will do that.
> 
>>> +    { .compatible = "qcom,qseecom", },
>>> +    { }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, qseecom_dt_match);
>>
>>
> 
> Regards,
> Max
  

On 08/03/2023 15:59, Maximilian Luz wrote:
> On 3/7/23 16:32, Dmitry Baryshkov wrote:
>> On 05/03/2023 04:21, Maximilian Luz wrote:
> 
> [...]
> 
>>> +/* -- DMA helpers. 
>>> ---------------------------------------------------------- */
>>> +
>>> +/* DMA requirements for QSEECOM SCM calls. */
>>> +#define QSEECOM_DMA_ALIGNMENT        8
>>> +#define QSEECOM_DMA_ALIGN(ptr)        ALIGN(ptr, QSEECOM_DMA_ALIGNMENT)
>>> +
>>> +/**
>>> + * struct qseecom_dma - DMA memory region.
>>> + * @size: Size of the memory region, in bytes.
>>> + * @virt: Pointer / virtual address to the memory, accessible by the 
>>> kernel.
>>> + * @phys: Physical address of the memory region.
>>> + */
>>> +struct qseecom_dma {
>>> +    unsigned long size;
>>
>> size_t ?
> 
> Will fix.
> 
>>> +    void *virt;
>>> +    dma_addr_t phys;
>>> +};
>>
>> Do we really need this wrapper and the wrappers bellow? They look like 
>> a pure syntax sugar for me, hiding the dma functions from the user.
> 
> The idea was that they take care of proper allocation. The Windows 
> driver that
> I've reverse-engineered this from allocates memory in multiples of 
> PAGE_SIZE,
> so I believe that this might be a requirement (at least on some 
> systems). These
> functions are there to ensure that and with that prevent potential bugs by
> taking that responsibility from the caller.

I see. As I wrote in another comment, it might be better to review the 
whole memory allocation system: pass required sizes, get the data filled.

> 
>>> +
>>> +/**
>>> + * qseecom_dma_alloc() - Allocate a DMA-able memory region suitable 
>>> for QSEECOM
>>> + * SCM calls.
>>> + * @dev:  The device used for DMA memory allocation.
>>> + * @dma:  Where to write the allocated memory addresses and size to.
>>> + * @size: Minimum size of the memory to be allocated.
>>> + * @gfp:  Flags used for allocation.
>>> + *
>>> + * Allocate a DMA-able memory region suitable for interaction with SEE
>>> + * services/applications and the TzOS. The provided size is treated 
>>> as the
>>> + * minimum required size and rounded up, if necessary. The actually 
>>> allocated
>>> + * memory region will be stored in @dma. Allocated memory must be 
>>> freed via
>>> + * qseecom_dma_free().
>>> + *
>>> + * Return: Returns zero on success, -ENOMEM on allocation failure.
>>> + */
>>> +static inline int qseecom_dma_alloc(struct device *dev, struct 
>>> qseecom_dma *dma,
>>> +                    unsigned long size, gfp_t gfp)
>>
>> size_t size
>>
>> gfp is not used
> 
> Right, that should have been passed to dma_alloc_coherent(). Will fix that.
> 
>>> +{
>>> +    size = PAGE_ALIGN(size);
>>> +
>>> +    dma->virt = dma_alloc_coherent(dev, size, &dma->phys, GFP_KERNEL);
>>> +    if (!dma->virt)
>>> +        return -ENOMEM;
>>> +
>>> +    dma->size = size;
>>> +    return 0;
>>> +}
>>> +
>>> +/**
>>> + * qseecom_dma_free() - Free a DMA memory region.
>>> + * @dev: The device used for allocation.
>>> + * @dma: The DMA region to be freed.
>>> + *
>>> + * Free a DMA region previously allocated via qseecom_dma_alloc(). 
>>> Note that
>>> + * freeing sub-regions is not supported.
>>> + */
>>> +static inline void qseecom_dma_free(struct device *dev, struct 
>>> qseecom_dma *dma)
>>> +{
>>> +    dma_free_coherent(dev, dma->size, dma->virt, dma->phys);
>>> +}
>>> +
>>> +/**
>>> + * qseecom_dma_realloc() - Re-allocate DMA memory region with the 
>>> requested size.
>>> + * @dev:  The device used for allocation.
>>> + * @dma:  The region descriptor to be updated.
>>> + * @size: The new requested size.
>>> + * @gfp:  Flags used for allocation.
>>> + *
>>> + * Re-allocates a DMA memory region suitable for QSEECOM SCM calls 
>>> to fit the
>>> + * requested amount of bytes, if necessary. Does nothing if the 
>>> provided region
>>> + * already has enough space to store the requested data.
>>> + *
>>> + * See qseecom_dma_alloc() for details.
>>> + *
>>> + * Return: Returns zero on success, -ENOMEM on allocation failure.
>>> + */
>>> +static inline int qseecom_dma_realloc(struct device *dev, struct 
>>> qseecom_dma *dma,
>>> +                      unsigned long size, gfp_t gfp)
>>> +{
>>> +    if (PAGE_ALIGN(size) <= dma->size)
>>> +        return 0;
>>> +
>>> +    qseecom_dma_free(dev, dma);
>>> +    return qseecom_dma_alloc(dev, dma, size, gfp);
>>> +}
>>
>> I'll comment on this function when commenting patch 4.
>>
>>> +
>>> +/**
>>> + * qseecom_dma_aligned() - Create a aligned DMA memory sub-region 
>>> suitable for
>>> + * QSEECOM SCM calls.
>>> + * @base:   Base DMA memory region, in which the new region will 
>>> reside.
>>> + * @out:    Descriptor to store the aligned sub-region in.
>>> + * @offset: The offset inside base region at which to place the new 
>>> sub-region.
>>> + *
>>> + * Creates an aligned DMA memory region suitable for QSEECOM SCM 
>>> calls at or
>>> + * after the given offset. The size of the sub-region will be set to 
>>> the
>>> + * remaining size in the base region after alignment, i.e., the end 
>>> of the
>>> + * sub-region will be equal the end of the base region.
>>> + *
>>> + * Return: Returns zero on success or -EINVAL if the new aligned 
>>> memory address
>>> + * would point outside the base region.
>>> + */
>>> +static inline int qseecom_dma_aligned(const struct qseecom_dma 
>>> *base, struct qseecom_dma *out,
>>> +                      unsigned long offset)
>>> +{
>>> +    void *aligned = (void *)QSEECOM_DMA_ALIGN((uintptr_t)base->virt 
>>> + offset);
>>> +
>>> +    if (aligned - base->virt > base->size)
>>> +        return -EINVAL;
>>> +
>>> +    out->virt = aligned;
>>> +    out->phys = base->phys + (out->virt - base->virt);
>>> +    out->size = base->size - (out->virt - base->virt);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +
>>> +/* -- Common interface. 
>>> ----------------------------------------------------- */
>>> +
>>> +struct qseecom_device {
>>> +    struct device *dev;
>>> +    struct mutex scm_call_lock;    /* Guards QSEECOM SCM calls. */
>>
>> There can be only one instance of the qseecom call infrastructure. 
>> Make this mutex static in the qcom_scm.c
> 
> Right, will do that.
> 
>>> +};
>>> +
>>> +
>>> +/* -- Secure-OS SCM call interface. 
>>> ----------------------------------------- */
>>> +
>>> +#define QSEECOM_TZ_OWNER_TZ_APPS        48
>>> +#define QSEECOM_TZ_OWNER_QSEE_OS        50
>>> +
>>> +#define QSEECOM_TZ_SVC_APP_ID_PLACEHOLDER    0
>>> +#define QSEECOM_TZ_SVC_APP_MGR            1
>>> +
>>> +enum qseecom_scm_result {
>>> +    QSEECOM_RESULT_SUCCESS            = 0,
>>> +    QSEECOM_RESULT_INCOMPLETE        = 1,
>>> +    QSEECOM_RESULT_BLOCKED_ON_LISTENER    = 2,
>>> +    QSEECOM_RESULT_FAILURE            = 0xFFFFFFFF,
>>> +};
>>> +
>>> +enum qseecom_scm_resp_type {
>>> +    QSEECOM_SCM_RES_APP_ID            = 0xEE01,
>>> +    QSEECOM_SCM_RES_QSEOS_LISTENER_ID    = 0xEE02,
>>> +};
>>> +
>>> +/**
>>> + * struct qseecom_scm_resp - QSEECOM SCM call response.
>>> + * @status:    Status of the SCM call. See &enum qseecom_scm_result.
>>> + * @resp_type: Type of the response. See &enum qseecom_scm_resp_type.
>>> + * @data:      Response data. The type of this data is given in 
>>> @resp_type.
>>> + */
>>> +struct qseecom_scm_resp {
>>> +    u64 status;
>>> +    u64 resp_type;
>>> +    u64 data;
>>> +};
>>> +
>>> +int qseecom_scm_call(struct qseecom_device *qsee, const struct 
>>> qcom_scm_desc *desc,
>>> +             struct qseecom_scm_resp *res);
>>> +
>>> +
>>> +/* -- Secure App interface. 
>>> ------------------------------------------------- */
>>> +
>>> +#define QSEECOM_MAX_APP_NAME_SIZE            64
>>> +
>>> +int qseecom_app_get_id(struct qseecom_device *qsee, const char 
>>> *app_name, u32 *app_id);
>>> +int qseecom_app_send(struct qseecom_device *qsee, u32 app_id, struct 
>>> qseecom_dma *req,
>>> +             struct qseecom_dma *rsp);
>>
>> I think that only these calls should be made public / available to 
>> other modules. qseecom_scm_call also is an internal helper.
> 
> So move all calls to qcom_scm and only make these two public? Or move only
> qseecom_scm_call() to qcom_scm (which would require to make it public 
> there,
> however). Or how would you want this to look?

I think we can make it with just these calls being public. Or even with 
just the second one being public and available to other drivers. If the 
app_id is a part of qseecom_app_device, we can pass that device to the 
qseecom_app_send(). And qseecom_app_get_id() becomes a part of app 
registration.

> 
>>> +
>>> +#endif /* _LINUX_QCOM_QSEECOM_H */
>>
> 
> Regards,
> Max
  

On 3/9/23 09:45, Dmitry Baryshkov wrote:
> On 08/03/2023 15:59, Maximilian Luz wrote:
>> On 3/7/23 16:32, Dmitry Baryshkov wrote:
>>> On 05/03/2023 04:21, Maximilian Luz wrote:

[...]

>>>> +int qseecom_scm_call(struct qseecom_device *qsee, const struct qcom_scm_desc *desc,
>>>> +             struct qseecom_scm_resp *res);
>>>> +
>>>> +
>>>> +/* -- Secure App interface. ------------------------------------------------- */
>>>> +
>>>> +#define QSEECOM_MAX_APP_NAME_SIZE            64
>>>> +
>>>> +int qseecom_app_get_id(struct qseecom_device *qsee, const char *app_name, u32 *app_id);
>>>> +int qseecom_app_send(struct qseecom_device *qsee, u32 app_id, struct qseecom_dma *req,
>>>> +             struct qseecom_dma *rsp);
>>>
>>> I think that only these calls should be made public / available to other modules. qseecom_scm_call also is an internal helper.
>>
>> So move all calls to qcom_scm and only make these two public? Or move only
>> qseecom_scm_call() to qcom_scm (which would require to make it public there,
>> however). Or how would you want this to look?
> 
> I think we can make it with just these calls being public. Or even with just the second one being public and available to other drivers. If the app_id is a part of qseecom_app_device, we can pass that device to the qseecom_app_send(). And qseecom_app_get_id() becomes a part of app registration.

Right, with the bus structure, we only really need qseecom_app_send().

I'm still a bit concerned about things maybe getting too complex for
qcom_scm with the blocking/re-entrant calls (and maybe other future
stuff), but I guess we can always try to break things up when/if we get
around to that.

I'll try to implement that and your other suggestions and see how that
goes. I think that should work quite well overall.

Thanks again for your review and comments.

Regards,
Max