LinuxLists.cc - [PATCH 0/4] firmware: Add support for Qualcomm UEFI Secure Application

[permalink] [raw]

Subject: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

Add bindings for the Qualcomm Trusted Execution Environment (TrEE) UEFI
Secure application (uefisecapp) client.

Signed-off-by: Maximilian Luz <[email protected]>
---
.../firmware/qcom,tee-uefisecapp.yaml | 38 +++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 39 insertions(+)
create mode 100644 Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml

diff --git a/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml b/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
new file mode 100644
index 000000000000..9e5de1005d5c
--- /dev/null
+++ b/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/qcom/qcom,rpmh-rsc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Trusted Execution Environment UEFI Secure Application
+
+maintainers:
+ - Maximilian Luz <[email protected]>
+
+description: |
+ Various Qualcomm SoCs do not allow direct access to UEFI variables. Instead,
+ these need to be accessed via the UEFI Secure Application (uefisecapp),
+ residing in the Trusted Execution Environment (TrEE). These bindings mark the
+ presence of uefisecapp and allow the respective client driver to load and
+ install efivar operations, providing the kernel with access to UEFI
+ variables.
+
+properties:
+ compatible:
+ const: qcom,tee-uefisecapp
+
+required:
+ - compatible
+
+additionalProperties: false
+
+examples:
+ - |
+ firmware {
+ scm {
+ compatible = "qcom,scm-sc8180x", "qcom,scm";
+ };
+ tee-uefisecapp {
+ compatible = "qcom,tee-uefisecapp";
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 6e014e16fc82..00436245189d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16607,6 +16607,7 @@ QUALCOMM UEFISECAPP DRIVER
M: Maximilian Luz <[email protected]>
L: [email protected]
S: Maintained
+F: Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
F: drivers/firmware/qcom_tee_uefisecapp.c

QUALCOMM VENUS VIDEO ACCELERATOR DRIVER
--
2.37.1

2022-07-23 22:51:47

[permalink] [raw]

Subject: [PATCH 2/4] firmware: Add support for Qualcomm Trusted Execution Environment SCM calls

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

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 <[email protected]>
---
MAINTAINERS | 7 ++
drivers/firmware/Kconfig | 4 +
drivers/firmware/Makefile | 1 +
drivers/firmware/qcom_tee.c | 213 ++++++++++++++++++++++++++++++++++++
include/linux/qcom_tee.h | 179 ++++++++++++++++++++++++++++++
5 files changed, 404 insertions(+)
create mode 100644 drivers/firmware/qcom_tee.c
create mode 100644 include/linux/qcom_tee.h

diff --git a/MAINTAINERS b/MAINTAINERS
index e9eaceeb61ef..e174747df92f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16587,6 +16587,13 @@ F: Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
F: drivers/net/ethernet/qualcomm/rmnet/
F: include/linux/if_rmnet.h

+QUALCOMM TRUSTED EXECUTION ENVIRONMENT DRIVER
+M: Maximilian Luz <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/firmware/qcom_tee.c
+F: include/linux/qcom_tee.h
+
QUALCOMM TSENS THERMAL DRIVER
M: Amit Kucheria <[email protected]>
M: Thara Gopinath <[email protected]>
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index b59e3041fd62..cde60a332b3c 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -226,6 +226,10 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT

Say Y here to enable "download mode" by default.

+config QCOM_TEE
+ tristate
+ select QCOM_SCM
+
config SYSFB
bool
select BOOT_VESA_SUPPORT
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 28fcddcd688f..93dbc6b5a603 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_TEE) += qcom_tee.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_tee.c b/drivers/firmware/qcom_tee.c
new file mode 100644
index 000000000000..7a93776a901d
--- /dev/null
+++ b/drivers/firmware/qcom_tee.c
@@ -0,0 +1,213 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Interface driver for the Qualcomm Trusted Execution Environment (TrEE or
+ * TEE) / TrustZone secure OS (TzOS). Manages communication via Secure Channel
+ * Manager (SCM) calls.
+ *
+ * Copyright (C) 2022 Maximilian Luz <[email protected]>
+ */
+
+#include <asm/barrier.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/qcom_scm.h>
+#include <linux/string.h>
+
+#include <linux/qcom_tee.h>
+
+
+/* -- Secure-OS SCM call interface. ----------------------------------------- */
+
+static DEFINE_MUTEX(scm_call_lock);
+
+static int __qctee_os_scm_call(const struct qcom_scm_desc *desc,
+ struct qctee_os_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;
+}
+
+/**
+ * qctee_os_scm_call() - Perform a TrEE SCM call.
+ * @dev: The (client) device to use for logging.
+ * @desc: SCM call descriptor.
+ * @res: SCM call response (output).
+ *
+ * Performs the TrEE SCM call described by @desc, returning the response in
+ * @rsp. The provided device @dev is used exclusively for logging.
+ *
+ * Return: Returns zero on success, nonzero on failure.
+ */
+int qctee_os_scm_call(struct device *dev, const struct qcom_scm_desc *desc,
+ struct qctee_os_scm_resp *res)
+{
+ int status;
+
+ /*
+ * Note: Multiple TrEE 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(&scm_call_lock);
+ status = __qctee_os_scm_call(desc, res);
+ mutex_unlock(&scm_call_lock);
+
+ dev_dbg(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(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 == QCTEE_OS_RESULT_INCOMPLETE);
+ WARN_ON(res->status == QCTEE_OS_RESULT_BLOCKED_ON_LISTENER);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(qctee_os_scm_call);
+
+
+/* -- Secure App interface. ------------------------------------------------- */
+
+/**
+ * qctee_app_get_id() - Query the app ID for a given TrEE app name.
+ * @dev: The (client) device used for logging and DMA mapping.
+ * @app_name: The name of the app.
+ * @app_id: The returned app ID.
+ *
+ * Query and return the application ID of the TrEE 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 qctee_app_get_id(struct device *dev, const char *app_name, u32 *app_id)
+{
+ unsigned long name_buf_size = QCTEE_MAX_APP_NAME_SIZE;
+ unsigned long app_name_len = strlen(app_name);
+ struct qcom_scm_desc desc = {};
+ struct qctee_os_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(dev, name_buf, name_buf_size, DMA_TO_DEVICE);
+ if (dma_mapping_error(dev, name_buf_phys)) {
+ kfree(name_buf);
+ dev_err(dev, "failed to map dma address\n");
+ return -EFAULT;
+ }
+
+ desc.owner = QCTEE_TZ_OWNER_QSEE_OS;
+ desc.svc = QCTEE_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 = qctee_os_scm_call(dev, &desc, &res);
+ dma_unmap_single(dev, name_buf_phys, name_buf_size, DMA_TO_DEVICE);
+ kfree(name_buf);
+
+ if (status)
+ return status;
+
+ if (res.status != QCTEE_OS_RESULT_SUCCESS)
+ return -ENOENT;
+
+ *app_id = res.data;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(qctee_app_get_id);
+
+/**
+ * qctee_app_send() - Send to and receive data from a given TrEE app.
+ * @dev: The (client) device used for logging.
+ * @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 TrEE 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 TrEE app reads this and returns
+ * its response in the @rsp region.
+ *
+ * Return: Returns zero on success, nonzero on failure.
+ */
+int qctee_app_send(struct device *dev, u32 app_id, struct qctee_dma *req, struct qctee_dma *rsp)
+{
+ struct qctee_os_scm_resp res = {};
+ int status;
+
+ struct qcom_scm_desc desc = {
+ .owner = QCTEE_TZ_OWNER_TZ_APPS,
+ .svc = QCTEE_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 = qctee_os_scm_call(dev, &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 != QCTEE_OS_RESULT_SUCCESS)
+ return -EIO;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(qctee_app_send);
+
+
+/* -- Module metadata. ------------------------------------------------------ */
+
+MODULE_AUTHOR("Maximilian Luz <[email protected]>");
+MODULE_DESCRIPTION("Interface for Qualcomm TrEE/TZ secure OS and secure applications");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/qcom_tee.h b/include/linux/qcom_tee.h
new file mode 100644
index 000000000000..b904d6a010d7
--- /dev/null
+++ b/include/linux/qcom_tee.h
@@ -0,0 +1,179 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Interface driver for the Qualcomm Trusted Execution Environment (TrEE/TEE) /
+ * TrustZone OS (TzOS). Manages communication via Secure Channel Manager (SCM)
+ * calls.
+ *
+ * Copyright (C) 2022 Maximilian Luz <[email protected]>
+ */
+
+#ifndef _LINUX_QCOM_TEE_H
+#define _LINUX_QCOM_TEE_H
+
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/qcom_scm.h>
+#include <linux/types.h>
+
+
+/* -- DMA helpers. ---------------------------------------------------------- */
+
+/* DMA requirements for TrEE SCM calls. */
+#define QCTEE_DMA_ALIGNMENT 8
+#define QCTEE_DMA_ALIGN(ptr) ALIGN(ptr, QCTEE_DMA_ALIGNMENT)
+
+/**
+ * struct qctee_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 qctee_dma {
+ unsigned long size;
+ void *virt;
+ dma_addr_t phys;
+};
+
+/**
+ * qctee_dma_alloc() - Allocate a DMA-able memory region suitable for TrEE 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 TrEE
+ * services 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 qctee_dma_free().
+ *
+ * Return: Returns zero on success, -ENOMEM on allocation failure.
+ */
+static inline int qctee_dma_alloc(struct device *dev, struct qctee_dma *dma,
+ unsigned long size, gfp_t gfp)
+{
+ 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;
+}
+
+/**
+ * qctee_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 qctee_dma_alloc(). Note that
+ * freeing sub-regions is not supported.
+ */
+static inline void qctee_dma_free(struct device *dev, struct qctee_dma *dma)
+{
+ dma_free_coherent(dev, dma->size, dma->virt, dma->phys);
+}
+
+/**
+ * qctee_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 TrEE 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 qctee_dma_alloc() for details.
+ *
+ * Return: Returns zero on success, -ENOMEM on allocation failure.
+ */
+static inline int qctee_dma_realloc(struct device *dev, struct qctee_dma *dma,
+ unsigned long size, gfp_t gfp)
+{
+ if (PAGE_ALIGN(size) <= dma->size)
+ return 0;
+
+ qctee_dma_free(dev, dma);
+ return qctee_dma_alloc(dev, dma, size, gfp);
+}
+
+/**
+ * qctee_dma_aligned() - Create a aligned DMA memory sub-region suitable for
+ * TrEE 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 TrEE 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 qctee_dma_aligned(const struct qctee_dma *base, struct qctee_dma *out,
+ unsigned long offset)
+{
+ void *aligned = (void *)QCTEE_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;
+}
+
+
+/* -- Secure-OS SCM call interface. ----------------------------------------- */
+
+#define QCTEE_TZ_OWNER_TZ_APPS 48
+#define QCTEE_TZ_OWNER_QSEE_OS 50
+
+#define QCTEE_TZ_SVC_APP_ID_PLACEHOLDER 0
+#define QCTEE_TZ_SVC_APP_MGR 1
+
+enum qctee_os_scm_result {
+ QCTEE_OS_RESULT_SUCCESS = 0,
+ QCTEE_OS_RESULT_INCOMPLETE = 1,
+ QCTEE_OS_RESULT_BLOCKED_ON_LISTENER = 2,
+ QCTEE_OS_RESULT_FAILURE = 0xFFFFFFFF,
+};
+
+enum qctee_os_scm_resp_type {
+ QCTEE_OS_SCM_RES_APP_ID = 0xEE01,
+ QCTEE_OS_SCM_RES_QSEOS_LISTENER_ID = 0xEE02,
+};
+
+/**
+ * struct qctee_os_scm_resp - TrEE / TzOS SCM call response.
+ * @status: Status of the SCM call. See &enum qctee_os_scm_result.
+ * @resp_type: Type of the response. See &enum qctee_os_scm_resp_type.
+ * @data: Response data. The type of this data is given in @resp_type.
+ */
+struct qctee_os_scm_resp {
+ u64 status;
+ u64 resp_type;
+ u64 data;
+};
+
+int qctee_os_scm_call(struct device *dev, const struct qcom_scm_desc *desc,
+ struct qctee_os_scm_resp *res);
+
+
+/* -- Secure App interface. ------------------------------------------------- */
+
+#define QCTEE_MAX_APP_NAME_SIZE 64
+
+int qctee_app_get_id(struct device *dev, const char *app_name, u32 *app_id);
+int qctee_app_send(struct device *dev, u32 app_id, struct qctee_dma *req, struct qctee_dma *rsp);
+
+#endif /* _LINUX_QCOM_TEE_H */
--
2.37.1

2022-07-23 22:51:48

[permalink] [raw]

Subject: [PATCH 1/4] firmware: qcom_scm: Export SCM call functions

Make qcom_scm_call, qcom_scm_call_atomic and associated types accessible
to other modules.

Signed-off-by: Maximilian Luz <[email protected]>
---
drivers/firmware/qcom_scm.c | 118 ++++++++++++++++++++++++------------
drivers/firmware/qcom_scm.h | 47 --------------
include/linux/qcom_scm.h | 49 +++++++++++++++
3 files changed, 128 insertions(+), 86 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index cdbfe54c8146..1dd330ffbb9f 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -205,16 +205,17 @@ static enum qcom_scm_convention __get_convention(void)
}

/**
- * qcom_scm_call() - Invoke a syscall in the secure world
- * @dev: device
+ * __qcom_scm_call() - Invoke a syscall in the secure world
+ * @dev: Device. Depending on the command and number of arguments, this
+ * is optional.
* @desc: Descriptor structure containing arguments and return values
* @res: Structure containing results from SMC/HVC call
*
* Sends a command to the SCM and waits for the command to finish processing.
* This should *only* be called in pre-emptible context.
*/
-static int qcom_scm_call(struct device *dev, const struct qcom_scm_desc *desc,
- struct qcom_scm_res *res)
+static int __qcom_scm_call(struct device *dev, const struct qcom_scm_desc *desc,
+ struct qcom_scm_res *res)
{
might_sleep();
switch (__get_convention()) {
@@ -229,18 +230,39 @@ static int qcom_scm_call(struct device *dev, const struct qcom_scm_desc *desc,
}
}

+/**
+ * qcom_scm_call() - Invoke a syscall in the secure world
+ * @desc: Descriptor structure containing arguments and return values
+ * @res: Structure containing results from SMC/HVC call
+ *
+ * Sends a command to the SCM and waits for the command to finish processing.
+ * This should *only* be called in pre-emptible context.
+ *
+ * Returns zero on success, -ENODEV if the SCM device has not been set up yet,
+ * or other non-zero status codes on failure.
+ */
+int qcom_scm_call(const struct qcom_scm_desc *desc, struct qcom_scm_res *res)
+{
+ if (!__scm)
+ return -ENODEV;
+
+ return __qcom_scm_call(__scm->dev, desc, res);
+}
+EXPORT_SYMBOL_GPL(qcom_scm_call);
+
/**
* qcom_scm_call_atomic() - atomic variation of qcom_scm_call()
- * @dev: device
+ * @dev: Device. Depending on the command and number of arguments, this
+ * is optional.
* @desc: Descriptor structure containing arguments and return values
* @res: Structure containing results from SMC/HVC call
*
* Sends a command to the SCM and waits for the command to finish processing.
* This can be called in atomic context.
*/
-static int qcom_scm_call_atomic(struct device *dev,
- const struct qcom_scm_desc *desc,
- struct qcom_scm_res *res)
+static int __qcom_scm_call_atomic(struct device *dev,
+ const struct qcom_scm_desc *desc,
+ struct qcom_scm_res *res)
{
switch (__get_convention()) {
case SMC_CONVENTION_ARM_32:
@@ -254,6 +276,26 @@ static int qcom_scm_call_atomic(struct device *dev,
}
}

+/**
+ * qcom_scm_call_atomic() - atomic variation of qcom_scm_call()
+ * @desc: Descriptor structure containing arguments and return values
+ * @res: Structure containing results from SMC/HVC call
+ *
+ * Sends a command to the SCM and waits for the command to finish processing.
+ * This can be called in atomic context.
+ *
+ * Returns zero on success, -ENODEV if the SCM device has not been set up yet,
+ * or other non-zero status codes on failure.
+ */
+int qcom_scm_call_atomic(const struct qcom_scm_desc *desc, struct qcom_scm_res *res)
+{
+ if (!__scm)
+ return -ENODEV;
+
+ return __qcom_scm_call_atomic(__scm->dev, desc, res);
+}
+EXPORT_SYMBOL_GPL(qcom_scm_call_atomic);
+
static bool __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
u32 cmd_id)
{
@@ -280,7 +322,7 @@ static bool __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
return false;
}

- ret = qcom_scm_call(dev, &desc, &res);
+ ret = __qcom_scm_call(dev, &desc, &res);

return ret ? false : !!res.result[0];
}
@@ -305,7 +347,7 @@ static int qcom_scm_set_boot_addr(void *entry, const u8 *cpu_bits)
desc.args[0] = flags;
desc.args[1] = virt_to_phys(entry);

- return qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
+ return __qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
}

static int qcom_scm_set_boot_addr_mc(void *entry, unsigned int flags)
@@ -327,7 +369,7 @@ static int qcom_scm_set_boot_addr_mc(void *entry, unsigned int flags)
if (!__scm || __get_convention() == SMC_CONVENTION_LEGACY)
return -EOPNOTSUPP;

- return qcom_scm_call(__scm->dev, &desc, NULL);
+ return __qcom_scm_call(__scm->dev, &desc, NULL);
}

/**
@@ -377,7 +419,7 @@ void qcom_scm_cpu_power_down(u32 flags)
.owner = ARM_SMCCC_OWNER_SIP,
};

- qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
+ __qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
}
EXPORT_SYMBOL(qcom_scm_cpu_power_down);

@@ -394,7 +436,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
struct qcom_scm_res res;
int ret;

- ret = qcom_scm_call(__scm->dev, &desc, &res);
+ ret = qcom_scm_call(&desc, &res);

return ret ? : res.result[0];
}
@@ -412,7 +454,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)

desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;

- return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
+ return qcom_scm_call_atomic(&desc, NULL);
}

static void qcom_scm_set_download_mode(bool enable)
@@ -492,7 +534,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,

desc.args[1] = mdata_phys;

- ret = qcom_scm_call(__scm->dev, &desc, &res);
+ ret = __qcom_scm_call(__scm->dev, &desc, &res);

qcom_scm_bw_disable();
qcom_scm_clk_disable();
@@ -558,7 +600,7 @@ int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size)
if (ret)
return ret;

- ret = qcom_scm_call(__scm->dev, &desc, &res);
+ ret = qcom_scm_call(&desc, &res);
qcom_scm_bw_disable();
qcom_scm_clk_disable();

@@ -593,7 +635,7 @@ int qcom_scm_pas_auth_and_reset(u32 peripheral)
if (ret)
return ret;

- ret = qcom_scm_call(__scm->dev, &desc, &res);
+ ret = qcom_scm_call(&desc, &res);
qcom_scm_bw_disable();
qcom_scm_clk_disable();

@@ -627,7 +669,7 @@ int qcom_scm_pas_shutdown(u32 peripheral)
if (ret)
return ret;

- ret = qcom_scm_call(__scm->dev, &desc, &res);
+ ret = qcom_scm_call(&desc, &res);

qcom_scm_bw_disable();
qcom_scm_clk_disable();
@@ -659,7 +701,7 @@ bool qcom_scm_pas_supported(u32 peripheral)
QCOM_SCM_PIL_PAS_IS_SUPPORTED))
return false;

- ret = qcom_scm_call(__scm->dev, &desc, &res);
+ ret = __qcom_scm_call(__scm->dev, &desc, &res);

return ret ? false : !!res.result[0];
}
@@ -678,7 +720,7 @@ static int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
struct qcom_scm_res res;
int ret;

- ret = qcom_scm_call(__scm->dev, &desc, &res);
+ ret = qcom_scm_call(&desc, &res);

return ret ? : res.result[0];
}
@@ -718,8 +760,7 @@ int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val)
struct qcom_scm_res res;
int ret;

-
- ret = qcom_scm_call_atomic(__scm->dev, &desc, &res);
+ ret = qcom_scm_call_atomic(&desc, &res);
if (ret >= 0)
*val = res.result[0];

@@ -738,7 +779,7 @@ int qcom_scm_io_writel(phys_addr_t addr, unsigned int val)
.owner = ARM_SMCCC_OWNER_SIP,
};

- return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
+ return qcom_scm_call_atomic(&desc, NULL);
}
EXPORT_SYMBOL(qcom_scm_io_writel);

@@ -768,7 +809,7 @@ int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare)
struct qcom_scm_res res;
int ret;

- ret = qcom_scm_call(__scm->dev, &desc, &res);
+ ret = qcom_scm_call(&desc, &res);

return ret ? : res.result[0];
}
@@ -786,7 +827,7 @@ int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size)
struct qcom_scm_res res;
int ret;

- ret = qcom_scm_call(__scm->dev, &desc, &res);
+ ret = qcom_scm_call(&desc, &res);

if (size)
*size = res.result[0];
@@ -809,7 +850,7 @@ int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare)
};
int ret;

- ret = qcom_scm_call(__scm->dev, &desc, NULL);
+ ret = qcom_scm_call(&desc, NULL);

/* the pg table has been initialized already, ignore the error */
if (ret == -EPERM)
@@ -830,7 +871,7 @@ int qcom_scm_iommu_set_cp_pool_size(u32 spare, u32 size)
.owner = ARM_SMCCC_OWNER_SIP,
};

- return qcom_scm_call(__scm->dev, &desc, NULL);
+ return qcom_scm_call(&desc, NULL);
}
EXPORT_SYMBOL(qcom_scm_iommu_set_cp_pool_size);

@@ -852,7 +893,7 @@ int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
};
struct qcom_scm_res res;

- ret = qcom_scm_call(__scm->dev, &desc, &res);
+ ret = qcom_scm_call(&desc, &res);

return ret ? : res.result[0];
}
@@ -880,7 +921,7 @@ static int __qcom_scm_assign_mem(struct device *dev, phys_addr_t mem_region,
};
struct qcom_scm_res res;

- ret = qcom_scm_call(dev, &desc, &res);
+ ret = __qcom_scm_call(dev, &desc, &res);

return ret ? : res.result[0];
}
@@ -997,7 +1038,7 @@ int qcom_scm_ocmem_lock(enum qcom_scm_ocmem_client id, u32 offset, u32 size,
.arginfo = QCOM_SCM_ARGS(4),
};

- return qcom_scm_call(__scm->dev, &desc, NULL);
+ return qcom_scm_call(&desc, NULL);
}
EXPORT_SYMBOL(qcom_scm_ocmem_lock);

@@ -1020,7 +1061,7 @@ int qcom_scm_ocmem_unlock(enum qcom_scm_ocmem_client id, u32 offset, u32 size)
.arginfo = QCOM_SCM_ARGS(3),
};

- return qcom_scm_call(__scm->dev, &desc, NULL);
+ return qcom_scm_call(&desc, NULL);
}
EXPORT_SYMBOL(qcom_scm_ocmem_unlock);

@@ -1061,7 +1102,7 @@ int qcom_scm_ice_invalidate_key(u32 index)
.owner = ARM_SMCCC_OWNER_SIP,
};

- return qcom_scm_call(__scm->dev, &desc, NULL);
+ return qcom_scm_call(&desc, NULL);
}
EXPORT_SYMBOL(qcom_scm_ice_invalidate_key);

@@ -1122,7 +1163,7 @@ int qcom_scm_ice_set_key(u32 index, const u8 *key, u32 key_size,
memcpy(keybuf, key, key_size);
desc.args[1] = key_phys;

- ret = qcom_scm_call(__scm->dev, &desc, NULL);
+ ret = qcom_scm_call(&desc, NULL);

memzero_explicit(keybuf, key_size);

@@ -1191,7 +1232,7 @@ int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
if (ret)
return ret;

- ret = qcom_scm_call(__scm->dev, &desc, &res);
+ ret = qcom_scm_call(&desc, &res);
*resp = res.result[0];

qcom_scm_clk_disable();
@@ -1212,7 +1253,7 @@ int qcom_scm_iommu_set_pt_format(u32 sec_id, u32 ctx_num, u32 pt_fmt)
.owner = ARM_SMCCC_OWNER_SIP,
};

- return qcom_scm_call(__scm->dev, &desc, NULL);
+ return qcom_scm_call(&desc, NULL);
}
EXPORT_SYMBOL(qcom_scm_iommu_set_pt_format);

@@ -1227,8 +1268,7 @@ int qcom_scm_qsmmu500_wait_safe_toggle(bool en)
.owner = ARM_SMCCC_OWNER_SIP,
};

-
- return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
+ return qcom_scm_call_atomic(&desc, NULL);
}
EXPORT_SYMBOL(qcom_scm_qsmmu500_wait_safe_toggle);

@@ -1248,7 +1288,7 @@ int qcom_scm_lmh_profile_change(u32 profile_id)
.owner = ARM_SMCCC_OWNER_SIP,
};

- return qcom_scm_call(__scm->dev, &desc, NULL);
+ return qcom_scm_call(&desc, NULL);
}
EXPORT_SYMBOL(qcom_scm_lmh_profile_change);

@@ -1283,7 +1323,7 @@ int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,

desc.args[0] = payload_phys;

- ret = qcom_scm_call(__scm->dev, &desc, NULL);
+ ret = __qcom_scm_call(__scm->dev, &desc, NULL);

dma_free_coherent(__scm->dev, payload_size, payload_buf, payload_phys);
return ret;
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 0d51eef2472f..d058adcc62a3 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -13,53 +13,6 @@ enum qcom_scm_convention {

extern enum qcom_scm_convention qcom_scm_convention;

-#define MAX_QCOM_SCM_ARGS 10
-#define MAX_QCOM_SCM_RETS 3
-
-enum qcom_scm_arg_types {
- QCOM_SCM_VAL,
- QCOM_SCM_RO,
- QCOM_SCM_RW,
- QCOM_SCM_BUFVAL,
-};
-
-#define QCOM_SCM_ARGS_IMPL(num, a, b, c, d, e, f, g, h, i, j, ...) (\
- (((a) & 0x3) << 4) | \
- (((b) & 0x3) << 6) | \
- (((c) & 0x3) << 8) | \
- (((d) & 0x3) << 10) | \
- (((e) & 0x3) << 12) | \
- (((f) & 0x3) << 14) | \
- (((g) & 0x3) << 16) | \
- (((h) & 0x3) << 18) | \
- (((i) & 0x3) << 20) | \
- (((j) & 0x3) << 22) | \
- ((num) & 0xf))
-
-#define QCOM_SCM_ARGS(...) QCOM_SCM_ARGS_IMPL(__VA_ARGS__, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
-
-
-/**
- * struct qcom_scm_desc
- * @arginfo: Metadata describing the arguments in args[]
- * @args: The array of arguments for the secure syscall
- */
-struct qcom_scm_desc {
- u32 svc;
- u32 cmd;
- u32 arginfo;
- u64 args[MAX_QCOM_SCM_ARGS];
- u32 owner;
-};
-
-/**
- * struct qcom_scm_res
- * @result: The values returned by the secure syscall
- */
-struct qcom_scm_res {
- u64 result[MAX_QCOM_SCM_RETS];
-};
-
#define SCM_SMC_FNID(s, c) ((((s) & 0xFF) << 8) | ((c) & 0xFF))
extern int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
enum qcom_scm_convention qcom_convention,
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index f8335644a01a..87b768dedec6 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -9,6 +9,55 @@
#include <linux/types.h>
#include <linux/cpumask.h>

+#define QCOM_SCM_ARGS_IMPL(num, a, b, c, d, e, f, g, h, i, j, ...) (\
+ (((a) & 0x3) << 4) | \
+ (((b) & 0x3) << 6) | \
+ (((c) & 0x3) << 8) | \
+ (((d) & 0x3) << 10) | \
+ (((e) & 0x3) << 12) | \
+ (((f) & 0x3) << 14) | \
+ (((g) & 0x3) << 16) | \
+ (((h) & 0x3) << 18) | \
+ (((i) & 0x3) << 20) | \
+ (((j) & 0x3) << 22) | \
+ ((num) & 0xf))
+
+#define QCOM_SCM_ARGS(...) QCOM_SCM_ARGS_IMPL(__VA_ARGS__, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
+
+#define MAX_QCOM_SCM_ARGS 10
+#define MAX_QCOM_SCM_RETS 3
+
+enum qcom_scm_arg_types {
+ QCOM_SCM_VAL,
+ QCOM_SCM_RO,
+ QCOM_SCM_RW,
+ QCOM_SCM_BUFVAL,
+};
+
+/**
+ * struct qcom_scm_desc - SCM call descriptor.
+ * @arginfo: Metadata describing the arguments in args[]
+ * @args: The array of arguments for the secure syscall
+ */
+struct qcom_scm_desc {
+ u32 svc;
+ u32 cmd;
+ u32 arginfo;
+ u64 args[MAX_QCOM_SCM_ARGS];
+ u32 owner;
+};
+
+/**
+ * struct qcom_scm_res - SCM call response.
+ * @result: The values returned by the secure syscall
+ */
+struct qcom_scm_res {
+ u64 result[MAX_QCOM_SCM_RETS];
+};
+
+int qcom_scm_call(const struct qcom_scm_desc *desc, struct qcom_scm_res *res);
+int qcom_scm_call_atomic(const struct qcom_scm_desc *desc, struct qcom_scm_res *res);
+
#define QCOM_SCM_VERSION(major, minor) (((major) << 16) | ((minor) & 0xFF))
#define QCOM_SCM_CPU_PWR_DOWN_L2_ON 0x0
#define QCOM_SCM_CPU_PWR_DOWN_L2_OFF 0x1
--
2.37.1

