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

Message ID 20200506191246.237790-3-ilias.apalodimas@linaro.org
State New
Headers show
Series
  • EFI variable support via OP-TEE
Related show

Commit Message

Ilias Apalodimas May 6, 2020, 7:12 p.m.
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

Comments

Heinrich Schuchardt May 9, 2020, 8:12 a.m. | #1
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_ */
>
Ilias Apalodimas May 11, 2020, 10:25 a.m. | #2
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_ */
> >
>

Patch

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_ */