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-u...
+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-u...
+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_ */