[PATCH 0/6] EFI variable support via OP-TEE

[PATCH 0/6] EFI variable support via OP-TEE

[Ilias Apalodimas]

With new OP-TEE and EDK2 patches (not yet upstreamed [1][2]) we can run a
secure world application which manages UEFI variables. 
Leveraging the U-Boot's OP-TEE supplicant we can then store those values 
in an RPMB device.

The Secure World application responsible for doing that is coming from
EDK2 (StandAloneMM). The StandAloneMM application is compiled from EDK2 
and then appended in the OP-TEE binary which executes that in Secure World.

There are various advantages in using StandAloneMM directly. Apart from the
obvious ones, such as running the whole code in Secure World, we are using
the EDK2 Fault Tolerant Write protocol to protect the variables against
corruption. We also avoid adding complicated code to U-Boot for the variable 
management. The only code U-Boot needs is a set OP-TEE APIs to access the new
application.

From a user's perspective this changes nothing to the usual way UEFI variables
are treated. The user will still use 'setenv/printenv -e' to perform the 
variable operations.
An 'efidebug query' command is added to retrieve information about the 
container used to store UEFI variables.

Since patches for the rest of the projects are not yet upstreamed I've tried to
provide a QEMU emulation for anyone interested in testing the patchset.

1. Download precompiled TF-A, OP-TEE binaries and a DTB from
   https://people.linaro.org/~ilias.apalodimas/secure_boot/
   bl33.bin is a precompiled U-Boot binary. If you use that skip (2)

2. git clone --single-branch --branch rpmb_hack \
    https://git.linaro.org/people/ilias.apalodimas/u-boot.git
   
   I've included a defconfig to make your life easier
   export CROSS_COMPILE=aarch64-linux-gnu-
   export ARCH=arm64
   make qemu_tfa_mm_defconfig && make -j4
   Safely ignore the warnings, they are a result of the RPMB emulation
   monstrosity ...
   The branch contains an extra patch which adds RPMB emulation into OP-TEE
   supplicant but breaks proper RPMB hardware. The patch is a gross hack 
   (but can be upstreamed later for Gitlab testing if needed).
   Since QEMU has no RPMB emulation providing one through the OP-TEE supplicant
   was the easiest way to test the patchset.

3. Create PK, KEK etc
   - PK:
   openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_PK/ -keyout \
    PK.key -out PK.crt -nodes -days 365
   cert-to-efi-sig-list -g 11111111-2222-3333-4444-123456789abc PK.crt \
    PK.esl; sign-efi-sig-list -c PK.crt -k PK.key PK PK.esl PK.auth

   - KEK:
   openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_KEK/ -keyout \
    KEK.key -out KEK.crt -nodes -days 365
   cert-to-efi-sig-list -g 11111111-2222-3333-4444-123456789abc KEK.crt \
    KEK.esl; sign-efi-sig-list -c PK.crt -k PK.key KEK KEK.esl KEK.auth

   - put them in a file and create an image
   mkdir tmp && cp PK.auth KEK.auth tmp/
   virt-make-fs -s 1M -t ext4 tmp certs.img

4. Launch QEMU (note tested on QEMU 3.1 and 5.0)
   qemu-system-aarch64  -m 1024 -smp 2 -show-cursor -serial stdio \
   -monitor null -nographic \
   -cpu cortex-a57 -bios bl1.bin  -machine virt,secure=on -d unimp \
   -semihosting-config enable,target=native -dtb ledge-qemuarm64.dtb \
   -drive if=none,file=certs.img,id=mydisk \
   -device nvme,drive=mydisk,serial=foo

   You can now load and test the certificates 
   nvme scan
   load nvme 0 0x70000000 PK.auth
   setenv -e -nv -bs -rt -at -i 0x70000000,$filesize PK; 
   load nvme 0 0x70000000 KEK.auth
   setenv -e -nv -bs -rt -at -i 0x70000000,$filesize KEK; 
   efidebug query
   printenv -e -all