2022-07-23 22:51:47

[permalink] [raw]

Subject: [PATCH 3/4] firmware: Add support for Qualcomm UEFI Secure Application

On platforms using the Qualcomm UEFI Secure Application (uefisecapp),
EFI variables cannot be accessed via the standard interface in EFI
runtime mode. The respective functions return EFI_UNSUPPORTED. On these
platforms, we instead need to talk to uefisecapp. This commit provides
support for this and registers the respective efivars operations to
access EFI variables from the kernel.

Communication with uefisecapp follows the standard Qualcomm Trusted
Environment (TEE or TrEE) / Secure OS conventions via the respective SCM
call interface. This is also the reason why variable access works
normally while boot services are active. During this time, said SCM
interface is managed by the boot services. When calling
ExitBootServices(), the ownership is transferred to the kernel.
Therefore, UEFI must not use that interface itself (as multiple parties
accessing this interface at the same time may lead to complications) and
cannot access variables for us.

Signed-off-by: Maximilian Luz <[email protected]>
---
MAINTAINERS | 6 +
drivers/firmware/Kconfig | 16 +
drivers/firmware/Makefile | 1 +
drivers/firmware/qcom_tee_uefisecapp.c | 761 +++++++++++++++++++++++++
4 files changed, 784 insertions(+)
create mode 100644 drivers/firmware/qcom_tee_uefisecapp.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e174747df92f..6e014e16fc82 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16603,6 +16603,12 @@ S: Maintained
F: Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
F: drivers/thermal/qcom/

+QUALCOMM UEFISECAPP DRIVER
+M: Maximilian Luz <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/firmware/qcom_tee_uefisecapp.c
+
QUALCOMM VENUS VIDEO ACCELERATOR DRIVER
M: Stanimir Varbanov <[email protected]>
L: [email protected]
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index cde60a332b3c..4e9e2c227899 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -230,6 +230,22 @@ config QCOM_TEE
tristate
select QCOM_SCM

+config QCOM_TEE_UEFISECAPP
+ tristate "Qualcomm TrEE UEFI Secure App client driver"
+ select QCOM_TEE
+ depends on EFI
+ help
+ Various Qualcomm SoCs do not allow direct access to EFI variables.
+ Instead, these need to be accessed via the UEFI Secure Application
+ (uefisecapp), residing in the Trusted Execution Environment (TrEE).
+
+ This module provides a client driver for uefisecapp, installing efivar
+ operations to allow the kernel accessing EFI variables, and via that also
+ provide user-space with access to EFI variables to via efivarfs.
+
+ Select Y or M here to provide access to EFI variables on the
+ aforementioned platforms.
+
config SYSFB
bool
select BOOT_VESA_SUPPORT
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 93dbc6b5a603..3d8e28368b55 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -21,6 +21,7 @@ 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_TEE) += qcom_tee.o
+obj-$(CONFIG_QCOM_TEE_UEFISECAPP) += qcom_tee_uefisecapp.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_tee_uefisecapp.c b/drivers/firmware/qcom_tee_uefisecapp.c
new file mode 100644
index 000000000000..65573e4b815a
--- /dev/null
+++ b/drivers/firmware/qcom_tee_uefisecapp.c
@@ -0,0 +1,761 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Client driver for Qualcomm UEFI Secure Application (qcom.tz.uefisecapp).
+ * Provides access to UEFI variables on platforms where they are secured by the
+ * aforementioned Trusted Execution Environment (TEE) application.
+ *
+ * Copyright (C) 2022 Maximilian Luz <[email protected]>
+ */
+
+#include <linux/efi.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/qcom_tee.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+
+/* -- UTF-16 helpers. ------------------------------------------------------- */
+
+static unsigned long utf16_strnlen(const efi_char16_t *str, unsigned long max)
+{
+ size_t i;
+
+ for (i = 0; *str != 0 && i < max; i++, str++) {
+ /* Do nothing, all is handled in the for statement. */
+ }
+
+ return i;
+}
+
+/**
+ * utf16_strsize() - Compute the number of bytes required to store a
+ * null-terminated UTF-16 string.
+ * @str: The string to compute the size for.
+ *
+ * Return: Returns the minimum number of bytes required to store the given
+ * null-terminated string, including its null-terminator.
+ */
+static unsigned long utf16_strsize(const efi_char16_t *str)
+{
+ return (utf16_strnlen(str, U32_MAX) + 1) * sizeof(str[0]);
+}
+
+static unsigned long utf16_strlcpy(efi_char16_t *dst, const efi_char16_t *src, unsigned long size)
+{
+ unsigned long actual = utf16_strnlen(src, size - 1);
+
+ memcpy(dst, src, actual * sizeof(src[0]));
+ dst[actual] = 0;
+
+ return actual;
+}
+
+/**
+ * utf16_copy_to_buf() - Copy the given UTF-16 string to a buffer.
+ * @dst: Pointer to the output buffer
+ * @src: Pointer to the null-terminated UTF-16 string to be copied.
+ * @bytes: Maximum number of bytes to copy.
+ *
+ * Copies the given string to the given buffer, ensuring that the output buffer
+ * is not overrun and that the string in the output buffer will always be
+ * null-terminated.
+ *
+ * Return: Returns the length of the copied string, without null-terminator.
+ */
+static unsigned long utf16_copy_to_buf(efi_char16_t *dst, const efi_char16_t *src,
+ unsigned long bytes)
+{
+ return utf16_strlcpy(dst, src, bytes / sizeof(src[0]));
+}
+
+
+/* -- Qualcomm "uefisecapp" interface definitions. -------------------------- */
+
+#define QCTEE_UEFISEC_APP_NAME "qcom.tz.uefisecapp"
+
+#define QCTEE_CMD_UEFI(x) (0x8000 | (x))
+#define QCTEE_CMD_UEFI_GET_VARIABLE QCTEE_CMD_UEFI(0)
+#define QCTEE_CMD_UEFI_SET_VARIABLE QCTEE_CMD_UEFI(1)
+#define QCTEE_CMD_UEFI_GET_NEXT_VARIABLE QCTEE_CMD_UEFI(2)
+#define QCTEE_CMD_UEFI_QUERY_VARIABLE_INFO QCTEE_CMD_UEFI(3)
+
+/**
+ * struct qctee_req_uefi_get_variable - Request for GetVariable command.
+ * @command_id: The ID of the command. Must be %QCTEE_CMD_UEFI_GET_VARIABLE.
+ * @length: Length of the request in bytes, including this struct and any
+ * parameters (name, GUID) stored after it as well as any padding
+ * thereof for alignment.
+ * @name_offset: Offset from the start of this struct to where the variable
+ * name is stored (as utf-16 string), in bytes.
+ * @name_size: Size of the name parameter in bytes, including null-terminator.
+ * @guid_offset: Offset from the start of this struct to where the GUID
+ * parameter is stored, in bytes.
+ * @guid_size: Size of the GUID parameter in bytes, i.e. sizeof(efi_guid_t).
+ * @data_size: Size of the output buffer, in bytes.
+ */
+struct qctee_req_uefi_get_variable {
+ u32 command_id;
+ u32 length;
+ u32 name_offset;
+ u32 name_size;
+ u32 guid_offset;
+ u32 guid_size;
+ u32 data_size;
+} __packed;
+
+/**
+ * struct qctee_rsp_uefi_get_variable - Response for GetVariable command.
+ * @command_id: The ID of the command. Should be %QCTEE_CMD_UEFI_GET_VARIABLE.
+ * @length: Length of the response in bytes, including this struct and the
+ * returned data.
+ * @status: Status of this command.
+ * @attributes: EFI variable attributes.
+ * @data_offset: Offset from the start of this struct to where the data is
+ * stored, in bytes.
+ * @data_size: Size of the returned data, in bytes. In case status indicates
+ * that the buffer is too small, this will be the size required
+ * to store the EFI variable data.
+ */
+struct qctee_rsp_uefi_get_variable {
+ u32 command_id;
+ u32 length;
+ u32 status;
+ u32 attributes;
+ u32 data_offset;
+ u32 data_size;
+} __packed;
+
+/**
+ * struct qctee_req_uefi_set_variable - Request for the SetVariable command.
+ * @command_id: The ID of the command. Must be %QCTEE_CMD_UEFI_SET_VARIABLE.
+ * @length: Length of the request in bytes, including this struct and any
+ * parameters (name, GUID, data) stored after it as well as any
+ * padding thereof required for alignment.
+ * @name_offset: Offset from the start of this struct to where the variable
+ * name is stored (as utf-16 string), in bytes.
+ * @name_size: Size of the name parameter in bytes, including null-terminator.
+ * @guid_offset: Offset from the start of this struct to where the GUID
+ * parameter is stored, in bytes.
+ * @guid_size: Size of the GUID parameter in bytes, i.e. sizeof(efi_guid_t).
+ * @attributes: The EFI variable attributes to set for this variable.
+ * @data_offset: Offset from the start of this struct to where the EFI variable
+ * data is stored, in bytes.
+ * @data_size: Size of EFI variable data, in bytes.
+ *
+ */
+struct qctee_req_uefi_set_variable {
+ u32 command_id;
+ u32 length;
+ u32 name_offset;
+ u32 name_size;
+ u32 guid_offset;
+ u32 guid_size;
+ u32 attributes;
+ u32 data_offset;
+ u32 data_size;
+} __packed;
+
+/**
+ * struct qctee_rsp_uefi_set_variable - Response for the SetVariable command.
+ * @command_id: The ID of the command. Should be %QCTEE_CMD_UEFI_SET_VARIABLE.
+ * @length: The length of this response, i.e. the size of this struct in
+ * bytes.
+ * @status: Status of this command.
+ * @_unknown1: Unknown response field.
+ * @_unknown2: Unknown response field.
+ */
+struct qctee_rsp_uefi_set_variable {
+ u32 command_id;
+ u32 length;
+ u32 status;
+ u32 _unknown1;
+ u32 _unknown2;
+} __packed;
+
+/**
+ * struct qctee_req_uefi_get_next_variable - Request for the
+ * GetNextVariableName command.
+ * @command_id: The ID of the command. Must be
+ * %QCTEE_CMD_UEFI_GET_NEXT_VARIABLE.
+ * @length: Length of the request in bytes, including this struct and any
+ * parameters (name, GUID) stored after it as well as any padding
+ * thereof for alignment.
+ * @guid_offset: Offset from the start of this struct to where the GUID
+ * parameter is stored, in bytes.
+ * @guid_size: Size of the GUID parameter in bytes, i.e. sizeof(efi_guid_t).
+ * @name_offset: Offset from the start of this struct to where the variable
+ * name is stored (as utf-16 string), in bytes.
+ * @name_size: Size of the name parameter in bytes, including null-terminator.
+ */
+struct qctee_req_uefi_get_next_variable {
+ u32 command_id;
+ u32 length;
+ u32 guid_offset;
+ u32 guid_size;
+ u32 name_offset;
+ u32 name_size;
+} __packed;
+
+/**
+ * struct qctee_rsp_uefi_get_next_variable - Response for the
+ * GetNextVariableName command.
+ * @command_id: The ID of the command. Should be
+ * %QCTEE_CMD_UEFI_GET_NEXT_VARIABLE.
+ * @length: Length of the response in bytes, including this struct and any
+ * parameters (name, GUID) stored after it as well as any padding
+ * thereof for alignment.
+ * @status: Status of this command.
+ * @guid_size: Size of the GUID parameter in bytes, i.e. sizeof(efi_guid_t).
+ * @name_offset: Offset from the start of this struct to where the variable
+ * name is stored (as utf-16 string), in bytes.
+ * @name_size: Size of the name parameter in bytes, including null-terminator.
+ */
+struct qctee_rsp_uefi_get_next_variable {
+ u32 command_id;
+ u32 length;
+ u32 status;
+ u32 guid_offset;
+ u32 guid_size;
+ u32 name_offset;
+ u32 name_size;
+} __packed;
+
+/**
+ * struct qctee_req_uefi_query_variable_info - Response for the
+ * GetNextVariableName command.
+ * @command_id: The ID of the command. Must be
+ * %QCTEE_CMD_UEFI_QUERY_VARIABLE_INFO.
+ * @length: The length of this request, i.e. the size of this struct in
+ * bytes.
+ * @attributes: The storage attributes to query the info for.
+ */
+struct qctee_req_uefi_query_variable_info {
+ u32 command_id;
+ u32 length;
+ u32 attributes;
+} __packed;
+
+/**
+ * struct qctee_rsp_uefi_query_variable_info - Response for the
+ * GetNextVariableName command.
+ * @command_id: The ID of the command. Must be
+ * %QCTEE_CMD_UEFI_QUERY_VARIABLE_INFO.
+ * @length: The length of this response, i.e. the size of this
+ * struct in bytes.
+ * @status: Status of this command.
+ * @_pad: Padding.
+ * @storage_space: Full storage space size, in bytes.
+ * @remaining_space: Free storage space available, in bytes.
+ * @max_variable_size: Maximum variable data size, in bytes.
+ */
+struct qctee_rsp_uefi_query_variable_info {
+ u32 command_id;
+ u32 length;
+ u32 status;
+ u32 _pad;
+ u64 storage_space;
+ u64 remaining_space;
+ u64 max_variable_size;
+} __packed;
+
+
+/* -- UEFI app interface. --------------------------------------------------- */
+
+struct qcuefi_client {
+ struct device *dev;
+ struct kobject *kobj;
+ struct efivars efivars;
+ struct qctee_dma dma;
+ u32 app_id;
+};
+
+static efi_status_t qctee_uefi_status_to_efi(u32 status)
+{
+ u64 category = status & 0xf0000000;
+ u64 code = status & 0x0fffffff;
+
+ return category << (BITS_PER_LONG - 32) | code;
+}
+
+static efi_status_t qctee_uefi_get_variable(struct qcuefi_client *qcuefi, const efi_char16_t *name,
+ const efi_guid_t *guid, u32 *attributes,
+ unsigned long *data_size, void *data)
+{
+ struct qctee_req_uefi_get_variable *req_data;
+ struct qctee_rsp_uefi_get_variable *rsp_data;
+ struct qctee_dma dma_req;
+ struct qctee_dma dma_rsp;
+ unsigned long name_size = utf16_strsize(name);
+ unsigned long buffer_size = *data_size;
+ unsigned long size;
+ efi_status_t efi_status;
+ int status;
+
+ /* Validation: We need a name and GUID. */
+ if (!name || !guid)
+ return EFI_INVALID_PARAMETER;
+
+ /* Validation: We need a buffer if the buffer_size is nonzero. */
+ if (buffer_size && !data)
+ return EFI_INVALID_PARAMETER;
+
+ /* Compute required size (upper limit with alignments). */
+ size = sizeof(*req_data) + sizeof(*guid) + name_size /* Inputs. */
+ + sizeof(*rsp_data) + buffer_size /* Outputs. */
+ + 2 * (QCTEE_DMA_ALIGNMENT - 1) /* Input parameter alignments. */
+ + 1 * (QCTEE_DMA_ALIGNMENT - 1); /* Output parameter alignments. */
+
+ /* Make sure we have enough DMA memory. */
+ status = qctee_dma_realloc(qcuefi->dev, &qcuefi->dma, size, GFP_KERNEL);
+ if (status)
+ return EFI_OUT_OF_RESOURCES;
+
+ /* Align request struct. */
+ qctee_dma_aligned(&qcuefi->dma, &dma_req, 0);
+ req_data = dma_req.virt;
+
+ /* Set up request data. */
+ req_data->command_id = QCTEE_CMD_UEFI_GET_VARIABLE;
+ req_data->data_size = buffer_size;
+ req_data->name_offset = sizeof(*req_data);
+ req_data->name_size = name_size;
+ req_data->guid_offset = QCTEE_DMA_ALIGN(req_data->name_offset + name_size);
+ req_data->guid_size = sizeof(*guid);
+ req_data->length = req_data->guid_offset + req_data->guid_size;
+
+ dma_req.size = req_data->length;
+
+ /* Copy request parameters. */
+ utf16_copy_to_buf(dma_req.virt + req_data->name_offset, name, name_size);
+ memcpy(dma_req.virt + req_data->guid_offset, guid, req_data->guid_size);
+
+ /* Align response struct. */
+ qctee_dma_aligned(&qcuefi->dma, &dma_rsp, req_data->length);
+ rsp_data = dma_rsp.virt;
+
+ /* Perform SCM call. */
+ status = qctee_app_send(qcuefi->dev, qcuefi->app_id, &dma_req, &dma_rsp);
+
+ /* Check for errors and validate. */
+ if (status)
+ return EFI_DEVICE_ERROR;
+
+ if (rsp_data->command_id != QCTEE_CMD_UEFI_GET_VARIABLE)
+ return EFI_DEVICE_ERROR;
+
+ if (rsp_data->length < sizeof(*rsp_data) || rsp_data->length > dma_rsp.size)
+ return EFI_DEVICE_ERROR;
+
+ if (rsp_data->status) {
+ dev_dbg(qcuefi->dev, "%s: uefisecapp error: 0x%x\n", __func__, rsp_data->status);
+ efi_status = qctee_uefi_status_to_efi(rsp_data->status);
+
+ /* Update size and attributes in case buffer is too small. */
+ if (efi_status == EFI_BUFFER_TOO_SMALL) {
+ *data_size = rsp_data->data_size;
+ if (attributes)
+ *attributes = rsp_data->attributes;
+ }
+
+ return efi_status;
+ }
+
+ if (rsp_data->data_offset + rsp_data->data_size > rsp_data->length)
+ return EFI_DEVICE_ERROR;
+
+ /* Set attributes and data size even if buffer is too small. */
+ *data_size = rsp_data->data_size;
+ if (attributes)
+ *attributes = rsp_data->attributes;
+
+ /*
+ * If we have a buffer size of zero and no buffer, just return
+ * attributes and required size.
+ */
+ if (buffer_size == 0 && !data)
+ return EFI_SUCCESS;
+
+ /* Validate output buffer size. */
+ if (buffer_size < rsp_data->data_size)
+ return EFI_BUFFER_TOO_SMALL;
+
+ /* Copy to output buffer. Note: We're guaranteed to have one at this point. */
+ memcpy(data, dma_rsp.virt + rsp_data->data_offset, rsp_data->data_size);
+ return EFI_SUCCESS;
+}
+
+static efi_status_t qctee_uefi_set_variable(struct qcuefi_client *qcuefi, const efi_char16_t *name,
+ const efi_guid_t *guid, u32 attributes,
+ unsigned long data_size, const void *data)
+{
+ struct qctee_req_uefi_set_variable *req_data;
+ struct qctee_rsp_uefi_set_variable *rsp_data;
+ struct qctee_dma dma_req;
+ struct qctee_dma dma_rsp;
+ unsigned long name_size = utf16_strsize(name);
+ unsigned long size;
+ int status;
+
+ /* Validate inputs. */
+ if (!name || !guid)
+ return EFI_INVALID_PARAMETER;
+
+ /*
+ * Make sure we have some data if data_size is nonzero. Note: Using a
+ * size of zero is valid and deletes the variable.
+ */
+ if (data_size && !data)
+ return EFI_INVALID_PARAMETER;
+
+ /* Compute required size (upper limit with alignments). */
+ size = sizeof(*req_data) + name_size + sizeof(*guid) + data_size /* Inputs. */
+ + sizeof(*rsp_data) /* Outputs. */
+ + 2 * (QCTEE_DMA_ALIGNMENT - 1) /* Input parameter alignments. */
+ + 1 * (QCTEE_DMA_ALIGNMENT - 1); /* Output parameter alignments. */
+
+ /* Make sure we have enough DMA memory. */
+ status = qctee_dma_realloc(qcuefi->dev, &qcuefi->dma, size, GFP_KERNEL);
+ if (status)
+ return EFI_OUT_OF_RESOURCES;
+
+ /* Align request struct. */
+ qctee_dma_aligned(&qcuefi->dma, &dma_req, 0);
+ req_data = dma_req.virt;
+
+ /* Set up request data. */
+ req_data->command_id = QCTEE_CMD_UEFI_SET_VARIABLE;
+ req_data->attributes = attributes;
+ req_data->name_offset = sizeof(*req_data);
+ req_data->name_size = name_size;
+ req_data->guid_offset = QCTEE_DMA_ALIGN(req_data->name_offset + name_size);
+ req_data->guid_size = sizeof(*guid);
+ req_data->data_offset = req_data->guid_offset + req_data->guid_size;
+ req_data->data_size = data_size;
+ req_data->length = req_data->data_offset + data_size;
+
+ /* Copy request parameters. */
+ utf16_copy_to_buf(dma_req.virt + req_data->name_offset, name, req_data->name_size);
+ memcpy(dma_req.virt + req_data->guid_offset, guid, req_data->guid_size);
+
+ if (data_size)
+ memcpy(dma_req.virt + req_data->data_offset, data, req_data->data_size);
+
+ /* Align response struct. */
+ qctee_dma_aligned(&qcuefi->dma, &dma_rsp, req_data->length);
+ rsp_data = dma_rsp.virt;
+
+ /* Perform SCM call. */
+ dma_req.size = req_data->length;
+ dma_rsp.size = sizeof(*rsp_data);
+
+ status = qctee_app_send(qcuefi->dev, qcuefi->app_id, &dma_req, &dma_rsp);
+
+ /* Check for errors and validate. */
+ if (status)
+ return EFI_DEVICE_ERROR;
+
+ if (rsp_data->command_id != QCTEE_CMD_UEFI_SET_VARIABLE)
+ return EFI_DEVICE_ERROR;
+
+ if (rsp_data->length < sizeof(*rsp_data) || rsp_data->length > dma_rsp.size)
+ return EFI_DEVICE_ERROR;
+
+ if (rsp_data->status) {
+ dev_dbg(qcuefi->dev, "%s: uefisecapp error: 0x%x\n", __func__, rsp_data->status);
+ return qctee_uefi_status_to_efi(rsp_data->status);
+ }
+
+ return EFI_SUCCESS;
+}
+
+static efi_status_t qctee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
+ unsigned long *name_size, efi_char16_t *name,
+ efi_guid_t *guid)
+{
+ struct qctee_req_uefi_get_next_variable *req_data;
+ struct qctee_rsp_uefi_get_next_variable *rsp_data;
+ struct qctee_dma dma_req;
+ struct qctee_dma dma_rsp;
+ unsigned long size;
+ efi_status_t efi_status;
+ int status;
+
+ /* We need some buffers. */
+ if (!name_size || !name || !guid)
+ return EFI_INVALID_PARAMETER;
+
+ /* There needs to be at least a single null-character. */
+ if (*name_size == 0)
+ return EFI_INVALID_PARAMETER;
+
+ /* Compute required size (upper limit with alignments). */
+ size = sizeof(*req_data) + sizeof(*guid) + *name_size /* Inputs. */
+ + sizeof(*rsp_data) + sizeof(*guid) + *name_size /* Outputs. */
+ + 2 * (QCTEE_DMA_ALIGNMENT - 1) /* Input parameter alignments. */
+ + 1 * (QCTEE_DMA_ALIGNMENT - 1); /* Output parameter alignments. */
+
+ /* Make sure we have enough DMA memory. */
+ status = qctee_dma_realloc(qcuefi->dev, &qcuefi->dma, size, GFP_KERNEL);
+ if (status)
+ return EFI_OUT_OF_RESOURCES;
+
+ /* Align request struct. */
+ qctee_dma_aligned(&qcuefi->dma, &dma_req, 0);
+ req_data = dma_req.virt;
+
+ /* Set up request data. */
+ req_data->command_id = QCTEE_CMD_UEFI_GET_NEXT_VARIABLE;
+ req_data->guid_offset = QCTEE_DMA_ALIGN(sizeof(*req_data));
+ req_data->guid_size = sizeof(*guid);
+ req_data->name_offset = req_data->guid_offset + req_data->guid_size;
+ req_data->name_size = *name_size;
+ req_data->length = req_data->name_offset + req_data->name_size;
+
+ dma_req.size = req_data->length;
+
+ /* Copy request parameters. */
+ memcpy(dma_req.virt + req_data->guid_offset, guid, req_data->guid_size);
+ utf16_copy_to_buf(dma_req.virt + req_data->name_offset, name, *name_size);
+
+ /* Align response struct. */
+ qctee_dma_aligned(&qcuefi->dma, &dma_rsp, req_data->length);
+ rsp_data = dma_rsp.virt;
+
+ /* Perform SCM call. */
+ status = qctee_app_send(qcuefi->dev, qcuefi->app_id, &dma_req, &dma_rsp);
+
+ /* Check for errors and validate. */
+ if (status)
+ return EFI_DEVICE_ERROR;
+
+ if (rsp_data->command_id != QCTEE_CMD_UEFI_GET_NEXT_VARIABLE)
+ return EFI_DEVICE_ERROR;
+
+ if (rsp_data->length < sizeof(*rsp_data) || rsp_data->length > dma_rsp.size)
+ return EFI_DEVICE_ERROR;
+
+ if (rsp_data->status) {
+ dev_dbg(qcuefi->dev, "%s: uefisecapp error: 0x%x\n", __func__, rsp_data->status);
+ efi_status = qctee_uefi_status_to_efi(rsp_data->status);
+
+ /* Update size with required size in case buffer is too small. */
+ if (efi_status == EFI_BUFFER_TOO_SMALL)
+ *name_size = rsp_data->name_size;
+
+ return efi_status;
+ }
+
+ if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length)
+ return EFI_DEVICE_ERROR;
+
+ if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length)
+ return EFI_DEVICE_ERROR;
+
+ if (rsp_data->name_size > *name_size) {
+ *name_size = rsp_data->name_size;
+ return EFI_BUFFER_TOO_SMALL;
+ }
+
+ if (rsp_data->guid_size != sizeof(*guid))
+ return EFI_DEVICE_ERROR;
+
+ /* Copy response fields. */
+ memcpy(guid, dma_rsp.virt + rsp_data->guid_offset, rsp_data->guid_size);
+ utf16_copy_to_buf(name, dma_rsp.virt + rsp_data->name_offset, rsp_data->name_size);
+ *name_size = rsp_data->name_size;
+
+ return 0;
+}
+
+
+/* -- Global efivar interface. ---------------------------------------------- */
+
+static struct qcuefi_client *__qcuefi;
+static DEFINE_MUTEX(__qcuefi_lock);
+
+static int qcuefi_set_reference(struct qcuefi_client *qcuefi)
+{
+ mutex_lock(&__qcuefi_lock);
+
+ if (qcuefi && __qcuefi) {
+ mutex_unlock(&__qcuefi_lock);
+ return -EEXIST;
+ }
+
+ __qcuefi = qcuefi;
+
+ mutex_unlock(&__qcuefi_lock);
+ return 0;
+}
+
+static struct qcuefi_client *qcuefi_acquire(void)
+{
+ mutex_lock(&__qcuefi_lock);
+ return __qcuefi;
+}
+
+static void qcuefi_release(void)
+{
+ mutex_unlock(&__qcuefi_lock);
+}
+
+static efi_status_t qcuefi_get_variable(efi_char16_t *name, efi_guid_t *vendor, u32 *attr,
+ unsigned long *data_size, void *data)
+{
+ struct qcuefi_client *qcuefi;
+ efi_status_t status;
+
+ qcuefi = qcuefi_acquire();
+ if (!qcuefi)
+ return EFI_NOT_READY;
+
+ status = qctee_uefi_get_variable(qcuefi, name, vendor, attr, data_size, data);
+
+ qcuefi_release();
+ return status;
+}
+
+static efi_status_t qcuefi_set_variable(efi_char16_t *name, efi_guid_t *vendor,
+ u32 attr, unsigned long data_size, void *data)
+{
+ struct qcuefi_client *qcuefi;
+ efi_status_t status;
+
+ qcuefi = qcuefi_acquire();
+ if (!qcuefi)
+ return EFI_NOT_READY;
+
+ status = qctee_uefi_set_variable(qcuefi, name, vendor, attr, data_size, data);
+
+ qcuefi_release();
+ return status;
+}
+
+static efi_status_t qcuefi_get_next_variable(unsigned long *name_size, efi_char16_t *name,
+ efi_guid_t *vendor)
+{
+ struct qcuefi_client *qcuefi;
+ efi_status_t status;
+
+ qcuefi = qcuefi_acquire();
+ if (!qcuefi)
+ return EFI_NOT_READY;
+
+ status = qctee_uefi_get_next_variable(qcuefi, name_size, name, vendor);
+
+ qcuefi_release();
+ return status;
+}
+
+static const struct efivar_operations qcom_efivar_ops = {
+ .get_variable = qcuefi_get_variable,
+ .set_variable = qcuefi_set_variable,
+ .get_next_variable = qcuefi_get_next_variable,
+};
+
+
+/* -- Driver setup. --------------------------------------------------------- */
+
+static int qcom_uefisecapp_probe(struct platform_device *pdev)
+{
+ struct qcuefi_client *qcuefi;
+ int status;
+
+ /* Defer until SCM is available. */
+ if (!qcom_scm_is_available())
+ return -EPROBE_DEFER;
+
+ /* Allocate driver data. */
+ qcuefi = devm_kzalloc(&pdev->dev, sizeof(*qcuefi), GFP_KERNEL);
+ if (!qcuefi)
+ return -ENOMEM;
+
+ qcuefi->dev = &pdev->dev;
+
+ /* Get application id for uefisecapp. */
+ status = qctee_app_get_id(&pdev->dev, QCTEE_UEFISEC_APP_NAME, &qcuefi->app_id);
+ if (status) {
+ dev_err(&pdev->dev, "failed to query app ID: %d\n", status);
+ return status;
+ }
+
+ /* Set up DMA. One page should be plenty to start with. */
+ if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(64))) {
+ dev_warn(&pdev->dev, "no suitable DMA available\n");
+ return -EFAULT;
+ }
+
+ status = qctee_dma_alloc(&pdev->dev, &qcuefi->dma, PAGE_SIZE, GFP_KERNEL);
+ if (status)
+ return status;
+
+ /* Set up kobject for efivars interface. */
+ qcuefi->kobj = kobject_create_and_add("qcom_tee_uefisecapp", firmware_kobj);
+ if (!qcuefi->kobj) {
+ status = -ENOMEM;
+ goto err_kobj;
+ }
+
+ /* Register global reference. */
+ platform_set_drvdata(pdev, qcuefi);
+ status = qcuefi_set_reference(qcuefi);
+ if (status)
+ goto err_ref;
+
+ /* Register efivar ops. */
+ status = efivars_register(&qcuefi->efivars, &qcom_efivar_ops, qcuefi->kobj);
+ if (status)
+ goto err_register;
+
+ return 0;
+
+err_register:
+ qcuefi_set_reference(NULL);
+err_ref:
+ kobject_put(qcuefi->kobj);
+err_kobj:
+ qctee_dma_free(qcuefi->dev, &qcuefi->dma);
+ return status;
+}
+
+static int qcom_uefisecapp_remove(struct platform_device *pdev)
+{
+ struct qcuefi_client *qcuefi = platform_get_drvdata(pdev);
+
+ /* Unregister efivar ops. */
+ efivars_unregister(&qcuefi->efivars);
+
+ /* Block on pending calls and unregister global reference. */
+ qcuefi_set_reference(NULL);
+
+ /* Free remaining resources. */
+ kobject_put(qcuefi->kobj);
+ qctee_dma_free(qcuefi->dev, &qcuefi->dma);
+
+ return 0;
+}
+
+static const struct of_device_id qcom_uefisecapp_dt_match[] = {
+ { .compatible = "qcom,tee-uefisecapp", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, qcom_uefisecapp_dt_match);
+
+static struct platform_driver qcom_uefisecapp_driver = {
+ .probe = qcom_uefisecapp_probe,
+ .remove = qcom_uefisecapp_remove,
+ .driver = {
+ .name = "qcom_tee_uefisecapp",
+ .of_match_table = qcom_uefisecapp_dt_match,
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ },
+};
+module_platform_driver(qcom_uefisecapp_driver);
+
+MODULE_AUTHOR("Maximilian Luz <[email protected]>");
+MODULE_DESCRIPTION("Client driver for Qualcomm TrEE/TZ UEFI Secure App");
+MODULE_LICENSE("GPL");
--
2.37.1

2022-07-25 01:13:25

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On Sun, 24 Jul 2022 00:49:49 +0200, Maximilian Luz wrote:
> Add bindings for the Qualcomm Trusted Execution Environment (TrEE) UEFI
> Secure application (uefisecapp) client.
>
> Signed-off-by: Maximilian Luz <[email protected]>
> ---
> .../firmware/qcom,tee-uefisecapp.yaml | 38 +++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 39 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
>

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml: $id: relative path/filename doesn't match actual path or filename
expected: http://devicetree.org/schemas/firmware/qcom,tee-uefisecapp.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/soc/qcom/qcom,rpmh-rsc.yaml: duplicate '$id' value 'http://devicetree.org/schemas/soc/qcom/qcom,rpmh-rsc.yaml#'
Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.example.dtb:0:0: /example-0/firmware/scm: failed to match any schema with compatible: ['qcom,scm-sc8180x', 'qcom,scm']
Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.example.dtb:0:0: /example-0/firmware/scm: failed to match any schema with compatible: ['qcom,scm-sc8180x', 'qcom,scm']
Documentation/devicetree/bindings/soc/qcom/qcom,rpmh-rsc.example.dtb:0:0: /example-0/rsc@179c0000: failed to match any schema with compatible: ['qcom,rpmh-rsc']
Documentation/devicetree/bindings/soc/qcom/qcom,rpmh-rsc.example.dtb:0:0: /example-1/rsc@af20000: failed to match any schema with compatible: ['qcom,rpmh-rsc']
Documentation/devicetree/bindings/soc/qcom/qcom,rpmh-rsc.example.dtb:0:0: /example-2/rsc@18200000: failed to match any schema with compatible: ['qcom,rpmh-rsc']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

2022-07-25 19:56:43

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH 0/4] firmware: Add support for Qualcomm UEFI Secure Application

