| Message ID | 20200506191246.237790-3-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | EFI variable support via OP-TEE | expand |
On 5/6/20 9:12 PM, Ilias Apalodimas wrote: > From: Sughosh Ganu <sughosh.ganu at 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 at linaro.org> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at 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 at linaro.org> > + * Copyright (C) 2020 Linaro Ltd. <ilias.apalodimas at 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 at linaro.org> > + * Copyright (C) 2020 Linaro Ltd. <ilias.apalodimas at 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_ */ >
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 at 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 at linaro.org> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at 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 at linaro.org> > > + * Copyright (C) 2020 Linaro Ltd. <ilias.apalodimas at 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 at linaro.org> > > + * Copyright (C) 2020 Linaro Ltd. <ilias.apalodimas at 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_ */ > > >
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 at linaro.org> + * Copyright (C) 2020 Linaro Ltd. <ilias.apalodimas at 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 at linaro.org> + * Copyright (C) 2020 Linaro Ltd. <ilias.apalodimas at 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_ */