This has been tested against U-Boot efi_sefltest but since StandAloneMM will
write a non-existent variable if APPEND_WRITE is specified, this test will fail.
It has also been tested for the runtime variable support (or rather absence of
it) with FWTS (https://wiki.ubuntu.com/FirmwareTestSuite).

This patchset adds the needed APIs between U-Boot and OP-TEE and StandAloneMM
to perform the variable storage.

[1] https://github.com/apalos/optee_os/tree/stmm_upstream_03_clean
[2] https://git.linaro.org/people/ilias.apalodimas/edk2-platforms.git/log/?h=patch_pcd


Ilias Apalodimas (4):
  efi_loader: Implement EFI variable handling via OP-TEE
  cmd: efidebug: Add support for querying UEFI variable storage
  MAINTAINERS: Add maintainer for EFI variables via OP-TEE
  doc: uefi.rst: Add OP-TEE variable storage config options

Sughosh Ganu (2):
  charset: Add support for calculating bytes occupied by a u16 string
  efi_loader: Add headers for EDK2 StandAloneMM communication

 MAINTAINERS                       |   7 +
 cmd/efidebug.c                    |  45 ++-
 doc/uefi/uefi.rst                 |  10 +
 include/charset.h                 |  11 +
 include/mm_communication.h        |  28 ++
 include/mm_variable.h             |  78 ++++
 lib/charset.c                     |   5 +
 lib/efi_loader/Kconfig            |   9 +
 lib/efi_loader/Makefile           |   4 +
 lib/efi_loader/efi_variable_tee.c | 645 ++++++++++++++++++++++++++++++
 10 files changed, 841 insertions(+), 1 deletion(-)
 create mode 100644 include/mm_communication.h
 create mode 100644 include/mm_variable.h
 create mode 100644 lib/efi_loader/efi_variable_tee.c

-- 
2.26.2

[PATCH 1/6] charset: Add support for calculating bytes occupied by a u16 string

[Ilias Apalodimas]

From: Sughosh Ganu <sughosh.ganu@linaro.org>

The current code uses 'u16_strlen(x) + 1) * sizeof(u16)' in various
places to calculate the number of bytes occupied by a u16 string.
Let's introduce a wrapper around this. This wrapper is used on following
patches

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 include/charset.h | 11 +++++++++++
 lib/charset.c     |  5 +++++
 2 files changed, 16 insertions(+)

diff --git a/include/charset.h b/include/charset.h
index fde6bddbc2fb..30faa72285e6 100644
--- a/include/charset.h
+++ b/include/charset.h
@@ -195,6 +195,17 @@ int u16_strncmp(const u16 *s1, const u16 *s2, size_t n);
  */
 size_t u16_strlen(const void *in);
 
+/**
+ * u16_strsize - count size of u16 string in bytes including the null character
+ *
+ * Counts the number of bytes occupied by a u16 string
+ *
+ * @in:			null terminated u16 string
+ * Return:		bytes in a u16 string
+ *
+ */
+size_t u16_strsize(const void *in);
+
 /**
  * u16_strlen - count non-zero words
  *
diff --git a/lib/charset.c b/lib/charset.c
index 1c6a7f693de4..a28034ee1f1e 100644
--- a/lib/charset.c
+++ b/lib/charset.c
@@ -379,6 +379,11 @@ size_t u16_strnlen(const u16 *in, size_t count)
 	return i;
 }
 
+size_t u16_strsize(const void *in)
+{
+	return (u16_strlen(in) + 1) * sizeof(u16);
+}
+
 u16 *u16_strcpy(u16 *dest, const u16 *src)
 {
 	u16 *tmp = dest;
-- 
2.26.2

[PATCH 2/6] efi_loader: Add headers for EDK2 StandAloneMM communication

[Ilias Apalodimas]

From: Sughosh Ganu <sughosh.ganu@linaro.org>

In Arm devices OP-TEE has the ability to run StandAloneMM (from EDK2)
in a separate partition and handle UEFI variables.
A following patch introduces this functionality.

Add the headers needed for OP-TEE <--> StandAloneMM communication

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 include/mm_communication.h | 28 ++++++++++++++
 include/mm_variable.h      | 78 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 106 insertions(+)
 create mode 100644 include/mm_communication.h
 create mode 100644 include/mm_variable.h

diff --git a/include/mm_communication.h b/include/mm_communication.h
new file mode 100644
index 000000000000..fb4c91103400
--- /dev/null
+++ b/include/mm_communication.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ *  Headers for EFI variable service via StandAloneMM, EDK2 application running
+ *  in OP-TEE
+ *
+ *  Copyright (C) 2020 Linaro Ltd. <sughosh.ganu@linaro.org>
+ *  Copyright (C) 2020 Linaro Ltd. <ilias.apalodimas@linaro.org>
+ */
+
+#if !defined _MM_COMMUNICATION_H_
+#define _MM_COMMUNICATION_H_
+
+/* defined in EDK2 MmCommunication.h */
+struct mm_communicate_header {
+	efi_guid_t header_guid;
+	size_t     message_len;
+	u8         data[1];
+};
+
+#define MM_COMMUNICATE_HEADER_SIZE \
+	(offsetof(struct mm_communicate_header, data))
+
+#define MM_RET_SUCCESS         0
+#define MM_RET_INVALID_PARAMS -2
+#define MM_RET_DENIED         -3
+#define MM_RET_NO_MEMORY      -4
+
+#endif /* _MM_COMMUNICATION_H_*/
diff --git a/include/mm_variable.h b/include/mm_variable.h
new file mode 100644
index 000000000000..f56c52597629
--- /dev/null
+++ b/include/mm_variable.h
@@ -0,0 +1,78 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ *  Headers for EFI variable service via StandAloneMM, EDK2 application running
+ *  in OP-TEE
+ *
+ *  Copyright (C) 2020 Linaro Ltd. <sughosh.ganu@linaro.org>
+ *  Copyright (C) 2020 Linaro Ltd. <ilias.apalodimas@linaro.org>
+ */
+
+#if !defined _MM_VARIABLE_H_
+#define _MM_VARIABLE_H_
+
+#include <part_efi.h>
+
+/* defined in EDK2 SmmVariableCommon.h */
+struct mm_variable_communicate_header {
+	efi_uintn_t  function;
+	efi_status_t ret_status;
+	u8           data[1];
+};
+
+#define MM_VARIABLE_COMMUNICATE_SIZE \
+	(offsetof(struct mm_variable_communicate_header, data))
+
+#define MM_VARIABLE_FUNCTION_GET_VARIABLE			1
+
+#define MM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME		2
+
+#define MM_VARIABLE_FUNCTION_SET_VARIABLE			3
+
+#define MM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO		4
+
+#define MM_VARIABLE_FUNCTION_READY_TO_BOOT			5
+
+#define MM_VARIABLE_FUNCTION_EXIT_BOOT_SERVICE			6
+
+#define MM_VARIABLE_FUNCTION_GET_STATISTICS			7
+
+#define MM_VARIABLE_FUNCTION_LOCK_VARIABLE			8
+
+#define MM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_SET	9
+
+#define MM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET	10
+
+#define MM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE			11
+
+struct mm_variable_access {
+	efi_guid_t  guid;
+	efi_uintn_t data_size;
+	efi_uintn_t name_size;
+	u32         attr;
+	u16         name[1];
+};
+
+#define MM_VARIABLE_ACCESS_HEADER_SIZE \
+	(offsetof(struct mm_variable_access, name))
+
+struct mm_variable_payload_size {
+	efi_uintn_t size;
+};
+
+struct mm_variable_getnext {
+	efi_guid_t  guid;
+	efi_uintn_t name_size;
+	u16         name[1];
+};
+
+#define MM_VARIABLE_GET_NEXT_HEADER_SIZE \
+	(offsetof(struct mm_variable_getnext, name))
+
+struct mm_variable_query_info {
+	u64 max_variable_storage;
+	u64 remaining_variable_storage;
+	u64 max_variable_size;
+	u32 attr;
+};
+
+#endif /* _MM_VARIABLE_H_ */
-- 
2.26.2

[PATCH 3/6] efi_loader: Implement EFI variable handling via OP-TEE

[Ilias Apalodimas]

In OP-TEE we can run EDK2's StandAloneMM on a secure partition.
StandAloneMM is responsible for the UEFI variable support. In
combination with OP-TEE and it's U-Boot supplicant, variables are
authenticated/validated in secure world and stored on an RPMB partition.

So let's add a new config option in U-Boot implementing the necessary
calls to OP-TEE for the variable management.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Pipat Methavanitpong <pipat1010@gmail.com>
Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 lib/efi_loader/Kconfig            |   9 +
 lib/efi_loader/Makefile           |   4 +
 lib/efi_loader/efi_variable_tee.c | 645 ++++++++++++++++++++++++++++++
 3 files changed, 658 insertions(+)
 create mode 100644 lib/efi_loader/efi_variable_tee.c

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 1cfa24ffcf72..e385cd0b9dae 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -164,4 +164,13 @@ config EFI_SECURE_BOOT
 	  it is signed with a trusted key. To do that, you need to install,
 	  at least, PK, KEK and db.
 
+config EFI_MM_COMM_TEE
+	bool "UEFI variables storage service via OP-TEE"
+	depends on SUPPORT_EMMC_RPMB
+	default n
+	help
+	  If OP-TEE is present and running StandAloneMM dispatch all UEFI variable
+	  related operations to that. The application will verify, authenticate and
+	  store the variables on an RPMB
+
 endif
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index eff3c25ec301..dba652121eab 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -34,7 +34,11 @@ obj-y += efi_root_node.o
 obj-y += efi_runtime.o
 obj-y += efi_setup.o
 obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
+ifeq ($(CONFIG_EFI_MM_COMM_TEE),y)
+obj-y += efi_variable_tee.o
+else
 obj-y += efi_variable.o
+endif
 obj-y += efi_watchdog.o
 obj-$(CONFIG_LCD) += efi_gop.o
 obj-$(CONFIG_DM_VIDEO) += efi_gop.o
diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
new file mode 100644
index 000000000000..d4e206e22073
--- /dev/null
+++ b/lib/efi_loader/efi_variable_tee.c
@@ -0,0 +1,645 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  EFI variable service via OP-TEE
+ *
+ *  Copyright (C) 2019 Linaro Ltd. <sughosh.ganu@linaro.org>
+ *  Copyright (C) 2019 Linaro Ltd. <ilias.apalodimas@linaro.org>
+ */
+
+#include <common.h>
+#include <efi.h>
+#include <efi_api.h>
+#include <efi_loader.h>
+#include <tee.h>
+#include <malloc.h>
+#include <mm_communication.h>
+#include <mm_variable.h>
+
+#define PTA_STMM_CMDID_COMMUNICATE 0
+#define PTA_STMM_UUID { 0xed32d533, 0x99e6, 0x4209, {\
+			0x9c, 0xc0, 0x2d, 0x72, 0xcd, 0xd9, 0x98, 0xa7 } }
+
+#define EFI_MM_VARIABLE_GUID \
+	EFI_GUID(0xed32d533, 0x99e6, 0x4209, \
+		 0x9c, 0xc0, 0x2d, 0x72, 0xcd, 0xd9, 0x98, 0xa7)
+
+static efi_uintn_t max_buffer_size;	/* comm + var + func + data */
+static efi_uintn_t max_payload_size;	/* func + data */
+
+struct mm_connection {
+	struct udevice *tee;
+	u32 session;
+};
+
+int get_connection(struct mm_connection *conn)
+{
+	static const struct tee_optee_ta_uuid uuid = PTA_STMM_UUID;
+	struct udevice *tee = NULL;
+	struct tee_open_session_arg arg;
+	int rc;
+
+	tee = tee_find_device(tee, NULL, NULL, NULL);
+	if (!tee)
+		return -ENODEV;
+
+	memset(&arg, 0, sizeof(arg));
+	tee_optee_ta_uuid_to_octets(arg.uuid, &uuid);
+	rc = tee_open_session(tee, &arg, 0, NULL);
+	if (!rc) {
+		conn->tee = tee;
+		conn->session = arg.session;
+	}
+
+	return rc;
+}
+
+/**
+ * optee_mm_communicate() - Pass a buffer to StandaloneMM running in OP-TEE
+ *
+ * @comm_buf:		Locally allocted communcation buffer
+ * @dsize:		Buffer size
+ * Return:		status code
+ */
+static efi_status_t optee_mm_communicate(void *comm_buf, ulong dsize)
+{
+	ulong buf_size;
+	efi_status_t ret;
+	struct mm_communicate_header *mm_hdr;
+	struct mm_connection conn = { NULL, 0 };
+	struct tee_invoke_arg arg;
+	struct tee_param param[2];
+	struct tee_shm *shm = NULL;
+	int rc;
+
+	if (!comm_buf)
+		return EFI_INVALID_PARAMETER;
+
+	mm_hdr = (struct mm_communicate_header *)comm_buf;
+	buf_size = mm_hdr->message_len + sizeof(efi_guid_t) + sizeof(size_t);
+
+	if (dsize != buf_size)
+		return EFI_INVALID_PARAMETER;
+
+	rc = get_connection(&conn);
+	if (rc) {
+		log_err("Unable to open OP-TEE session (err=%d)\n", rc);
+		return EFI_UNSUPPORTED;
+	}
+
+	if (tee_shm_register(conn.tee, comm_buf, buf_size, 0, &shm)) {
+		log_err("Unable to register shared memory\n");
+		return EFI_UNSUPPORTED;
+	}
+
+	memset(&arg, 0, sizeof(arg));
+	arg.func = PTA_STMM_CMDID_COMMUNICATE;
+	arg.session = conn.session;
+
+	memset(param, 0, sizeof(param));
+	param[0].attr = TEE_PARAM_ATTR_TYPE_MEMREF_INOUT;
+	param[0].u.memref.size = buf_size;
+	param[0].u.memref.shm = shm;
+	param[1].attr = TEE_PARAM_ATTR_TYPE_VALUE_OUTPUT;
+
+	rc = tee_invoke_func(conn.tee, &arg, 2, param);
+	if (rc)
+		return EFI_INVALID_PARAMETER;
+	tee_shm_free(shm);
+	tee_close_session(conn.tee, conn.session);
+
+	switch (param[1].u.value.a) {
+	case MM_RET_SUCCESS:
+		ret = EFI_SUCCESS;
+		break;
+
+	case MM_RET_INVALID_PARAMS:
+		ret = EFI_INVALID_PARAMETER;
+		break;
+
+	case MM_RET_DENIED:
+		ret = EFI_ACCESS_DENIED;
+		break;
+
+	case MM_RET_NO_MEMORY:
+		ret = EFI_OUT_OF_RESOURCES;
+		break;
+
+	default:
+		ret = EFI_ACCESS_DENIED;
+	}
+
+	return ret;
+}
+
+/**
+ * mm_communicate() - Adjust the cmonnucation buffer to StandAlonneMM and send
+ * it to OP-TEE
+ *
+ * @comm_buf:		Locally allocted communcation buffer
+ * @dsize:		Buffer size
+ * Return:		status code
+ */
+static efi_status_t mm_communicate(u8 *comm_buf, efi_uintn_t dsize)
+{
+	efi_status_t ret;
+	struct mm_communicate_header *mm_hdr;
+	struct mm_variable_communicate_header *var_hdr;
+
+	dsize += MM_COMMUNICATE_HEADER_SIZE + MM_VARIABLE_COMMUNICATE_SIZE;
+	mm_hdr = (struct mm_communicate_header *)comm_buf;
+	var_hdr = (struct mm_variable_communicate_header *)mm_hdr->data;
+
+	ret = optee_mm_communicate(comm_buf, dsize);
+	if (ret != EFI_SUCCESS) {
+		log_err("%s failed!\n", __func__);
+		return ret;
+	}
+
+	return var_hdr->ret_status;
+}
+
+/**
+ * setup_mm_hdr() -	Allocate a buffer for StandAloneMM and initialize the
+ *			header data.
+ *
+ * @dptr:		Pointer address of the corresponding StandAloneMM
+ *			function
+ * @payload_size:	Buffer size
+ * @func:		StandAloneMM function number
+ * @ret:		EFI return code
+ * Return:		Buffer or NULL
+ */
+static u8 *setup_mm_hdr(void **dptr, efi_uintn_t payload_size,
+			efi_uintn_t func, efi_status_t *ret)
+{
+	const efi_guid_t mm_var_guid = EFI_MM_VARIABLE_GUID;
+	struct mm_communicate_header *mm_hdr;
+	struct mm_variable_communicate_header *var_hdr;
+	u8 *comm_buf;
+
+	/* On the init function we initialize max_buffer_size with
+	 * get_max_payload(). So skip the test if max_buffer_size is initialized
+	 * StandAloneMM will perform similar checks and drop the buffer if it's
+	 * too long
+	 */
+	if (max_buffer_size && max_buffer_size <
+			(MM_COMMUNICATE_HEADER_SIZE +
+			 MM_VARIABLE_COMMUNICATE_SIZE +
+			 payload_size)) {
+		*ret = EFI_INVALID_PARAMETER;
+		return NULL;
+	}
+
+	comm_buf = calloc(1, MM_COMMUNICATE_HEADER_SIZE +
+			  MM_VARIABLE_COMMUNICATE_SIZE +
+			  payload_size);
+	if (!comm_buf) {
+		*ret = EFI_OUT_OF_RESOURCES;
+		return NULL;
+	}
+
+	mm_hdr = (struct mm_communicate_header *)comm_buf;
+	guidcpy(&mm_hdr->header_guid, &mm_var_guid);
+	mm_hdr->message_len = MM_VARIABLE_COMMUNICATE_SIZE + payload_size;
+
+	var_hdr = (struct mm_variable_communicate_header *)mm_hdr->data;
+	var_hdr->function = func;
+	if (dptr)
+		*dptr = var_hdr->data;
+	*ret = EFI_SUCCESS;
+
+	return comm_buf;
+}
+
+/**
+ * Get variable payload size from StandAloneMM
+ *
+ * @size:    Size of the variable in storage
+ * Return:   status code
+ */
+efi_status_t EFIAPI get_max_payload(efi_uintn_t *size)
+{
+	struct mm_variable_payload_size *var_payload = NULL;
+	efi_uintn_t payload_size;
+	efi_status_t ret;
+	u8 *comm_buf;
+
+	if (!size)
+		return EFI_INVALID_PARAMETER;
+
+	payload_size = sizeof(*var_payload);
+	comm_buf = setup_mm_hdr((void **)&var_payload, payload_size,
+				MM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE, &ret);
+	if (!comm_buf)
+		return EFI_EXIT(ret);
+
+	ret = mm_communicate(comm_buf, payload_size);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	*size = var_payload->size;
+
+out:
+	free(comm_buf);
+	return ret;
+}
+
+/**
+ * efi_get_variable() - retrieve value of a UEFI variable
+ *
+ * This function implements the GetVariable runtime service.
+ *
+ * See the Unified Extensible Firmware Interface (UEFI) specification for
+ * details.
+ *
+ * @name:		name of the variable
+ * @guid:		vendor GUID
+ * @attr:		attributes of the variable
+ * @data_size:		size of the buffer to which the variable value is copied
+ * @data:		buffer to which the variable value is copied
+ * Return:		status code
+ */
+efi_status_t EFIAPI efi_get_variable(u16 *name, const efi_guid_t *guid,
+				     u32 *attr, efi_uintn_t *data_size,
+				     void *data)
+{
+	struct mm_variable_access *var_acc;
+	efi_uintn_t payload_size;
+	efi_uintn_t name_size;
+	efi_uintn_t tmp_dsize;
+	efi_status_t ret;
+	u8 *comm_buf;
+
+	EFI_ENTRY("\"%ls\" %pUl %p %p %p", name, guid, attr, data_size, data);
+
+	if (!name || !guid || !data_size)
+		return EFI_EXIT(EFI_INVALID_PARAMETER);
+
+	/* Check payload size */
+	name_size = u16_strsize(name);
+	if (name_size > max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE)
+		return EFI_EXIT(EFI_INVALID_PARAMETER);
+
+	/* Trim output buffer size */
+	tmp_dsize = *data_size;
+	if (name_size + tmp_dsize >
+			max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE) {
+		tmp_dsize = max_payload_size -
+				MM_VARIABLE_ACCESS_HEADER_SIZE -
+				name_size;
+	}
+
+	/* Get comm buffer and initialize header */
+	payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + tmp_dsize;
+	comm_buf = setup_mm_hdr((void **)&var_acc, payload_size,
+				MM_VARIABLE_FUNCTION_GET_VARIABLE, &ret);
+	if (!comm_buf)
+		return EFI_EXIT(ret);
+
+	/* Fill in contents */
+	guidcpy(&var_acc->guid, guid);
+	var_acc->data_size = tmp_dsize;
+	var_acc->name_size = name_size;
+	var_acc->attr = attr ? *attr : 0;
+	memcpy(var_acc->name, name, name_size);
+
+	/* Communicate */
+	ret = mm_communicate(comm_buf, payload_size);
+	if (ret == EFI_SUCCESS || ret == EFI_BUFFER_TOO_SMALL) {
+		/* Update with reported data size for trimmed case */
+		*data_size = var_acc->data_size;
+	}
+	if (ret != EFI_SUCCESS)
+		goto done;
+
+	if (attr)
+		*attr = var_acc->attr;
+	if (data)
+		memcpy(data, (u8 *)var_acc->name + var_acc->name_size,
+		       var_acc->data_size);
+	else
+		ret = EFI_INVALID_PARAMETER;
+
+done:
+	free(comm_buf);
+	return EFI_EXIT(ret);
+}
+
+/**
+ * efi_get_next_variable_name() - enumerate the current variable names
+ *
+ * @variable_name_size:	size of variable_name buffer in byte
+ * @variable_name:	name of uefi variable's name in u16
+ * @guid:		vendor's guid
+ *
+ * This function implements the GetNextVariableName service.
+ *
+ * See the Unified Extensible Firmware Interface (UEFI) specification for
+ * details.
+ *
+ * Return: status code
+ */
+efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
+					       u16 *variable_name,
+					       efi_guid_t *guid)
+{
+	struct mm_variable_getnext *var_getnext;
+	efi_uintn_t payload_size;
+	efi_uintn_t tmp_dsize;
+	efi_uintn_t name_size;
+	efi_status_t ret;
+	efi_uintn_t out_name_size;
+	efi_uintn_t in_name_size;
+	u8 *comm_buf;
+
+	EFI_ENTRY("%p \"%ls\" %pUl", variable_name_size, variable_name, guid);
+
+	if (!variable_name_size || !variable_name || !guid)
+		return EFI_EXIT(EFI_INVALID_PARAMETER);
+
+	out_name_size = *variable_name_size;
+	in_name_size = u16_strsize(variable_name);
+
+	name_size = u16_strsize(variable_name);
+	if (name_size > max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE)
+		return EFI_EXIT(EFI_INVALID_PARAMETER);
+
+	/* Trim output buffer size */
+	tmp_dsize = *variable_name_size;
+	if (name_size + tmp_dsize >
+			max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE) {
+		tmp_dsize = max_payload_size -
+				MM_VARIABLE_GET_NEXT_HEADER_SIZE -
+				name_size;
+	}
+
+	payload_size = MM_VARIABLE_GET_NEXT_HEADER_SIZE +
+			max(out_name_size, in_name_size);
+	comm_buf = setup_mm_hdr((void **)&var_getnext, payload_size,
+				MM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME,
+				&ret);
+	if (!comm_buf)
+		return EFI_EXIT(ret);
+
+	/* Fill in contents */
+	guidcpy(&var_getnext->guid, guid);
+	var_getnext->name_size = out_name_size;
+	memcpy(var_getnext->name, variable_name, in_name_size);
+	if (out_name_size > in_name_size) {
+		memset((u8 *)var_getnext->name + in_name_size, 0x0,
+		       out_name_size - in_name_size);
+	}
+
+	/* Communicate */
+	ret = mm_communicate(comm_buf, payload_size);
+	if (ret == EFI_SUCCESS || ret == EFI_BUFFER_TOO_SMALL) {
+		/* Update with reported data size for trimmed case */
+		*variable_name_size = var_getnext->name_size;
+	}
+	if (ret != EFI_SUCCESS)
+		goto done;
+
+	guidcpy(guid, &var_getnext->guid);
+	memcpy(variable_name, (u8 *)var_getnext->name,
+	       var_getnext->name_size);
+
+done:
+	free(comm_buf);
+	return EFI_EXIT(ret);
+}
+
+/**
+ * efi_set_variable() - set value of a UEFI variable
+ *
+ * This function implements the SetVariable runtime service.
+ *
+ * See the Unified Extensible Firmware Interface (UEFI) specification for
+ * details.
+ *
+ * @name:		name of the variable
+ * @guid:		vendor GUID
+ * @attr:		attributes of the variable
+ * @data_size:		size of the buffer with the variable value
+ * @data:		buffer with the variable value
+ * Return:		status code
+ */
+efi_status_t EFIAPI efi_set_variable(u16 *name, const efi_guid_t *guid,
+				     u32 attr, efi_uintn_t data_size,
+				     const void *data)
+{
+	struct mm_variable_access *var_acc;
+	efi_uintn_t payload_size;
+	efi_uintn_t name_size;
+	efi_status_t ret;
+	u8 *comm_buf;
+
+	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", name, guid, attr, data_size, data);
+
+	if (!name || name[0] == 0 || !guid)
+		return EFI_EXIT(EFI_INVALID_PARAMETER);
+	if (data_size > 0 && !data)
+		return EFI_EXIT(EFI_INVALID_PARAMETER);
+
+	/* Check payload size */
+	name_size = u16_strsize(name);
+	payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + data_size;
+	if (payload_size > max_payload_size)
+		return EFI_EXIT(EFI_INVALID_PARAMETER);
+
+	/* Get comm buffer and initialize header */
+	comm_buf = setup_mm_hdr((void **)&var_acc, payload_size,
+				MM_VARIABLE_FUNCTION_SET_VARIABLE, &ret);
+	if (!comm_buf)
+		return EFI_EXIT(ret);
+
+	/* Fill in contents */
+	guidcpy(&var_acc->guid, guid);
+	var_acc->data_size = data_size;
+	var_acc->name_size = name_size;
+	var_acc->attr = attr;
+	memcpy(var_acc->name, name, name_size);
+	memcpy((u8 *)var_acc->name + name_size, data, data_size);
+
+	/* Communicate */
+	ret = mm_communicate(comm_buf, payload_size);
+	free(comm_buf);
+
+	return EFI_EXIT(ret);
+}
+
+/**
+ * efi_query_variable_info() - get information about EFI variables
+ *
+ * This function implements the QueryVariableInfo() runtime service.
+ *
+ * See the Unified Extensible Firmware Interface (UEFI) specification for
+ * details.
+ *
+ * @attributes:				bitmask to select variables to be
+ *					queried
+ * @maximum_variable_storage_size:	maximum size of storage area for the
+ *					selected variable types
+ * @remaining_variable_storage_size:	remaining size of storage are for the
+ *					selected variable types
+ * @maximum_variable_size:		maximum size of a variable of the
+ *					selected type
+ * Returns:				status code
+ */
+efi_status_t EFIAPI __efi_runtime
+efi_query_variable_info(u32 attributes, u64 *max_variable_storage_size,
+			u64 *remain_variable_storage_size,
+			u64 *max_variable_size)
+{
+	struct mm_variable_query_info *mm_query_info;
+	efi_uintn_t payload_size;
+	efi_status_t ret;
+	u8 *comm_buf;
+
+	EFI_ENTRY("%x %p %p %p", attributes, max_variable_storage_size,
+		  remain_variable_storage_size, max_variable_size);
+
+	payload_size = sizeof(*mm_query_info);
+	comm_buf = setup_mm_hdr((void **)&mm_query_info, payload_size,
+				MM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO, &ret);
+	if (!comm_buf)
+		return EFI_EXIT(ret);
+
+	mm_query_info->attr = attributes;
+	ret = mm_communicate(comm_buf, payload_size);
+	if (ret != EFI_SUCCESS)
+		goto out;
+	*max_variable_storage_size = mm_query_info->max_variable_storage;
+	*remain_variable_storage_size =
+			mm_query_info->remaining_variable_storage;
+	*max_variable_size = mm_query_info->max_variable_size;
+
+out:
+	free(comm_buf);
+	return EFI_EXIT(ret);
+}
+
+/**
+ * efi_get_variable_runtime() - runtime implementation of GetVariable()
+ *
+ * @variable_name:	name of the variable
+ * @guid:		vendor GUID
+ * @attributes:		attributes of the variable
+ * @data_size:		size of the buffer to which the variable value is copied
+ * @data:		buffer to which the variable value is copied
+ * Return:		status code
+ */
+static efi_status_t __efi_runtime EFIAPI
+efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
+			 u32 *attributes, efi_uintn_t *data_size, void *data)
+{
+	return EFI_UNSUPPORTED;
+}
+
+/**
+ * efi_get_next_variable_name_runtime() - runtime implementation of
+ *					  GetNextVariable()
+ *
+ * @variable_name_size:	size of variable_name buffer in byte
+ * @variable_name:	name of uefi variable's name in u16
+ * @guid:		vendor's guid
+ * Return: status code
+ */
+static efi_status_t __efi_runtime EFIAPI
+efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
+				   u16 *variable_name, efi_guid_t *guid)
+{
+	return EFI_UNSUPPORTED;
+}
+
+/**
+ * efi_query_variable_info() - get information about EFI variables
+ *
+ * This function implements the QueryVariableInfo() runtime service.
+ *
+ * See the Unified Extensible Firmware Interface (UEFI) specification for
+ * details.
+ *
+ * @attributes:				bitmask to select variables to be
+ *					queried
+ * @maximum_variable_storage_size:	maximum size of storage area for the
+ *					selected variable types
+ * @remaining_variable_storage_size:	remaining size of storage are for the
+ *					selected variable types
+ * @maximum_variable_size:		maximum size of a variable of the
+ *					selected type
+ * Returns:				status code
+ */
+efi_status_t EFIAPI __efi_runtime
+efi_query_variable_info_runtime(u32 attributes, u64 *max_variable_storage_size,
+				u64 *remain_variable_storage_size,
+				u64 *max_variable_size)
+{
+	return EFI_UNSUPPORTED;
+}
+
+/**
+ * efi_set_variable_runtime() - runtime implementation of SetVariable()
+ *
+ * @variable_name:	name of the variable
+ * @guid:		vendor GUID
+ * @attributes:		attributes of the variable
+ * @data_size:		size of the buffer with the variable value
+ * @data:		buffer with the variable value
+ * Return:		status code
+ */
+static efi_status_t __efi_runtime EFIAPI
+efi_set_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
+			 u32 attributes, efi_uintn_t data_size,
+			 const void *data)
+{
+	return EFI_UNSUPPORTED;
+}
+
+/**
+ * efi_variables_boot_exit_notify() - notify ExitBootServices() is called
+ */
+void efi_variables_boot_exit_notify(void)
+{
+	u8 *comm_buf;
+	efi_status_t ret;
+
+	comm_buf = setup_mm_hdr(NULL, 0, MM_VARIABLE_FUNCTION_EXIT_BOOT_SERVICE,
+				&ret);
+	if (comm_buf)
+		ret = mm_communicate(comm_buf, 0);
+	else
+		ret = EFI_NOT_FOUND;
+
+	if (ret != EFI_SUCCESS)
+		log_err("Unable to notify StMM for ExitBootServices\n");
+	free(comm_buf);
+
+	/* Update runtime service table */
+	efi_runtime_services.query_variable_info =
+			efi_query_variable_info_runtime;
+	efi_runtime_services.get_variable = efi_get_variable_runtime;
+	efi_runtime_services.get_next_variable_name =
+			efi_get_next_variable_name_runtime;
+	efi_runtime_services.set_variable = efi_set_variable_runtime;
+	efi_update_table_header_crc32(&efi_runtime_services.hdr);
+}
+
+/**
+ * efi_init_variables() - initialize variable services
+ *
+ * Return:	status code
+ */
+efi_status_t efi_init_variables(void)
+{
+	efi_status_t ret;
+
+	ret = get_max_payload(&max_payload_size);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	max_buffer_size = MM_COMMUNICATE_HEADER_SIZE +
+			  MM_VARIABLE_COMMUNICATE_SIZE +
+			  max_payload_size;
+
+	return EFI_SUCCESS;
+}
-- 
2.26.2