On Sun, Jul 24, 2022 at 12:49:45AM +0200, Maximilian Luz wrote:
> On modern Qualcomm platforms, access to EFI variables is restricted to
> the secure world / TrustZone, i.e. the Trusted Execution Environment
> (TrEE or TEE) as Qualcomm seems to call it. To access EFI variables, we
> therefore need to talk to the UEFI Secure Application (uefisecapp),
> residing in the TrEE.

The whole point of UEFI is providing a standard interface. Why can't the
UEFI implementation call the TEE itself?

I'm not sure custom interfaces is something we want.

> This series adds support for accessing EFI variables on those platforms.
>
> To do this, we first need to add some SCM call functions used to manage
> and talk to Secure Applications. A very small subset of this interface
> is added in the second patch (whereas the first one exports the required
> functions for that). Interface specifications are extracted from [1].
> While this does not (yet) support re-entrant SCM calls (including
> callbacks and listeners), this is enough to talk to the aforementioned
> uefisecapp on a couple of platforms (I've tested this on a Surface Pro X
> and heard reports from Lenovo Flex 5G, Lenovo Thinkpad x13s, and Lenovo
> Yoga C630 devices).

What does Windows do on these devices? I'm surprised something like this
would fly with Microsoft.

Rob

2022-07-25 21:06:17

[permalink] [raw]

Subject: Re: [PATCH 0/4] firmware: Add support for Qualcomm UEFI Secure Application

On 7/25/22 21:27, Rob Herring wrote:
> On Sun, Jul 24, 2022 at 12:49:45AM +0200, Maximilian Luz wrote:
>> On modern Qualcomm platforms, access to EFI variables is restricted to
>> the secure world / TrustZone, i.e. the Trusted Execution Environment
>> (TrEE or TEE) as Qualcomm seems to call it. To access EFI variables, we
>> therefore need to talk to the UEFI Secure Application (uefisecapp),
>> residing in the TrEE.
>
> The whole point of UEFI is providing a standard interface. Why can't the
> UEFI implementation call the TEE itself?
>
> I'm not sure custom interfaces is something we want.

Unfortunately, I'm not a Qualcomm engineer and in no way affiliated with
them. So I probably can't convince them otherwise. Believe me, I'd like
to :/

First: The uefisecapp-driver is based on reverse-engineering. So please
take things below with a grain of salt, I may be wrong. I've tried to
lay this out in a bit more detail in patch 3, but I'll try to be more
precise here:

For some reason unknown to me, Qualcomm decided to lock away UEFI
variable access via their TrEE framework for applications running in the
TrustZone (or whatever that is exactly). To call to TrEE applications
(like uefisecapp), they use SCM calls. Those SCM calls unfortunately can
be a bit more complex. As far as I can tell, you essentially call to
some hypervisor in the TrustZone which redistributes them (if necessary)
to the respective application. Their downstream driver for that is at
[1] and supports callbacks and re-entrant calls. As far as I can tell,
the latter means that you can't run multiple SCM calls in parallel (at
least not to that TzOS/TrEE interface) and can only ever have one
"client" performing them.

And as you can only ever have one entity performing those SCM calls, you
cannot have both UEFI and the kernel doing them. To me, it seems to be a
deliberate decision by Qualcomm to return EFI_UNSUPPORTED from the
GetVariable etc. calls after exiting boot services. They do work just
fine before that. Essentially exiting boot services transfers ownership
of that SCM interface from UEFI to the kernel.

Note: uefisecapp is also not the only TrEE application in use by those
kinds of devices. For example, on the SC8180X based Surface Pro X that
I'm using, there are at least an app for the TPM, a bunch of apps for
HDCP, some winsecapp, and as far as I can tell also other cryptographic
interfaces. I've tried to collect my findings about those in [2].

>> This series adds support for accessing EFI variables on those platforms.
>>
>> To do this, we first need to add some SCM call functions used to manage
>> and talk to Secure Applications. A very small subset of this interface
>> is added in the second patch (whereas the first one exports the required
>> functions for that). Interface specifications are extracted from [1].
>> While this does not (yet) support re-entrant SCM calls (including
>> callbacks and listeners), this is enough to talk to the aforementioned
>> uefisecapp on a couple of platforms (I've tested this on a Surface Pro X
>> and heard reports from Lenovo Flex 5G, Lenovo Thinkpad x13s, and Lenovo
>> Yoga C630 devices).
>
> What does Windows do on these devices? I'm surprised something like this
> would fly with Microsoft.

It looks like Microsoft accepts this. They even seem to have some sort
of interface for EFI variables via trusted execution environments: [3].
This is essentially what the QcTrEE8180.sys driver I've reverse
engineered this from seems to provide.

So unless there's some way to make EFI variables work via the standard
functions that I've missed, I don't see any alternatives. I think it's
fairly unlikely that we can convince Qualcomm to make their UEFI
implementation behave properly (variables are not the only issue, it
seems that other functions are either partially or completely broken in
some way or another...) and then also push updates for a bunch of
devices (e.g. the Lenovo C630 also using this stuff is discontinued, as
far as I can tell).

I am open to suggestions though...

Note that this series also doesn't really introduce a new interface for
EFI variables themselves to the kernel. It relies on the existing
efivars_register() / efivars_unregister() functions and the interface
provided by them to enable access to EFI variables. So we already do
have a "workaround" for broken UEFI variable access in the kernel.

Regards,
Max

[1]: https://git.codelinaro.org/clo/la/kernel/msm-4.14/-/blob/auto-kernel.lnx.4.14.c34/drivers/misc/qseecom.c
[2]: https://github.com/linux-surface/surface-pro-x/issues/37
[3]: https://github.com/tpn/winsdk-10/blob/9b69fd26ac0c7d0b83d378dba01080e93349c2ed/Include/10.0.16299.0/km/treevariableservice.h#L11-L12

2022-07-26 10:25:17

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 24/07/2022 00:49, Maximilian Luz wrote:
> Add bindings for the Qualcomm Trusted Execution Environment (TrEE) UEFI
> Secure application (uefisecapp) client.
>
> Signed-off-by: Maximilian Luz <[email protected]>
> ---
> .../firmware/qcom,tee-uefisecapp.yaml | 38 +++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 39 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
>
> diff --git a/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml b/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
> new file mode 100644
> index 000000000000..9e5de1005d5c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/qcom/qcom,rpmh-rsc.yaml#

Does not look like you tested the bindings. Please run `make
dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Trusted Execution Environment UEFI Secure Application
> +
> +maintainers:
> + - Maximilian Luz <[email protected]>
> +
> +description: |
> + Various Qualcomm SoCs do not allow direct access to UEFI variables. Instead,
> + these need to be accessed via the UEFI Secure Application (uefisecapp),
> + residing in the Trusted Execution Environment (TrEE). These bindings mark the
> + presence of uefisecapp and allow the respective client driver to load and
> + install efivar operations, providing the kernel with access to UEFI
> + variables.
> +
> +properties:
> + compatible:
> + const: qcom,tee-uefisecapp

Isn't this SoC-specific device? Generic compatibles are usually not
expected.

> +
> +required:
> + - compatible
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + firmware {
> + scm {
> + compatible = "qcom,scm-sc8180x", "qcom,scm";
> + };
> + tee-uefisecapp {
> + compatible = "qcom,tee-uefisecapp";

You did not model here any dependency on SCM. This is not full
description of the firmware/hardware.

Best regards,
Krzysztof

2022-07-26 11:50:34

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

Hi,

On 7/26/22 12:17, Krzysztof Kozlowski wrote:
> On 24/07/2022 00:49, Maximilian Luz wrote:
>> Add bindings for the Qualcomm Trusted Execution Environment (TrEE) UEFI
>> Secure application (uefisecapp) client.
>>
>> Signed-off-by: Maximilian Luz <[email protected]>
>> ---
>> .../firmware/qcom,tee-uefisecapp.yaml | 38 +++++++++++++++++++
>> MAINTAINERS | 1 +
>> 2 files changed, 39 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml b/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
>> new file mode 100644
>> index 000000000000..9e5de1005d5c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
>> @@ -0,0 +1,38 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/soc/qcom/qcom,rpmh-rsc.yaml#
>
> Does not look like you tested the bindings. Please run `make
> dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).

Sorry, first time submitting a schema. Already saw the warning of Rob's
bot and Will fix this in v2.

>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Trusted Execution Environment UEFI Secure Application
>> +
>> +maintainers:
>> + - Maximilian Luz <[email protected]>
>> +
>> +description: |
>> + Various Qualcomm SoCs do not allow direct access to UEFI variables. Instead,
>> + these need to be accessed via the UEFI Secure Application (uefisecapp),
>> + residing in the Trusted Execution Environment (TrEE). These bindings mark the
>> + presence of uefisecapp and allow the respective client driver to load and
>> + install efivar operations, providing the kernel with access to UEFI
>> + variables.
>> +
>> +properties:
>> + compatible:
>> + const: qcom,tee-uefisecapp
>
> Isn't this SoC-specific device? Generic compatibles are usually not
> expected.

This is essentially software (kernel driver) talking to software (in the
TrustZone), so I don't expect there to be anything SoC specific about it.

