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