[PATCH 4/6] cmd: efidebug: Add support for querying UEFI variable storage

[Ilias Apalodimas]

With the previous patches that use OP-TEE and StandAloneMM for UEFI
variable storage we've added functionality for efi_query_variable_info.
So let's add the relevant command to efidebug and retrieve information
about the container used to store UEFI variables

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 cmd/efidebug.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index d8a76d78a388..17e36ef76d69 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -1160,6 +1160,45 @@ static int do_efi_test(cmd_tbl_t *cmdtp, int flag,
 	return cp->cmd(cmdtp, flag, argc, argv);
 }
 
+/**
+ * do_efi_query_info() - QueryVariableInfo EFI service
+ *
+ * @cmdtp:	Command table
+ * @flag:	Command flag
+ * @argc:	Number of arguments
+ * @argv:	Argument array
+ * Return:	CMD_RET_SUCCESS on success,
+ *		CMD_RET_USAGE or CMD_RET_FAILURE on failure
+ *
+ * Implement efidebug "test" sub-command.
+ */
+
+static int do_efi_query_info(cmd_tbl_t *cmdtp, int flag,
+			     int argc, char * const argv[])
+{
+	efi_status_t ret;
+	u32 attr = EFI_VARIABLE_BOOTSERVICE_ACCESS |
+			EFI_VARIABLE_RUNTIME_ACCESS |
+			EFI_VARIABLE_NON_VOLATILE;
+	u64 max_variable_storage_size;
+	u64 remain_variable_storage_size;
+	u64 max_variable_size;
+
+	ret = EFI_CALL(efi_query_variable_info(attr,
+					       &max_variable_storage_size,
+					       &remain_variable_storage_size,
+					       &max_variable_size));
+	if (ret != EFI_SUCCESS)
+		return CMD_RET_FAILURE;
+
+	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
+	printf("Max storage size %llu\n", max_variable_storage_size);
+	printf("Remaining storage size %llu\n", remain_variable_storage_size);
+	printf("Max variable size %llu\n", max_variable_size);
+
+	return CMD_RET_SUCCESS;
+}
+
 static cmd_tbl_t cmd_efidebug_sub[] = {
 	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
 	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
@@ -1176,6 +1215,8 @@ static cmd_tbl_t cmd_efidebug_sub[] = {
 			 "", ""),
 	U_BOOT_CMD_MKENT(test, CONFIG_SYS_MAXARGS, 1, do_efi_test,
 			 "", ""),
+	U_BOOT_CMD_MKENT(query, CONFIG_SYS_MAXARGS, 1, do_efi_query_info,
+			 "", ""),
 };
 
 /**
@@ -1247,7 +1288,9 @@ static char efidebug_help_text[] =
 	"efidebug tables\n"
 	"  - show UEFI configuration tables\n"
 	"efidebug test bootmgr\n"
-	"  - run simple bootmgr for test\n";
+	"  - run simple bootmgr for test\n"
+	"efidebug query\n"
+	"  - show information of the container used to store UEFI variables\n";
 #endif
 
 U_BOOT_CMD(
-- 
2.26.2

[PATCH 5/6] MAINTAINERS: Add maintainer for EFI variables via OP-TEE

[Ilias Apalodimas]

Add myself as maintainer for the OP-TEE related UEFI variable storage
and add the headers files on the existing EFI list

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index ec59ce8b8802..f33fd74b330b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -625,6 +625,8 @@ F:	include/cp437.h
 F:	include/efi*
 F:	include/pe.h
 F:	include/asm-generic/pe.h
+F:	include/mm_communication.h
+F:	include/mm_variable.h
 F:	lib/charset.c
 F:	lib/efi*/
 F:	test/py/tests/test_efi*
@@ -635,6 +637,11 @@ F:	cmd/efidebug.c
 F:	cmd/nvedit_efi.c
 F:	tools/file2include.c
 
+EFI VARIABLES VIA OP-TEE
+M:	Ilias Apalodimas <ilias.apalodimas@linaro.org>
+S:	Maintained
+F:	lib/efi_loader/efi_variable_tee.c
+
 ENVIRONMENT
 M:	Joe Hershberger <joe.hershberger@ni.com>
 R:	Wolfgang Denk <wd@denx.de>
-- 
2.26.2

[PATCH 6/6] doc: uefi.rst: Add OP-TEE variable storage config options

[Ilias Apalodimas]

If OP-TEE is compiled with an EDK2 application running in secure world
it can process and store UEFI variables in an RPMB.
Add documentation for the config options enabling this

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 doc/uefi/uefi.rst | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/doc/uefi/uefi.rst b/doc/uefi/uefi.rst
index 4fda00d68721..93b0faadd26e 100644
--- a/doc/uefi/uefi.rst
+++ b/doc/uefi/uefi.rst
@@ -188,6 +188,16 @@ on the sandbox
     cd <U-Boot source directory>
     pytest.py test/py/tests/test_efi_secboot/test_signed.py --bd sandbox
 
+Using OP-TEE for EFI variables
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+If an RPMB and it's drivers is available in U-Boot, OP-TEE can be used for
+variable services.
+Enabling CONFIG_EFI_MM_COMM_TEE=y will dispatch the variables services to
+OP-TEE. OP-TEE needs to be compiled with a secure application (coming from EDK2)
+which will process variables in the Secure World and store them in the RPMB
+using the OP-TEE supplicant.
+
 Executing the boot manager
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-- 
2.26.2

[PATCH 1/6] charset: Add support for calculating bytes occupied by a u16 string

[Heinrich Schuchardt]