>> +
>> +required:
>> + - compatible
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + firmware {
>> + scm {
>> + compatible = "qcom,scm-sc8180x", "qcom,scm";
>> + };
>> + tee-uefisecapp {
>> + compatible = "qcom,tee-uefisecapp";
>
> You did not model here any dependency on SCM. This is not full
> description of the firmware/hardware

How would I do that? A lot of other stuff also depends on SCM being
present (e.g. qcom_q6v5_pas for loading mdt files) and I don't see them
declare this in the device tree. As far as I can tell, SCM is pretty
much expected to be there at all times (i.e. can't be unloaded) and
drivers check for it when probing via qcom_scm_is_available(),
deferring probe if not.

Don't take this as an excuse as in "I want to leave that out", it's just
that I don't know how one would declare such a dependency explicitly. If
you can tell me how to fix it, I'll include that for v2.

Regards,
Max

2022-07-26 13:32:53

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 26/07/2022 13:15, Maximilian Luz wrote:
>>> +properties:
>>> + compatible:
>>> + const: qcom,tee-uefisecapp
>>
>> Isn't this SoC-specific device? Generic compatibles are usually not
>> expected.
>
> This is essentially software (kernel driver) talking to software (in the
> TrustZone), so I don't expect there to be anything SoC specific about it.

You are documenting here firmware in TZ (not kernel driver). Isn't this
a specific piece which might vary from device to device?

IOW, do you expect the same compatible to work for all possible Qualcomm
boards (past and future like in 10 years from now)?

>
>>> +
>>> +required:
>>> + - compatible
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + firmware {
>>> + scm {
>>> + compatible = "qcom,scm-sc8180x", "qcom,scm";
>>> + };
>>> + tee-uefisecapp {
>>> + compatible = "qcom,tee-uefisecapp";
>>
>> You did not model here any dependency on SCM. This is not full
>> description of the firmware/hardware
>
> How would I do that? A lot of other stuff also depends on SCM being
> present (e.g. qcom_q6v5_pas for loading mdt files) and I don't see them
> declare this in the device tree. As far as I can tell, SCM is pretty
> much expected to be there at all times (i.e. can't be unloaded) and
> drivers check for it when probing via qcom_scm_is_available(),
> deferring probe if not.

It seems this will be opening a can of worms...

The problem with existing approach is:
1. Lack of any probe ordering or probe deferral support.
2. Lack of any other dependencies, e.g. for PM.

Unloading is "solved" only by disallowing the unload, not by proper
device links and module get/put.

I understand that SCM must be there, but the same for several other
components and for these others we have ways to pass reference around
(e.g. syscon regmap, PHYs handles).

>
> Don't take this as an excuse as in "I want to leave that out", it's just
> that I don't know how one would declare such a dependency explicitly. If
> you can tell me how to fix it, I'll include that for v2.

I think there are no dedicated subsystem helpers for this (like for
provider/consumer of resets/power domains/clocks etc), so one way would
be something like nvidia,bpmp is doing.

meson_sm_get is a bit similar - looking by compatible. This is less
portable and I would prefer the bpmp way (just like syscon phandles).

The qcom_q6v5_pas could be converted later to use similar approach and
eventually the "tatic struct qcom_scm *__scm;" can be entirely removed.

Any comments on this approach from Konrad, Bjorn, Dmitry, Vinod and
anyone else?

Best regards,
Krzysztof

2022-07-26 14:37:28

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On Sun, Jul 24, 2022 at 12:49:49AM +0200, Maximilian Luz wrote:
> Add bindings for the Qualcomm Trusted Execution Environment (TrEE) UEFI
> Secure application (uefisecapp) client.
>
> Signed-off-by: Maximilian Luz <[email protected]>
> ---
> .../firmware/qcom,tee-uefisecapp.yaml | 38 +++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 39 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
>
> diff --git a/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml b/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
> new file mode 100644
> index 000000000000..9e5de1005d5c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/qcom/qcom,rpmh-rsc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Trusted Execution Environment UEFI Secure Application
> +
> +maintainers:
> + - Maximilian Luz <[email protected]>
> +
> +description: |
> + Various Qualcomm SoCs do not allow direct access to UEFI variables. Instead,
> + these need to be accessed via the UEFI Secure Application (uefisecapp),
> + residing in the Trusted Execution Environment (TrEE). These bindings mark the
> + presence of uefisecapp and allow the respective client driver to load and
> + install efivar operations, providing the kernel with access to UEFI
> + variables.
> +
> +properties:
> + compatible:
> + const: qcom,tee-uefisecapp
> +
> +required:
> + - compatible
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + firmware {
> + scm {
> + compatible = "qcom,scm-sc8180x", "qcom,scm";
> + };
> + tee-uefisecapp {
> + compatible = "qcom,tee-uefisecapp";
> + };

Do you expect some issues using the scm driver APIs without the
any additions in the DT ? I mean can't you auto-discover by using the
APIs. I haven't looked at the driver or any other patches in the series,
but I would like to know if we can avoid adding any new bindings if it
can be discovered via those SCM driver APIs.

--
Regards,
Sudeep

2022-07-26 15:18:22

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 7/26/22 16:30, Sudeep Holla wrote:
> On Sun, Jul 24, 2022 at 12:49:49AM +0200, Maximilian Luz wrote:
>> Add bindings for the Qualcomm Trusted Execution Environment (TrEE) UEFI
>> Secure application (uefisecapp) client.
>>

[...]

>> +examples:
>> + - |
>> + firmware {
>> + scm {
>> + compatible = "qcom,scm-sc8180x", "qcom,scm";
>> + };
>> + tee-uefisecapp {
>> + compatible = "qcom,tee-uefisecapp";
>> + };
>
> Do you expect some issues using the scm driver APIs without the
> any additions in the DT ? I mean can't you auto-discover by using the
> APIs. I haven't looked at the driver or any other patches in the series,
> but I would like to know if we can avoid adding any new bindings if it
> can be discovered via those SCM driver APIs.

Not at scale, at least as far as I can tell.

Part of the setup-process of this driver is to query an "application ID"
from a unique string identifying the application (in this case
"qcom.tz.uefisecapp"). If that call fails, we know the app is not there.

But: If we'd want to support more than just "uefisecapp" we'd have to
query each app in some predefined list. As far as I can tell, there's no
method to enumerate all present/loaded ones. The Windows driver seems to
use a hard-coded list of apps that are present on some specific SoC.

It might be possible that there exists such a method, but if it does, the
Windows driver doesn't seem to use it and I don't know about it.

Also, there would need to be at least some type of compatible to
indicate the presence of that TrEE / Secure Application interface used by
uefisecapp. Unless you want to send some potentially unsupported SCM
commands on every platform with qcom,scm and see what comes back.

So ultimately I think it's better to add a DT entry for it. That also
(hopefully) ensures that someone tested and (at least in some way)
validated this. Again, It's a reverse engineered driver.

Regards,
Max

2022-07-26 15:37:16

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 7/26/22 15:25, Krzysztof Kozlowski wrote:
> On 26/07/2022 13:15, Maximilian Luz wrote:
>>>> +properties:
>>>> + compatible:
>>>> + const: qcom,tee-uefisecapp
>>>
>>> Isn't this SoC-specific device? Generic compatibles are usually not
>>> expected.
>>
>> This is essentially software (kernel driver) talking to software (in the
>> TrustZone), so I don't expect there to be anything SoC specific about it.
>
> You are documenting here firmware in TZ (not kernel driver). Isn't this
> a specific piece which might vary from device to device?
>
> IOW, do you expect the same compatible to work for all possible Qualcomm
> boards (past and future like in 10 years from now)?

I'm not sure if Qualcomm will still use the "uefisecapp" approach in 10
years, but I don't expect the interface of uefisecapp to change. The
interface is modeled after the respective UEFI functions, which are spec
and thus I don't expect those to change. Also, it seems to have been
around for a couple of generations and it hasn't changed. The oldest
tested is sdm850 (Lenovo Yoga C630), and the latest is sc8280xp
(Thinkpad X13s).

Why not make this behave like a "normal" third-party device? If the
interface ever changes use qcom,tee-uefisecapp-v2 or something like
that? Again, this does not seem to be directly tied to the SoC.

Then again, if you prefer to name everything based on
"qcom,<device>-<soc>" I don't have any strong arguments against it and
I'm happy to change that. I just think it will unnecessarily introduce
a bunch of compatibles and doesn't reflect the interface "versioning"
situation as I see it.

>>>> +
>>>> +required:
>>>> + - compatible
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> + - |
>>>> + firmware {
>>>> + scm {
>>>> + compatible = "qcom,scm-sc8180x", "qcom,scm";
>>>> + };
>>>> + tee-uefisecapp {
>>>> + compatible = "qcom,tee-uefisecapp";
>>>
>>> You did not model here any dependency on SCM. This is not full
>>> description of the firmware/hardware
>>
>> How would I do that? A lot of other stuff also depends on SCM being
>> present (e.g. qcom_q6v5_pas for loading mdt files) and I don't see them
>> declare this in the device tree. As far as I can tell, SCM is pretty
>> much expected to be there at all times (i.e. can't be unloaded) and
>> drivers check for it when probing via qcom_scm_is_available(),
>> deferring probe if not.
>
> It seems this will be opening a can of worms...

Indeed.

> The problem with existing approach is:
> 1. Lack of any probe ordering or probe deferral support.
> 2. Lack of any other dependencies, e.g. for PM.

I'm not entirely sure what you mean by "lack of probe deferral support".
We have qcom_scm_is_available() and defer probe if that fails. So
deferral works, unless I'm misunderstanding something.

But yes, correct on the other points.

> Unloading is "solved" only by disallowing the unload, not by proper
> device links and module get/put.
>
> I understand that SCM must be there, but the same for several other
> components and for these others we have ways to pass reference around
> (e.g. syscon regmap, PHYs handles).
>
>>
>> Don't take this as an excuse as in "I want to leave that out", it's just
>> that I don't know how one would declare such a dependency explicitly. If
>> you can tell me how to fix it, I'll include that for v2.
>
> I think there are no dedicated subsystem helpers for this (like for
> provider/consumer of resets/power domains/clocks etc), so one way would
> be something like nvidia,bpmp is doing.

I assume you're referring to tegra_bpmp_get()? Does this correctly
handle PM dependencies? At least as far as I can tell it doesn't
explicitly establish a device link, it only gets a reference to the
device, which doesn't guarantee the presence of a driver. Nor correct PM
ordering. Please correct me if I'm wrong. As far as I know automatic
creation of device links only works with certain props defined in
of_supplier_bindings, right?

So unless I'm wrong there is also a bunch of other stuff that may be
subtly broken. (Again, not a justification to include these changes,
just wondering whether there should be a conscious approach to find and
fix these things... rather than discover them patch-by-patch).

> meson_sm_get is a bit similar - looking by compatible. This is less
> portable and I would prefer the bpmp way (just like syscon phandles).

I have another example (that could be improved via a phandle in DT): For
the Surface System Aggregator (in ACPI-land), we have ssam_client_bind().
This function 1) checks if the controller is available and ready, 2) if
it is gets a reference to it, and 3) establishes a device link for
PM-ordering, before 4) returning the reference to that controller to the
client. This combined with deferring probe ensures that we will always
have a valid reference. And since we're in DT-land, we could hook that
up with a phandle reference to SCM and load that instead of having to
use a global static.

> The qcom_q6v5_pas could be converted later to use similar approach and
> eventually the "tatic struct qcom_scm *__scm;" can be entirely removed.
>
> Any comments on this approach from Konrad, Bjorn, Dmitry, Vinod and
> anyone else?

Regards,
Max

2022-07-26 15:43:35

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On Tue, Jul 26, 2022 at 05:15:41PM +0200, Maximilian Luz wrote:
>
> So ultimately I think it's better to add a DT entry for it.

I disagree for the reason that once you discover more apps running on the
secure side, you want to add more entries and update DT on the platform
every time you discover some new firmware entity and you wish to interact
with it from the non-secure side.

As along as get this application ID can handle any random name, I prefer
to use that as the discover mechanism and not have this DT.
--
Regards,
Sudeep

2022-07-26 17:11:46

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 7/26/22 17:41, Sudeep Holla wrote:
> On Tue, Jul 26, 2022 at 05:15:41PM +0200, Maximilian Luz wrote:
>>
>> So ultimately I think it's better to add a DT entry for it.
>
> I disagree for the reason that once you discover more apps running on the
> secure side, you want to add more entries and update DT on the platform
> every time you discover some new firmware entity and you wish to interact
> with it from the non-secure side.

Just as you'll have to add a driver to the kernel and update whatever is
probing the TrEE interface and add those strings to that interface. If
you then start doing SoC-specific lists, I think you'd be pretty much
re-implementing a DT in the kernel driver...

I don't quite understand why this is a problem. I think per device,
there's a reasonably limited set of apps that we would want to interact
with from the kernel. And for one single device, that set doesn't change
over time. So what's the difference to, say, an I2C device?

> As along as get this application ID can handle any random name, I prefer
> to use that as the discover mechanism and not have this DT.

Apart from the above, some apps must also be loaded from the system. And
those you can't detect: If an app isn't running, it doesn't have an ID
(uefisecapp and the tpm app are loaded by the firmware at boot). Those
are mostly vendor-specific things as far as I can tell, or HDCP stuff.
So you'd need to specify those as firmware somehow, and since (as far as
I can tell) those are signed specifically by/for that vendor and
potentially device (similar to the GPU zap shader or remoteproc
firmware), you'll need to use per-device paths.

That means you either hard-code them in the driver and have a compatible
per model, do DMI matching, or something similar (again, essentially
baking DTs into the kernel driver...), or just store them in the DT
(like we already do for GPU/remoteprocs). While you could hard-code some
known loaded-by-firmware apps and use the DT for others, I think we
should keep everything in the same place.

Windows uses a hard-coded list of supported apps in the driver (to
specify which apps the driver supports) and in addition to that uses
Registry entries (added via .inf files) to specify which app is supposed
to be present on the device and, for apps that need to be loaded, where
the firmware to be loaded is stored. It only ever attempts to connect to
those apps that are marked present in the registry. Which, again, may be
model/vendor specific.

And this is another reason I'm hesitant to use that function: Windows
doesn't use it in this way and that I don't know whether there can be
any subtle breakage or unexpected behavior if we use the function like
that (i.e., who's to say some broken firmware returns "app is present"
but the app is broken?).

Regards,
Max

2022-07-27 11:34:41

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 26/07/2022 17:00, Maximilian Luz wrote:
> On 7/26/22 15:25, Krzysztof Kozlowski wrote:
>> On 26/07/2022 13:15, Maximilian Luz wrote:
>>>>> +properties:
>>>>> + compatible:
>>>>> + const: qcom,tee-uefisecapp
>>>>
>>>> Isn't this SoC-specific device? Generic compatibles are usually not
>>>> expected.
>>>
>>> This is essentially software (kernel driver) talking to software (in the
>>> TrustZone), so I don't expect there to be anything SoC specific about it.
>>
>> You are documenting here firmware in TZ (not kernel driver). Isn't this
>> a specific piece which might vary from device to device?
>>
>> IOW, do you expect the same compatible to work for all possible Qualcomm
>> boards (past and future like in 10 years from now)?
>
> I'm not sure if Qualcomm will still use the "uefisecapp" approach in 10
> years, but I don't expect the interface of uefisecapp to change. The
> interface is modeled after the respective UEFI functions, which are spec
> and thus I don't expect those to change. Also, it seems to have been
> around for a couple of generations and it hasn't changed. The oldest
> tested is sdm850 (Lenovo Yoga C630), and the latest is sc8280xp
> (Thinkpad X13s).

Expectation is not the same as having a specification saying it will not
change.
>
> Why not make this behave like a "normal" third-party device? If the
> interface ever changes use qcom,tee-uefisecapp-v2 or something like
> that? Again, this does not seem to be directly tied to the SoC.

Such approach is not "normal" for third-party devices. Compatible for
devices has model number. If the block has specification, then v2 would
have sense, otherwise you would invent own versioning...

I would say that firmware implementation can easily change. How much of
your code is tied to it, I don't know, but argument "I don't expect
Qualcomm to change something in their firmware" is not the correct argument.

>
> Then again, if you prefer to name everything based on
> "qcom,<device>-<soc>" I don't have any strong arguments against it and
> I'm happy to change that. I just think it will unnecessarily introduce
> a bunch of compatibles and doesn't reflect the interface "versioning"
> situation as I see it.

Why bunch? All devices could bind to one specific compatible, as they
are compatible.

>
>>>>> +
>>>>> +required:
>>>>> + - compatible
>>>>> +
>>>>> +additionalProperties: false
>>>>> +
>>>>> +examples:
>>>>> + - |
>>>>> + firmware {
>>>>> + scm {
>>>>> + compatible = "qcom,scm-sc8180x", "qcom,scm";
>>>>> + };
>>>>> + tee-uefisecapp {
>>>>> + compatible = "qcom,tee-uefisecapp";
>>>>
>>>> You did not model here any dependency on SCM. This is not full
>>>> description of the firmware/hardware
>>>
>>> How would I do that? A lot of other stuff also depends on SCM being
>>> present (e.g. qcom_q6v5_pas for loading mdt files) and I don't see them
>>> declare this in the device tree. As far as I can tell, SCM is pretty
>>> much expected to be there at all times (i.e. can't be unloaded) and
>>> drivers check for it when probing via qcom_scm_is_available(),
>>> deferring probe if not.
>>
>> It seems this will be opening a can of worms...
>
> Indeed.
>
>> The problem with existing approach is:
>> 1. Lack of any probe ordering or probe deferral support.
>> 2. Lack of any other dependencies, e.g. for PM.
>
> I'm not entirely sure what you mean by "lack of probe deferral support".
> We have qcom_scm_is_available() and defer probe if that fails. So
> deferral works, unless I'm misunderstanding something.

And how do you differentiate that qcom_scm_is_available() failed because
it is not yet available (defer probe) or it is broken and will never
load? All regular consumer-provider interfaces have it sorted out.

>
> But yes, correct on the other points.
>
>> Unloading is "solved" only by disallowing the unload, not by proper
>> device links and module get/put.
>>
>> I understand that SCM must be there, but the same for several other
>> components and for these others we have ways to pass reference around
>> (e.g. syscon regmap, PHYs handles).
>>
>>>
>>> Don't take this as an excuse as in "I want to leave that out", it's just
>>> that I don't know how one would declare such a dependency explicitly. If
>>> you can tell me how to fix it, I'll include that for v2.
>>
>> I think there are no dedicated subsystem helpers for this (like for
>> provider/consumer of resets/power domains/clocks etc), so one way would
>> be something like nvidia,bpmp is doing.
>
> I assume you're referring to tegra_bpmp_get()? Does this correctly
> handle PM dependencies? At least as far as I can tell it doesn't
> explicitly establish a device link, it only gets a reference to the
> device, which doesn't guarantee the presence of a driver. Nor correct PM
> ordering. Please correct me if I'm wrong. As far as I know automatic
> creation of device links only works with certain props defined in
> of_supplier_bindings, right?

The Tegra choice is not complete, but it implements at least parts of it
and firmware dependencies are modeled in DTS. Other way would be to add
your device as child of SMC firmware and then you do not need bindings
at all...

>
> So unless I'm wrong there is also a bunch of other stuff that may be
> subtly broken. (Again, not a justification to include these changes,
> just wondering whether there should be a conscious approach to find and
> fix these things... rather than discover them patch-by-patch).
>
>> meson_sm_get is a bit similar - looking by compatible. This is less
>> portable and I would prefer the bpmp way (just like syscon phandles).
>
> I have another example (that could be improved via a phandle in DT): For
> the Surface System Aggregator (in ACPI-land), we have ssam_client_bind().
> This function 1) checks if the controller is available and ready, 2) if
> it is gets a reference to it, and 3) establishes a device link for
> PM-ordering, before 4) returning the reference to that controller to the
> client. This combined with deferring probe ensures that we will always
> have a valid reference. And since we're in DT-land, we could hook that
> up with a phandle reference to SCM and load that instead of having to
> use a global static.

Yes, that's better example than Tegra BPMP.

>> The qcom_q6v5_pas could be converted later to use similar approach and
>> eventually the "tatic struct qcom_scm *__scm;" can be entirely removed.
>>
>> Any comments on this approach from Konrad, Bjorn, Dmitry, Vinod and
>> anyone else?
>
> Regards,
> Max

Best regards,
Krzysztof

2022-07-27 11:46:33

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 26/07/2022 19:01, Maximilian Luz wrote:
> On 7/26/22 17:41, Sudeep Holla wrote:
>> On Tue, Jul 26, 2022 at 05:15:41PM +0200, Maximilian Luz wrote:
>>>
>>> So ultimately I think it's better to add a DT entry for it.
>>
>> I disagree for the reason that once you discover more apps running on the
>> secure side, you want to add more entries and update DT on the platform
>> every time you discover some new firmware entity and you wish to interact
>> with it from the non-secure side.
>
> Just as you'll have to add a driver to the kernel and update whatever is
> probing the TrEE interface and add those strings to that interface. If
> you then start doing SoC-specific lists, I think you'd be pretty much
> re-implementing a DT in the kernel driver...

But you don't have any of these names in the DT either. Your DT node
only indicates the presence of your driver, but does not hold any
additional information like these IDs.

Basically we start modelling firmware components in devicetree. :/

Best regards,
Krzysztof

2022-07-27 13:40:24

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On Wed, Jul 27, 2022 at 03:03:49PM +0200, Maximilian Luz wrote:
>
> Is there really a good way around it?

Yes rely on the firmware preferably auto discover, if that is not an option,
how about query. It seem to be working in your case.

> As far as I can see the alternative (especially for the apps that
> need to be loaded manually) is hard-coding everything in the driver.
> Which IMHO just spreads device specific information everywhere.
>

It may not be too bad compared to putting loads of firmware details
in the DT. What happens if you get a firmware upgrade with changed
number of firmware entities or even if the names are changed.

Are these name user ABI in a way that they won't be changed ? Generally
these entities tend to use UUID and the name you have might get changed.

I would ideally prefer even the name to be supplied from the userspace.
In this particular case, make this a driver and have the name as the
parameter. If the secure side services are used by some non-secure
applications, then you will need to have a user-interface which means
you can get the named from the userspace. No need to change the driver
in either case. Please let me know if I am missing anything to consider
here.

> Also: Let's use the TPM app as example. If that would be a SPI or I2C
> device, you'd model it in the DT. Just because it's a hardware device
> that's accessible via SCM/firmware you now don't?
>

Not sure if I understand the comparison here. But if there is some device
that is access restricted but needs to be accessed and has mechanism to
access, then you would model it as device in DT.

But the one $subject is addressing looks pure software and doesn't make
sense to model in DT IMO.

> If I were absolutely certain that there is a reliable mechanism to
> detect these apps, I'd agree with having a driver to instantiate those
> devices. But I am not.
>

You did say you use some query API to check this. I haven't seen the driver,
so relying on what you said earlier.

--
Regards,
Sudeep

2022-07-27 13:42:34

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 7/27/22 13:38, Krzysztof Kozlowski wrote:
> On 26/07/2022 19:01, Maximilian Luz wrote:
>> On 7/26/22 17:41, Sudeep Holla wrote:
>>> On Tue, Jul 26, 2022 at 05:15:41PM +0200, Maximilian Luz wrote:
>>>>
>>>> So ultimately I think it's better to add a DT entry for it.
>>>
>>> I disagree for the reason that once you discover more apps running on the
>>> secure side, you want to add more entries and update DT on the platform
>>> every time you discover some new firmware entity and you wish to interact
>>> with it from the non-secure side.
>>
>> Just as you'll have to add a driver to the kernel and update whatever is
>> probing the TrEE interface and add those strings to that interface. If
>> you then start doing SoC-specific lists, I think you'd be pretty much
>> re-implementing a DT in the kernel driver...
>
> But you don't have any of these names in the DT either. Your DT node
> only indicates the presence of your driver, but does not hold any
> additional information like these IDs.

Because the compatible implicates the ID-string which implicates the driver
interface. If the ID-string for uefisecapp would be different we'd very likely
need a different driver for that as well, meaning a new compatible too. I
thought it would be superfluous to put that in the DT.

> Basically we start modelling firmware components in devicetree. :/

Is there really a good way around it? As far as I can see the
alternative (especially for the apps that need to be loaded manually) is
hard-coding everything in the driver. Which IMHO just spreads device
specific information everywhere.

Also: Let's use the TPM app as example. If that would be a SPI or I2C
device, you'd model it in the DT. Just because it's a hardware device
that's accessible via SCM/firmware you now don't?

If I were absolutely certain that there is a reliable mechanism to
detect these apps, I'd agree with having a driver to instantiate those
devices. But I am not.

Regards,
Max

2022-07-27 13:47:13

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 7/27/22 13:24, Krzysztof Kozlowski wrote:
> On 26/07/2022 17:00, Maximilian Luz wrote:
>> On 7/26/22 15:25, Krzysztof Kozlowski wrote:
>>> On 26/07/2022 13:15, Maximilian Luz wrote:
>>>>>> +properties:
>>>>>> + compatible:
>>>>>> + const: qcom,tee-uefisecapp
>>>>>
>>>>> Isn't this SoC-specific device? Generic compatibles are usually not
>>>>> expected.
>>>>
>>>> This is essentially software (kernel driver) talking to software (in the
>>>> TrustZone), so I don't expect there to be anything SoC specific about it.
>>>
>>> You are documenting here firmware in TZ (not kernel driver). Isn't this
>>> a specific piece which might vary from device to device?
>>>
>>> IOW, do you expect the same compatible to work for all possible Qualcomm
>>> boards (past and future like in 10 years from now)?
>>
>> I'm not sure if Qualcomm will still use the "uefisecapp" approach in 10
>> years, but I don't expect the interface of uefisecapp to change. The
>> interface is modeled after the respective UEFI functions, which are spec
>> and thus I don't expect those to change. Also, it seems to have been
>> around for a couple of generations and it hasn't changed. The oldest
>> tested is sdm850 (Lenovo Yoga C630), and the latest is sc8280xp
>> (Thinkpad X13s).
>
> Expectation is not the same as having a specification saying it will not
> change.
>>
>> Why not make this behave like a "normal" third-party device? If the
>> interface ever changes use qcom,tee-uefisecapp-v2 or something like
>> that? Again, this does not seem to be directly tied to the SoC.
>
> Such approach is not "normal" for third-party devices. Compatible for
> devices has model number. If the block has specification, then v2 would
> have sense, otherwise you would invent own versioning...
>
> I would say that firmware implementation can easily change. How much of
> your code is tied to it, I don't know, but argument "I don't expect
> Qualcomm to change something in their firmware" is not the correct argument.

Fair points.

>> Then again, if you prefer to name everything based on
>> "qcom,<device>-<soc>" I don't have any strong arguments against it and
>> I'm happy to change that. I just think it will unnecessarily introduce
>> a bunch of compatibles and doesn't reflect the interface "versioning"
>> situation as I see it.
>
> Why bunch? All devices could bind to one specific compatible, as they
> are compatible.

Ah, I think I misunderstood you there. I thought you were advocating for
creating compatibles for each SoC just because it's a new SoC and things
might be different. I'm not at all against naming this something like
qcom,tee-uefisecapp-sc8180x then using that on all platforms that work.
I just didn't like the idea of having a bunch of different
qcom,tee-uefisecapp-<soc> pointing to the exact same thing without any
difference at all.

>>>>>> +
>>>>>> +required:
>>>>>> + - compatible
>>>>>> +
>>>>>> +additionalProperties: false
>>>>>> +
>>>>>> +examples:
>>>>>> + - |
>>>>>> + firmware {
>>>>>> + scm {
>>>>>> + compatible = "qcom,scm-sc8180x", "qcom,scm";
>>>>>> + };
>>>>>> + tee-uefisecapp {
>>>>>> + compatible = "qcom,tee-uefisecapp";
>>>>>
>>>>> You did not model here any dependency on SCM. This is not full
>>>>> description of the firmware/hardware
>>>>
>>>> How would I do that? A lot of other stuff also depends on SCM being
>>>> present (e.g. qcom_q6v5_pas for loading mdt files) and I don't see them
>>>> declare this in the device tree. As far as I can tell, SCM is pretty
>>>> much expected to be there at all times (i.e. can't be unloaded) and
>>>> drivers check for it when probing via qcom_scm_is_available(),
>>>> deferring probe if not.
>>>
>>> It seems this will be opening a can of worms...
>>
>> Indeed.
>>
>>> The problem with existing approach is:
>>> 1. Lack of any probe ordering or probe deferral support.
>>> 2. Lack of any other dependencies, e.g. for PM.
>>
>> I'm not entirely sure what you mean by "lack of probe deferral support".
>> We have qcom_scm_is_available() and defer probe if that fails. So
>> deferral works, unless I'm misunderstanding something.
>
> And how do you differentiate that qcom_scm_is_available() failed because
> it is not yet available (defer probe) or it is broken and will never
> load? All regular consumer-provider interfaces have it sorted out.

Fair point. By shifting that to device links you'll at least know what
it's waiting for and the driver won't attempt to probe until that's
resolved. But your question applies to that then as well: How do you
differentiate between the device link or supplier being broken somehow
and the supplier being just not ready yet?

>> But yes, correct on the other points.
>>
>>> Unloading is "solved" only by disallowing the unload, not by proper
>>> device links and module get/put.
>>>
>>> I understand that SCM must be there, but the same for several other
>>> components and for these others we have ways to pass reference around
>>> (e.g. syscon regmap, PHYs handles).
>>>
>>>>
>>>> Don't take this as an excuse as in "I want to leave that out", it's just
>>>> that I don't know how one would declare such a dependency explicitly. If
>>>> you can tell me how to fix it, I'll include that for v2.
>>>
>>> I think there are no dedicated subsystem helpers for this (like for
>>> provider/consumer of resets/power domains/clocks etc), so one way would
>>> be something like nvidia,bpmp is doing.
>>
>> I assume you're referring to tegra_bpmp_get()? Does this correctly
>> handle PM dependencies? At least as far as I can tell it doesn't
>> explicitly establish a device link, it only gets a reference to the
>> device, which doesn't guarantee the presence of a driver. Nor correct PM
>> ordering. Please correct me if I'm wrong. As far as I know automatic
>> creation of device links only works with certain props defined in
>> of_supplier_bindings, right?
>
> The Tegra choice is not complete, but it implements at least parts of it
> and firmware dependencies are modeled in DTS. Other way would be to add
> your device as child of SMC firmware and then you do not need bindings
> at all...

Looking at the TrEE driver in [1] and thinking of future additions
(re-entrant calls, callbacks/listeners, ..., all of which would require
some state), combining both might be the best option: Have a TrEE main
device for the interface that links up with SCM and then define the apps
as children under that TrEE device.

Regards,
Max

[1]: https://git.codelinaro.org/clo/la/kernel/msm-4.14/-/blob/auto-kernel.lnx.4.14.c34/drivers/misc/qseecom.c

2022-07-27 14:55:28

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 7/27/22 15:24, Sudeep Holla wrote:
> On Wed, Jul 27, 2022 at 03:03:49PM +0200, Maximilian Luz wrote:
>>
>> Is there really a good way around it?
>
> Yes rely on the firmware preferably auto discover, if that is not an option,
> how about query. It seem to be working in your case.
>
>> As far as I can see the alternative (especially for the apps that
>> need to be loaded manually) is hard-coding everything in the driver.
>> Which IMHO just spreads device specific information everywhere.
>>
>
> It may not be too bad compared to putting loads of firmware details
> in the DT. What happens if you get a firmware upgrade with changed
> number of firmware entities or even if the names are changed.
>
> Are these name user ABI in a way that they won't be changed ? Generally
> these entities tend to use UUID and the name you have might get changed.

I am pretty certain that these names do not change for a device once it's
been released. The full ID of the uefisecapp is "qcom.tz.uefisecapp". The
built-in firmware parts here are core components. So I really do not expect
them to just remove or rename things. If they would do that, that would
mean that, on Windows, access to things like the TPM or UEFI variables
would be broken if both the driver and Registry are not updated in parallel
with the firmware. So while I can't myself guarantee that this is a stable
name and interface, it's very much in MS/Qualcomm's interest to keep it
stable.

Also, I'm not advocating on putting loads of details in the DT. I'm (in
this series) advocating for a DT compatible that says "this device stores
EFI variables via that firmware interface". I'd be very surprised if
MS/Qualcomm suddenly decided to change that out for another interface,
potentially breaking their own software and devices.

> I would ideally prefer even the name to be supplied from the userspace.
> In this particular case, make this a driver and have the name as the
> parameter. If the secure side services are used by some non-secure
> applications, then you will need to have a user-interface which means
> you can get the named from the userspace. No need to change the driver
> in either case. Please let me know if I am missing anything to consider
> here.

From userspace? For access to EFI variables and (hopefully in the future
if I've managed to reverse-engineer that) the TPM? Those are things that
should work out-of-the-box and not require the user to first have to
configure something... Also, those are things that the kernel might want
to use (e.g. EFI variables as pstore for crashdumps) before the user is
even able to configure something (unless we now want to specify things
on the kernel command line...).

If this were something that only userspace would use then sure, let
userspace load it and do all the work. But it isn't.

>
>> Also: Let's use the TPM app as example. If that would be a SPI or I2C
>> device, you'd model it in the DT. Just because it's a hardware device
>> that's accessible via SCM/firmware you now don't?
>>
>
> Not sure if I understand the comparison here. But if there is some device
> that is access restricted but needs to be accessed and has mechanism to
> access, then you would model it as device in DT.
>
> But the one $subject is addressing looks pure software and doesn't make
> sense to model in DT IMO.

So as soon as access runs via some firmware mechanism, it should not be
in the DT? The TPM in the example above would also be accessed via some
firmware API. EFI variables are stored on some SPI flash that is managed
by the TrustZone. So in both cases kernel calls to firmware calls to
device. Where do you draw the line?

>> If I were absolutely certain that there is a reliable mechanism to
>> detect these apps, I'd agree with having a driver to instantiate those
>> devices. But I am not.
>>
>
> You did say you use some query API to check this. I haven't seen the driver,
> so relying on what you said earlier.

I did say that there is an API that turns a unique identifying string ID
of a secure application into a runtime-dependent integer ID of the
running application, returning an error if the application is not
running. I very much doubt that is supposed to be used for checking
support of certain applications. It could _maybe_ be used that way, but
the Windows driver doesn't, which makes me not very comfortable doing
that either.

Further: As far as I can tell, there is also no way of checking whether
that lookup failure is due to the application not being present or whether
something internal to the firmware failed. the respective results that the
call can (as far as I can tell) return are:

QCTEE_OS_RESULT_SUCCESS = 0,
QCTEE_OS_RESULT_INCOMPLETE = 1,
QCTEE_OS_RESULT_BLOCKED_ON_LISTENER = 2,
QCTEE_OS_RESULT_FAILURE = 0xFFFFFFFF,

And it will return QCTEE_OS_RESULT_FAILURE when the app name is wrong.

Again, while it _might_ be possible to use that, I don't think it makes a
very sound approach and I would really prefer not using it in that way.

Regards,
Max

2022-07-28 06:06:43

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

Hi all,

On Wed, 27 Jul 2022 at 16:24, Sudeep Holla <[email protected]> wrote:
>
> On Wed, Jul 27, 2022 at 03:03:49PM +0200, Maximilian Luz wrote:
> >
> > Is there really a good way around it?
>
> Yes rely on the firmware preferably auto discover, if that is not an option,
> how about query. It seem to be working in your case.

That's a good point. We have a similar situation with some Arm
devices and U-Boot. Let me try to explain a bit.

There's code plugged in in OP-TEE and U-Boot atm which allows you to
store EFI variables on an RPMB. This is a nice alternative if your
device doesn't have any other secure storage, however it presents
some challenges after ExitBootServices, similar to the ones you have
here.

The eMMC controller usually lives in the non-secure world. OP-TEE
can't access that, so it relies on a userspace supplicant to perform
the RPMB accesses. That supplicant is present in U-Boot and
Get/SetVariable works fine before ExitBootServices. Once Linux boots,
the 'U-Boot supplicant' goes away and we launch the linux equivalent
one from userspace. Since variable accessing is a runtime service and
it still has to go through the firmware we can't use those anymore
since U-Boot doesn't preserve the supplicant, the eMMC driver and the
OP-TEE portions needed in the runtime section(and even if it did we
would now have 2 drivers racing to access the same hardware). Instead
U-Boot copies the variables in runtime memory and
GetVariable/GetNextVariable still works, but SetVariable returns
EFI_UNSUPPORTED.

I've spent enough time looking at available solutions and although
this indeed breaks the EFI spec, something along the lines of
replacing the runtime services with ones that give you direct access
to the secure world, completely bypassing the firmware is imho our
least bad option.

I have an ancient branch somewhere that I can polish up and send an
RFC [1], but the way I enabled that was to install an empty config
table from the firmware. That empty table is basically an indication
to the kernel saying "Hey I can't store variables, can you do that for
me".

Is there any chance we can do something similar on that device (or
find a reasonable way of inferring that we need to replace some
services). That way we could at least have a common entry point to
the kernel and leave out the DT changes.

[1] https://git.linaro.org/people/ilias.apalodimas/net-next.git/log/?h=setvar_rt_optee_3

Thanks
/Ilias

>
> > As far as I can see the alternative (especially for the apps that
> > need to be loaded manually) is hard-coding everything in the driver.
> > Which IMHO just spreads device specific information everywhere.
> >
>
> It may not be too bad compared to putting loads of firmware details
> in the DT. What happens if you get a firmware upgrade with changed
> number of firmware entities or even if the names are changed.
>
> Are these name user ABI in a way that they won't be changed ? Generally
> these entities tend to use UUID and the name you have might get changed.
>
> I would ideally prefer even the name to be supplied from the userspace.
> In this particular case, make this a driver and have the name as the
> parameter. If the secure side services are used by some non-secure
> applications, then you will need to have a user-interface which means
> you can get the named from the userspace. No need to change the driver
> in either case. Please let me know if I am missing anything to consider
> here.
>
> > Also: Let's use the TPM app as example. If that would be a SPI or I2C
> > device, you'd model it in the DT. Just because it's a hardware device
> > that's accessible via SCM/firmware you now don't?
> >
>
> Not sure if I understand the comparison here. But if there is some device
> that is access restricted but needs to be accessed and has mechanism to
> access, then you would model it as device in DT.
>
> But the one $subject is addressing looks pure software and doesn't make
> sense to model in DT IMO.
>
> > If I were absolutely certain that there is a reliable mechanism to
> > detect these apps, I'd agree with having a driver to instantiate those
> > devices. But I am not.
> >
>
> You did say you use some query API to check this. I haven't seen the driver,
> so relying on what you said earlier.
>
> --
> Regards,
> Sudeep

2022-07-28 08:00:10

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 27/07/2022 15:00, Maximilian Luz wrote:
>>> Then again, if you prefer to name everything based on
>>> "qcom,<device>-<soc>" I don't have any strong arguments against it and
>>> I'm happy to change that. I just think it will unnecessarily introduce
>>> a bunch of compatibles and doesn't reflect the interface "versioning"
>>> situation as I see it.
>>
>> Why bunch? All devices could bind to one specific compatible, as they
>> are compatible.
>
> Ah, I think I misunderstood you there. I thought you were advocating for
> creating compatibles for each SoC just because it's a new SoC and things
> might be different. I'm not at all against naming this something like
> qcom,tee-uefisecapp-sc8180x then using that on all platforms that work.
> I just didn't like the idea of having a bunch of different
> qcom,tee-uefisecapp-<soc> pointing to the exact same thing without any
> difference at all.

You start with one specific compatible and if needed later either add
more specific upfront (qcom,sc8280x-tee-uefisecapp,
qcom,sc8180x-tee-uefisecapp) or as entirely new one if it is not compatible.

>
>>>>>>> +
>>>>>>> +required:
>>>>>>> + - compatible
>>>>>>> +
>>>>>>> +additionalProperties: false
>>>>>>> +
>>>>>>> +examples:
>>>>>>> + - |
>>>>>>> + firmware {
>>>>>>> + scm {
>>>>>>> + compatible = "qcom,scm-sc8180x", "qcom,scm";
>>>>>>> + };
>>>>>>> + tee-uefisecapp {
>>>>>>> + compatible = "qcom,tee-uefisecapp";
>>>>>>
>>>>>> You did not model here any dependency on SCM. This is not full
>>>>>> description of the firmware/hardware
>>>>>
>>>>> How would I do that? A lot of other stuff also depends on SCM being
>>>>> present (e.g. qcom_q6v5_pas for loading mdt files) and I don't see them
>>>>> declare this in the device tree. As far as I can tell, SCM is pretty
>>>>> much expected to be there at all times (i.e. can't be unloaded) and
>>>>> drivers check for it when probing via qcom_scm_is_available(),
>>>>> deferring probe if not.
>>>>
>>>> It seems this will be opening a can of worms...
>>>
>>> Indeed.
>>>
>>>> The problem with existing approach is:
>>>> 1. Lack of any probe ordering or probe deferral support.
>>>> 2. Lack of any other dependencies, e.g. for PM.
>>>
>>> I'm not entirely sure what you mean by "lack of probe deferral support".
>>> We have qcom_scm_is_available() and defer probe if that fails. So
>>> deferral works, unless I'm misunderstanding something.
>>
>> And how do you differentiate that qcom_scm_is_available() failed because
>> it is not yet available (defer probe) or it is broken and will never
>> load? All regular consumer-provider interfaces have it sorted out.
>
> Fair point. By shifting that to device links you'll at least know what
> it's waiting for and the driver won't attempt to probe until that's
> resolved. But your question applies to that then as well: How do you
> differentiate between the device link or supplier being broken somehow
> and the supplier being just not ready yet?

For example like tegra_bpmp_get() is doing.

Best regards,
Krzysztof

2022-07-28 08:34:47

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On Tue, Jul 26, 2022 at 07:01:28PM +0200, Maximilian Luz wrote:
> On 7/26/22 17:41, Sudeep Holla wrote:
> > On Tue, Jul 26, 2022 at 05:15:41PM +0200, Maximilian Luz wrote:
> > >
> > > So ultimately I think it's better to add a DT entry for it.
> >
> > I disagree for the reason that once you discover more apps running on the
> > secure side, you want to add more entries and update DT on the platform
> > every time you discover some new firmware entity and you wish to interact
> > with it from the non-secure side.
>
> Just as you'll have to add a driver to the kernel and update whatever is
> probing the TrEE interface and add those strings to that interface. If
> you then start doing SoC-specific lists, I think you'd be pretty much
> re-implementing a DT in the kernel driver...
>

Yes at the cost of DT being dumping ground for all the SoC specific firmware
crap. Firmware can be and must be discoverable, no point in dumping it in
DT as it forces DT upgrade every time something changes in the firmware i.e.
it can go out of sync quite quickly.

> I don't quite understand why this is a problem. I think per device,
> there's a reasonably limited set of apps that we would want to interact
> with from the kernel. And for one single device, that set doesn't change
> over time. So what's the difference to, say, an I2C device?
>

As I said we don't want DT to be dumping ground for all the not well designed
firmware interface. The whole point of firmware being another piece of
software that can be change unlike hardware makes it fragile to present any
more that what you need in the DT. I see this as one of the example.

Anyways I don't have the final say, I leave it to the DT maintainers.

> > As along as get this application ID can handle any random name, I prefer
> > to use that as the discover mechanism and not have this DT.
>
> Apart from the above, some apps must also be loaded from the system. And
> those you can't detect: If an app isn't running, it doesn't have an ID
> (uefisecapp and the tpm app are loaded by the firmware at boot). Those
> are mostly vendor-specific things as far as I can tell, or HDCP stuff.
> So you'd need to specify those as firmware somehow, and since (as far as
> I can tell) those are signed specifically by/for that vendor and
> potentially device (similar to the GPU zap shader or remoteproc
> firmware), you'll need to use per-device paths.
>

Sounds to me like more can be pushed to user space as it gets loaded at
runtime.

> That means you either hard-code them in the driver and have a compatible
> per model, do DMI matching, or something similar (again, essentially
> baking DTs into the kernel driver...), or just store them in the DT
> (like we already do for GPU/remoteprocs). While you could hard-code some
> known loaded-by-firmware apps and use the DT for others, I think we
> should keep everything in the same place.
>

Worst case I am fine with that as this needs to be one of and future
platforms must get their act right in designing their f/w interface.

--
Regards,
Sudeep

2022-07-28 10:08:24

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 7/28/22 10:23, Sudeep Holla wrote:
> On Tue, Jul 26, 2022 at 07:01:28PM +0200, Maximilian Luz wrote:
>> On 7/26/22 17:41, Sudeep Holla wrote:
>>> On Tue, Jul 26, 2022 at 05:15:41PM +0200, Maximilian Luz wrote:
>>>>
>>>> So ultimately I think it's better to add a DT entry for it.
>>>
>>> I disagree for the reason that once you discover more apps running on the
>>> secure side, you want to add more entries and update DT on the platform
>>> every time you discover some new firmware entity and you wish to interact
>>> with it from the non-secure side.
>>
>> Just as you'll have to add a driver to the kernel and update whatever is
>> probing the TrEE interface and add those strings to that interface. If
>> you then start doing SoC-specific lists, I think you'd be pretty much
>> re-implementing a DT in the kernel driver...
>>
>
> Yes at the cost of DT being dumping ground for all the SoC specific firmware
> crap. Firmware can be and must be discoverable, no point in dumping it in
> DT as it forces DT upgrade every time something changes in the firmware i.e.
> it can go out of sync quite quickly.

I fully agree with you here on the design level. Firmware _should_ be
discoverable. Unfortunately, in this case it really isn't.

Again, in Windows, this information is stored via the Registry and set
when the driver is installed. An example:

; UEFIVAR SECURE APP SERVICE
HKR,%EFIVarService.RegKey%,Enabled,%REG_DWORD%,1
HKR,%EFIVarService.RegKey%,MajorVersion,%REG_DWORD%,1
HKR,%EFIVarService.RegKey%,MinorVersion,%REG_DWORD%,0

; WINSECAPP SECURE APP SERVICE
HKR,%WinSecAppService.RegKey%,Enabled,%REG_DWORD%,1
HKR,%WinSecAppService.RegKey%,SecureApp,%REG_DWORD%,1
HKR,%WinSecAppService.RegKey%,LoadApp,%REG_DWORD%,0
HKR,%WinSecAppService.RegKey%,AppName,,"qcom.tz.winsecapp"
HKR,%WinSecAppService.RegKey%,MajorVersion,%REG_DWORD%,1
HKR,%WinSecAppService.RegKey%,MinorVersion,%REG_DWORD%,0
HKR,%WinSecAppService.RegKey%,OSDependencies,%REG_MULTI_SZ%,%RpmbOsService%

; HDCP v2.2 SECURE APP SERVICE
HKR,%Hdcp2p2Service.RegKey%,Enabled,%REG_DWORD%,1
HKR,%Hdcp2p2Service.RegKey%,SecureApp,%REG_DWORD%,1
HKR,%Hdcp2p2Service.RegKey%,LoadApp,%REG_DWORD%,1
HKR,%Hdcp2p2Service.RegKey%,AppName,,"qcom.tz.hdcp2p2"
HKR,%Hdcp2p2Service.RegKey%,FileName,,"hdcp2p2.mbn"
HKR,%Hdcp2p2Service.RegKey%,MajorVersion,%REG_DWORD%,1
HKR,%Hdcp2p2Service.RegKey%,MinorVersion,%REG_DWORD%,0
HKR,%Hdcp2p2Service.RegKey%,OSDependencies,%REG_MULTI_SZ%,%RpmbOsService%,%TzAppsOsService%

The '.RegKey' contains a GUID that specifies the _driver_ interface that
is registered by the driver to the kernel (i.e. is not related to the
specific firmware and firmware version), e.g. [1]. For uefisecapp, the
driver also maps this GUID to the name-string.

[1]: https://github.com/tpn/winsdk-10/blob/9b69fd26ac0c7d0b83d378dba01080e93349c2ed/Include/10.0.16299.0/km/treevariableservice.h#L35

>> I don't quite understand why this is a problem. I think per device,
>> there's a reasonably limited set of apps that we would want to interact
>> with from the kernel. And for one single device, that set doesn't change
>> over time. So what's the difference to, say, an I2C device?
>>
>
> As I said we don't want DT to be dumping ground for all the not well designed
> firmware interface. The whole point of firmware being another piece of
> software that can be change unlike hardware makes it fragile to present any
> more that what you need in the DT. I see this as one of the example.

I can see your point. But this interface has apparently been around
since at least sdm850 (e.g. Lenovo C630) and hasn't changed. As I've
argued elsewhere: All parties involved have a vested interest that this
interface doesn't change in a breaking way. The interface is modeled
similar to syscalls, so I very much expect them to extend it if needed,
instead of changing/breaking it.

Sure, it _could_ be changed in a breaking way. But again, I believe that
to be _very_ unlikely.

> Anyways I don't have the final say, I leave it to the DT maintainers.
>
>>> As along as get this application ID can handle any random name, I prefer
>>> to use that as the discover mechanism and not have this DT.
>>
>> Apart from the above, some apps must also be loaded from the system. And
>> those you can't detect: If an app isn't running, it doesn't have an ID
>> (uefisecapp and the tpm app are loaded by the firmware at boot). Those
>> are mostly vendor-specific things as far as I can tell, or HDCP stuff.
>> So you'd need to specify those as firmware somehow, and since (as far as
>> I can tell) those are signed specifically by/for that vendor and
>> potentially device (similar to the GPU zap shader or remoteproc
>> firmware), you'll need to use per-device paths.
>>
>
> Sounds to me like more can be pushed to user space as it gets loaded at
> runtime.

If we have user-space available at the time when these things should be
loaded or if they are more or less optional, sure.

>> That means you either hard-code them in the driver and have a compatible
>> per model, do DMI matching, or something similar (again, essentially
>> baking DTs into the kernel driver...), or just store them in the DT
>> (like we already do for GPU/remoteprocs). While you could hard-code some
>> known loaded-by-firmware apps and use the DT for others, I think we
>> should keep everything in the same place.
>>
>
> Worst case I am fine with that as this needs to be one of and future
> platforms must get their act right in designing their f/w interface.

Again, I fully agree with you that this situation shouldn't exist. But
reality is sadly different.

Regards,
Max

2022-07-28 10:38:09

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 7/28/22 09:48, Krzysztof Kozlowski wrote:

[...]

>>>>> The problem with existing approach is:
>>>>> 1. Lack of any probe ordering or probe deferral support.
>>>>> 2. Lack of any other dependencies, e.g. for PM.
>>>>
>>>> I'm not entirely sure what you mean by "lack of probe deferral support".
>>>> We have qcom_scm_is_available() and defer probe if that fails. So
>>>> deferral works, unless I'm misunderstanding something.
>>>
>>> And how do you differentiate that qcom_scm_is_available() failed because
>>> it is not yet available (defer probe) or it is broken and will never
>>> load? All regular consumer-provider interfaces have it sorted out.
>>
>> Fair point. By shifting that to device links you'll at least know what
>> it's waiting for and the driver won't attempt to probe until that's
>> resolved. But your question applies to that then as well: How do you
>> differentiate between the device link or supplier being broken somehow
>> and the supplier being just not ready yet?
>
> For example like tegra_bpmp_get() is doing.

But tegra_bpmp_get() can also not differentiate whether the supplier driver is
ever going to be successfully probed or not. I'm not sure you can ever really
solve that. The only thing it does in addition is check whether the phandle and
device is there. Or do you mean those not being present by "broken"? That's a
point I agree should be improved with SCM.

Regards,
Max

2022-07-28 10:44:59

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 28/07/2022 12:25, Maximilian Luz wrote:
> On 7/28/22 09:48, Krzysztof Kozlowski wrote:
>
> [...]
>
>>
>> For example like tegra_bpmp_get() is doing.
>
> But tegra_bpmp_get() can also not differentiate whether the supplier driver is
> ever going to be successfully probed or not. I'm not sure you can ever really
> solve that. The only thing it does in addition is check whether the phandle and
> device is there. Or do you mean those not being present by "broken"? That's a
> point I agree should be improved with SCM.

Yes, at least it checks if phandles points to proper device and device
is there. That's what we want.

We are not solving here case of providing being in a module which never
gets loaded (thus endless EPROBE_DEFER). Such case is ok.

Best regards,
Krzysztof

2022-07-28 10:58:56

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 7/28/22 08:03, Ilias Apalodimas wrote:
> Hi all,
>
> On Wed, 27 Jul 2022 at 16:24, Sudeep Holla <[email protected]> wrote:
>>
>> On Wed, Jul 27, 2022 at 03:03:49PM +0200, Maximilian Luz wrote:
>>>
>>> Is there really a good way around it?
>>
>> Yes rely on the firmware preferably auto discover, if that is not an option,
>> how about query. It seem to be working in your case.
>
> That's a good point. We have a similar situation with some Arm
> devices and U-Boot. Let me try to explain a bit.
>
> There's code plugged in in OP-TEE and U-Boot atm which allows you to
> store EFI variables on an RPMB. This is a nice alternative if your
> device doesn't have any other secure storage, however it presents
> some challenges after ExitBootServices, similar to the ones you have
> here.
>
> The eMMC controller usually lives in the non-secure world. OP-TEE
> can't access that, so it relies on a userspace supplicant to perform
> the RPMB accesses. That supplicant is present in U-Boot and
> Get/SetVariable works fine before ExitBootServices. Once Linux boots,
> the 'U-Boot supplicant' goes away and we launch the linux equivalent
> one from userspace. Since variable accessing is a runtime service and
> it still has to go through the firmware we can't use those anymore
> since U-Boot doesn't preserve the supplicant, the eMMC driver and the
> OP-TEE portions needed in the runtime section(and even if it did we
> would now have 2 drivers racing to access the same hardware). Instead
> U-Boot copies the variables in runtime memory and
> GetVariable/GetNextVariable still works, but SetVariable returns
> EFI_UNSUPPORTED.
>
> I've spent enough time looking at available solutions and although
> this indeed breaks the EFI spec, something along the lines of
> replacing the runtime services with ones that give you direct access
> to the secure world, completely bypassing the firmware is imho our
> least bad option.

This sounds very similar to what Qualcomm may be doing on some devices.
The TrEE interface allows for callbacks and there are indications that
one such callback-service is for RPMB. I believe that at least on some
platforms, Qualcomm also stores UEFI variables in RPMB and uses the same
uefisecapp interface in combination with RPMB listeners installed by the
kernel to access them.

> I have an ancient branch somewhere that I can polish up and send an
> RFC [1], but the way I enabled that was to install an empty config
> table from the firmware. That empty table is basically an indication
> to the kernel saying "Hey I can't store variables, can you do that for
> me".
>
> Is there any chance we can do something similar on that device (or
> find a reasonable way of inferring that we need to replace some
> services). That way we could at least have a common entry point to
> the kernel and leave out the DT changes.
>
> [1] https://git.linaro.org/people/ilias.apalodimas/net-next.git/log/?h=setvar_rt_optee_3

I would very much like to avoid the need for special bootloaders. The
devices we're talking about are WoA devices, meaning they _should_
ideally boot just fine with EFI and ACPI.

From an end-user perspective, it's annoying enough that we'll have to
stick with DTs for the time being due to the use of PEPs in ACPI. I
really don't want to add some special bootloader for fixups to that.
Also, this would just move the problem from kernel to bootloader.

If you have any suggestions for another way of detecting this, please
feel free to share. I, unfortunately, don't.

Regards,
Max

2022-07-28 11:18:59

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 7/28/22 12:38, Krzysztof Kozlowski wrote:
> On 28/07/2022 12:25, Maximilian Luz wrote:
>> On 7/28/22 09:48, Krzysztof Kozlowski wrote:
>>
>> [...]
>>
>>>
>>> For example like tegra_bpmp_get() is doing.
>>
>> But tegra_bpmp_get() can also not differentiate whether the supplier driver is
>> ever going to be successfully probed or not. I'm not sure you can ever really
>> solve that. The only thing it does in addition is check whether the phandle and
>> device is there. Or do you mean those not being present by "broken"? That's a
>> point I agree should be improved with SCM.
>
> Yes, at least it checks if phandles points to proper device and device
> is there. That's what we want.
>
> We are not solving here case of providing being in a module which never
> gets loaded (thus endless EPROBE_DEFER). Such case is ok.

Got it, thanks for that clarification!

Regards,
Max

2022-07-28 11:49:27

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On Thu, Jul 28, 2022 at 12:05:15PM +0200, Maximilian Luz wrote:
> On 7/28/22 10:23, Sudeep Holla wrote:

[...]

> > Worst case I am fine with that as this needs to be one of and future
> > platforms must get their act right in designing their f/w interface.
>
> Again, I fully agree with you that this situation shouldn't exist. But
> reality is sadly different.
>

As I mentioned I don't have final authority to say yes or no to DT bindings.
I have expressed my opinion and I thing allowing this to be generic via DT
bindings gives no incentive to get the firmware story right. Hence I am happy
to see this as one-off driver change and then we more changes are added to
the driver or similar drivers get added in the future, we have a change to
demand what action has been taken to fix the firmware story.

Just adding DT support(which I disagree) will make future platform to just
use it and not get improvements in areas of discovery or query from the
firmware.

--
Regards,
Sudeep

2022-07-28 11:55:54

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 7/28/22 13:21, Sudeep Holla wrote:
> On Thu, Jul 28, 2022 at 12:05:15PM +0200, Maximilian Luz wrote:
>> On 7/28/22 10:23, Sudeep Holla wrote:
>
> [...]
>
>>> Worst case I am fine with that as this needs to be one of and future
>>> platforms must get their act right in designing their f/w interface.
>>
>> Again, I fully agree with you that this situation shouldn't exist. But
>> reality is sadly different.
>>
>
> As I mentioned I don't have final authority to say yes or no to DT bindings.
> I have expressed my opinion and I thing allowing this to be generic via DT
> bindings gives no incentive to get the firmware story right. Hence I am happy
> to see this as one-off driver change and then we more changes are added to
> the driver or similar drivers get added in the future, we have a change to
> demand what action has been taken to fix the firmware story.
>
> Just adding DT support(which I disagree) will make future platform to just
> use it and not get improvements in areas of discovery or query from the
> firmware.

Okay, that is a good point. Although it's probably debatable how much
control we have over what goes on with WoA devices.

Would something like this work for you: Add a compatible for the TrEE
interface (e.g. qcom,sc8180x-tee) but not for the specific apps running
in that and then instantiate the app-specific sub-devices from that. We
would still have to hard-code app-names in the driver (i.e. shift the
problem from DT to driver and potentially create soc-specific lists),
but they're no longer in DT (again, I'm not a particular fan of this but
I could live with that, if needed).

We can then look for a solution for apps that need to be manually loaded
or vendor/device specific apps once those becomes an issue.

Regards,
Max

2022-07-28 12:21:43

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On Thu, Jul 28, 2022 at 12:48:19PM +0200, Maximilian Luz wrote:

[...]

>
> I would very much like to avoid the need for special bootloaders. The
> devices we're talking about are WoA devices, meaning they _should_
> ideally boot just fine with EFI and ACPI.
>

Completely agreed.

> From an end-user perspective, it's annoying enough that we'll have to
> stick with DTs for the time being due to the use of PEPs in ACPI.

But have we explored or investigated what it takes to rewrite ACPI f/w
to just use standard methods ? Does it require more firmware changes or
new firmware entities or impossible at any cost ?

For me that is more important than just getting this one on DT. Because
if you take that path, we will have to keep doing that, with loads of
unnecessary drivers if they are not shared with any other SoC with DT
support upstream. We might also miss chance to get things added to the ACPI
spec as we don't care which means that we never be able to use ACPI on
similar future platforms even though they get shipped with ACPI.

It will be a loop where we constantly keep converting this ACPI shipped
platform into DT upstream. IMHO we don't want to be there.

--
Regards,
Sudeep

2022-07-28 12:22:55

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 7/28/22 13:33, Sudeep Holla wrote:
> On Thu, Jul 28, 2022 at 12:48:19PM +0200, Maximilian Luz wrote:
>
> [...]
>
>>
>> I would very much like to avoid the need for special bootloaders. The
>> devices we're talking about are WoA devices, meaning they _should_
>> ideally boot just fine with EFI and ACPI.
>>
>
> Completely agreed.
>
>> From an end-user perspective, it's annoying enough that we'll have to
>> stick with DTs for the time being due to the use of PEPs in ACPI.
>
> But have we explored or investigated what it takes to rewrite ACPI f/w
> to just use standard methods ? Does it require more firmware changes or
> new firmware entities or impossible at any cost ?

Again, I'm not a Qualcomm employee. I would prefer it they'd use
standard methods in the future. Rewriting the ACPI tables based on the
information that we have is probably possible, but we'd again have to do
this on a device-by-device basis, so why not just write a DT instead?

Again, I'm not a Qualcomm employee. I would prefer it they'd use
standard methods in the future. I cannot say why they are using PEPs and
whether they can't just use something "normal". Rewriting the ACPI
tables based on the information that we have is probably possible, but
we'd again have to do this manually, on a device-by-device basis. So why
not just write a DT instead?

Apart from that they also unfortunately hard-code a lot of SoC specific
MMIO addresses into their drivers, so, for each SoC, they essentially
have their own ACPI HID even if the specific hardware interface hasn't
changed. It's bad all around... and I don't like it one bit either.

> For me that is more important than just getting this one on DT. Because
> if you take that path, we will have to keep doing that, with loads of
> unnecessary drivers if they are not shared with any other SoC with DT
> support upstream. We might also miss chance to get things added to the ACPI
> spec as we don't care which means that we never be able to use ACPI on
> similar future platforms even though they get shipped with ACPI.
>
> It will be a loop where we constantly keep converting this ACPI shipped
> platform into DT upstream. IMHO we don't want to be there.

I fully agree with that. And that is also something that I fear.

Unfortunately, the only way out that I can see is either Qualcomm
changing its ways or us supporting ACPI PEPs, doing hard-coded register
addresses based on ACPI HIDs, and converting a lot of existing drivers
written for DT/OF to support ACPI. I personally would prefer if we'd do
all that and hope that we can one day support PEPs.

Once we do, we'd at least "only" have to add the needed MMIO definitions
for drivers via HID matches and write a PEP driver for that specific SoC
(which would then be similar to regulator or clock controllers). Still
some work but a lot less than having to write DTs for each and every
possible model.

As much as I'd like to support and work on that, I'm doing this in my
free time, and this sounds like a big undertaking. At the moment, my
efforts are focused on making the Surface Pro X play (relatively) nice
with Linux (via DT). I had thought about this, but my time to work on
this is unfortunately limited. You'd probably have to ask e.g. the
Linaro folks for help and input (some of which, e.g. Bjorn Andersson
are currently working on DTs for WoA devices), and also convince the
ACPI maintainers.

Regards,
Max

2022-07-28 12:28:21

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On Thu, 28 Jul 2022 at 14:33, Sudeep Holla <[email protected]> wrote:
>
> On Thu, Jul 28, 2022 at 12:48:19PM +0200, Maximilian Luz wrote:
>
> [...]
>
> >
> > I would very much like to avoid the need for special bootloaders. The
> > devices we're talking about are WoA devices, meaning they _should_
> > ideally boot just fine with EFI and ACPI.
> >
>
> Completely agreed.

This is not a special bootloader though. Quite the opposite. It's a
standard UEFI compliant bootloader, which uses the fact that EFI is
supposed to be extensible. It installs a linux specific config table,
similar to how we install a linux specific protocol to load our initrd
and it's certainly lot more scalable than adding new stuff to the
device tree.

>
> > From an end-user perspective, it's annoying enough that we'll have to
> > stick with DTs for the time being due to the use of PEPs in ACPI.
>
> But have we explored or investigated what it takes to rewrite ACPI f/w
> to just use standard methods ? Does it require more firmware changes or
> new firmware entities or impossible at any cost ?
>
> For me that is more important than just getting this one on DT. Because
> if you take that path, we will have to keep doing that, with loads of
> unnecessary drivers if they are not shared with any other SoC with DT
> support upstream. We might also miss chance to get things added to the ACPI
> spec as we don't care which means that we never be able to use ACPI on
> similar future platforms even though they get shipped with ACPI.
>
> It will be a loop where we constantly keep converting this ACPI shipped
> platform into DT upstream. IMHO we don't want to be there.
>
> --
> Regards,
> Sudeep

Regards
/Ilias

2022-07-28 12:37:19

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

Hi Maximilian

On Thu, 28 Jul 2022 at 13:48, Maximilian Luz <[email protected]> wrote:
>
> On 7/28/22 08:03, Ilias Apalodimas wrote:
> > Hi all,
> >
> > On Wed, 27 Jul 2022 at 16:24, Sudeep Holla <[email protected]> wrote:
> >>
> >> On Wed, Jul 27, 2022 at 03:03:49PM +0200, Maximilian Luz wrote:
> >>>
> >>> Is there really a good way around it?
> >>
> >> Yes rely on the firmware preferably auto discover, if that is not an option,
> >> how about query. It seem to be working in your case.
> >
> > That's a good point. We have a similar situation with some Arm
> > devices and U-Boot. Let me try to explain a bit.
> >
> > There's code plugged in in OP-TEE and U-Boot atm which allows you to
> > store EFI variables on an RPMB. This is a nice alternative if your
> > device doesn't have any other secure storage, however it presents
> > some challenges after ExitBootServices, similar to the ones you have
> > here.
> >
> > The eMMC controller usually lives in the non-secure world. OP-TEE
> > can't access that, so it relies on a userspace supplicant to perform
> > the RPMB accesses. That supplicant is present in U-Boot and
> > Get/SetVariable works fine before ExitBootServices. Once Linux boots,
> > the 'U-Boot supplicant' goes away and we launch the linux equivalent
> > one from userspace. Since variable accessing is a runtime service and
> > it still has to go through the firmware we can't use those anymore
> > since U-Boot doesn't preserve the supplicant, the eMMC driver and the
> > OP-TEE portions needed in the runtime section(and even if it did we
> > would now have 2 drivers racing to access the same hardware). Instead
> > U-Boot copies the variables in runtime memory and
> > GetVariable/GetNextVariable still works, but SetVariable returns
> > EFI_UNSUPPORTED.
> >
> > I've spent enough time looking at available solutions and although
> > this indeed breaks the EFI spec, something along the lines of
> > replacing the runtime services with ones that give you direct access
> > to the secure world, completely bypassing the firmware is imho our
> > least bad option.
>
> This sounds very similar to what Qualcomm may be doing on some devices.
> The TrEE interface allows for callbacks and there are indications that
> one such callback-service is for RPMB. I believe that at least on some
> platforms, Qualcomm also stores UEFI variables in RPMB and uses the same
> uefisecapp interface in combination with RPMB listeners installed by the
> kernel to access them.
>
> > I have an ancient branch somewhere that I can polish up and send an
> > RFC [1], but the way I enabled that was to install an empty config
> > table from the firmware. That empty table is basically an indication
> > to the kernel saying "Hey I can't store variables, can you do that for
> > me".
> >
> > Is there any chance we can do something similar on that device (or
> > find a reasonable way of inferring that we need to replace some
> > services). That way we could at least have a common entry point to
> > the kernel and leave out the DT changes.
> >
> > [1] https://git.linaro.org/people/ilias.apalodimas/net-next.git/log/?h=setvar_rt_optee_3
>
> I would very much like to avoid the need for special bootloaders. The
> devices we're talking about are WoA devices, meaning they _should_
> ideally boot just fine with EFI and ACPI.

I've already responded to following email, but I'll repeat it here for
completeness. It's not a special bootloader. It's the opposite, it's
a generic UEFI compliant bootloader which takes advantage of the fact
EFI is extensible. We are doing something very similar in how we load
our initrd via the EFI_LOAD_FILE2 protocol. Whether Qualcomm can add
that to their bootloaders is a different topic though. But at some
point we need to draw a line than keep overloading the DT because a
vendor decided to go down it's own path.

>
> From an end-user perspective, it's annoying enough that we'll have to
> stick with DTs for the time being due to the use of PEPs in ACPI. I
> really don't want to add some special bootloader for fixups to that.
> Also, this would just move the problem from kernel to bootloader.

But it *is* a bootloader problem. The bootloader is aware of the fact
that it can't provide runtime services for X reasons and that's
exactly why we are trying to set EFI_RT_PROPERTIES_TABLE correctly
from the firmware. All we are doing is install a config table to tell
the OS "I can't do that, can you find a way around it?".

Regards
/Ilias

>
> If you have any suggestions for another way of detecting this, please
> feel free to share. I, unfortunately, don't.
>
> Regards,
> Max

2022-07-28 13:26:28

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 7/28/22 14:35, Ilias Apalodimas wrote:
> Hi Maximilian
>
> On Thu, 28 Jul 2022 at 13:48, Maximilian Luz <[email protected]> wrote:
>>
>> On 7/28/22 08:03, Ilias Apalodimas wrote:
>>> Hi all,
>>>
>>> On Wed, 27 Jul 2022 at 16:24, Sudeep Holla <[email protected]> wrote:
>>>>
>>>> On Wed, Jul 27, 2022 at 03:03:49PM +0200, Maximilian Luz wrote:
>>>>>
>>>>> Is there really a good way around it?
>>>>
>>>> Yes rely on the firmware preferably auto discover, if that is not an option,
>>>> how about query. It seem to be working in your case.
>>>
>>> That's a good point. We have a similar situation with some Arm
>>> devices and U-Boot. Let me try to explain a bit.
>>>
>>> There's code plugged in in OP-TEE and U-Boot atm which allows you to
>>> store EFI variables on an RPMB. This is a nice alternative if your
>>> device doesn't have any other secure storage, however it presents
>>> some challenges after ExitBootServices, similar to the ones you have
>>> here.
>>>
>>> The eMMC controller usually lives in the non-secure world. OP-TEE
>>> can't access that, so it relies on a userspace supplicant to perform
>>> the RPMB accesses. That supplicant is present in U-Boot and
>>> Get/SetVariable works fine before ExitBootServices. Once Linux boots,
>>> the 'U-Boot supplicant' goes away and we launch the linux equivalent
>>> one from userspace. Since variable accessing is a runtime service and
>>> it still has to go through the firmware we can't use those anymore
>>> since U-Boot doesn't preserve the supplicant, the eMMC driver and the
>>> OP-TEE portions needed in the runtime section(and even if it did we
>>> would now have 2 drivers racing to access the same hardware). Instead
>>> U-Boot copies the variables in runtime memory and
>>> GetVariable/GetNextVariable still works, but SetVariable returns
>>> EFI_UNSUPPORTED.
>>>
>>> I've spent enough time looking at available solutions and although
>>> this indeed breaks the EFI spec, something along the lines of
>>> replacing the runtime services with ones that give you direct access
>>> to the secure world, completely bypassing the firmware is imho our
>>> least bad option.
>>
>> This sounds very similar to what Qualcomm may be doing on some devices.
>> The TrEE interface allows for callbacks and there are indications that
>> one such callback-service is for RPMB. I believe that at least on some
>> platforms, Qualcomm also stores UEFI variables in RPMB and uses the same
>> uefisecapp interface in combination with RPMB listeners installed by the
>> kernel to access them.
>>
>>> I have an ancient branch somewhere that I can polish up and send an
>>> RFC [1], but the way I enabled that was to install an empty config
>>> table from the firmware. That empty table is basically an indication
>>> to the kernel saying "Hey I can't store variables, can you do that for
>>> me".
>>>
>>> Is there any chance we can do something similar on that device (or
>>> find a reasonable way of inferring that we need to replace some
>>> services). That way we could at least have a common entry point to
>>> the kernel and leave out the DT changes.
>>>
>>> [1] https://git.linaro.org/people/ilias.apalodimas/net-next.git/log/?h=setvar_rt_optee_3
>>
>> I would very much like to avoid the need for special bootloaders. The
>> devices we're talking about are WoA devices, meaning they _should_
>> ideally boot just fine with EFI and ACPI.
>
> I've already responded to following email, but I'll repeat it here for
> completeness. It's not a special bootloader. It's the opposite, it's
> a generic UEFI compliant bootloader which takes advantage of the fact
> EFI is extensible. We are doing something very similar in how we load
> our initrd via the EFI_LOAD_FILE2 protocol. Whether Qualcomm can add
> that to their bootloaders is a different topic though. But at some
> point we need to draw a line than keep overloading the DT because a
> vendor decided to go down it's own path.

But still, you're asking users to install an extra thing in the boot
chain. That's what I mean by "special". So the situation would then be
this: User needs a) GRUB (or something similar) for booting the kernel
(or dual-booting, ...), b) DTBLoader for loading the device-tree because
we don't support the ACPI Qualcomm provided, and c) your thing for EFI
variables and potentially other firmware fix-ups. b) and c) are both
things that "normal" users don't expect. IMHO we should try to get rid
of those "non-standard" things, not add more.

>> From an end-user perspective, it's annoying enough that we'll have to
>> stick with DTs for the time being due to the use of PEPs in ACPI. I
>> really don't want to add some special bootloader for fixups to that.
>> Also, this would just move the problem from kernel to bootloader.
>
> But it *is* a bootloader problem. The bootloader is aware of the fact
> that it can't provide runtime services for X reasons and that's
> exactly why we are trying to set EFI_RT_PROPERTIES_TABLE correctly
> from the firmware. All we are doing is install a config table to tell
> the OS "I can't do that, can you find a way around it?".

Sure, but is making the Linux installation process more device
dependent and complicated really the best way to solve this?

Regards,
Max

2022-07-28 14:29:17

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 7/28/22 15:42, Sudeep Holla wrote:
> On Thu, Jul 28, 2022 at 01:45:57PM +0200, Maximilian Luz wrote:
>>
>> Would something like this work for you: Add a compatible for the TrEE
>> interface (e.g. qcom,sc8180x-tee) but not for the specific apps running
>
> IIUC, you would introduce a compatible for each unique production if there
> is a need. This constitutes as a strong need for that, but you need just
> that, no need to have any notion or info to indicate TrEE/TEE as you know
> this product runs those in TEE.
>
> In short, just use the platform specific(not SoC or SoC family) specific
> compatible to initialise your driver and don't introduce any specific
> compatible for this particular firmware interface need.

As Krzysztof mentioned, it would be good to ensure proper device
ordering wrt. SCM. Having a device node for the overall TrEE interface
would allow specifying that via DT. We could then still use the platform
compatible to load the specific things inside that driver.
Alternatively, we would need to do this extra stuff in qcom_scm.

I think separating this from qcom_scm into a new driver would be better
from a code separation and maintenance point of view. Also, this
reflects what's present in ACPI: There is a QCOM040B device for SCM and
a QCOM0476 device for TrEE.

Regards,
Max

2022-07-28 14:54:10

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On Thu, Jul 28, 2022 at 01:45:57PM +0200, Maximilian Luz wrote:
>
> Would something like this work for you: Add a compatible for the TrEE
> interface (e.g. qcom,sc8180x-tee) but not for the specific apps running

IIUC, you would introduce a compatible for each unique production if there
is a need. This constitutes as a strong need for that, but you need just
that, no need to have any notion or info to indicate TrEE/TEE as you know
this product runs those in TEE.

In short, just use the platform specific(not SoC or SoC family) specific
compatible to initialise your driver and don't introduce any specific
compatible for this particular firmware interface need.

--
Regards,
Sudeep

2022-07-28 15:13:24

by Ard Biesheuvel

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On Thu, 28 Jul 2022 at 04:33, Sudeep Holla <[email protected]> wrote:
>
> On Thu, Jul 28, 2022 at 12:48:19PM +0200, Maximilian Luz wrote:
>
> [...]
>
> >
> > I would very much like to avoid the need for special bootloaders. The
> > devices we're talking about are WoA devices, meaning they _should_
> > ideally boot just fine with EFI and ACPI.
> >
>
> Completely agreed.
>
> > From an end-user perspective, it's annoying enough that we'll have to
> > stick with DTs for the time being due to the use of PEPs in ACPI.
>
> But have we explored or investigated what it takes to rewrite ACPI f/w
> to just use standard methods ? Does it require more firmware changes or
> new firmware entities or impossible at any cost ?
>
> For me that is more important than just getting this one on DT. Because
> if you take that path, we will have to keep doing that, with loads of
> unnecessary drivers if they are not shared with any other SoC with DT
> support upstream. We might also miss chance to get things added to the ACPI
> spec as we don't care which means that we never be able to use ACPI on
> similar future platforms even though they get shipped with ACPI.
>
> It will be a loop where we constantly keep converting this ACPI shipped
> platform into DT upstream. IMHO we don't want to be there.
>

Supporting these devices in Linux in ACPI mode would involve
reimplementing the PEP subsystem, and reimplementing PEP drivers for
all these QCOM peripherals to manage the probing and the power states.
I don't think this is realistic at all, and a huge waste of
engineering effort otherwise.

It is also orthogonal to the discussion, as far as I understand: ACPI
is not telling the system whether or not these TZ services should be
used instead of EFI runtime calls.

So I think this is a reasonable way to expose these EFI services,
although I am not thrilled about the fact that it is needed.
Surprisingly, Microsoft also supports this model both on x86 and arm64
for platforms that keep their variables on eMMC (or any other kind of
storage that sits behind a controller that cannot be shared between
the OS and the firmware). So if we agree that we will support these
systems as best we can, supporting EFI variables at runtime is
something that we should support as well. (Note that I am not
convinced about the latter point myself: on many systems, the EFI
variable store is used precisely once, when GRUB gets installed and
its path added to the boot order, so if we could find a way to
streamline that without EFI runtime services, the story around why EFI
runtime services are important becomes quite weak)

As for the use of efivars_register(): I think this is reasonable, and
the way these patches use it is exactly why it exists in the first
place. Do not that a substantial overhaul of efivars is queued up in
efi/next, although I suspect those changes do not conflict with this
series.

2022-07-28 15:43:20

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

Hi Ard,

[...]

> > > From an end-user perspective, it's annoying enough that we'll have to
> > > stick with DTs for the time being due to the use of PEPs in ACPI.
> >
> > But have we explored or investigated what it takes to rewrite ACPI f/w
> > to just use standard methods ? Does it require more firmware changes or
> > new firmware entities or impossible at any cost ?
> >
> > For me that is more important than just getting this one on DT. Because
> > if you take that path, we will have to keep doing that, with loads of
> > unnecessary drivers if they are not shared with any other SoC with DT
> > support upstream. We might also miss chance to get things added to the ACPI
> > spec as we don't care which means that we never be able to use ACPI on
> > similar future platforms even though they get shipped with ACPI.
> >
> > It will be a loop where we constantly keep converting this ACPI shipped
> > platform into DT upstream. IMHO we don't want to be there.
> >
>
> Supporting these devices in Linux in ACPI mode would involve
> reimplementing the PEP subsystem, and reimplementing PEP drivers for
> all these QCOM peripherals to manage the probing and the power states.
> I don't think this is realistic at all, and a huge waste of
> engineering effort otherwise.
>
> It is also orthogonal to the discussion, as far as I understand: ACPI
> is not telling the system whether or not these TZ services should be
> used instead of EFI runtime calls.
>
> So I think this is a reasonable way to expose these EFI services,
> although I am not thrilled about the fact that it is needed.
> Surprisingly, Microsoft also supports this model both on x86 and arm64
> for platforms that keep their variables on eMMC (or any other kind of
> storage that sits behind a controller that cannot be shared between
> the OS and the firmware). So if we agree that we will support these
> systems as best we can, supporting EFI variables at runtime is
> something that we should support as well. (Note that I am not
> convinced about the latter point myself: on many systems, the EFI
> variable store is used precisely once, when GRUB gets installed and
> its path added to the boot order, so if we could find a way to
> streamline that without EFI runtime services, the story around why EFI
> runtime services are important becomes quite weak)

Unfortunately this is not entirely true. Yes the majority of use
cases is what you describe, however we also need SetVariable for
capsule update on disk.

[...]

2022-07-28 16:34:15

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On Thu, Jul 28, 2022 at 08:05:58AM -0700, Ard Biesheuvel wrote:
> On Thu, 28 Jul 2022 at 04:33, Sudeep Holla <[email protected]> wrote:
> >
> > On Thu, Jul 28, 2022 at 12:48:19PM +0200, Maximilian Luz wrote:
> >
> > [...]
> >
> > >
> > > I would very much like to avoid the need for special bootloaders. The
> > > devices we're talking about are WoA devices, meaning they _should_
> > > ideally boot just fine with EFI and ACPI.
> > >
> >
> > Completely agreed.
> >
> > > From an end-user perspective, it's annoying enough that we'll have to
> > > stick with DTs for the time being due to the use of PEPs in ACPI.
> >
> > But have we explored or investigated what it takes to rewrite ACPI f/w
> > to just use standard methods ? Does it require more firmware changes or
> > new firmware entities or impossible at any cost ?
> >
> > For me that is more important than just getting this one on DT. Because
> > if you take that path, we will have to keep doing that, with loads of
> > unnecessary drivers if they are not shared with any other SoC with DT
> > support upstream. We might also miss chance to get things added to the ACPI
> > spec as we don't care which means that we never be able to use ACPI on
> > similar future platforms even though they get shipped with ACPI.
> >
> > It will be a loop where we constantly keep converting this ACPI shipped
> > platform into DT upstream. IMHO we don't want to be there.
> >
>
> Supporting these devices in Linux in ACPI mode would involve
> reimplementing the PEP subsystem, and reimplementing PEP drivers for
> all these QCOM peripherals to manage the probing and the power states.
> I don't think this is realistic at all, and a huge waste of
> engineering effort otherwise.
>

I am aware of that and hence I am happy to see these as one off drivers
if needed. But if we don't stop that or keep converting them to DT,
IMO we will be in vicious circle of this conversion and will never be
able to support ACPI natively on these platforms. I know it is huge
effort and not expecting that to be done here, but we need to convey the
message to use ACPI standards or improve it if there is a need. Using
PEP is not helpful to run Linux in the long run. Also we may hit a point
when it may not be trivial to do that ACPI<->DT conversion.

> It is also orthogonal to the discussion, as far as I understand: ACPI
> is not telling the system whether or not these TZ services should be
> used instead of EFI runtime calls.
>

Agreed and I don't want to block any such discussions. Sorry if I derailed
the discussion, that was not my intentions.

--
Regards,
Sudeep

2022-07-28 16:35:11

by Konrad Dybcio

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 28.07.2022 18:16, Sudeep Holla wrote:
> On Thu, Jul 28, 2022 at 08:05:58AM -0700, Ard Biesheuvel wrote:
>> On Thu, 28 Jul 2022 at 04:33, Sudeep Holla <[email protected]> wrote:
>>>
>>> On Thu, Jul 28, 2022 at 12:48:19PM +0200, Maximilian Luz wrote:
>>>
>>> [...]
>>>
>>>>
>>>> I would very much like to avoid the need for special bootloaders. The
>>>> devices we're talking about are WoA devices, meaning they _should_
>>>> ideally boot just fine with EFI and ACPI.
>>>>
>>>
>>> Completely agreed.
>>>
>>>> From an end-user perspective, it's annoying enough that we'll have to
>>>> stick with DTs for the time being due to the use of PEPs in ACPI.
>>>
>>> But have we explored or investigated what it takes to rewrite ACPI f/w
>>> to just use standard methods ? Does it require more firmware changes or
>>> new firmware entities or impossible at any cost ?
>>>
>>> For me that is more important than just getting this one on DT. Because
>>> if you take that path, we will have to keep doing that, with loads of
>>> unnecessary drivers if they are not shared with any other SoC with DT
>>> support upstream. We might also miss chance to get things added to the ACPI
>>> spec as we don't care which means that we never be able to use ACPI on
>>> similar future platforms even though they get shipped with ACPI.
>>>
>>> It will be a loop where we constantly keep converting this ACPI shipped
>>> platform into DT upstream. IMHO we don't want to be there.
>>>
>>
>> Supporting these devices in Linux in ACPI mode would involve
>> reimplementing the PEP subsystem, and reimplementing PEP drivers for
>> all these QCOM peripherals to manage the probing and the power states.
>> I don't think this is realistic at all, and a huge waste of
>> engineering effort otherwise.
>>
>
> I am aware of that and hence I am happy to see these as one off drivers
> if needed. But if we don't stop that or keep converting them to DT,
> IMO we will be in vicious circle of this conversion and will never be
> able to support ACPI natively on these platforms.
I think that people have given up on ACPI on Snapdragon, as it was not
providing enough information in some cases (such as TLMM pins that are
not accessible from the AP due to being marked 'secure') that needed to
be hardcoded.

New WoA laptop support is added using FDT and I haven't seen any patches
even adding ACPI matchlists for a long long time.

Konrad
I know it is huge
> effort and not expecting that to be done here, but we need to convey the
> message to use ACPI standards or improve it if there is a need. Using
> PEP is not helpful to run Linux in the long run. Also we may hit a point
> when it may not be trivial to do that ACPI<->DT conversion.
>
>> It is also orthogonal to the discussion, as far as I understand: ACPI
>> is not telling the system whether or not these TZ services should be
>> used instead of EFI runtime calls.
>>
>
> Agreed and I don't want to block any such discussions. Sorry if I derailed
> the discussion, that was not my intentions.
>
> --
> Regards,
> Sudeep

2022-07-28 17:39:01

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On Thu, 28 Jul 2022 at 15:49, Maximilian Luz <[email protected]> wrote:
>

[...]

> >>
> >>> I have an ancient branch somewhere that I can polish up and send an
> >>> RFC [1], but the way I enabled that was to install an empty config
> >>> table from the firmware. That empty table is basically an indication
> >>> to the kernel saying "Hey I can't store variables, can you do that for
> >>> me".
> >>>
> >>> Is there any chance we can do something similar on that device (or
> >>> find a reasonable way of inferring that we need to replace some
> >>> services). That way we could at least have a common entry point to
> >>> the kernel and leave out the DT changes.
> >>>
> >>> [1] https://git.linaro.org/people/ilias.apalodimas/net-next.git/log/?h=setvar_rt_optee_3
> >>
> >> I would very much like to avoid the need for special bootloaders. The
> >> devices we're talking about are WoA devices, meaning they _should_
> >> ideally boot just fine with EFI and ACPI.
> >
> > I've already responded to following email, but I'll repeat it here for
> > completeness. It's not a special bootloader. It's the opposite, it's
> > a generic UEFI compliant bootloader which takes advantage of the fact
> > EFI is extensible. We are doing something very similar in how we load
> > our initrd via the EFI_LOAD_FILE2 protocol. Whether Qualcomm can add
> > that to their bootloaders is a different topic though. But at some
> > point we need to draw a line than keep overloading the DT because a
> > vendor decided to go down it's own path.
>
> But still, you're asking users to install an extra thing in the boot
> chain.

Not users. EFI firmware implementations that want to support this in
a generic way.

> That's what I mean by "special". So the situation would then be
> this: User needs a) GRUB (or something similar) for booting the kernel
> (or dual-booting, ...), b) DTBLoader for loading the device-tree because
> we don't support the ACPI Qualcomm provided, and c) your thing for EFI
> variables and potentially other firmware fix-ups. b) and c) are both
> things that "normal" users don't expect. IMHO we should try to get rid
> of those "non-standard" things, not add more.

But that's exactly why EFI is extensible . You can have non standard
functionality on your firmware for cases like this which doesn't need
to land in the spec.

>
> >> From an end-user perspective, it's annoying enough that we'll have to
> >> stick with DTs for the time being due to the use of PEPs in ACPI. I
> >> really don't want to add some special bootloader for fixups to that.
> >> Also, this would just move the problem from kernel to bootloader.
> >
> > But it *is* a bootloader problem. The bootloader is aware of the fact
> > that it can't provide runtime services for X reasons and that's
> > exactly why we are trying to set EFI_RT_PROPERTIES_TABLE correctly
> > from the firmware. All we are doing is install a config table to tell
> > the OS "I can't do that, can you find a way around it?".
>
> Sure, but is making the Linux installation process more device
> dependent and complicated really the best way to solve this?

Isn't it device dependent already? That boat has sailed already since
we need to change the very definition of runtime services and replace
them with OS specific ones. If we add it on the DT, you'll end up
with different DTs per OS and potentially per use case. In my head
the DTs should be part of the firmware (and authenticated by the
firmware as well) instead of loading whatever we want each time. By
using a config table we can add a u64 (random thought), that tells
the kernel which TEE implementation will handle variable storage. So
we can have a common extension to boot loaders, which at least uses
EFI interfaces to communicate the functionality.

I really prefer this to adding it on a DT, but I am not that picky.
Your email raises an important topic of replacing runtime services
with OS specific ones, which is unfortunately very much needed and we
should fix that.

Thanks
/Ilias
>
> Regards,
> Max

2022-07-28 18:14:11

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 7/28/22 18:56, Ilias Apalodimas wrote:
> On Thu, 28 Jul 2022 at 15:49, Maximilian Luz <[email protected]> wrote:
>>
>
> [...]
>
>>>>
>>>>> I have an ancient branch somewhere that I can polish up and send an
>>>>> RFC [1], but the way I enabled that was to install an empty config
>>>>> table from the firmware. That empty table is basically an indication
>>>>> to the kernel saying "Hey I can't store variables, can you do that for
>>>>> me".
>>>>>
>>>>> Is there any chance we can do something similar on that device (or
>>>>> find a reasonable way of inferring that we need to replace some
>>>>> services). That way we could at least have a common entry point to
>>>>> the kernel and leave out the DT changes.
>>>>>
>>>>> [1] https://git.linaro.org/people/ilias.apalodimas/net-next.git/log/?h=setvar_rt_optee_3
>>>>
>>>> I would very much like to avoid the need for special bootloaders. The
>>>> devices we're talking about are WoA devices, meaning they _should_
>>>> ideally boot just fine with EFI and ACPI.
>>>
>>> I've already responded to following email, but I'll repeat it here for
>>> completeness. It's not a special bootloader. It's the opposite, it's
>>> a generic UEFI compliant bootloader which takes advantage of the fact
>>> EFI is extensible. We are doing something very similar in how we load
>>> our initrd via the EFI_LOAD_FILE2 protocol. Whether Qualcomm can add
>>> that to their bootloaders is a different topic though. But at some
>>> point we need to draw a line than keep overloading the DT because a
>>> vendor decided to go down it's own path.
>>
>> But still, you're asking users to install an extra thing in the boot
>> chain.
>
> Not users. EFI firmware implementations that want to support this in
> a generic way.

The whole point here is that we don't have control over that. I'd like
to fix the firmware, but we're talking about WoA devices where, let's
face it, both device and SoC vendor don't really care about Linux. Even
if you'd convince them to implement that for future generations, you'd
still need them to push firmware updates for older generations.
Generations that are end-of-life. IMHO, we should still try support
those. Or we just say "sorry, Linux doesn't support that on your WoA
device".

>> That's what I mean by "special". So the situation would then be
>> this: User needs a) GRUB (or something similar) for booting the kernel
>> (or dual-booting, ...), b) DTBLoader for loading the device-tree because
>> we don't support the ACPI Qualcomm provided, and c) your thing for EFI
>> variables and potentially other firmware fix-ups. b) and c) are both
>> things that "normal" users don't expect. IMHO we should try to get rid
>> of those "non-standard" things, not add more.
>
> But that's exactly why EFI is extensible . You can have non standard
> functionality on your firmware for cases like this which doesn't need
> to land in the spec.
>
>>
>>>> From an end-user perspective, it's annoying enough that we'll have to
>>>> stick with DTs for the time being due to the use of PEPs in ACPI. I
>>>> really don't want to add some special bootloader for fixups to that.
>>>> Also, this would just move the problem from kernel to bootloader.
>>>
>>> But it *is* a bootloader problem. The bootloader is aware of the fact
>>> that it can't provide runtime services for X reasons and that's
>>> exactly why we are trying to set EFI_RT_PROPERTIES_TABLE correctly
>>> from the firmware. All we are doing is install a config table to tell
>>> the OS "I can't do that, can you find a way around it?".
>>
>> Sure, but is making the Linux installation process more device
>> dependent and complicated really the best way to solve this?
>
> Isn't it device dependent already? That boat has sailed already since
> we need to change the very definition of runtime services and replace
> them with OS specific ones. If we add it on the DT, you'll end up
> with different DTs per OS and potentially per use case. In my head
> the DTs should be part of the firmware (and authenticated by the
> firmware as well) instead of loading whatever we want each time. By
> using a config table we can add a u64 (random thought), that tells
> the kernel which TEE implementation will handle variable storage. So
> we can have a common extension to boot loaders, which at least uses
> EFI interfaces to communicate the functionality.