On 5/6/20 9:12 PM, Ilias Apalodimas wrote:
> From: Sughosh Ganu <sughosh.ganu@linaro.org>
>
> The current code uses 'u16_strlen(x) + 1) * sizeof(u16)' in various
> places to calculate the number of bytes occupied by a u16 string.
> Let's introduce a wrapper around this. This wrapper is used on following
> patches
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  include/charset.h | 11 +++++++++++
>  lib/charset.c     |  5 +++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/include/charset.h b/include/charset.h
> index fde6bddbc2fb..30faa72285e6 100644
> --- a/include/charset.h
> +++ b/include/charset.h
> @@ -195,6 +195,17 @@ int u16_strncmp(const u16 *s1, const u16 *s2, size_t n);
>   */
>  size_t u16_strlen(const void *in);
>
> +/**
> + * u16_strsize - count size of u16 string in bytes including the null character

Parentheses missing, cf.
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

A test in test/unicode_ut.c is missing for the function.

I will add both.

Otherwise
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> + *
> + * Counts the number of bytes occupied by a u16 string
> + *
> + * @in:			null terminated u16 string
> + * Return:		bytes in a u16 string
> + *
> + */
> +size_t u16_strsize(const void *in);
> +
>  /**
>   * u16_strlen - count non-zero words
>   *
> diff --git a/lib/charset.c b/lib/charset.c
> index 1c6a7f693de4..a28034ee1f1e 100644
> --- a/lib/charset.c
> +++ b/lib/charset.c
> @@ -379,6 +379,11 @@ size_t u16_strnlen(const u16 *in, size_t count)
>  	return i;
>  }
>
> +size_t u16_strsize(const void *in)
> +{
> +	return (u16_strlen(in) + 1) * sizeof(u16);
> +}
> +
>  u16 *u16_strcpy(u16 *dest, const u16 *src)
>  {
>  	u16 *tmp = dest;
>

[PATCH 2/6] efi_loader: Add headers for EDK2 StandAloneMM communication

[Heinrich Schuchardt]

On 5/6/20 9:12 PM, Ilias Apalodimas wrote:
> From: Sughosh Ganu <sughosh.ganu@linaro.org>
>
> In Arm devices OP-TEE has the ability to run StandAloneMM (from EDK2)
> in a separate partition and handle UEFI variables.
> A following patch introduces this functionality.
>
> Add the headers needed for OP-TEE <--> StandAloneMM communication
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  include/mm_communication.h | 28 ++++++++++++++
>  include/mm_variable.h      | 78 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 106 insertions(+)
>  create mode 100644 include/mm_communication.h
>  create mode 100644 include/mm_variable.h
>
> diff --git a/include/mm_communication.h b/include/mm_communication.h
> new file mode 100644
> index 000000000000..fb4c91103400
> --- /dev/null
> +++ b/include/mm_communication.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */

BSD-2-Clause-Patent is compatible with GPL-2. So relicensing as GPL
should be ok.

> +/*
> + *  Headers for EFI variable service via StandAloneMM, EDK2 application running
> + *  in OP-TEE
> + *
> + *  Copyright (C) 2020 Linaro Ltd. <sughosh.ganu@linaro.org>
> + *  Copyright (C) 2020 Linaro Ltd. <ilias.apalodimas@linaro.org>

EDK2, MdePkg/Include/Protocol/MmCommunication.h has:
Copyright (c) 2017, Intel Corporation. All rights reserved.

Why did you replace their copyright by yours?
Who is the original copyright holder of the MM module?

> + */
> +
> +#if !defined _MM_COMMUNICATION_H_

I would use #ifndef here.

> +#define _MM_COMMUNICATION_H_
> +
> +/* defined in EDK2 MmCommunication.h */

Please, add a description of the structure, cf.
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation

> +struct mm_communicate_header {
> +	efi_guid_t header_guid;
> +	size_t     message_len;
> +	u8         data[1];

In C11 you can use data[].

> +};
> +
> +#define MM_COMMUNICATE_HEADER_SIZE \
> +	(offsetof(struct mm_communicate_header, data))

SMM_COMMUNICATE_HEADER_SIZE?
Why not simply use data[] and sizeof(struct mm_communicate_header)
instead of defining this constant.

> +
> +#define MM_RET_SUCCESS         0

Why don't you use the same names as in EDK2, e.g.
ARM_SMC_MM_RET_SUCCESS?

Please, add ARM_SMC_MM_RET_NOT_SUPPORTED.

> +#define MM_RET_INVALID_PARAMS -2
> +#define MM_RET_DENIED         -3
> +#define MM_RET_NO_MEMORY      -4
> +
> +#endif /* _MM_COMMUNICATION_H_*/
> diff --git a/include/mm_variable.h b/include/mm_variable.h
> new file mode 100644
> index 000000000000..f56c52597629
> --- /dev/null
> +++ b/include/mm_variable.h
> @@ -0,0 +1,78 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + *  Headers for EFI variable service via StandAloneMM, EDK2 application running
> + *  in OP-TEE
> + *
> + *  Copyright (C) 2020 Linaro Ltd. <sughosh.ganu@linaro.org>
> + *  Copyright (C) 2020 Linaro Ltd. <ilias.apalodimas@linaro.org>

If you copied this from SmmVariableCommon.h shouldn't you mention:
Copyright (c) 2011 - 2019, Intel Corporation. All rights reserved.

> + */
> +
> +#if !defined _MM_VARIABLE_H_

#ifndef would be shorter.

> +#define _MM_VARIABLE_H_
> +
> +#include <part_efi.h>
> +
> +/* defined in EDK2 SmmVariableCommon.h */

Please, add a description of the structure, cf.
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation

> +struct mm_variable_communicate_header {
> +	efi_uintn_t  function;
> +	efi_status_t ret_status;
> +	u8           data[1];
> +};
> +
> +#define MM_VARIABLE_COMMUNICATE_SIZE \
> +	(offsetof(struct mm_variable_communicate_header, data))

Why not the original name SMM_VARIABLE_COMMUNICATE_HEADER_SIZE?

> +
> +#define MM_VARIABLE_FUNCTION_GET_VARIABLE			1

MdeModulePkg/Include/Guid/SmmVariableCommon.h uses
SMM_VARIABLE_FUNCTION_GET_VARIABLE. Why invent a new name?

> +
> +#define MM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME		2
> +
> +#define MM_VARIABLE_FUNCTION_SET_VARIABLE			3
> +
> +#define MM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO		4
> +
> +#define MM_VARIABLE_FUNCTION_READY_TO_BOOT			5
> +
> +#define MM_VARIABLE_FUNCTION_EXIT_BOOT_SERVICE			6
> +
> +#define MM_VARIABLE_FUNCTION_GET_STATISTICS			7
> +
> +#define MM_VARIABLE_FUNCTION_LOCK_VARIABLE			8
> +
> +#define MM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_SET	9
> +
> +#define MM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET	10
> +
> +#define MM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE			11

Values 12 - 14 are missing. I think we should provide all values.

> +

Missing structure description.

> +struct mm_variable_access {

smm_variable_access?

> +	efi_guid_t  guid;
> +	efi_uintn_t data_size;
> +	efi_uintn_t name_size;
> +	u32         attr;
> +	u16         name[1];

This name[1] was needed in old compilers. It is not needed anymore in
C11.  For a variable length structure component, please, use name[].

> +};
> +
> +#define MM_VARIABLE_ACCESS_HEADER_SIZE \
> +	(offsetof(struct mm_variable_access, name))

If you have name[] as component, you can use sizeof(struct
smm_variable_access) instead of this define.

> +

Structure description missing.

> +struct mm_variable_payload_size {
> +	efi_uintn_t size;
> +};

In EDK2 PayloadSize is simply UINTN.

Isn't this structure superfluous? Can't we directly pass a type UINTN
variable instead and get rid of a level of indirection?

> +

Structure description missing.

> +struct mm_variable_getnext {
> +	efi_guid_t  guid;
> +	efi_uintn_t name_size;
> +	u16         name[1];

name[]?

> +};
> +
> +#define MM_VARIABLE_GET_NEXT_HEADER_SIZE \
> +	(offsetof(struct mm_variable_getnext, name))
> +

Structure description missing.

You could think about merging the two include files into one.

Best regards

Heinrich

> +struct mm_variable_query_info {
> +	u64 max_variable_storage;
> +	u64 remaining_variable_storage;
> +	u64 max_variable_size;
> +	u32 attr;
> +};
> +
> +#endif /* _MM_VARIABLE_H_ */
>

[PATCH 3/6] efi_loader: Implement EFI variable handling via OP-TEE

[Heinrich Schuchardt]

On 5/6/20 9:12 PM, Ilias Apalodimas wrote:
> In OP-TEE we can run EDK2's StandAloneMM on a secure partition.
> StandAloneMM is responsible for the UEFI variable support. In
> combination with OP-TEE and it's U-Boot supplicant, variables are
> authenticated/validated in secure world and stored on an RPMB partition.
>
> So let's add a new config option in U-Boot implementing the necessary
> calls to OP-TEE for the variable management.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Signed-off-by: Pipat Methavanitpong <pipat1010@gmail.com>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  lib/efi_loader/Kconfig            |   9 +
>  lib/efi_loader/Makefile           |   4 +
>  lib/efi_loader/efi_variable_tee.c | 645 ++++++++++++++++++++++++++++++
>  3 files changed, 658 insertions(+)
>  create mode 100644 lib/efi_loader/efi_variable_tee.c
>
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 1cfa24ffcf72..e385cd0b9dae 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -164,4 +164,13 @@ config EFI_SECURE_BOOT
>  	  it is signed with a trusted key. To do that, you need to install,
>  	  at least, PK, KEK and db.
>
> +config EFI_MM_COMM_TEE
> +	bool "UEFI variables storage service via OP-TEE"
> +	depends on SUPPORT_EMMC_RPMB
> +	default n
> +	help
> +	  If OP-TEE is present and running StandAloneMM dispatch all UEFI variable
> +	  related operations to that. The application will verify, authenticate and
> +	  store the variables on an RPMB
> +
>  endif
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index eff3c25ec301..dba652121eab 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -34,7 +34,11 @@ obj-y += efi_root_node.o
>  obj-y += efi_runtime.o
>  obj-y += efi_setup.o
>  obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
> +ifeq ($(CONFIG_EFI_MM_COMM_TEE),y)
> +obj-y += efi_variable_tee.o
> +else
>  obj-y += efi_variable.o
> +endif
>  obj-y += efi_watchdog.o
>  obj-$(CONFIG_LCD) += efi_gop.o
>  obj-$(CONFIG_DM_VIDEO) += efi_gop.o
> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> new file mode 100644
> index 000000000000..d4e206e22073
> --- /dev/null
> +++ b/lib/efi_loader/efi_variable_tee.c
> @@ -0,0 +1,645 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  EFI variable service via OP-TEE
> + *
> + *  Copyright (C) 2019 Linaro Ltd. <sughosh.ganu@linaro.org>
> + *  Copyright (C) 2019 Linaro Ltd. <ilias.apalodimas@linaro.org>
> + */
> +
> +#include <common.h>
> +#include <efi.h>
> +#include <efi_api.h>
> +#include <efi_loader.h>
> +#include <tee.h>
> +#include <malloc.h>
> +#include <mm_communication.h>
> +#include <mm_variable.h>
> +
> +#define PTA_STMM_CMDID_COMMUNICATE 0

This seems to be a part of the OP-TEE communication interface to SMM and
should be in an include with a comment describing the meaning of the
constant.

> +#define PTA_STMM_UUID { 0xed32d533, 0x99e6, 0x4209, {\
> +			0x9c, 0xc0, 0x2d, 0x72, 0xcd, 0xd9, 0x98, 0xa7 } }
> +
> +#define EFI_MM_VARIABLE_GUID \
> +	EFI_GUID(0xed32d533, 0x99e6, 0x4209, \
> +		 0x9c, 0xc0, 0x2d, 0x72, 0xcd, 0xd9, 0x98, 0xa7)

These two GUIDs are just the same by value. Looking at EFI_GUID() and
tee_optee_ta_uuid_to_octets() it seems that TEE is using big endian
GUIDs while UEFI uses low endian ones. I think this deserves a comment
in your code.

I would prefer to see the GUIDs in the includes. Anyway you copied from
MdeModulePkg/Include/Protocol/SmmVariable.h

> +
> +static efi_uintn_t max_buffer_size;	/* comm + var + func + data */
> +static efi_uintn_t max_payload_size;	/* func + data */
> +
> +struct mm_connection {
> +	struct udevice *tee;
> +	u32 session;
> +};
> +
> +int get_connection(struct mm_connection *conn)

This function is only used locally. Please, mark it as static.

Please, comment your functions according to
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

> +{
> +	static const struct tee_optee_ta_uuid uuid = PTA_STMM_UUID;
> +	struct udevice *tee = NULL;
> +	struct tee_open_session_arg arg;
> +	int rc;
> +
> +	tee = tee_find_device(tee, NULL, NULL, NULL);
> +	if (!tee)
> +		return -ENODEV;
> +
> +	memset(&arg, 0, sizeof(arg));
> +	tee_optee_ta_uuid_to_octets(arg.uuid, &uuid);
> +	rc = tee_open_session(tee, &arg, 0, NULL);
> +	if (!rc) {
> +		conn->tee = tee;
> +		conn->session = arg.session;
> +	}
> +
> +	return rc;
> +}
> +
> +/**
> + * optee_mm_communicate() - Pass a buffer to StandaloneMM running in OP-TEE
> + *
> + * @comm_buf:		Locally allocted communcation buffer

%s/allocted/allocated/

> + * @dsize:		Buffer size
> + * Return:		status code

Please, be consistent in the capitalization of the argument descriptions.

> + */
> +static efi_status_t optee_mm_communicate(void *comm_buf, ulong dsize)
> +{
> +	ulong buf_size;
> +	efi_status_t ret;
> +	struct mm_communicate_header *mm_hdr;
> +	struct mm_connection conn = { NULL, 0 };
> +	struct tee_invoke_arg arg;
> +	struct tee_param param[2];
> +	struct tee_shm *shm = NULL;
> +	int rc;
> +
> +	if (!comm_buf)
> +		return EFI_INVALID_PARAMETER;
> +
> +	mm_hdr = (struct mm_communicate_header *)comm_buf;
> +	buf_size = mm_hdr->message_len + sizeof(efi_guid_t) + sizeof(size_t);
> +
> +	if (dsize != buf_size)
> +		return EFI_INVALID_PARAMETER;
> +
> +	rc = get_connection(&conn);
> +	if (rc) {
> +		log_err("Unable to open OP-TEE session (err=%d)\n", rc);
> +		return EFI_UNSUPPORTED;
> +	}
> +
> +	if (tee_shm_register(conn.tee, comm_buf, buf_size, 0, &shm)) {
> +		log_err("Unable to register shared memory\n");
> +		return EFI_UNSUPPORTED;
> +	}
> +
> +	memset(&arg, 0, sizeof(arg));
> +	arg.func = PTA_STMM_CMDID_COMMUNICATE;
> +	arg.session = conn.session;
> +
> +	memset(param, 0, sizeof(param));
> +	param[0].attr = TEE_PARAM_ATTR_TYPE_MEMREF_INOUT;
> +	param[0].u.memref.size = buf_size;
> +	param[0].u.memref.shm = shm;
> +	param[1].attr = TEE_PARAM_ATTR_TYPE_VALUE_OUTPUT;
> +
> +	rc = tee_invoke_func(conn.tee, &arg, 2, param);
> +	if (rc)
> +		return EFI_INVALID_PARAMETER;
> +	tee_shm_free(shm);
> +	tee_close_session(conn.tee, conn.session);
> +
> +	switch (param[1].u.value.a) {
> +	case MM_RET_SUCCESS:
> +		ret = EFI_SUCCESS;
> +		break;
> +
> +	case MM_RET_INVALID_PARAMS:
> +		ret = EFI_INVALID_PARAMETER;
> +		break;
> +
> +	case MM_RET_DENIED:
> +		ret = EFI_ACCESS_DENIED;
> +		break;
> +
> +	case MM_RET_NO_MEMORY:
> +		ret = EFI_OUT_OF_RESOURCES;
> +		break;
> +
> +	default:
> +		ret = EFI_ACCESS_DENIED;
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * mm_communicate() - Adjust the cmonnucation buffer to StandAlonneMM and send

%s/cmonnucation/communication/

> + * it to OP-TEE
> + *
> + * @comm_buf:		Locally allocted communcation buffer

%s/allocted/allocated/

You can use spell-checking in vim with 'set spell'.

> + * @dsize:		Buffer size
> + * Return:		status code
> + */
> +static efi_status_t mm_communicate(u8 *comm_buf, efi_uintn_t dsize)
> +{
> +	efi_status_t ret;
> +	struct mm_communicate_header *mm_hdr;
> +	struct mm_variable_communicate_header *var_hdr;
> +
> +	dsize += MM_COMMUNICATE_HEADER_SIZE + MM_VARIABLE_COMMUNICATE_SIZE;
> +	mm_hdr = (struct mm_communicate_header *)comm_buf;
> +	var_hdr = (struct mm_variable_communicate_header *)mm_hdr->data;
> +
> +	ret = optee_mm_communicate(comm_buf, dsize);
> +	if (ret != EFI_SUCCESS) {
> +		log_err("%s failed!\n", __func__);
> +		return ret;
> +	}
> +
> +	return var_hdr->ret_status;
> +}
> +
> +/**
> + * setup_mm_hdr() -	Allocate a buffer for StandAloneMM and initialize the
> + *			header data.
> + *
> + * @dptr:		Pointer address of the corresponding StandAloneMM
> + *			function
> + * @payload_size:	Buffer size
> + * @func:		StandAloneMM function number
> + * @ret:		EFI return code
> + * Return:		Buffer or NULL
> + */
> +static u8 *setup_mm_hdr(void **dptr, efi_uintn_t payload_size,
> +			efi_uintn_t func, efi_status_t *ret)
> +{
> +	const efi_guid_t mm_var_guid = EFI_MM_VARIABLE_GUID;
> +	struct mm_communicate_header *mm_hdr;
> +	struct mm_variable_communicate_header *var_hdr;
> +	u8 *comm_buf;
> +
> +	/* On the init function we initialize max_buffer_size with

%s/On the init/In the init/

> +	 * get_max_payload(). So skip the test if max_buffer_size is initialized
> +	 * StandAloneMM will perform similar checks and drop the buffer if it's
> +	 * too long

%s/too long/too long./

> +	 */
> +	if (max_buffer_size && max_buffer_size <
> +			(MM_COMMUNICATE_HEADER_SIZE +
> +			 MM_VARIABLE_COMMUNICATE_SIZE +
> +			 payload_size)) {
> +		*ret = EFI_INVALID_PARAMETER;
> +		return NULL;
> +	}
> +
> +	comm_buf = calloc(1, MM_COMMUNICATE_HEADER_SIZE +
> +			  MM_VARIABLE_COMMUNICATE_SIZE +
> +			  payload_size);
> +	if (!comm_buf) {
> +		*ret = EFI_OUT_OF_RESOURCES;
> +		return NULL;
> +	}
> +
> +	mm_hdr = (struct mm_communicate_header *)comm_buf;
> +	guidcpy(&mm_hdr->header_guid, &mm_var_guid);
> +	mm_hdr->message_len = MM_VARIABLE_COMMUNICATE_SIZE + payload_size;
> +
> +	var_hdr = (struct mm_variable_communicate_header *)mm_hdr->data;
> +	var_hdr->function = func;
> +	if (dptr)
> +		*dptr = var_hdr->data;
> +	*ret = EFI_SUCCESS;
> +
> +	return comm_buf;
> +}
> +
> +/**
> + * Get variable payload size from StandAloneMM

get_max_payload() - get variable ....

> + *
> + * @size:    Size of the variable in storage
> + * Return:   status code
> + */
> +efi_status_t EFIAPI get_max_payload(efi_uintn_t *size)
> +{
> +	struct mm_variable_payload_size *var_payload = NULL;
> +	efi_uintn_t payload_size;
> +	efi_status_t ret;
> +	u8 *comm_buf;
> +
> +	if (!size)
> +		return EFI_INVALID_PARAMETER;
> +
> +	payload_size = sizeof(*var_payload);
> +	comm_buf = setup_mm_hdr((void **)&var_payload, payload_size,
> +				MM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE, &ret);
> +	if (!comm_buf)
> +		return EFI_EXIT(ret);
> +
> +	ret = mm_communicate(comm_buf, payload_size);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	*size = var_payload->size;
> +
> +out:
> +	free(comm_buf);
> +	return ret;
> +}
> +
> +/**
> + * efi_get_variable() - retrieve value of a UEFI variable
> + *
> + * This function implements the GetVariable runtime service.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * @name:		name of the variable
> + * @guid:		vendor GUID
> + * @attr:		attributes of the variable
> + * @data_size:		size of the buffer to which the variable value is copied
> + * @data:		buffer to which the variable value is copied
> + * Return:		status code
> + */
> +efi_status_t EFIAPI efi_get_variable(u16 *name, const efi_guid_t *guid,
> +				     u32 *attr, efi_uintn_t *data_size,
> +				     void *data)
> +{
> +	struct mm_variable_access *var_acc;
> +	efi_uintn_t payload_size;
> +	efi_uintn_t name_size;
> +	efi_uintn_t tmp_dsize;
> +	efi_status_t ret;
> +	u8 *comm_buf;
> +
> +	EFI_ENTRY("\"%ls\" %pUl %p %p %p", name, guid, attr, data_size, data);
> +
> +	if (!name || !guid || !data_size)
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +	/* Check payload size */
> +	name_size = u16_strsize(name);
> +	if (name_size > max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE)
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +	/* Trim output buffer size */
> +	tmp_dsize = *data_size;
> +	if (name_size + tmp_dsize >
> +			max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE) {
> +		tmp_dsize = max_payload_size -
> +				MM_VARIABLE_ACCESS_HEADER_SIZE -
> +				name_size;
> +	}
> +
> +	/* Get comm buffer and initialize header */

%s/comm/communication/

> +	payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + tmp_dsize;
> +	comm_buf = setup_mm_hdr((void **)&var_acc, payload_size,
> +				MM_VARIABLE_FUNCTION_GET_VARIABLE, &ret);
> +	if (!comm_buf)
> +		return EFI_EXIT(ret);
> +
> +	/* Fill in contents */
> +	guidcpy(&var_acc->guid, guid);
> +	var_acc->data_size = tmp_dsize;
> +	var_acc->name_size = name_size;
> +	var_acc->attr = attr ? *attr : 0;
> +	memcpy(var_acc->name, name, name_size);
> +
> +	/* Communicate */
> +	ret = mm_communicate(comm_buf, payload_size);
> +	if (ret == EFI_SUCCESS || ret == EFI_BUFFER_TOO_SMALL) {
> +		/* Update with reported data size for trimmed case */
> +		*data_size = var_acc->data_size;
> +	}
> +	if (ret != EFI_SUCCESS)
> +		goto done;
> +
> +	if (attr)
> +		*attr = var_acc->attr;
> +	if (data)
> +		memcpy(data, (u8 *)var_acc->name + var_acc->name_size,
> +		       var_acc->data_size);
> +	else
> +		ret = EFI_INVALID_PARAMETER;
> +
> +done:
> +	free(comm_buf);
> +	return EFI_EXIT(ret);
> +}
> +
> +/**
> + * efi_get_next_variable_name() - enumerate the current variable names
> + *
> + * @variable_name_size:	size of variable_name buffer in byte
> + * @variable_name:	name of uefi variable's name in u16
> + * @guid:		vendor's guid
> + *
> + * This function implements the GetNextVariableName service.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * Return: status code
> + */
> +efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
> +					       u16 *variable_name,
> +					       efi_guid_t *guid)
> +{
> +	struct mm_variable_getnext *var_getnext;
> +	efi_uintn_t payload_size;
> +	efi_uintn_t tmp_dsize;
> +	efi_uintn_t name_size;
> +	efi_status_t ret;
> +	efi_uintn_t out_name_size;
> +	efi_uintn_t in_name_size;
> +	u8 *comm_buf;
> +
> +	EFI_ENTRY("%p \"%ls\" %pUl", variable_name_size, variable_name, guid);
> +
> +	if (!variable_name_size || !variable_name || !guid)
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +	out_name_size = *variable_name_size;
> +	in_name_size = u16_strsize(variable_name);

The UEFI spec requires: "The size must be large enough to fit input
string supplied in VariableName buffer."

Further it is required to return EFI_INVALID_PARAMETER if the
"Null-terminator is not found in the first VariableNameSize bytes of the
input VariableName buffer."

Please, investigate if SMM takes care of the check or we should do it.

> +
> +	name_size = u16_strsize(variable_name);
> +	if (name_size > max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE)
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +	/* Trim output buffer size */
> +	tmp_dsize = *variable_name_size;
> +	if (name_size + tmp_dsize >
> +			max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE) {
> +		tmp_dsize = max_payload_size -
> +				MM_VARIABLE_GET_NEXT_HEADER_SIZE -
> +				name_size;
> +	}
> +
> +	payload_size = MM_VARIABLE_GET_NEXT_HEADER_SIZE +
> +			max(out_name_size, in_name_size);

Only out_name_size is relevant here.

> +	comm_buf = setup_mm_hdr((void **)&var_getnext, payload_size,
> +				MM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME,
> +				&ret);
> +	if (!comm_buf)
> +		return EFI_EXIT(ret);
> +
> +	/* Fill in contents */
> +	guidcpy(&var_getnext->guid, guid);
> +	var_getnext->name_size = out_name_size;
> +	memcpy(var_getnext->name, variable_name, in_name_size);
> +	if (out_name_size > in_name_size) {
> +		memset((u8 *)var_getnext->name + in_name_size, 0x0,
> +		       out_name_size - in_name_size);
> +	}
> +
> +	/* Communicate */
> +	ret = mm_communicate(comm_buf, payload_size);
> +	if (ret == EFI_SUCCESS || ret == EFI_BUFFER_TOO_SMALL) {
> +		/* Update with reported data size for trimmed case */
> +		*variable_name_size = var_getnext->name_size;
> +	}
> +	if (ret != EFI_SUCCESS)
> +		goto done;
> +
> +	guidcpy(guid, &var_getnext->guid);
> +	memcpy(variable_name, (u8 *)var_getnext->name,
> +	       var_getnext->name_size);
> +
> +done:
> +	free(comm_buf);
> +	return EFI_EXIT(ret);
> +}
> +
> +/**
> + * efi_set_variable() - set value of a UEFI variable
> + *
> + * This function implements the SetVariable runtime service.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * @name:		name of the variable
> + * @guid:		vendor GUID
> + * @attr:		attributes of the variable
> + * @data_size:		size of the buffer with the variable value
> + * @data:		buffer with the variable value
> + * Return:		status code
> + */
> +efi_status_t EFIAPI efi_set_variable(u16 *name, const efi_guid_t *guid,
> +				     u32 attr, efi_uintn_t data_size,
> +				     const void *data)
> +{
> +	struct mm_variable_access *var_acc;
> +	efi_uintn_t payload_size;
> +	efi_uintn_t name_size;
> +	efi_status_t ret;
> +	u8 *comm_buf;
> +
> +	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", name, guid, attr, data_size, data);
> +
> +	if (!name || name[0] == 0 || !guid)
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +	if (data_size > 0 && !data)
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +	/* Check payload size */
> +	name_size = u16_strsize(name);
> +	payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + data_size;
> +	if (payload_size > max_payload_size)
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +	/* Get comm buffer and initialize header */
> +	comm_buf = setup_mm_hdr((void **)&var_acc, payload_size,
> +				MM_VARIABLE_FUNCTION_SET_VARIABLE, &ret);
> +	if (!comm_buf)
> +		return EFI_EXIT(ret);
> +
> +	/* Fill in contents */
> +	guidcpy(&var_acc->guid, guid);
> +	var_acc->data_size = data_size;
> +	var_acc->name_size = name_size;
> +	var_acc->attr = attr;
> +	memcpy(var_acc->name, name, name_size);
> +	memcpy((u8 *)var_acc->name + name_size, data, data_size);
> +
> +	/* Communicate */
> +	ret = mm_communicate(comm_buf, payload_size);
> +	free(comm_buf);
> +
> +	return EFI_EXIT(ret);
> +}
> +
> +/**
> + * efi_query_variable_info() - get information about EFI variables
> + *
> + * This function implements the QueryVariableInfo() runtime service.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * @attributes:				bitmask to select variables to be
> + *					queried
> + * @maximum_variable_storage_size:	maximum size of storage area for the
> + *					selected variable types
> + * @remaining_variable_storage_size:	remaining size of storage are for the
> + *					selected variable types
> + * @maximum_variable_size:		maximum size of a variable of the
> + *					selected type
> + * Returns:				status code
> + */
> +efi_status_t EFIAPI __efi_runtime
> +efi_query_variable_info(u32 attributes, u64 *max_variable_storage_size,
> +			u64 *remain_variable_storage_size,
> +			u64 *max_variable_size)
> +{
> +	struct mm_variable_query_info *mm_query_info;
> +	efi_uintn_t payload_size;
> +	efi_status_t ret;
> +	u8 *comm_buf;
> +
> +	EFI_ENTRY("%x %p %p %p", attributes, max_variable_storage_size,
> +		  remain_variable_storage_size, max_variable_size);
> +
> +	payload_size = sizeof(*mm_query_info);
> +	comm_buf = setup_mm_hdr((void **)&mm_query_info, payload_size,
> +				MM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO, &ret);
> +	if (!comm_buf)
> +		return EFI_EXIT(ret);
> +
> +	mm_query_info->attr = attributes;
> +	ret = mm_communicate(comm_buf, payload_size);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +	*max_variable_storage_size = mm_query_info->max_variable_storage;
> +	*remain_variable_storage_size =
> +			mm_query_info->remaining_variable_storage;
> +	*max_variable_size = mm_query_info->max_variable_size;
> +
> +out:
> +	free(comm_buf);
> +	return EFI_EXIT(ret);
> +}

The code below for runtime seems to be duplicated from efi_variable.c.
Let's keep it like this for now. We can look for a better solution once
we investigate a runtime solution.

Overall the code looks good and clean (except for my nitpicky comments).

Thanks a lot for all the effort you invested here.

Best regards

Heinrich

> +
> +/**
> + * efi_get_variable_runtime() - runtime implementation of GetVariable()
> + *
> + * @variable_name:	name of the variable
> + * @guid:		vendor GUID
> + * @attributes:		attributes of the variable
> + * @data_size:		size of the buffer to which the variable value is copied
> + * @data:		buffer to which the variable value is copied
> + * Return:		status code
> + */
> +static efi_status_t __efi_runtime EFIAPI
> +efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
> +			 u32 *attributes, efi_uintn_t *data_size, void *data)
> +{
> +	return EFI_UNSUPPORTED;
> +}
> +
> +/**
> + * efi_get_next_variable_name_runtime() - runtime implementation of
> + *					  GetNextVariable()
> + *
> + * @variable_name_size:	size of variable_name buffer in byte
> + * @variable_name:	name of uefi variable's name in u16
> + * @guid:		vendor's guid
> + * Return: status code
> + */
> +static efi_status_t __efi_runtime EFIAPI
> +efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
> +				   u16 *variable_name, efi_guid_t *guid)
> +{
> +	return EFI_UNSUPPORTED;
> +}
> +
> +/**
> + * efi_query_variable_info() - get information about EFI variables
> + *
> + * This function implements the QueryVariableInfo() runtime service.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * @attributes:				bitmask to select variables to be
> + *					queried
> + * @maximum_variable_storage_size:	maximum size of storage area for the
> + *					selected variable types
> + * @remaining_variable_storage_size:	remaining size of storage are for the
> + *					selected variable types
> + * @maximum_variable_size:		maximum size of a variable of the
> + *					selected type
> + * Returns:				status code
> + */
> +efi_status_t EFIAPI __efi_runtime
> +efi_query_variable_info_runtime(u32 attributes, u64 *max_variable_storage_size,
> +				u64 *remain_variable_storage_size,
> +				u64 *max_variable_size)
> +{
> +	return EFI_UNSUPPORTED;
> +}
> +
> +/**
> + * efi_set_variable_runtime() - runtime implementation of SetVariable()
> + *
> + * @variable_name:	name of the variable
> + * @guid:		vendor GUID
> + * @attributes:		attributes of the variable
> + * @data_size:		size of the buffer with the variable value
> + * @data:		buffer with the variable value
> + * Return:		status code
> + */
> +static efi_status_t __efi_runtime EFIAPI
> +efi_set_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
> +			 u32 attributes, efi_uintn_t data_size,
> +			 const void *data)
> +{
> +	return EFI_UNSUPPORTED;
> +}
> +
> +/**
> + * efi_variables_boot_exit_notify() - notify ExitBootServices() is called
> + */
> +void efi_variables_boot_exit_notify(void)
> +{
> +	u8 *comm_buf;
> +	efi_status_t ret;
> +
> +	comm_buf = setup_mm_hdr(NULL, 0, MM_VARIABLE_FUNCTION_EXIT_BOOT_SERVICE,
> +				&ret);
> +	if (comm_buf)
> +		ret = mm_communicate(comm_buf, 0);
> +	else
> +		ret = EFI_NOT_FOUND;
> +
> +	if (ret != EFI_SUCCESS)
> +		log_err("Unable to notify StMM for ExitBootServices\n");
> +	free(comm_buf);
> +
> +	/* Update runtime service table */
> +	efi_runtime_services.query_variable_info =
> +			efi_query_variable_info_runtime;
> +	efi_runtime_services.get_variable = efi_get_variable_runtime;
> +	efi_runtime_services.get_next_variable_name =
> +			efi_get_next_variable_name_runtime;
> +	efi_runtime_services.set_variable = efi_set_variable_runtime;
> +	efi_update_table_header_crc32(&efi_runtime_services.hdr);
> +}
> +
> +/**
> + * efi_init_variables() - initialize variable services
> + *
> + * Return:	status code
> + */
> +efi_status_t efi_init_variables(void)
> +{
> +	efi_status_t ret;
> +
> +	ret = get_max_payload(&max_payload_size);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	max_buffer_size = MM_COMMUNICATE_HEADER_SIZE +
> +			  MM_VARIABLE_COMMUNICATE_SIZE +
> +			  max_payload_size;
> +
> +	return EFI_SUCCESS;
> +}
>