The only thing that is making the installation-process for end-users
device dependent is installing the DTB. We can handle the device
specific stuff in the kernel, just as we already handle buggy devices.

Further, you seem to assume that these devices provide a DT in the first
place. WoA devices use ACPI, so they don't. But for the time being (as
discussed elsewhere) we unfortunately need to stick with DTs and can't
really use ACPI. I agree that we should avoid OS and use-case specific
DTs, but I don't see how this would make a DT use-case or OS specific.
Things are firmware specific, the interface doesn't change with a
different OS, and we're only indicating the presence of that interface.

My current suggestion (already sent to Sudeep earlier) is (roughly)
this: Add one compatible for the TrEE / TrustZone interface. Then decide
to load or instantiate what needs to be loaded in the driver for that.
That (depending on maybe SoC / platform / vendor) includes installing
the efivar operations. This way we don't have to fill the DT with the
specific things running in firmware.

Regards,
Max

2022-07-29 09:58:37

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On Thu, Jul 28, 2022 at 07:27:19PM +0200, Maximilian Luz wrote:

[...]

> My current suggestion (already sent to Sudeep earlier) is (roughly)
> this: Add one compatible for the TrEE / TrustZone interface.

Still I don't understand why you need extra compatible if you know
this laptop(with a unique compatible to identify it) always runs this
TrEE interface.