[PATCH 5/6] MAINTAINERS: Add maintainer for EFI variables via OP-TEE

[Heinrich Schuchardt]

On 5/6/20 9:12 PM, Ilias Apalodimas wrote:
> Add myself as maintainer for the OP-TEE related UEFI variable storage
> and add the headers files on the existing EFI list
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  MAINTAINERS | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ec59ce8b8802..f33fd74b330b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -625,6 +625,8 @@ F:	include/cp437.h
>  F:	include/efi*
>  F:	include/pe.h
>  F:	include/asm-generic/pe.h
> +F:	include/mm_communication.h
> +F:	include/mm_variable.h
>  F:	lib/charset.c
>  F:	lib/efi*/
>  F:	test/py/tests/test_efi*
> @@ -635,6 +637,11 @@ F:	cmd/efidebug.c
>  F:	cmd/nvedit_efi.c
>  F:	tools/file2include.c
>
> +EFI VARIABLES VIA OP-TEE
> +M:	Ilias Apalodimas <ilias.apalodimas@linaro.org>
> +S:	Maintained
> +F:	lib/efi_loader/efi_variable_tee.c

I think the mm* includes and efi_variable_tee.c should rest with the
same maintainer.

Best regards

Heinrich

> +
>  ENVIRONMENT
>  M:	Joe Hershberger <joe.hershberger@ni.com>
>  R:	Wolfgang Denk <wd@denx.de>
>

[PATCH 6/6] doc: uefi.rst: Add OP-TEE variable storage config options

[Heinrich Schuchardt]

On 5/6/20 9:12 PM, Ilias Apalodimas wrote:
> If OP-TEE is compiled with an EDK2 application running in secure world
> it can process and store UEFI variables in an RPMB.
> Add documentation for the config options enabling this
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  doc/uefi/uefi.rst | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/doc/uefi/uefi.rst b/doc/uefi/uefi.rst
> index 4fda00d68721..93b0faadd26e 100644
> --- a/doc/uefi/uefi.rst
> +++ b/doc/uefi/uefi.rst
> @@ -188,6 +188,16 @@ on the sandbox
>      cd <U-Boot source directory>
>      pytest.py test/py/tests/test_efi_secboot/test_signed.py --bd sandbox
>
> +Using OP-TEE for EFI variables
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +If an RPMB and it's drivers is available in U-Boot, OP-TEE can be used for

%s/is available/are available/

..., OP-TEE in conjunction with EDK2's secure management module (SMM)
can be used to provide variable services.

> +variable services.
> +Enabling CONFIG_EFI_MM_COMM_TEE=y will dispatch the variables services to

%s/dispatch/delegate/

> +OP-TEE. OP-TEE needs to be compiled with a secure application (coming from EDK2)

Is it really compiling? I thought it was only linking.

... needs to be linked with EDK2's secure management module (SMM) which
will process the variables ...

> +which will process variables in the Secure World and store them in the RPMB
> +using the OP-TEE supplicant.
> +
>  Executing the boot manager
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>

We should separate in the description between OP-TEE being used to
provide variable services and the specific embodiment using SMM, e.g.

How about:


Using OP-TEE for EFI variables
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Instead of implementing UEFI variable services inside U-Boot they can
also be provided in the secure world by a module for OP-TEE[1]. The
interface between U-Boot and OP-TEE for variable services is enabled by
CONFIG_EFI_MM_COMM_TEE=y.

Tianocore EDK II's standalone management mode driver for variables can
be linked to OP-TEE for this purpose. This module uses the Replay
Protected Memory Block (RPMB) of an eMMC device for persisting
non-volatile variables. When calling the variable services via the
OP-TEE API U-Boot's OP-TEE supplicant relays calls to the RPMB driver
which has to be enabled via CONFIG_SUPPORT_EMMC_RPMB=y.

[1] https://optee.readthedocs.io/ - OP-TEE documentation

Best regards

Heinrich

[PATCH 4/6] cmd: efidebug: Add support for querying UEFI variable storage

[Heinrich Schuchardt]

On 5/6/20 9:12 PM, Ilias Apalodimas wrote:
> With the previous patches that use OP-TEE and StandAloneMM for UEFI
> variable storage we've added functionality for efi_query_variable_info.
> So let's add the relevant command to efidebug and retrieve information
> about the container used to store UEFI variables
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  cmd/efidebug.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index d8a76d78a388..17e36ef76d69 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -1160,6 +1160,45 @@ static int do_efi_test(cmd_tbl_t *cmdtp, int flag,
>  	return cp->cmd(cmdtp, flag, argc, argv);
>  }
>
> +/**
> + * do_efi_query_info() - QueryVariableInfo EFI service
> + *
> + * @cmdtp:	Command table
> + * @flag:	Command flag
> + * @argc:	Number of arguments
> + * @argv:	Argument array
> + * Return:	CMD_RET_SUCCESS on success,
> + *		CMD_RET_USAGE or CMD_RET_FAILURE on failure
> + *
> + * Implement efidebug "test" sub-command.
> + */
> +
> +static int do_efi_query_info(cmd_tbl_t *cmdtp, int flag,
> +			     int argc, char * const argv[])
> +{
> +	efi_status_t ret;
> +	u32 attr = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +			EFI_VARIABLE_RUNTIME_ACCESS |
> +			EFI_VARIABLE_NON_VOLATILE;
> +	u64 max_variable_storage_size;
> +	u64 remain_variable_storage_size;
> +	u64 max_variable_size;
> +
> +	ret = EFI_CALL(efi_query_variable_info(attr,
> +					       &max_variable_storage_size,
> +					       &remain_variable_storage_size,
> +					       &max_variable_size));
> +	if (ret != EFI_SUCCESS)
> +		return CMD_RET_FAILURE;
> +
> +	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);

We are not printing handles. Please remove the line.