--
Regards,
Sudeep

2022-07-29 15:37:32

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

On 7/29/22 10:52, Sudeep Holla wrote:
> On Thu, Jul 28, 2022 at 07:27:19PM +0200, Maximilian Luz wrote:
>
> [...]
>
>> My current suggestion (already sent to Sudeep earlier) is (roughly)
>> this: Add one compatible for the TrEE / TrustZone interface.
>
> Still I don't understand why you need extra compatible if you know
> this laptop(with a unique compatible to identify it) always runs this
> TrEE interface.

First of all, to recap: I suggest adding a device and driver for the TrEE
interface, with a compatible for that. That then (based on platform)
instantiates devices and drivers for the applications running in TrEE. The
compatible I'm talking about is for that general TrEE interface. Not any
specific application.

a) Because this better reflects the ACPI tables on those devices. As I've said,
there is a HID specifically for the TrEE interface. You were concerned
earlier that we should try to add support for that, and now you want to
create a purely artificial divide between ACPI and DT? Ideally, we can have
the driver load via both the DT compatible and the ACPI HID depending on
whether we use one or the other without many other changes.

Would you equally suggest that we not load the driver by its ACPI HID and
instead do DMI matching?

b) Qualcomm also has a DT compatible for this (qcom,qseecom), see e.g. [1].
Note: they seem to have changed the name from Secure Execution Environment
to Trusted Execution Environment, at least in their Windows driver. This is
why I used "tee" instead of "see" (also their naming of things is somewhat
confusing and seems to change randomly). Fundamentally, this is the same
interface (they just implement a lot more things in their driver, the couple
of functions I proposed here handle the absolute minimum required for
uefisecapp, it can always be extended later when needed).

c) Given their naming of the DT compatible, this interface itself is pretty
much guaranteed to be stable. It's definitely not going away with some
firmware update. So your earlier concerns about having to update the DT in
case of firmware changes do simply not apply here. It is a core component of
these platforms. As far as I can see, your "let's load the TrEE driver via
the platform compatible" suggestion is now exactly the same as a "let's load
some PCIe controller via the platform/SoC compatible". It's an interface
that is either present or not present, depending on the device. We're not
encoding any firmware specifics (ie. what's running inside the TrEE) in the
DT, we just say that it's there (the rest is decided by the driver, e.g. via
platform compatibles or DMI matching).

d) By specifying it in the DT, we can properly link it up via a phandle to the
SCM and properly model the supplier/client relation between them. While we
can't do that with ACPI, I think it's still a good idea to handle this
properly in times we can.

Regards,
Max

[1]: https://git.codelinaro.org/clo/la/kernel/msm-4.14/-/blob/auto-kernel.lnx.4.14.c34/drivers/misc/qseecom.c

2022-07-31 10:06:11

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

Hi Maximilian,

On Thu, 28 Jul 2022 at 20:27, Maximilian Luz <[email protected]> wrote:
>

[...]

> >>>>>
> >>>>> [1] https://git.linaro.org/people/ilias.apalodimas/net-next.git/log/?h=setvar_rt_optee_3
> >>>>
> >>>> I would very much like to avoid the need for special bootloaders. The
> >>>> devices we're talking about are WoA devices, meaning they _should_
> >>>> ideally boot just fine with EFI and ACPI.
> >>>
> >>> I've already responded to following email, but I'll repeat it here for
> >>> completeness. It's not a special bootloader. It's the opposite, it's
> >>> a generic UEFI compliant bootloader which takes advantage of the fact
> >>> EFI is extensible. We are doing something very similar in how we load
> >>> our initrd via the EFI_LOAD_FILE2 protocol. Whether Qualcomm can add
> >>> that to their bootloaders is a different topic though. But at some
> >>> point we need to draw a line than keep overloading the DT because a
> >>> vendor decided to go down it's own path.
> >>
> >> But still, you're asking users to install an extra thing in the boot
> >> chain.
> >
> > Not users. EFI firmware implementations that want to support this in
> > a generic way.
>
> The whole point here is that we don't have control over that. I'd like
> to fix the firmware, but we're talking about WoA devices where, let's
> face it, both device and SoC vendor don't really care about Linux. Even
> if you'd convince them to implement that for future generations, you'd
> still need them to push firmware updates for older generations.
> Generations that are end-of-life. IMHO, we should still try support
> those. Or we just say "sorry, Linux doesn't support that on your WoA
> device".

Yea we agree on that. I've mentioned on a previous mail that whether
Qualcomm wants to support this in a generic way is questionable, since
we have no control over the firmware. My only 'objection' is that the
kernel has a generic way of discovering which runtime services are
supported, which we will completely ignore based on some DT entries.

In any case let's find something that fits OP-TEE as well, since I can
send those patches afterwards.

>
> >> That's what I mean by "special". So the situation would then be
> >> this: User needs a) GRUB (or something similar) for booting the kernel
> >> (or dual-booting, ...), b) DTBLoader for loading the device-tree because
> >> we don't support the ACPI Qualcomm provided, and c) your thing for EFI
> >> variables and potentially other firmware fix-ups. b) and c) are both
> >> things that "normal" users don't expect. IMHO we should try to get rid
> >> of those "non-standard" things, not add more.
> >
> > But that's exactly why EFI is extensible . You can have non standard
> > functionality on your firmware for cases like this which doesn't need
> > to land in the spec.
> >
> >>
> >>>> From an end-user perspective, it's annoying enough that we'll have to
> >>>> stick with DTs for the time being due to the use of PEPs in ACPI. I
> >>>> really don't want to add some special bootloader for fixups to that.
> >>>> Also, this would just move the problem from kernel to bootloader.
> >>>
> >>> But it *is* a bootloader problem. The bootloader is aware of the fact
> >>> that it can't provide runtime services for X reasons and that's
> >>> exactly why we are trying to set EFI_RT_PROPERTIES_TABLE correctly
> >>> from the firmware. All we are doing is install a config table to tell
> >>> the OS "I can't do that, can you find a way around it?".
> >>
> >> Sure, but is making the Linux installation process more device
> >> dependent and complicated really the best way to solve this?
> >
> > Isn't it device dependent already? That boat has sailed already since
> > we need to change the very definition of runtime services and replace
> > them with OS specific ones. If we add it on the DT, you'll end up
> > with different DTs per OS and potentially per use case. In my head
> > the DTs should be part of the firmware (and authenticated by the
> > firmware as well) instead of loading whatever we want each time. By
> > using a config table we can add a u64 (random thought), that tells
> > the kernel which TEE implementation will handle variable storage. So
> > we can have a common extension to boot loaders, which at least uses
> > EFI interfaces to communicate the functionality.
>
> The only thing that is making the installation-process for end-users
> device dependent is installing the DTB. We can handle the device
> specific stuff in the kernel, just as we already handle buggy devices.
>
> Further, you seem to assume that these devices provide a DT in the first
> place. WoA devices use ACPI, so they don't. But for the time being (as
> discussed elsewhere) we unfortunately need to stick with DTs and can't
> really use ACPI. I agree that we should avoid OS and use-case specific
> DTs, but I don't see how this would make a DT use-case or OS specific.
> Things are firmware specific, the interface doesn't change with a
> different OS, and we're only indicating the presence of that interface.
>
> My current suggestion (already sent to Sudeep earlier) is (roughly)
> this: Add one compatible for the TrEE / TrustZone interface. Then decide
> to load or instantiate what needs to be loaded in the driver for that.
> That (depending on maybe SoC / platform / vendor) includes installing
> the efivar operations. This way we don't have to fill the DT with the
> specific things running in firmware.