> +	printf("Max storage size %llu\n", max_variable_storage_size);
> +	printf("Remaining storage size %llu\n", remain_variable_storage_size);
> +	printf("Max variable size %llu\n", max_variable_size);
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
>  static cmd_tbl_t cmd_efidebug_sub[] = {
>  	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
>  	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
> @@ -1176,6 +1215,8 @@ static cmd_tbl_t cmd_efidebug_sub[] = {
>  			 "", ""),
>  	U_BOOT_CMD_MKENT(test, CONFIG_SYS_MAXARGS, 1, do_efi_test,
>  			 "", ""),
> +	U_BOOT_CMD_MKENT(query, CONFIG_SYS_MAXARGS, 1, do_efi_query_info,
> +			 "", ""),
>  };
>
>  /**
> @@ -1247,7 +1288,9 @@ static char efidebug_help_text[] =
>  	"efidebug tables\n"
>  	"  - show UEFI configuration tables\n"
>  	"efidebug test bootmgr\n"
> -	"  - run simple bootmgr for test\n";
> +	"  - run simple bootmgr for test\n"
> +	"efidebug query\n"
> +	"  - show information of the container used to store UEFI variables\n";

This text does not make it clear that we will get size information. How
about:

"show size of UEFI variables store\n"

Best regards

Heinrich

>  #endif
>
>  U_BOOT_CMD(
>

[PATCH 3/6] efi_loader: Implement EFI variable handling via OP-TEE

[Ilias Apalodimas]

Hi Heinrich,

On Sat, May 09, 2020 at 11:14:51AM +0200, Heinrich Schuchardt wrote:
> On 5/6/20 9:12 PM, Ilias Apalodimas wrote:
> > In OP-TEE we can run EDK2's StandAloneMM on a secure partition.
> > StandAloneMM is responsible for the UEFI variable support. In
> > combination with OP-TEE and it's U-Boot supplicant, variables are
> > authenticated/validated in secure world and stored on an RPMB partition.
> >
> > So let's add a new config option in U-Boot implementing the necessary
> > calls to OP-TEE for the variable management.
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Signed-off-by: Pipat Methavanitpong <pipat1010@gmail.com>
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >  lib/efi_loader/Kconfig            |   9 +
> >  lib/efi_loader/Makefile           |   4 +
> >  lib/efi_loader/efi_variable_tee.c | 645 ++++++++++++++++++++++++++++++
> >  3 files changed, 658 insertions(+)
> >  create mode 100644 lib/efi_loader/efi_variable_tee.c
> >
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 1cfa24ffcf72..e385cd0b9dae 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -164,4 +164,13 @@ config EFI_SECURE_BOOT
> >  	  it is signed with a trusted key. To do that, you need to install,
> >  	  at least, PK, KEK and db.
> >
> > +config EFI_MM_COMM_TEE
> > +	bool "UEFI variables storage service via OP-TEE"
> > +	depends on SUPPORT_EMMC_RPMB
> > +	default n
> > +	help
> > +	  If OP-TEE is present and running StandAloneMM dispatch all UEFI variable
> > +	  related operations to that. The application will verify, authenticate and
> > +	  store the variables on an RPMB
> > +
> >  endif
> > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > index eff3c25ec301..dba652121eab 100644
> > --- a/lib/efi_loader/Makefile
> > +++ b/lib/efi_loader/Makefile
> > @@ -34,7 +34,11 @@ obj-y += efi_root_node.o
> >  obj-y += efi_runtime.o
> >  obj-y += efi_setup.o
> >  obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
> > +ifeq ($(CONFIG_EFI_MM_COMM_TEE),y)
> > +obj-y += efi_variable_tee.o
> > +else
> >  obj-y += efi_variable.o
> > +endif
> >  obj-y += efi_watchdog.o
> >  obj-$(CONFIG_LCD) += efi_gop.o
> >  obj-$(CONFIG_DM_VIDEO) += efi_gop.o
> > diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> > new file mode 100644
> > index 000000000000..d4e206e22073
> > --- /dev/null
> > +++ b/lib/efi_loader/efi_variable_tee.c
> > @@ -0,0 +1,645 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + *  EFI variable service via OP-TEE
> > + *
> > + *  Copyright (C) 2019 Linaro Ltd. <sughosh.ganu@linaro.org>
> > + *  Copyright (C) 2019 Linaro Ltd. <ilias.apalodimas@linaro.org>
> > + */
> > +
> > +#include <common.h>
> > +#include <efi.h>
> > +#include <efi_api.h>
> > +#include <efi_loader.h>
> > +#include <tee.h>
> > +#include <malloc.h>
> > +#include <mm_communication.h>
> > +#include <mm_variable.h>
> > +
> > +#define PTA_STMM_CMDID_COMMUNICATE 0
> 
> This seems to be a part of the OP-TEE communication interface to SMM and
> should be in an include with a comment describing the meaning of the
> constant.

Ok

> 
> > +#define PTA_STMM_UUID { 0xed32d533, 0x99e6, 0x4209, {\
> > +			0x9c, 0xc0, 0x2d, 0x72, 0xcd, 0xd9, 0x98, 0xa7 } }
> > +
> > +#define EFI_MM_VARIABLE_GUID \
> > +	EFI_GUID(0xed32d533, 0x99e6, 0x4209, \
> > +		 0x9c, 0xc0, 0x2d, 0x72, 0xcd, 0xd9, 0x98, 0xa7)
> 
> These two GUIDs are just the same by value. Looking at EFI_GUID() and
> tee_optee_ta_uuid_to_octets() it seems that TEE is using big endian
> GUIDs while UEFI uses low endian ones. I think this deserves a comment
> in your code.
> 
> I would prefer to see the GUIDs in the includes. Anyway you copied from
> MdeModulePkg/Include/Protocol/SmmVariable.h

Fair enough. The reasopn we had two discrete header files was because we
followed the EDK2 example. Your recoomendation makes sense though, we'll move
everything to a single header file with sufficient commenting and add these
values as well.

> 
> > +
> > +static efi_uintn_t max_buffer_size;	/* comm + var + func + data */
> > +static efi_uintn_t max_payload_size;	/* func + data */
> > +
> > +struct mm_connection {
> > +	struct udevice *tee;
> > +	u32 session;
> > +};
> > +
> > +int get_connection(struct mm_connection *conn)
> 
> This function is only used locally. Please, mark it as static.
> 
> Please, comment your functions according to
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation
> 

Will do 

> > +{
> > + * optee_mm_communicate() - Pass a buffer to StandaloneMM running in OP-TEE

[...]

> > + *
> > + * @comm_buf:		Locally allocted communcation buffer
> 
> %s/allocted/allocated/
> 
> > + * @dsize:		Buffer size
> > + * Return:		status code
> 
> Please, be consistent in the capitalization of the argument descriptions.
> 

Ok 

> > + */
> 
> %s/cmonnucation/communication/
> 
> > + * it to OP-TEE
> > + *
> > + * @comm_buf:		Locally allocted communcation buffer
> 
> %s/allocted/allocated/
> 
> You can use spell-checking in vim with 'set spell'.
> 

Ok 

> > +	/* On the init function we initialize max_buffer_size with

[...]

> 
> %s/On the init/In the init/
> 
> > +	 * get_max_payload(). So skip the test if max_buffer_size is initialized
> > +	 * StandAloneMM will perform similar checks and drop the buffer if it's
> > +	 * too long
> 
> %s/too long/too long./
> 

Ok

> > +	 */
> > +	if (max_buffer_size && max_buffer_size <
> > +			(MM_COMMUNICATE_HEADER_SIZE +
> > +			 MM_VARIABLE_COMMUNICATE_SIZE +
> > +			 payload_size)) {
> > +		*ret = EFI_INVALID_PARAMETER;
> > +		return NULL;
> > +	}
> > +
> > +	comm_buf = calloc(1, MM_COMMUNICATE_HEADER_SIZE +
> > +			  MM_VARIABLE_COMMUNICATE_SIZE +
> > +			  payload_size);
> > +	if (!comm_buf) {
> > +		*ret = EFI_OUT_OF_RESOURCES;
> > +		return NULL;
> > +	}
> > +
> > +	mm_hdr = (struct mm_communicate_header *)comm_buf;
> > +	guidcpy(&mm_hdr->header_guid, &mm_var_guid);
> > +	mm_hdr->message_len = MM_VARIABLE_COMMUNICATE_SIZE + payload_size;
> > +
> > +	var_hdr = (struct mm_variable_communicate_header *)mm_hdr->data;
> > +	var_hdr->function = func;
> > +	if (dptr)
> > +		*dptr = var_hdr->data;
> > +	*ret = EFI_SUCCESS;
> > +
> > +	return comm_buf;
> > +}
> > +
> > +/**
> > + * Get variable payload size from StandAloneMM
> 
> get_max_payload() - get variable ....
> 
> > + *
> > + * @size:    Size of the variable in storage
> 

[...]

> %s/comm/communication/
> 

Ok

> > +	payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + tmp_dsize;
> > +	comm_buf = setup_mm_hdr((void **)&var_acc, payload_size,
> > +				MM_VARIABLE_FUNCTION_GET_VARIABLE, &ret);
> > +	if (!comm_buf)
> > +		return EFI_EXIT(ret);
> > +
> > +	/* Fill in contents */
> > +	guidcpy(&var_acc->guid, guid);
> > +	var_acc->data_size = tmp_dsize;
> > +	var_acc->name_size = name_size;
> > +	var_acc->attr = attr ? *attr : 0;
> > +	memcpy(var_acc->name, name, name_size);
> > +
> > +	/* Communicate */
> > +	ret = mm_communicate(comm_buf, payload_size);
> > +	if (ret == EFI_SUCCESS || ret == EFI_BUFFER_TOO_SMALL) {
> > +		/* Update with reported data size for trimmed case */
> > +		*data_size = var_acc->data_size;
> > +	}
> > +	if (ret != EFI_SUCCESS)
> > +		goto done;
> > +
> > +	if (attr)
> > +		*attr = var_acc->attr;
> > +	if (data)
> > +		memcpy(data, (u8 *)var_acc->name + var_acc->name_size,
> > +		       var_acc->data_size);
> > +	else
> > +		ret = EFI_INVALID_PARAMETER;
> > +
> > +done:
> > +	free(comm_buf);
> > +	return EFI_EXIT(ret);
> > +}
> > +
> > +/**
> > + * efi_get_next_variable_name() - enumerate the current variable names
> > + *
> > + * @variable_name_size:	size of variable_name buffer in byte
> > + * @variable_name:	name of uefi variable's name in u16
> > + * @guid:		vendor's guid
> > + *
> > + * This function implements the GetNextVariableName service.
> > + *
> > + * See the Unified Extensible Firmware Interface (UEFI) specification for
> > + * details.
> > + *
> > + * Return: status code
> > + */
> > +efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
> > +					       u16 *variable_name,
> > +					       efi_guid_t *guid)
> > +{
> > +	struct mm_variable_getnext *var_getnext;
> > +	efi_uintn_t payload_size;
> > +	efi_uintn_t tmp_dsize;
> > +	efi_uintn_t name_size;
> > +	efi_status_t ret;
> > +	efi_uintn_t out_name_size;
> > +	efi_uintn_t in_name_size;
> > +	u8 *comm_buf;
> > +
> > +	EFI_ENTRY("%p \"%ls\" %pUl", variable_name_size, variable_name, guid);
> > +
> > +	if (!variable_name_size || !variable_name || !guid)
> > +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> > +
> > +	out_name_size = *variable_name_size;
> > +	in_name_size = u16_strsize(variable_name);
> 
> The UEFI spec requires: "The size must be large enough to fit input
> string supplied in VariableName buffer."
> 
> Further it is required to return EFI_INVALID_PARAMETER if the
> "Null-terminator is not found in the first VariableNameSize bytes of the
> input VariableName buffer."
> 
> Please, investigate if SMM takes care of the check or we should do it.
> 

I think it already does, but I'll double check and make sure

> > +
> > +	name_size = u16_strsize(variable_name);
> > +	if (name_size > max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE)
> > +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> > +
> > +	/* Trim output buffer size */
> > +	tmp_dsize = *variable_name_size;
> > +	if (name_size + tmp_dsize >
> > +			max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE) {
> > +		tmp_dsize = max_payload_size -
> > +				MM_VARIABLE_GET_NEXT_HEADER_SIZE -
> > +				name_size;
> > +	}
> > +
> > +	payload_size = MM_VARIABLE_GET_NEXT_HEADER_SIZE +
> > +			max(out_name_size, in_name_size);
> 
> Only out_name_size is relevant here.
> 

Ths was copied from edk2, I haven't really looked into further details on why
they had that check. I'll have a look 

> > +	comm_buf = setup_mm_hdr((void **)&var_getnext, payload_size,
> > +				MM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME,
> > +				&ret);
> > +	if (!comm_buf)
> > +		return EFI_EXIT(ret);
> > +
> > +	*max_variable_size = mm_query_info->max_variable_size;

[...]

> > +
> > +out:
> > +	free(comm_buf);
> > +	return EFI_EXIT(ret);
> > +}
> 
> The code below for runtime seems to be duplicated from efi_variable.c.
> 

Yes is it. 

> Let's keep it like this for now. We can look for a better solution once
> we investigate a runtime solution.

As we discussed on IRC, i *think* having runtime variables is doable.
The OP-TEE supplicant uses ioctls to communicate with the kernel and access the
RPMB once linux is up and running. So the kernel will take care of any
concurrent accesses.
The only problem I can think of now, is what happens to the variable *writes*
until the supplicant is up. Your patches shadowing variables into a different
memory location might be one solution to that problem.
We might not even need those, because of the internal of StMM. EDK2 expects byte
addressable memory for the variable access. Since the RPMB cant be memory
mapped, my EDK2 driver creates a memory copy of the variables and syncs the
variables on read/write with the hardware. As long as the linux EFI stub only
reads variables we might be fine using the code as -is.

In any case I agree, let's merge this as is, since accessing runtime variables
will return EFI_UNSUPPORTED and won't crash the system and I'll look into
runtime variables details afterwards.

> Overall the code looks good and clean (except for my nitpicky comments).

Thanks! I'll fix up the comments and send a v2.

> 
> Thanks a lot for all the effort you invested here.
> 


Regards
/Ilias

[PATCH 5/6] MAINTAINERS: Add maintainer for EFI variables via OP-TEE

[Ilias Apalodimas]

On Sat, May 09, 2020 at 11:17:59AM +0200, Heinrich Schuchardt wrote:
> On 5/6/20 9:12 PM, Ilias Apalodimas wrote:
> > Add myself as maintainer for the OP-TEE related UEFI variable storage
> > and add the headers files on the existing EFI list
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  MAINTAINERS | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ec59ce8b8802..f33fd74b330b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -625,6 +625,8 @@ F:	include/cp437.h
> >  F:	include/efi*
> >  F:	include/pe.h
> >  F:	include/asm-generic/pe.h
> > +F:	include/mm_communication.h
> > +F:	include/mm_variable.h
> >  F:	lib/charset.c
> >  F:	lib/efi*/
> >  F:	test/py/tests/test_efi*
> > @@ -635,6 +637,11 @@ F:	cmd/efidebug.c
> >  F:	cmd/nvedit_efi.c
> >  F:	tools/file2include.c
> >
> > +EFI VARIABLES VIA OP-TEE
> > +M:	Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > +S:	Maintained
> > +F:	lib/efi_loader/efi_variable_tee.c
> 
> I think the mm* includes and efi_variable_tee.c should rest with the
> same maintainer.

Ok. 

Regards
/Ilias
> 
> Best regards
> 
> Heinrich
> 
> > +
> >  ENVIRONMENT
> >  M:	Joe Hershberger <joe.hershberger@ni.com>
> >  R:	Wolfgang Denk <wd@denx.de>
> >
> 