As far as OP-TEE is concerned, I think we can make the 'feature'
discoverable. I'll go propose that to op-tee but if that gets
accepted, we don't need any extra nodes (other than the existing one),
to wire up efivars_register().

Thanks
/Ilias
>
> Regards,
> Max

2022-07-31 23:01:16

[permalink] [raw]

Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

Hi,

On 7/31/22 11:54, Ilias Apalodimas wrote:
> Hi Maximilian,
>
> On Thu, 28 Jul 2022 at 20:27, Maximilian Luz <[email protected]> wrote:
>>
>
> [...]
>
>>>>>>>
>>>>>>> [1] https://git.linaro.org/people/ilias.apalodimas/net-next.git/log/?h=setvar_rt_optee_3
>>>>>>
>>>>>> I would very much like to avoid the need for special bootloaders. The
>>>>>> devices we're talking about are WoA devices, meaning they _should_
>>>>>> ideally boot just fine with EFI and ACPI.
>>>>>
>>>>> I've already responded to following email, but I'll repeat it here for
>>>>> completeness. It's not a special bootloader. It's the opposite, it's
>>>>> a generic UEFI compliant bootloader which takes advantage of the fact
>>>>> EFI is extensible. We are doing something very similar in how we load
>>>>> our initrd via the EFI_LOAD_FILE2 protocol. Whether Qualcomm can add
>>>>> that to their bootloaders is a different topic though. But at some
>>>>> point we need to draw a line than keep overloading the DT because a
>>>>> vendor decided to go down it's own path.
>>>>
>>>> But still, you're asking users to install an extra thing in the boot
>>>> chain.
>>>
>>> Not users. EFI firmware implementations that want to support this in
>>> a generic way.
>>
>> The whole point here is that we don't have control over that. I'd like
>> to fix the firmware, but we're talking about WoA devices where, let's
>> face it, both device and SoC vendor don't really care about Linux. Even
>> if you'd convince them to implement that for future generations, you'd
>> still need them to push firmware updates for older generations.
>> Generations that are end-of-life. IMHO, we should still try support
>> those. Or we just say "sorry, Linux doesn't support that on your WoA
>> device".
>
> Yea we agree on that. I've mentioned on a previous mail that whether
> Qualcomm wants to support this in a generic way is questionable, since
> we have no control over the firmware. My only 'objection' is that the
> kernel has a generic way of discovering which runtime services are
> supported, which we will completely ignore based on some DT entries.

Right, sorry. That makes sense. If we have a realistic possibility, then
I agree that making it discoverable in firmware is the best option. My
point was just that we can't rely on Windows-focused vendors to
implement it.

> In any case let's find something that fits OP-TEE as well, since I can
> send those patches afterwards.

I think it's a great idea to try and find some sort of standardized
solution for OP-TEE and other interested projects similar to it, but we
still have to use a workaround for the Qualcomm WoA devices we have :(

Nevertheless, I'm happy to provide some input for a generic solution,
although I'm not sure I'm the best person to talk to about this.

>>>> That's what I mean by "special". So the situation would then be
>>>> this: User needs a) GRUB (or something similar) for booting the kernel
>>>> (or dual-booting, ...), b) DTBLoader for loading the device-tree because
>>>> we don't support the ACPI Qualcomm provided, and c) your thing for EFI
>>>> variables and potentially other firmware fix-ups. b) and c) are both
>>>> things that "normal" users don't expect. IMHO we should try to get rid
>>>> of those "non-standard" things, not add more.
>>>
>>> But that's exactly why EFI is extensible . You can have non standard
>>> functionality on your firmware for cases like this which doesn't need
>>> to land in the spec.
>>>
>>>>
>>>>>> From an end-user perspective, it's annoying enough that we'll have to
>>>>>> stick with DTs for the time being due to the use of PEPs in ACPI. I
>>>>>> really don't want to add some special bootloader for fixups to that.
>>>>>> Also, this would just move the problem from kernel to bootloader.
>>>>>
>>>>> But it *is* a bootloader problem. The bootloader is aware of the fact
>>>>> that it can't provide runtime services for X reasons and that's
>>>>> exactly why we are trying to set EFI_RT_PROPERTIES_TABLE correctly
>>>>> from the firmware. All we are doing is install a config table to tell
>>>>> the OS "I can't do that, can you find a way around it?".
>>>>
>>>> Sure, but is making the Linux installation process more device
>>>> dependent and complicated really the best way to solve this?
>>>
>>> Isn't it device dependent already? That boat has sailed already since
>>> we need to change the very definition of runtime services and replace
>>> them with OS specific ones. If we add it on the DT, you'll end up
>>> with different DTs per OS and potentially per use case. In my head
>>> the DTs should be part of the firmware (and authenticated by the
>>> firmware as well) instead of loading whatever we want each time. By
>>> using a config table we can add a u64 (random thought), that tells
>>> the kernel which TEE implementation will handle variable storage. So
>>> we can have a common extension to boot loaders, which at least uses
>>> EFI interfaces to communicate the functionality.
>>
>> The only thing that is making the installation-process for end-users
>> device dependent is installing the DTB. We can handle the device
>> specific stuff in the kernel, just as we already handle buggy devices.
>>
>> Further, you seem to assume that these devices provide a DT in the first
>> place. WoA devices use ACPI, so they don't. But for the time being (as
>> discussed elsewhere) we unfortunately need to stick with DTs and can't
>> really use ACPI. I agree that we should avoid OS and use-case specific
>> DTs, but I don't see how this would make a DT use-case or OS specific.
>> Things are firmware specific, the interface doesn't change with a
>> different OS, and we're only indicating the presence of that interface.
>>
>> My current suggestion (already sent to Sudeep earlier) is (roughly)
>> this: Add one compatible for the TrEE / TrustZone interface. Then decide
>> to load or instantiate what needs to be loaded in the driver for that.
>> That (depending on maybe SoC / platform / vendor) includes installing
>> the efivar operations. This way we don't have to fill the DT with the
>> specific things running in firmware.
>
> As far as OP-TEE is concerned, I think we can make the 'feature'
> discoverable. I'll go propose that to op-tee but if that gets
> accepted, we don't need any extra nodes (other than the existing one),
> to wire up efivars_register().

Right. I think you (either in your patches or mails) already mentioned
having an integer ID for the implementation (or maybe implementation +
vendor?). Apart from that, I think it might also make sense to have a
bit-field similar to efi.runtime_supported_mask that tells the kernel
which functions are taken over.

So with that you could call efivars_register() based on the firmware
table in the driver for linaro,optee-tz (I assume) whether for qcom,tee
(or whatever we'd call that) we'd have to hard-code it based on some
platform/model identifier.

Regards,
Max

2022-08-02 11:58:43

by Srinivas Kandagatla

[permalink] [raw]

Subject: Re: [PATCH 0/4] firmware: Add support for Qualcomm UEFI Secure Application

Hi Maximilian,

On 23/07/2022 23:49, Maximilian Luz wrote:
> On modern Qualcomm platforms, access to EFI variables is restricted to
> the secure world / TrustZone, i.e. the Trusted Execution Environment
> (TrEE or TEE) as Qualcomm seems to call it. To access EFI variables, we
> therefore need to talk to the UEFI Secure Application (uefisecapp),
> residing in the TrEE.
>
> This series adds support for accessing EFI variables on those platforms.
>
> To do this, we first need to add some SCM call functions used to manage
> and talk to Secure Applications. A very small subset of this interface
> is added in the second patch (whereas the first one exports the required
> functions for that). Interface specifications are extracted from [1].
> While this does not (yet) support re-entrant SCM calls (including
> callbacks and listeners), this is enough to talk to the aforementioned
> uefisecapp on a couple of platforms (I've tested this on a Surface Pro X
> and heard reports from Lenovo Flex 5G, Lenovo Thinkpad x13s, and Lenovo
> Yoga C630 devices).
>
> The third patch adds a client driver for uefisecapp, installing the
> respective efivar operations. The application interface has been reverse
> engineered from the Windows QcTrEE8180.sys driver.
>
> Apart from uefisecapp, there are more Secure Applications running that
> we might want to support in the future. For example, on the Surface Pro
> X (sc8180x-based), the TPM is also managed via one.
>
> I'm not sure whether this should go to drivers/firmware or to
> drivers/soc/qcom. I've put this into firmware as all of this is
> essentially an interface to the secure firmware running in the TrustZone
> (and SCM stuff is handled here already), but please let me know if I
> should move this.

From what I see so far is that this is adapted from downstream qseecom
driver, this approach could work for a limited usecases but not
scalable, as we cannot add drivers for each Qualcomm specific TA in kernel.
This has to be handled in much generic way using Linux TEE framework,
and let the userspace side deal with TA specific bits.

AFAIU, Qualcomm is moving away from qseecom interface to new smc-invoke
interface, most of Qualcomm SoCs starting from SDM660 already have
support to this.

This interface provides a better abstracted IPC mechanism to talk to TA.
Most of these TA specific interfaces are packed in closed userspace source.
Having said that QTEE smcinvoke driver can be modeled as a proper TEE
driver with Userspace driving the TA specific bits using existing tee uapis.
This also brings in other features like loading, Listeners aka
callbacks, secure memory allocations..etc.

In the past, I have tried to do a prototype of this smcinvoke driver as
a proper tee driver, incase you are interested patches are at
https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/log/?h=tracking-qcomlt-qcomtee
Plan is to discuss with Qualcomm and send it for upstream review.

I think its worth exploring if uefisecapp can talk smcinvoke.
I can ping Qualcomm engineers to see if that is doable.

thanks,
Srini

>
> Regards,
> Max
>
> [1]: https://git.codelinaro.org/clo/la/kernel/msm-4.14/-/blob/auto-kernel.lnx.4.14.c34/drivers/misc/qseecom.c
>
> Maximilian Luz (4):
> firmware: qcom_scm: Export SCM call functions
> firmware: Add support for Qualcomm Trusted Execution Environment SCM
> calls
> firmware: Add support for Qualcomm UEFI Secure Application
> dt-bindings: firmware: Add Qualcomm UEFI Secure Application client
>
> .../firmware/qcom,tee-uefisecapp.yaml | 38 +
> MAINTAINERS | 14 +
> drivers/firmware/Kconfig | 20 +
> drivers/firmware/Makefile | 2 +
> drivers/firmware/qcom_scm.c | 118 ++-
> drivers/firmware/qcom_scm.h | 47 --
> drivers/firmware/qcom_tee.c | 213 +++++
> drivers/firmware/qcom_tee_uefisecapp.c | 761 ++++++++++++++++++
> include/linux/qcom_scm.h | 49 ++
> include/linux/qcom_tee.h | 179 ++++
> 10 files changed, 1355 insertions(+), 86 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/firmware/qcom,tee-uefisecapp.yaml
> create mode 100644 drivers/firmware/qcom_tee.c
> create mode 100644 drivers/firmware/qcom_tee_uefisecapp.c
> create mode 100644 include/linux/qcom_tee.h
>

2022-08-02 13:44:10

[permalink] [raw]

Subject: Re: [PATCH 0/4] firmware: Add support for Qualcomm UEFI Secure Application

On 8/2/22 13:51, Srinivas Kandagatla wrote:
> Hi Maximilian,
>
> On 23/07/2022 23:49, Maximilian Luz wrote:
>> On modern Qualcomm platforms, access to EFI variables is restricted to
>> the secure world / TrustZone, i.e. the Trusted Execution Environment
>> (TrEE or TEE) as Qualcomm seems to call it. To access EFI variables, we
>> therefore need to talk to the UEFI Secure Application (uefisecapp),
>> residing in the TrEE.
>>
>> This series adds support for accessing EFI variables on those platforms.
>>
>> To do this, we first need to add some SCM call functions used to manage
>> and talk to Secure Applications. A very small subset of this interface
>> is added in the second patch (whereas the first one exports the required
>> functions for that). Interface specifications are extracted from [1].
>> While this does not (yet) support re-entrant SCM calls (including
>> callbacks and listeners), this is enough to talk to the aforementioned
>> uefisecapp on a couple of platforms (I've tested this on a Surface Pro X
>> and heard reports from Lenovo Flex 5G, Lenovo Thinkpad x13s, and Lenovo
>> Yoga C630 devices).
>>
>> The third patch adds a client driver for uefisecapp, installing the
>> respective efivar operations. The application interface has been reverse
>> engineered from the Windows QcTrEE8180.sys driver.
>>
>> Apart from uefisecapp, there are more Secure Applications running that
>> we might want to support in the future. For example, on the Surface Pro
>> X (sc8180x-based), the TPM is also managed via one.
>>
>> I'm not sure whether this should go to drivers/firmware or to
>> drivers/soc/qcom. I've put this into firmware as all of this is
>> essentially an interface to the secure firmware running in the TrustZone
>> (and SCM stuff is handled here already), but please let me know if I
>> should move this.
>
> From what I see so far is that this is adapted from downstream qseecom driver, this approach could work for a limited usecases but not scalable, as we cannot add drivers for each Qualcomm specific TA in kernel.
> This has to be handled in much generic way using Linux TEE framework, and let the userspace side deal with TA specific bits.

I generally agree with the sentiment, however UEFI variables should IMHO be
handled by the kernel. Moving handling of those to userspace breaks things like
EFI-based pstore and efivarfs. The latter will in turn break some user-space
tools (most notably efibootmgr used by e.g. GRUB and I think fwupdmgr which
needs to set some capsule variables). Ideally, we would find a way to not break
these, i.e. have them work out-of-the-box.

A similar argumentation might apply to the TPM app.

> AFAIU, Qualcomm is moving away from qseecom interface to new smc-invoke interface, most of Qualcomm SoCs starting from SDM660 already have support to this.
>
> This interface provides a better abstracted IPC mechanism to talk to TA. Most of these TA specific interfaces are packed in closed userspace source.
> Having said that QTEE smcinvoke driver can be modeled as a proper TEE driver with Userspace driving the TA specific bits using existing tee uapis.
> This also brings in other features like loading, Listeners aka callbacks, secure memory allocations..etc.
>
> In the past, I have tried to do a prototype of this smcinvoke driver as a proper tee driver, incase you are interested patches are at https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/log/?h=tracking-qcomlt-qcomtee
> Plan is to discuss with Qualcomm and send it for upstream review.

Thanks for this information! So as far as I understand it, this is currently an
interface to user-space only, i.e. does not allow in-kernel drivers for apps?
It would be great if this could then be extended to handle (the bare minimum
of) in-kernel drivers (i.e. only things that the kernel itself needs, like EFI
variables). Alternatively, I'm happy to hear suggestions on how we not break
the aforementioned things while moving handling off to userspace.

> I think its worth exploring if uefisecapp can talk smcinvoke.
> I can ping Qualcomm engineers to see if that is doable.

I think that would be great! Thanks!

Regards,
Max

2022-08-02 14:40:25

by Ard Biesheuvel

[permalink] [raw]

Subject: Re: [PATCH 0/4] firmware: Add support for Qualcomm UEFI Secure Application

On Tue, 2 Aug 2022 at 15:22, Maximilian Luz <[email protected]> wrote:
>
>
>
> On 8/2/22 13:51, Srinivas Kandagatla wrote:
> > Hi Maximilian,
> >
> > On 23/07/2022 23:49, Maximilian Luz wrote:
> >> On modern Qualcomm platforms, access to EFI variables is restricted to
> >> the secure world / TrustZone, i.e. the Trusted Execution Environment
> >> (TrEE or TEE) as Qualcomm seems to call it. To access EFI variables, we
> >> therefore need to talk to the UEFI Secure Application (uefisecapp),
> >> residing in the TrEE.
> >>
> >> This series adds support for accessing EFI variables on those platforms.
> >>
> >> To do this, we first need to add some SCM call functions used to manage
> >> and talk to Secure Applications. A very small subset of this interface
> >> is added in the second patch (whereas the first one exports the required
> >> functions for that). Interface specifications are extracted from [1].
> >> While this does not (yet) support re-entrant SCM calls (including
> >> callbacks and listeners), this is enough to talk to the aforementioned
> >> uefisecapp on a couple of platforms (I've tested this on a Surface Pro X
> >> and heard reports from Lenovo Flex 5G, Lenovo Thinkpad x13s, and Lenovo
> >> Yoga C630 devices).
> >>
> >> The third patch adds a client driver for uefisecapp, installing the
> >> respective efivar operations. The application interface has been reverse
> >> engineered from the Windows QcTrEE8180.sys driver.
> >>
> >> Apart from uefisecapp, there are more Secure Applications running that
> >> we might want to support in the future. For example, on the Surface Pro
> >> X (sc8180x-based), the TPM is also managed via one.
> >>
> >> I'm not sure whether this should go to drivers/firmware or to
> >> drivers/soc/qcom. I've put this into firmware as all of this is
> >> essentially an interface to the secure firmware running in the TrustZone
> >> (and SCM stuff is handled here already), but please let me know if I
> >> should move this.
> >
> > From what I see so far is that this is adapted from downstream qseecom driver, this approach could work for a limited usecases but not scalable, as we cannot add drivers for each Qualcomm specific TA in kernel.
> > This has to be handled in much generic way using Linux TEE framework, and let the userspace side deal with TA specific bits.
>
> I generally agree with the sentiment, however UEFI variables should IMHO be
> handled by the kernel. Moving handling of those to userspace breaks things like
> EFI-based pstore and efivarfs. The latter will in turn break some user-space
> tools (most notably efibootmgr used by e.g. GRUB and I think fwupdmgr which
> needs to set some capsule variables). Ideally, we would find a way to not break
> these, i.e. have them work out-of-the-box.
>

Only capsule-on-disk requires SetVariable() at runtime, and I doubt
whether these platforms implement any of that.

> A similar argumentation might apply to the TPM app.
>

There is a difference, though - the TPM is modeled as a device and
runtime access to it is implemented as a device driver, which is only
accessed from user space.

> > AFAIU, Qualcomm is moving away from qseecom interface to new smc-invoke interface, most of Qualcomm SoCs starting from SDM660 already have support to this.
> >
> > This interface provides a better abstracted IPC mechanism to talk to TA. Most of these TA specific interfaces are packed in closed userspace source.
> > Having said that QTEE smcinvoke driver can be modeled as a proper TEE driver with Userspace driving the TA specific bits using existing tee uapis.
> > This also brings in other features like loading, Listeners aka callbacks, secure memory allocations..etc.
> >
> > In the past, I have tried to do a prototype of this smcinvoke driver as a proper tee driver, incase you are interested patches are at https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/log/?h=tracking-qcomlt-qcomtee
> > Plan is to discuss with Qualcomm and send it for upstream review.
>
> Thanks for this information! So as far as I understand it, this is currently an
> interface to user-space only, i.e. does not allow in-kernel drivers for apps?
> It would be great if this could then be extended to handle (the bare minimum
> of) in-kernel drivers (i.e. only things that the kernel itself needs, like EFI
> variables). Alternatively, I'm happy to hear suggestions on how we not break
> the aforementioned things while moving handling off to userspace.
>
> > I think its worth exploring if uefisecapp can talk smcinvoke.
> > I can ping Qualcomm engineers to see if that is doable.
>
> I think that would be great! Thanks!
>
> Regards,
> Max

2022-08-02 19:14:01

[permalink] [raw]

Subject: Re: [PATCH 0/4] firmware: Add support for Qualcomm UEFI Secure Application

On 8/2/22 16:02, Ard Biesheuvel wrote:
> On Tue, 2 Aug 2022 at 15:22, Maximilian Luz <[email protected]> wrote:

[...]

>> I generally agree with the sentiment, however UEFI variables should IMHO be
>> handled by the kernel. Moving handling of those to userspace breaks things like
>> EFI-based pstore and efivarfs. The latter will in turn break some user-space
>> tools (most notably efibootmgr used by e.g. GRUB and I think fwupdmgr which
>> needs to set some capsule variables). Ideally, we would find a way to not break
>> these, i.e. have them work out-of-the-box.
>>
>
> Only capsule-on-disk requires SetVariable() at runtime, and I doubt
> whether these platforms implement any of that.
>
>> A similar argumentation might apply to the TPM app.
>>
>
> There is a difference, though - the TPM is modeled as a device and
> runtime access to it is implemented as a device driver, which is only
> accessed from user space.

Ah, thanks for that info! I wasn't sure about that last part.

But we'd still need _something_ in the kernel. All the common software
using TPMs would expect the TPM to be present as /dev/tpmX. So, while it
doesn't have to be a full secure-app driver, we'd need at least some way
to manage a TPM device from user-space (unless we want to tell all
software using TPMs to just support some non-standard thing instead).

For EFI variables, something similar might be possible (i.e. running
efivar operations through a user-space driver), but that will break
pstore in the times it's most usable (i.e. when no user-space exists or
things are sufficiently broken that we can't run things through it any
more).

And then (at least for me) there's the question whether that all seems
sound: Sure, we can maintain some userspace-daemon outside the kernel,
but if it is common enough (i.e. not a one-off used only by some single
vendor and model) and can be easily implemented in the kernel, why not?
Moving it to userspace makes things more complex. You'll need new
userspace APIs (as mentioned above, if you don't want to force all
existing software to adapt to some non-standard thing) and you need to
tell users to install and set up some daemon(s) (making it yet more
difficult to produce a single proper install media that works well on
all the common AArch64 or WoA platforms). All the while you still need
to maintain essentially the same piece of code (whether it is inside or
outside of the kernel), so you don't really win anything there either.

Regards,
Max

2022-09-02 07:51:37

by Sumit Garg

[permalink] [raw]

Subject: Re: [PATCH 0/4] firmware: Add support for Qualcomm UEFI Secure Application

Hi Maximilian,

On 02/08/22 18:52, Maximilian Luz wrote:
>
>
> On 8/2/22 13:51, Srinivas Kandagatla wrote:
>> Hi Maximilian,
>>
>> On 23/07/2022 23:49, Maximilian Luz wrote:
>>> On modern Qualcomm platforms, access to EFI variables is restricted to
>>> the secure world / TrustZone, i.e. the Trusted Execution Environment
>>> (TrEE or TEE) as Qualcomm seems to call it. To access EFI variables, we
>>> therefore need to talk to the UEFI Secure Application (uefisecapp),
>>> residing in the TrEE.
>>>
>>> This series adds support for accessing EFI variables on those
>>> platforms.
>>>
>>> To do this, we first need to add some SCM call functions used to manage
>>> and talk to Secure Applications. A very small subset of this interface
>>> is added in the second patch (whereas the first one exports the
>>> required
>>> functions for that). Interface specifications are extracted from [1].
>>> While this does not (yet) support re-entrant SCM calls (including
>>> callbacks and listeners), this is enough to talk to the aforementioned
>>> uefisecapp on a couple of platforms (I've tested this on a Surface
>>> Pro X
>>> and heard reports from Lenovo Flex 5G, Lenovo Thinkpad x13s, and Lenovo
>>> Yoga C630 devices).
>>>
>>> The third patch adds a client driver for uefisecapp, installing the
>>> respective efivar operations. The application interface has been
>>> reverse
>>> engineered from the Windows QcTrEE8180.sys driver.
>>>
>>> Apart from uefisecapp, there are more Secure Applications running that
>>> we might want to support in the future. For example, on the Surface Pro
>>> X (sc8180x-based), the TPM is also managed via one.
>>>
>>> I'm not sure whether this should go to drivers/firmware or to
>>> drivers/soc/qcom. I've put this into firmware as all of this is
>>> essentially an interface to the secure firmware running in the
>>> TrustZone
>>> (and SCM stuff is handled here already), but please let me know if I
>>> should move this.
>>
>> From what I see so far is that this is adapted from downstream
>> qseecom driver, this approach could work for a limited usecases but
>> not scalable, as we cannot add drivers for each Qualcomm specific TA
>> in kernel.
>> This has to be handled in much generic way using Linux TEE framework,
>> and let the userspace side deal with TA specific bits.
>
> I generally agree with the sentiment, however UEFI variables should
> IMHO be
> handled by the kernel. Moving handling of those to userspace breaks
> things like
> EFI-based pstore and efivarfs. The latter will in turn break some
> user-space
> tools (most notably efibootmgr used by e.g. GRUB and I think fwupdmgr
> which
> needs to set some capsule variables). Ideally, we would find a way to
> not break
> these, i.e. have them work out-of-the-box.
>
> A similar argumentation might apply to the TPM app.

See below, there is already an existing TPM app driver [2] in kernel
although the app is based on OP-TEE.

>
>> AFAIU, Qualcomm is moving away from qseecom interface to new
>> smc-invoke interface, most of Qualcomm SoCs starting from SDM660
>> already have support to this.
>>
>> This interface provides a better abstracted IPC mechanism to talk to
>> TA. Most of these TA specific interfaces are packed in closed
>> userspace source.
>> Having said that QTEE smcinvoke driver can be modeled as a proper TEE
>> driver with Userspace driving the TA specific bits using existing tee
>> uapis.
>> This also brings in other features like loading, Listeners aka
>> callbacks, secure memory allocations..etc.
>>
>> In the past, I have tried to do a prototype of this smcinvoke driver
>> as a proper tee driver, incase you are interested patches are at
>> https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/log/?h=tracking-qcomlt-qcomtee
>> Plan is to discuss with Qualcomm and send it for upstream review.
>
> Thanks for this information! So as far as I understand it, this is
> currently an
> interface to user-space only, i.e. does not allow in-kernel drivers
> for apps?

The Linux TEE framework already provides an in-kernel interface to TEE
as well via TEE bus [1]. There are already multiple kernel drivers [2]
[3] [4] [5] [6] [7] using it. So an EFI driver can be an addition to that.

Now coming on to TEE implementations, the drivers I mentioned are based
on OP-TEE where devices are queried/enumerated during OP-TEE probe here
[8]. So in similar manner QTEE smcinvoke driver should be able to
register devices on the TEE bus.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/staging/tee.rst#n56

[2] drivers/char/tpm/tpm_ftpm_tee.c

[3] drivers/char/hw_random/optee-rng.c

[4] drivers/firmware/arm_scmi/optee.c

[5] security/keys/trusted-keys/trusted_tee.c

[6] drivers/firmware/broadcom/tee_bnxt_fw.c

[7] drivers/rtc/rtc-optee.c

[8] drivers/tee/optee/device.c

-Sumit

PS. TBH, I haven't looked into detail workings for the QTEE smcinvoke
driver.

> It would be great if this could then be extended to handle (the bare
> minimum
> of) in-kernel drivers (i.e. only things that the kernel itself needs,
> like EFI
> variables). Alternatively, I'm happy to hear suggestions on how we not
> break
> the aforementioned things while moving handling off to userspace.
>
>> I think its worth exploring if uefisecapp can talk smcinvoke.
>> I can ping Qualcomm engineers to see if that is doable.
>
> I think that would be great! Thanks!
>
> Regards,
> Max
>

2022-09-02 14:31:45

[permalink] [raw]

Subject: Re: [PATCH 0/4] firmware: Add support for Qualcomm UEFI Secure Application

Hi,

On 9/2/22 09:26, Sumit Garg wrote:
> Hi Maximilian,
>
> On 02/08/22 18:52, Maximilian Luz wrote:

[...]

>> Thanks for this information! So as far as I understand it, this is currently an
>> interface to user-space only, i.e. does not allow in-kernel drivers for apps?
>
> The Linux TEE framework already provides an in-kernel interface to TEE as well via TEE bus [1]. There are already multiple kernel drivers [2] [3] [4] [5] [6] [7] using it. So an EFI driver can be an addition to that.
>
> Now coming on to TEE implementations, the drivers I mentioned are based on OP-TEE where devices are queried/enumerated during OP-TEE probe here [8]. So in similar manner QTEE smcinvoke driver should be able to register devices on the TEE bus.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/staging/tee.rst#n56
>
> [2] drivers/char/tpm/tpm_ftpm_tee.c
>
> [3] drivers/char/hw_random/optee-rng.c
>
> [4] drivers/firmware/arm_scmi/optee.c
>
> [5] security/keys/trusted-keys/trusted_tee.c
>
> [6] drivers/firmware/broadcom/tee_bnxt_fw.c
>
> [7] drivers/rtc/rtc-optee.c
>
> [8] drivers/tee/optee/device.c

Thanks for those links!

I think it would indeed be good if we could make it work via that
interface and I guess that should generally be possible. As far as I can
see, the biggest problem might be that the current firmware doesn't seem
to use UUIDs, so I guess we might need to emulate them somehow.

It would be great if someone with some actual knowledge of the firmware
used on those devices could have a look at this and provide some
insights.

My plan for now is to hold off on the UEFI variable driver until we have
a (proper) TEE driver, which unfortunately might be a bit out of my
depth. I'm happy to help out in any way I can though.

Regards,
Max

2022-09-05 06:57:59

by Sumit Garg

[permalink] [raw]