[PATCH 4/6] cmd: efidebug: Add support for querying UEFI variable storage

[Ilias Apalodimas]

On Sat, May 09, 2020 at 11:58:17AM +0200, Heinrich Schuchardt wrote:
> On 5/6/20 9:12 PM, Ilias Apalodimas wrote:
> > +

[...]

> > +	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
> 
> We are not printing handles. Please remove the line.
> 

Ok

> > +	printf("Max storage size %llu\n", max_variable_storage_size);
> > +	printf("Remaining storage size %llu\n", remain_variable_storage_size);
> > +	printf("Max variable size %llu\n", max_variable_size);
> > +
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> >  static cmd_tbl_t cmd_efidebug_sub[] = {
> >  	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
> >  	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
> > @@ -1176,6 +1215,8 @@ static cmd_tbl_t cmd_efidebug_sub[] = {
> >  			 "", ""),
> >  	U_BOOT_CMD_MKENT(test, CONFIG_SYS_MAXARGS, 1, do_efi_test,
> >  			 "", ""),
> > +	U_BOOT_CMD_MKENT(query, CONFIG_SYS_MAXARGS, 1, do_efi_query_info,
> > +			 "", ""),
> >  };
> >
> >  /**
> > @@ -1247,7 +1288,9 @@ static char efidebug_help_text[] =
> >  	"efidebug tables\n"
> >  	"  - show UEFI configuration tables\n"
> >  	"efidebug test bootmgr\n"
> > -	"  - run simple bootmgr for test\n";
> > +	"  - run simple bootmgr for test\n"
> > +	"efidebug query\n"
> > +	"  - show information of the container used to store UEFI variables\n";
> 
> This text does not make it clear that we will get size information. How
> about:
> 
> "show size of UEFI variables store\n"

Well the text was a c/p from here [1], but I agree I'll change it to what you
propose.


[1] https://edk2-docs.gitbook.io/edk-ii-uefi-driver-writer-s-guide/5_uefi_services/readme.2/526_queryvariableinfo


Regards
/Ilias
> 
> Best regards
> 
> Heinrich
> 
> >  #endif
> >
> >  U_BOOT_CMD(
> >
> 

[PATCH 6/6] doc: uefi.rst: Add OP-TEE variable storage config options

[Ilias Apalodimas]

On Sat, May 09, 2020 at 11:51:48AM +0200, Heinrich Schuchardt wrote:
> On 5/6/20 9:12 PM, Ilias Apalodimas wrote:
> > If OP-TEE is compiled with an EDK2 application running in secure world
> > it can process and store UEFI variables in an RPMB.
> > Add documentation for the config options enabling this
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  doc/uefi/uefi.rst | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/doc/uefi/uefi.rst b/doc/uefi/uefi.rst
> > index 4fda00d68721..93b0faadd26e 100644
> > --- a/doc/uefi/uefi.rst
> > +++ b/doc/uefi/uefi.rst
> > @@ -188,6 +188,16 @@ on the sandbox
> >      cd <U-Boot source directory>
> >      pytest.py test/py/tests/test_efi_secboot/test_signed.py --bd sandbox
> >
> > +Using OP-TEE for EFI variables
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +If an RPMB and it's drivers is available in U-Boot, OP-TEE can be used for
> 
> %s/is available/are available/
> 
> ..., OP-TEE in conjunction with EDK2's secure management module (SMM)
> can be used to provide variable services.
> 
> > +variable services.
> > +Enabling CONFIG_EFI_MM_COMM_TEE=y will dispatch the variables services to
> 
> %s/dispatch/delegate/
> 
> > +OP-TEE. OP-TEE needs to be compiled with a secure application (coming from EDK2)
> 
> Is it really compiling? I thought it was only linking.
> 
> ... needs to be linked with EDK2's secure management module (SMM) which
> will process the variables ...

It's a bit weird, you practically append the whole binary *after* OP-TEE source
code. So you compile OP-TEE with:
make CFG_ARM64_core=y PLATFORM=<plat> CFG_STMM_PATH=BL32_AP_MM.fd

> 
> > +which will process variables in the Secure World and store them in the RPMB
> > +using the OP-TEE supplicant.
> > +
> >  Executing the boot manager
> >  ~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> >
> 
> We should separate in the description between OP-TEE being used to
> provide variable services and the specific embodiment using SMM, e.g.
> 
> How about:
> 
> 
> Using OP-TEE for EFI variables
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Instead of implementing UEFI variable services inside U-Boot they can
> also be provided in the secure world by a module for OP-TEE[1]. The
> interface between U-Boot and OP-TEE for variable services is enabled by
> CONFIG_EFI_MM_COMM_TEE=y.
> 
> Tianocore EDK II's standalone management mode driver for variables can
> be linked to OP-TEE for this purpose. This module uses the Replay
> Protected Memory Block (RPMB) of an eMMC device for persisting
> non-volatile variables. When calling the variable services via the
> OP-TEE API U-Boot's OP-TEE supplicant relays calls to the RPMB driver
> which has to be enabled via CONFIG_SUPPORT_EMMC_RPMB=y.
> 
> [1] https://optee.readthedocs.io/ - OP-TEE documentation

Ok sounbds better, I'll use this.

Regards
/Ilias
> 
> Best regards
> 
> Heinrich

[PATCH 3/6] efi_loader: Implement EFI variable handling via OP-TEE

[Ilias Apalodimas]

On Sat, May 09, 2020 at 11:14:51AM +0200, Heinrich Schuchardt wrote:

> > +	in_name_size = u16_strsize(variable_name);

[...]

> 
> The UEFI spec requires: "The size must be large enough to fit input
> string supplied in VariableName buffer."
> 
> Further it is required to return EFI_INVALID_PARAMETER if the
> "Null-terminator is not found in the first VariableNameSize bytes of the
> input VariableName buffer."
> 
> Please, investigate if SMM takes care of the check or we should do it.
> 

Smm checks for both and returns EFI_ACCESS_DENIED. 
In any case I don't suggest convoluting this with extra UEFI spec requirements.
Variables are delegated into Smm for handling and it should handle *everything*.
Any bugs/missing corner cases we end up discovering should be fixed directly
into EDK2 and not apply random fixups here. This is an API to Smm and that's 
all it should ever do.

Regards
/Ilias
 

[PATCH 2/6] efi_loader: Add headers for EDK2 StandAloneMM communication

[Ilias Apalodimas]

On Sat, May 09, 2020 at 10:12:25AM +0200, Heinrich Schuchardt wrote:
> On 5/6/20 9:12 PM, Ilias Apalodimas wrote:
> > From: Sughosh Ganu <sughosh.ganu@linaro.org>
> >
> > In Arm devices OP-TEE has the ability to run StandAloneMM (from EDK2)
> > in a separate partition and handle UEFI variables.
> > A following patch introduces this functionality.
> >
> > Add the headers needed for OP-TEE <--> StandAloneMM communication
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  include/mm_communication.h | 28 ++++++++++++++
> >  include/mm_variable.h      | 78 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 106 insertions(+)
> >  create mode 100644 include/mm_communication.h
> >  create mode 100644 include/mm_variable.h
> >
> > diff --git a/include/mm_communication.h b/include/mm_communication.h
> > new file mode 100644
> > index 000000000000..fb4c91103400
> > --- /dev/null
> > +++ b/include/mm_communication.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> 
> BSD-2-Clause-Patent is compatible with GPL-2. So relicensing as GPL
> should be ok.
> 
> > +/*
> > + *  Headers for EFI variable service via StandAloneMM, EDK2 application running
> > + *  in OP-TEE
> > + *
> > + *  Copyright (C) 2020 Linaro Ltd. <sughosh.ganu@linaro.org>
> > + *  Copyright (C) 2020 Linaro Ltd. <ilias.apalodimas@linaro.org>
> 
> EDK2, MdePkg/Include/Protocol/MmCommunication.h has:
> Copyright (c) 2017, Intel Corporation. All rights reserved.
> 
> Why did you replace their copyright by yours?
> Who is the original copyright holder of the MM module?

Oops sorry will add the original Copyright as well.

> 
> > + */
> > +
> > +#if !defined _MM_COMMUNICATION_H_
> 
> I would use #ifndef here.
> 
> > +#define _MM_COMMUNICATION_H_
> > +
> > +/* defined in EDK2 MmCommunication.h */
> 
> Please, add a description of the structure, cf.
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation
> 
> > +struct mm_communicate_header {
> > +	efi_guid_t header_guid;
> > +	size_t     message_len;
> > +	u8         data[1];
> 
> In C11 you can use data[].
> 

Yes see below

> > +};
> > +
> > +#define MM_COMMUNICATE_HEADER_SIZE \
> > +	(offsetof(struct mm_communicate_header, data))
> 
> SMM_COMMUNICATE_HEADER_SIZE?
> Why not simply use data[] and sizeof(struct mm_communicate_header)
> instead of defining this constant.
> 

Because it would make future porting easier. This is not exactly speed critical
code.
I'll change it to C11.

> > +
> > +#define MM_RET_SUCCESS         0
> 
> Why don't you use the same names as in EDK2, e.g.
> ARM_SMC_MM_RET_SUCCESS?
> 
> Please, add ARM_SMC_MM_RET_NOT_SUPPORTED.

Ok 
Makes sense to keep the original naming.

> 
> > +#define MM_RET_INVALID_PARAMS -2
> > +#define MM_RET_DENIED         -3
> > +#define MM_RET_NO_MEMORY      -4
> > +
> > +#endif /* _MM_COMMUNICATION_H_*/
> > diff --git a/include/mm_variable.h b/include/mm_variable.h
> > new file mode 100644
> > index 000000000000..f56c52597629
> > --- /dev/null
> > +++ b/include/mm_variable.h
> > @@ -0,0 +1,78 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + *  Headers for EFI variable service via StandAloneMM, EDK2 application running
> > + *  in OP-TEE
> > + *
> > + *  Copyright (C) 2020 Linaro Ltd. <sughosh.ganu@linaro.org>
> > + *  Copyright (C) 2020 Linaro Ltd. <ilias.apalodimas@linaro.org>
> 
> If you copied this from SmmVariableCommon.h shouldn't you mention:
> Copyright (c) 2011 - 2019, Intel Corporation. All rights reserved.
> 

Ok 

> > + */
> > +
> > +#if !defined _MM_VARIABLE_H_
> 
> #ifndef would be shorter.
> 

Ok 

> > +#define _MM_VARIABLE_H_
> > +
> > +#include <part_efi.h>
> > +
> > +/* defined in EDK2 SmmVariableCommon.h */
> 
> Please, add a description of the structure, cf.
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation
> 
> > +struct mm_variable_communicate_header {
> > +	efi_uintn_t  function;
> > +	efi_status_t ret_status;
> > +	u8           data[1];
> > +};
> > +
> > +#define MM_VARIABLE_COMMUNICATE_SIZE \
> > +	(offsetof(struct mm_variable_communicate_header, data))
> 
> Why not the original name SMM_VARIABLE_COMMUNICATE_HEADER_SIZE?

Ok 

> 
> > +
> > +#define MM_VARIABLE_FUNCTION_GET_VARIABLE			1
> 
> MdeModulePkg/Include/Guid/SmmVariableCommon.h uses
> SMM_VARIABLE_FUNCTION_GET_VARIABLE. Why invent a new name?
> 
> > +
> > +#define MM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME		2
> > +
> > +#define MM_VARIABLE_FUNCTION_SET_VARIABLE			3
> > +
> > +#define MM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO		4
> > +
> > +#define MM_VARIABLE_FUNCTION_READY_TO_BOOT			5
> > +
> > +#define MM_VARIABLE_FUNCTION_EXIT_BOOT_SERVICE			6
> > +
> > +#define MM_VARIABLE_FUNCTION_GET_STATISTICS			7
> > +
> > +#define MM_VARIABLE_FUNCTION_LOCK_VARIABLE			8
> > +
> > +#define MM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_SET	9
> > +
> > +#define MM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET	10
> > +
> > +#define MM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE			11
> 
> Values 12 - 14 are missing. I think we should provide all values.
> 

Ok 

> > +
> 
> Missing structure description.
> 
> > +struct mm_variable_access {
> 
> smm_variable_access?
> 
> > +	efi_guid_t  guid;
> > +	efi_uintn_t data_size;
> > +	efi_uintn_t name_size;
> > +	u32         attr;
> > +	u16         name[1];
> 
> This name[1] was needed in old compilers. It is not needed anymore in
> C11.  For a variable length structure component, please, use name[].
> 
> > +};
> > +
> > +#define MM_VARIABLE_ACCESS_HEADER_SIZE \
> > +	(offsetof(struct mm_variable_access, name))
> 
> If you have name[] as component, you can use sizeof(struct
> smm_variable_access) instead of this define.
> 
> > +
> 
> Structure description missing.
> 
> > +struct mm_variable_payload_size {
> > +	efi_uintn_t size;
> > +};
> 
> In EDK2 PayloadSize is simply UINTN.
> 
> Isn't this structure superfluous? Can't we directly pass a type UINTN
> variable instead and get rid of a level of indirection?

This is defined as SMM_VARIABLE_COMMUNICATE_GET_PAYLOAD_SIZE which is what we
define here.

> 
> > +
> 
> Structure description missing.
> 
> > +struct mm_variable_getnext {
> > +	efi_guid_t  guid;
> > +	efi_uintn_t name_size;
> > +	u16         name[1];
> 
> name[]?
> 
> > +};
> > +
> > +#define MM_VARIABLE_GET_NEXT_HEADER_SIZE \
> > +	(offsetof(struct mm_variable_getnext, name))
> > +
> 
> Structure description missing.
> 
> You could think about merging the two include files into one.

Agreed

Regards
/Ilias
> 
> Best regards
> 
> Heinrich
> 
> > +struct mm_variable_query_info {
> > +	u64 max_variable_storage;
> > +	u64 remaining_variable_storage;
> > +	u64 max_variable_size;
> > +	u32 attr;
> > +};
> > +
> > +#endif /* _MM_VARIABLE_H_ */
> >
>