Subject: Re: [PATCH 0/4] firmware: Add support for Qualcomm UEFI Secure Application

+ TEE ML

On Fri, 2 Sept 2022 at 18:48, Maximilian Luz <[email protected]> wrote:
>
> Hi,
>
> On 9/2/22 09:26, Sumit Garg wrote:
> > Hi Maximilian,
> >
> > On 02/08/22 18:52, Maximilian Luz wrote:
>
> [...]
>
> >> Thanks for this information! So as far as I understand it, this is currently an
> >> interface to user-space only, i.e. does not allow in-kernel drivers for apps?
> >
> > The Linux TEE framework already provides an in-kernel interface to TEE as well via TEE bus [1]. There are already multiple kernel drivers [2] [3] [4] [5] [6] [7] using it. So an EFI driver can be an addition to that.
> >
> > Now coming on to TEE implementations, the drivers I mentioned are based on OP-TEE where devices are queried/enumerated during OP-TEE probe here [8]. So in similar manner QTEE smcinvoke driver should be able to register devices on the TEE bus.
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/staging/tee.rst#n56
> >
> > [2] drivers/char/tpm/tpm_ftpm_tee.c
> >
> > [3] drivers/char/hw_random/optee-rng.c
> >
> > [4] drivers/firmware/arm_scmi/optee.c
> >
> > [5] security/keys/trusted-keys/trusted_tee.c
> >
> > [6] drivers/firmware/broadcom/tee_bnxt_fw.c
> >
> > [7] drivers/rtc/rtc-optee.c
> >
> > [8] drivers/tee/optee/device.c
>
> Thanks for those links!
>
> I think it would indeed be good if we could make it work via that
> interface and I guess that should generally be possible. As far as I can
> see, the biggest problem might be that the current firmware doesn't seem
> to use UUIDs, so I guess we might need to emulate them somehow.
>

Okay, so I had a brief look at your driver to get an idea how QTEE
identifies its trusted/secure applications. AFAIU, it uses constant
strings as follows:

#define QCTEE_UEFISEC_APP_NAME "qcom.tz.uefisecapp"

I think we should be able to extend the TEE bus concept to accept
constant strings as device IDs as well. So if a driver wants to
support both OP-TEE and QTEE based apps then it can put corresponding
identifiers (UUID or a constant string) in the TEE device match ID
table. This way we should be able to support other TEE implementations
as I think any other identifier apart from UUID can be represented as
a constant string.

If anyone else has any better then feel free to discuss.

-Sumit

> It would be great if someone with some actual knowledge of the firmware
> used on those devices could have a look at this and provide some
> insights.
>
> My plan for now is to hold off on the UEFI variable driver until we have
> a (proper) TEE driver, which unfortunately might be a bit out of my
> depth. I'm happy to help out in any way I can though.
>
> Regards,
> Max

2022-11-23 12:26:17

by Srinivas Kandagatla

[permalink] [raw]

Subject: Re: [PATCH 0/4] firmware: Add support for Qualcomm UEFI Secure Application

Hi Max,

On 02/08/2022 14:22, Maximilian Luz wrote:
>
>
> On 8/2/22 13:51, Srinivas Kandagatla wrote:
>> Hi Maximilian,
>>
>> On 23/07/2022 23:49, Maximilian Luz wrote:
>>> On modern Qualcomm platforms, access to EFI variables is restricted to
>>> the secure world / TrustZone, i.e. the Trusted Execution Environment
>>> (TrEE or TEE) as Qualcomm seems to call it. To access EFI variables, we
>>> therefore need to talk to the UEFI Secure Application (uefisecapp),
>>> residing in the TrEE.
>>>
>>> This series adds support for accessing EFI variables on those platforms.
>>>
>>> To do this, we first need to add some SCM call functions used to manage
>>> and talk to Secure Applications. A very small subset of this interface
>>> is added in the second patch (whereas the first one exports the required
>>> functions for that). Interface specifications are extracted from [1].
>>> While this does not (yet) support re-entrant SCM calls (including
>>> callbacks and listeners), this is enough to talk to the aforementioned
>>> uefisecapp on a couple of platforms (I've tested this on a Surface Pro X
>>> and heard reports from Lenovo Flex 5G, Lenovo Thinkpad x13s, and Lenovo
>>> Yoga C630 devices).
>>>
>>> The third patch adds a client driver for uefisecapp, installing the
>>> respective efivar operations. The application interface has been reverse
>>> engineered from the Windows QcTrEE8180.sys driver.
>>>
>>> Apart from uefisecapp, there are more Secure Applications running that
>>> we might want to support in the future. For example, on the Surface Pro
>>> X (sc8180x-based), the TPM is also managed via one.
>>>
>>> I'm not sure whether this should go to drivers/firmware or to
>>> drivers/soc/qcom. I've put this into firmware as all of this is
>>> essentially an interface to the secure firmware running in the TrustZone
>>> (and SCM stuff is handled here already), but please let me know if I
>>> should move this.
>>
>> From what I see so far is that this is adapted from downstream
>> qseecom driver, this approach could work for a limited usecases but
>> not scalable, as we cannot add drivers for each Qualcomm specific TA
>> in kernel.
>> This has to be handled in much generic way using Linux TEE framework,
>> and let the userspace side deal with TA specific bits.
>
> I generally agree with the sentiment, however UEFI variables should IMHO be
> handled by the kernel. Moving handling of those to userspace breaks
> things like
> EFI-based pstore and efivarfs. The latter will in turn break some
> user-space
> tools (most notably efibootmgr used by e.g. GRUB and I think fwupdmgr which
> needs to set some capsule variables). Ideally, we would find a way to
> not break
> these, i.e. have them work out-of-the-box.
>
> A similar argumentation might apply to the TPM app.
>
>> AFAIU, Qualcomm is moving away from qseecom interface to new
>> smc-invoke interface, most of Qualcomm SoCs starting from SDM660
>> already have support to this.
>>
>> This interface provides a better abstracted IPC mechanism to talk to
>> TA. Most of these TA specific interfaces are packed in closed
>> userspace source.
>> Having said that QTEE smcinvoke driver can be modeled as a proper TEE
>> driver with Userspace driving the TA specific bits using existing tee
>> uapis.
>> This also brings in other features like loading, Listeners aka
>> callbacks, secure memory allocations..etc.
>>
>> In the past, I have tried to do a prototype of this smcinvoke driver
>> as a proper tee driver, incase you are interested patches are at
>> https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/log/?h=tracking-qcomlt-qcomtee
>> Plan is to discuss with Qualcomm and send it for upstream review.
>
> Thanks for this information! So as far as I understand it, this is
> currently an
> interface to user-space only, i.e. does not allow in-kernel drivers for
> apps?
> It would be great if this could then be extended to handle (the bare
> minimum
> of) in-kernel drivers (i.e. only things that the kernel itself needs,
> like EFI
> variables). Alternatively, I'm happy to hear suggestions on how we not
> break
> the aforementioned things while moving handling off to userspace.
>
>> I think its worth exploring if uefisecapp can talk smcinvoke.
>> I can ping Qualcomm engineers to see if that is doable.
>
> I think that would be great! Thanks!
Sorry for such a long delay to reply to this,

here is what I have.

Access to TA using SCM calls remain valid and it will continue to work
till SM8550 and probably after that if the TA is *NOT* loaded using
smcinvoke for some reasons.

But overall by default on all new SoCs after sm8550 all the access to TA
is only done via smcinvoke, and the underlying scm call that are used in
this patchset will not be supported anymore.

From SM8550, we will need to move this driver to a proper TEE client
kernel driver.

So, with that in mind, I would suggest that we carefully name the
compatibles based on SoC rather than generic ones.

--srini

>
> Regards,
> Max

2022-11-23 12:40:45

[permalink] [raw]

Subject: Re: [PATCH 0/4] firmware: Add support for Qualcomm UEFI Secure Application

Hi,

On 11/23/22 12:22, Srinivas Kandagatla wrote:
> Hi Max,
>
> On 02/08/2022 14:22, Maximilian Luz wrote:
>>
>>
>> On 8/2/22 13:51, Srinivas Kandagatla wrote:

[...]

>>> I think its worth exploring if uefisecapp can talk smcinvoke.
>>> I can ping Qualcomm engineers to see if that is doable.
>>
>> I think that would be great! Thanks!
> Sorry for such a long delay to reply to this,
>
> here is what I have.
>
> Access to TA using SCM calls remain valid and it will continue to work till SM8550 and probably after that if the TA is *NOT* loaded using smcinvoke for some reasons.
>
> But overall by default on all new SoCs after sm8550 all the access to TA is only done via smcinvoke, and the underlying scm call that are used in this patchset will not be supported anymore.
>
> From SM8550, we will need to move this driver to a proper TEE client kernel driver.
>
> So, with that in mind, I would suggest that we carefully name the compatibles based on SoC rather than generic ones.

Thanks! That makes sense.

Regards,
Max

2023-01-17 08:53:25

[permalink] [raw]

Subject: Re: [PATCH 3/4] firmware: Add support for Qualcomm UEFI Secure Application

On 1/17/23 09:24, Johan Hovold wrote:
> On Sun, Jul 24, 2022 at 12:49:48AM +0200, Maximilian Luz wrote:
>> On platforms using the Qualcomm UEFI Secure Application (uefisecapp),
>> EFI variables cannot be accessed via the standard interface in EFI
>> runtime mode. The respective functions return EFI_UNSUPPORTED. On these
>> platforms, we instead need to talk to uefisecapp. This commit provides
>> support for this and registers the respective efivars operations to
>> access EFI variables from the kernel.
>>
>> Communication with uefisecapp follows the standard Qualcomm Trusted
>> Environment (TEE or TrEE) / Secure OS conventions via the respective SCM
>> call interface. This is also the reason why variable access works
>> normally while boot services are active. During this time, said SCM
>> interface is managed by the boot services. When calling
>> ExitBootServices(), the ownership is transferred to the kernel.
>> Therefore, UEFI must not use that interface itself (as multiple parties
>> accessing this interface at the same time may lead to complications) and
>> cannot access variables for us.
>>
>> Signed-off-by: Maximilian Luz <[email protected]>
>> ---
>
>> +static struct platform_driver qcom_uefisecapp_driver = {
>> + .probe = qcom_uefisecapp_probe,
>> + .remove = qcom_uefisecapp_remove,
>> + .driver = {
>> + .name = "qcom_tee_uefisecapp",
>> + .of_match_table = qcom_uefisecapp_dt_match,
>> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>> + },
>> +};
>> +module_platform_driver(qcom_uefisecapp_driver);
>
> I noticed that for efivarfs to work, you're currently relying on having
> the firmware still claim that the variable services are supported in the
> RT_PROP table so that efi core registers the default ops at subsys init
> time (which are later overridden by this driver).
>
> Otherwise efivarfs may fail to initialise when built in:
>
> static __init int efivarfs_init(void)
> {
> if (!efivars_kobject())
> return -ENODEV;
>
> return register_filesystem(&efivarfs_type);
> }
>
> module_init(efivarfs_init);
>
> With recent X13s firmware the corresponding bit in the RT_PROP table has
> been cleared so that efivarfs would fail to initialise. Similar problem
> when booting with 'efi=noruntime'.
>
> One way to handle this is to register also the qcom_uefisecapp_driver at
> subsys init time and prevent it from being built as a module (e.g. as is
> done for the SCM driver). I'm using the below patch for this currently.
>
> I guess the Google GSMI implementation suffers from a similar problem.

Oh right, thanks for that tip!

I'll try to include that in v2 then. I'll also try to test that case
specifically.

Regards,
Max

> From 8fecce12d215bd8cab1b8c8f9f0d1e1fe20fe6e7 Mon Sep 17 00:00:00 2001
> From: Johan Hovold <[email protected]>
> Date: Sun, 15 Jan 2023 15:32:34 +0100
> Subject: [PATCH] firmware: qcom_tee_uefisecapp: register at subsys init
>
> Register efivars at subsys init time so that it is available when
> efivarfs probes. For the same reason, also prevent building the driver
> as a module.
>
> This is specifically needed on platforms such as the Lenovo Thinkpad
> X13s where the firmware has cleared the variable services in the RT_PROP
> table so that efi core does not register any efivar callbacks at subsys
> init time (which are later overridden).
>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/firmware/Kconfig | 2 +-
> drivers/firmware/qcom_tee_uefisecapp.c | 7 ++++++-
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 4e9e2c227899..48e712e363da 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -231,7 +231,7 @@ config QCOM_TEE
> select QCOM_SCM
>
> config QCOM_TEE_UEFISECAPP
> - tristate "Qualcomm TrEE UEFI Secure App client driver"
> + bool "Qualcomm TrEE UEFI Secure App client driver"
> select QCOM_TEE
> depends on EFI
> help
> diff --git a/drivers/firmware/qcom_tee_uefisecapp.c b/drivers/firmware/qcom_tee_uefisecapp.c
> index 65573e4b815a..e83bce4da70a 100644
> --- a/drivers/firmware/qcom_tee_uefisecapp.c
> +++ b/drivers/firmware/qcom_tee_uefisecapp.c
> @@ -754,7 +754,12 @@ static struct platform_driver qcom_uefisecapp_driver = {
> .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> },
> };
> -module_platform_driver(qcom_uefisecapp_driver);
> +
> +static int __init qcom_uefisecapp_init(void)
> +{
> + return platform_driver_register(&qcom_uefisecapp_driver);
> +}
> +subsys_initcall(qcom_uefisecapp_init);
>
> MODULE_AUTHOR("Maximilian Luz <[email protected]>");
> MODULE_DESCRIPTION("Client driver for Qualcomm TrEE/TZ UEFI Secure App");

2023-01-17 08:57:20

by Johan Hovold

[permalink] [raw]

Subject: Re: [PATCH 3/4] firmware: Add support for Qualcomm UEFI Secure Application

On Sun, Jul 24, 2022 at 12:49:48AM +0200, Maximilian Luz wrote:
> On platforms using the Qualcomm UEFI Secure Application (uefisecapp),
> EFI variables cannot be accessed via the standard interface in EFI
> runtime mode. The respective functions return EFI_UNSUPPORTED. On these
> platforms, we instead need to talk to uefisecapp. This commit provides
> support for this and registers the respective efivars operations to
> access EFI variables from the kernel.
>
> Communication with uefisecapp follows the standard Qualcomm Trusted
> Environment (TEE or TrEE) / Secure OS conventions via the respective SCM
> call interface. This is also the reason why variable access works
> normally while boot services are active. During this time, said SCM
> interface is managed by the boot services. When calling
> ExitBootServices(), the ownership is transferred to the kernel.
> Therefore, UEFI must not use that interface itself (as multiple parties
> accessing this interface at the same time may lead to complications) and
> cannot access variables for us.
>
> Signed-off-by: Maximilian Luz <[email protected]>
> ---

> +static struct platform_driver qcom_uefisecapp_driver = {
> + .probe = qcom_uefisecapp_probe,
> + .remove = qcom_uefisecapp_remove,
> + .driver = {
> + .name = "qcom_tee_uefisecapp",
> + .of_match_table = qcom_uefisecapp_dt_match,
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> +};
> +module_platform_driver(qcom_uefisecapp_driver);

I noticed that for efivarfs to work, you're currently relying on having
the firmware still claim that the variable services are supported in the
RT_PROP table so that efi core registers the default ops at subsys init
time (which are later overridden by this driver).

Otherwise efivarfs may fail to initialise when built in:

static __init int efivarfs_init(void)
{
if (!efivars_kobject())
return -ENODEV;

return register_filesystem(&efivarfs_type);
}

module_init(efivarfs_init);

With recent X13s firmware the corresponding bit in the RT_PROP table has
been cleared so that efivarfs would fail to initialise. Similar problem
when booting with 'efi=noruntime'.

One way to handle this is to register also the qcom_uefisecapp_driver at
subsys init time and prevent it from being built as a module (e.g. as is
done for the SCM driver). I'm using the below patch for this currently.

I guess the Google GSMI implementation suffers from a similar problem.

Johan

From 8fecce12d215bd8cab1b8c8f9f0d1e1fe20fe6e7 Mon Sep 17 00:00:00 2001
From: Johan Hovold <[email protected]>
Date: Sun, 15 Jan 2023 15:32:34 +0100
Subject: [PATCH] firmware: qcom_tee_uefisecapp: register at subsys init

Register efivars at subsys init time so that it is available when
efivarfs probes. For the same reason, also prevent building the driver
as a module.

This is specifically needed on platforms such as the Lenovo Thinkpad
X13s where the firmware has cleared the variable services in the RT_PROP
table so that efi core does not register any efivar callbacks at subsys
init time (which are later overridden).

Signed-off-by: Johan Hovold <[email protected]>
---
drivers/firmware/Kconfig | 2 +-
drivers/firmware/qcom_tee_uefisecapp.c | 7 ++++++-
2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 4e9e2c227899..48e712e363da 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -231,7 +231,7 @@ config QCOM_TEE
select QCOM_SCM

config QCOM_TEE_UEFISECAPP
- tristate "Qualcomm TrEE UEFI Secure App client driver"
+ bool "Qualcomm TrEE UEFI Secure App client driver"
select QCOM_TEE
depends on EFI
help
diff --git a/drivers/firmware/qcom_tee_uefisecapp.c b/drivers/firmware/qcom_tee_uefisecapp.c
index 65573e4b815a..e83bce4da70a 100644
--- a/drivers/firmware/qcom_tee_uefisecapp.c
+++ b/drivers/firmware/qcom_tee_uefisecapp.c
@@ -754,7 +754,12 @@ static struct platform_driver qcom_uefisecapp_driver = {
.probe_type = PROBE_PREFER_ASYNCHRONOUS,
},
};
-module_platform_driver(qcom_uefisecapp_driver);
+
+static int __init qcom_uefisecapp_init(void)
+{
+ return platform_driver_register(&qcom_uefisecapp_driver);
+}
+subsys_initcall(qcom_uefisecapp_init);

MODULE_AUTHOR("Maximilian Luz <[email protected]>");
MODULE_DESCRIPTION("Client driver for Qualcomm TrEE/TZ UEFI Secure App");
--
2.38.2

2023-01-17 11:34:54

by Johan Hovold

[permalink] [raw]

Subject: Re: [PATCH 3/4] firmware: Add support for Qualcomm UEFI Secure Application

2023-01-17 12:46:04

[permalink] [raw]

Subject: Re: [PATCH 3/4] firmware: Add support for Qualcomm UEFI Secure Application

On 1/17/23 12:05, Johan Hovold wrote:
> On Sun, Jul 24, 2022 at 12:49:48AM +0200, Maximilian Luz wrote:
>> On platforms using the Qualcomm UEFI Secure Application (uefisecapp),
>> EFI variables cannot be accessed via the standard interface in EFI
>> runtime mode. The respective functions return EFI_UNSUPPORTED. On these
>> platforms, we instead need to talk to uefisecapp. This commit provides
>> support for this and registers the respective efivars operations to
>> access EFI variables from the kernel.
>>
>> Communication with uefisecapp follows the standard Qualcomm Trusted
>> Environment (TEE or TrEE) / Secure OS conventions via the respective SCM
>> call interface. This is also the reason why variable access works
>> normally while boot services are active. During this time, said SCM
>> interface is managed by the boot services. When calling
>> ExitBootServices(), the ownership is transferred to the kernel.
>> Therefore, UEFI must not use that interface itself (as multiple parties
>> accessing this interface at the same time may lead to complications) and
>> cannot access variables for us.
>>
>> Signed-off-by: Maximilian Luz <[email protected]>
>> ---
>
>> +static int qcom_uefisecapp_probe(struct platform_device *pdev)
>> +{
>> + struct qcuefi_client *qcuefi;
>> + int status;
>
> [...]
>
>> + /* Set up kobject for efivars interface. */
>> + qcuefi->kobj = kobject_create_and_add("qcom_tee_uefisecapp", firmware_kobj);
>> + if (!qcuefi->kobj) {
>> + status = -ENOMEM;
>> + goto err_kobj;
>
> When preparing some related EFI patches, I noticed that the error labels
> here are named after where you jump from rather than after what they do
> (as is suggested by the coding standard).
>
> Would you mind changing that (throughout) for your v2?

Not at all. Will change that for v2.

Regards,
Max

>> + }
>> +
>> + /* Register global reference. */
>> + platform_set_drvdata(pdev, qcuefi);
>> + status = qcuefi_set_reference(qcuefi);
>> + if (status)
>> + goto err_ref;
>> +
>> + /* Register efivar ops. */
>> + status = efivars_register(&qcuefi->efivars, &qcom_efivar_ops, qcuefi->kobj);
>> + if (status)
>> + goto err_register;
>> +
>> + return 0;
>> +
>> +err_register:
>> + qcuefi_set_reference(NULL);
>> +err_ref:
>> + kobject_put(qcuefi->kobj);
>> +err_kobj:
>> + qctee_dma_free(qcuefi->dev, &qcuefi->dma);
>> + return status;
>> +}
>
> Johan

2023-01-18 21:29:17

[permalink] [raw]

Subject: Re: [PATCH 3/4] firmware: Add support for Qualcomm UEFI Secure Application

On 1/17/23 09:24, Johan Hovold wrote:
> On Sun, Jul 24, 2022 at 12:49:48AM +0200, Maximilian Luz wrote:
>> On platforms using the Qualcomm UEFI Secure Application (uefisecapp),
>> EFI variables cannot be accessed via the standard interface in EFI
>> runtime mode. The respective functions return EFI_UNSUPPORTED. On these
>> platforms, we instead need to talk to uefisecapp. This commit provides
>> support for this and registers the respective efivars operations to
>> access EFI variables from the kernel.
>>
>> Communication with uefisecapp follows the standard Qualcomm Trusted
>> Environment (TEE or TrEE) / Secure OS conventions via the respective SCM
>> call interface. This is also the reason why variable access works
>> normally while boot services are active. During this time, said SCM
>> interface is managed by the boot services. When calling
>> ExitBootServices(), the ownership is transferred to the kernel.
>> Therefore, UEFI must not use that interface itself (as multiple parties
>> accessing this interface at the same time may lead to complications) and
>> cannot access variables for us.
>>
>> Signed-off-by: Maximilian Luz <[email protected]>
>> ---
>
>> +static struct platform_driver qcom_uefisecapp_driver = {
>> + .probe = qcom_uefisecapp_probe,
>> + .remove = qcom_uefisecapp_remove,
>> + .driver = {
>> + .name = "qcom_tee_uefisecapp",
>> + .of_match_table = qcom_uefisecapp_dt_match,
>> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>> + },
>> +};
>> +module_platform_driver(qcom_uefisecapp_driver);
>
> I noticed that for efivarfs to work, you're currently relying on having
> the firmware still claim that the variable services are supported in the
> RT_PROP table so that efi core registers the default ops at subsys init
> time (which are later overridden by this driver).
>
> Otherwise efivarfs may fail to initialise when built in:
>
> static __init int efivarfs_init(void)
> {
> if (!efivars_kobject())
> return -ENODEV;
>
> return register_filesystem(&efivarfs_type);
> }
>
> module_init(efivarfs_init);
>
> With recent X13s firmware the corresponding bit in the RT_PROP table has
> been cleared so that efivarfs would fail to initialise. Similar problem
> when booting with 'efi=noruntime'.
>
> One way to handle this is to register also the qcom_uefisecapp_driver at
> subsys init time and prevent it from being built as a module (e.g. as is
> done for the SCM driver). I'm using the below patch for this currently.

So I've had another look and I'm not sure this will work reliably:

First, you are correct in case the RT_PROP table is cleared. In that
case, using subsys_initcall() will move the efivar registration before
the efivarfs_init() call.

However, in case EFI indicates support for variables, we will then have
generic_ops_register() and the uefisecapp's driver call running both in
subsys_initcall(). So if I'm not mistaken, this could cause the generic
ops to be registered after the uefisecapp ones, which we want to avoid.

One solution is bumping uefisecapp to fs_initcall(). Or do you have any
other suggestions?

Regards,
Max

> From 8fecce12d215bd8cab1b8c8f9f0d1e1fe20fe6e7 Mon Sep 17 00:00:00 2001
> From: Johan Hovold <[email protected]>
> Date: Sun, 15 Jan 2023 15:32:34 +0100
> Subject: [PATCH] firmware: qcom_tee_uefisecapp: register at subsys init
>
> Register efivars at subsys init time so that it is available when
> efivarfs probes. For the same reason, also prevent building the driver
> as a module.
>
> This is specifically needed on platforms such as the Lenovo Thinkpad
> X13s where the firmware has cleared the variable services in the RT_PROP
> table so that efi core does not register any efivar callbacks at subsys
> init time (which are later overridden).
>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/firmware/Kconfig | 2 +-
> drivers/firmware/qcom_tee_uefisecapp.c | 7 ++++++-
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 4e9e2c227899..48e712e363da 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -231,7 +231,7 @@ config QCOM_TEE
> select QCOM_SCM
>
> config QCOM_TEE_UEFISECAPP
> - tristate "Qualcomm TrEE UEFI Secure App client driver"
> + bool "Qualcomm TrEE UEFI Secure App client driver"
> select QCOM_TEE
> depends on EFI
> help
> diff --git a/drivers/firmware/qcom_tee_uefisecapp.c b/drivers/firmware/qcom_tee_uefisecapp.c
> index 65573e4b815a..e83bce4da70a 100644
> --- a/drivers/firmware/qcom_tee_uefisecapp.c
> +++ b/drivers/firmware/qcom_tee_uefisecapp.c
> @@ -754,7 +754,12 @@ static struct platform_driver qcom_uefisecapp_driver = {
> .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> },
> };
> -module_platform_driver(qcom_uefisecapp_driver);
> +
> +static int __init qcom_uefisecapp_init(void)
> +{
> + return platform_driver_register(&qcom_uefisecapp_driver);
> +}
> +subsys_initcall(qcom_uefisecapp_init);
>
> MODULE_AUTHOR("Maximilian Luz <[email protected]>");
> MODULE_DESCRIPTION("Client driver for Qualcomm TrEE/TZ UEFI Secure App");

2023-01-19 17:12:39

by Johan Hovold

[permalink] [raw]

Subject: Re: [PATCH 3/4] firmware: Add support for Qualcomm UEFI Secure Application

On Wed, Jan 18, 2023 at 09:45:18PM +0100, Maximilian Luz wrote:
> On 1/17/23 09:24, Johan Hovold wrote:
> > On Sun, Jul 24, 2022 at 12:49:48AM +0200, Maximilian Luz wrote:

> >> +module_platform_driver(qcom_uefisecapp_driver);
> >
> > I noticed that for efivarfs to work, you're currently relying on having
> > the firmware still claim that the variable services are supported in the
> > RT_PROP table so that efi core registers the default ops at subsys init
> > time (which are later overridden by this driver).
> >
> > Otherwise efivarfs may fail to initialise when built in:
> >
> > static __init int efivarfs_init(void)
> > {
> > if (!efivars_kobject())
> > return -ENODEV;
> >
> > return register_filesystem(&efivarfs_type);
> > }
> >
> > module_init(efivarfs_init);
> >
> > With recent X13s firmware the corresponding bit in the RT_PROP table has
> > been cleared so that efivarfs would fail to initialise. Similar problem
> > when booting with 'efi=noruntime'.
> >
> > One way to handle this is to register also the qcom_uefisecapp_driver at
> > subsys init time and prevent it from being built as a module (e.g. as is
> > done for the SCM driver). I'm using the below patch for this currently.
>
> So I've had another look and I'm not sure this will work reliably:
>
> First, you are correct in case the RT_PROP table is cleared. In that
> case, using subsys_initcall() will move the efivar registration before
> the efivarfs_init() call.
>
> However, in case EFI indicates support for variables, we will then have
> generic_ops_register() and the uefisecapp's driver call running both in
> subsys_initcall(). So if I'm not mistaken, this could cause the generic
> ops to be registered after the uefisecapp ones, which we want to avoid.

Good catch, I was using 'efi=noruntime' on the CRD so I did not notice
that race.

> One solution is bumping uefisecapp to fs_initcall(). Or do you have any
> other suggestions?

I think it would be best to avoid that if we can, but that should work.

The problem here is that the firmware claims to support the EFI variable
services even when it clearly does not and the corresponding callbacks
just return EFI_UNSUPPORTED. As far as I understand, this is still spec
compliant though so we just need to handle that.

One way to address this could be to have efi core not register the
default efivars ops in this case. That would require checking that the
services are indeed available by making one of those calls during
initialisation.

This would however expose the fact that the Google SMI implementation
implicitly relies on overriding the default ops, but I think that's a
good thing as what we have now is racy in multiple ways.

Instead I think we should move the efivarfs availability check from
module init to mount time. That should allow the Google driver, and your
SCM implementation, to continue to be built as modules.

Any consumers (e.g. the Qualcomm RTC driver) would instead need to
check if efivars is available or else defer probe.

Alternatively, it seems all efivars implementation would need to be
always-built in which is not ideal for generic kernels.

I just posted a series here as food for thought:

https://lore.kernel.org/r/[email protected]

Johan

2023-01-19 17:45:19