Re: [PATCH 6/6] arm_ffa: introduce FF-A MM communication

Hi Abdellatif,

Can you please keep me cc'ed in future revisions?

On Tue, Mar 29, 2022 at 04:16:59PM +0100, abdellatif.elkhlifi@arm.com wrote:

From: Abdellatif El Khlifi abdellatif.elkhlifi@arm.com

Add MM communication support using FF-A transport

FF-A MM communication allows exchanging data with StandAlonneMM or smm-gateway secure partitions which run in OP-TEE.

An MM shared buffer and a door bell event are used to exchange this data.

The data is used by EFI services such as GetVariable()/SetVariable() and copied from the communication buffer to the MM shared buffer.

The secure partition is notified about availability of data in the MM shared buffer by an FF-A message (door bell).

On such event, MM SP can read the data and updates the MM shared buffer with the response data.

The response data is copied back to the communication buffer and consumed by the EFI subsystem.

What I am missing from all this is a description on how to test this and what's needed in OP-TEE and EDK2.

Unless I am reading this wrong, there is a "new" Secure Partition that will handle both the efi variables (along with all the EFI rules you need to update those and the crypto checks you need for authenticated variables). But StMM includes the hardware drivers as well. How is that handled in the SP context?

Signed-off-by: Abdellatif El Khlifi abdellatif.elkhlifi@arm.com Signed-off-by: Gowtham Suresh Kumar gowtham.sureshkumar@arm.com Cc: Tom Rini trini@konsulko.com

---

arch/arm/cpu/armv8/cache.S | 16 ++ arch/arm/cpu/armv8/cache_v8.c | 3 +- include/mm_communication.h | 4 +- lib/efi_loader/Kconfig | 14 +- lib/efi_loader/efi_variable_tee.c | 294 +++++++++++++++++++++++++++++- 5 files changed, 321 insertions(+), 10 deletions(-)

diff --git a/arch/arm/cpu/armv8/cache.S b/arch/arm/cpu/armv8/cache.S index d1cee23437..bdbe89e0c5 100644 --- a/arch/arm/cpu/armv8/cache.S +++ b/arch/arm/cpu/armv8/cache.S @@ -21,7 +21,11 @@

• x1: 0 clean & invalidate, 1 invalidate only
• x2~x9: clobbered

*/ +#ifdef CONFIG_ARM_FFA_TRANSPORT +.pushsection .text.efi_runtime, "ax" +#else .pushsection .text.__asm_dcache_level, "ax" +#endif ENTRY(__asm_dcache_level) lsl x12, x0, #1 msr csselr_el1, x12 /* select cache level */ @@ -65,7 +69,11 @@ ENDPROC(__asm_dcache_level)

• flush or invalidate all data cache by SET/WAY.

*/ +#ifdef CONFIG_ARM_FFA_TRANSPORT +.pushsection .text.efi_runtime, "ax" +#else .pushsection .text.__asm_dcache_all, "ax" +#endif ENTRY(__asm_dcache_all) mov x1, x0 dsb sy @@ -109,7 +117,11 @@ ENTRY(__asm_flush_dcache_all) ENDPROC(__asm_flush_dcache_all) .popsection

+#ifdef CONFIG_ARM_FFA_TRANSPORT +.pushsection .text.efi_runtime, "ax" +#else .pushsection .text.__asm_invalidate_dcache_all, "ax" +#endif ENTRY(__asm_invalidate_dcache_all) mov x0, #0x1 b __asm_dcache_all @@ -182,7 +194,11 @@ ENTRY(__asm_invalidate_icache_all) ENDPROC(__asm_invalidate_icache_all) .popsection

+#ifdef CONFIG_ARM_FFA_TRANSPORT +.pushsection .text.efi_runtime, "ax" +#else .pushsection .text.__asm_invalidate_l3_dcache, "ax" +#endif WEAK(__asm_invalidate_l3_dcache) mov x0, #0 /* return status as success */ ret diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c index 3de18c7675..187a4497a7 100644 --- a/arch/arm/cpu/armv8/cache_v8.c +++ b/arch/arm/cpu/armv8/cache_v8.c @@ -9,6 +9,7 @@

#include <common.h> #include <cpu_func.h> +#include <efi_loader.h> #include <hang.h> #include <log.h> #include <asm/cache.h> @@ -425,7 +426,7 @@ __weak void mmu_setup(void) /*

• Performs a invalidation of the entire data cache at all levels

*/ -void invalidate_dcache_all(void) +void __efi_runtime invalidate_dcache_all(void) { __asm_invalidate_dcache_all(); __asm_invalidate_l3_dcache(); diff --git a/include/mm_communication.h b/include/mm_communication.h index e65fbde60d..bb99190956 100644 --- a/include/mm_communication.h +++ b/include/mm_communication.h @@ -123,7 +123,7 @@ struct __packed efi_mm_communicate_header {

• Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_HEADER.

*/ -struct smm_variable_communicate_header { +struct __packed smm_variable_communicate_header {

Why is this converted to packed?

efi_uintn_t function; efi_status_t ret_status; u8 data[]; @@ -145,7 +145,7 @@ struct smm_variable_communicate_header {

• Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE.

*/ -struct smm_variable_access { +struct __packed smm_variable_access {

Ditto

efi_guid_t guid; efi_uintn_t data_size; efi_uintn_t name_size; diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 28657f50c9..0d69133595 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -55,13 +55,23 @@ config EFI_VARIABLE_FILE_STORE stored as file /ubootefi.var on the EFI system partition.

config EFI_MM_COMM_TEE

• bool "UEFI variables storage service via OP-TEE"
• depends on OPTEE

• bool "UEFI variables storage service via the trusted world"

• depends on OPTEE || ARM_FFA_TRANSPORT help

•  The MM SP (also called partition) can be StandAlonneMM or smm-gateway.
  

•  When using the u-boot OP-TEE driver, StandAlonneMM is supported.
  

•  When using the u-boot FF-A  driver, StandAlonneMM and smm-gateway are supported.
  

• 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.

•  When ARM_FFA_TRANSPORT is used, dispatch all UEFI variable related
  

•  operations to the MM SP running under Optee in the trusted world.
  

•  A door bell mechanism is used to notify the SP when there is data in the shared
  

•  MM buffer. The data is copied by u-boot to the shared buffer before issuing
  

•  the door bell event.
  

The naming is a bit misleading imho. FF-A is the transport mechanism between the secure and non-secure world. You may as well have FF-A along with OP-TEE. That doesn't automatically mean the SP is used instead of StMM.

This code is complicated already, I don't think we want the additional ifdefery. Wouldn't it be better to have this discover to what kind of protocol (e.g FF-A vs SMC calling conventions) it talks to? SPs are discoverable, so we could reason about those as well without asking the user to understand Arm protocol internals.

endchoice

config EFI_VARIABLES_PRESEED diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c index dfef18435d..3d01985662 100644 --- a/lib/efi_loader/efi_variable_tee.c +++ b/lib/efi_loader/efi_variable_tee.c @@ -15,6 +15,53 @@ #include <malloc.h> #include <mm_communication.h>

+#if (IS_ENABLED(CONFIG_ARM_FFA_TRANSPORT))

+#include <arm_ffa_helper.h> +#include <cpu_func.h> +#include <mapmem.h>

+#ifndef FFA_SHARED_MM_BUFFER_SIZE

+#include <linux/sizes.h> +#define FFA_SHARED_MM_BUFFER_SIZE SZ_4K /* 4 KB */

+#endif

+#ifndef FFA_SHARED_MM_BUFFER_ADDR

+/*

• • shared buffer physical address used for communication between
• • u-boot and the MM SP
• */

+#define FFA_SHARED_MM_BUFFER_ADDR (0x023F8000)

Can you explain what this is used for ?

+#endif

+/* MM return codes */ +#define MM_SUCCESS (0)

+#define ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64 (0xC4000061) +#define ARM_SVC_ID_SP_EVENT_COMPLETE ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64

+#ifndef MM_SP_UUID_DATA

+/* MM SP UUID binary data (little-endian format) */ +#define MM_SP_UUID_DATA \

• 0xed, 0x32, 0xd5, 0x33, \
• 0x99, 0xe6, 0x42, 0x09, \
• 0x9c, 0xc0, 0x2d, 0x72, \
• 0xcd, 0xd9, 0x98, 0xa7

[...]

• /* Announce there is data in the shared buffer */
• ffa_ret = ffa_notify_mm_sp();
• if (ffa_ret)

• unmap_sysmem(virt_shared_buf);
  

There are several calls around on runtime code which call functions not marked as runtime. This will just crash when called.

• switch (ffa_ret) {
• case 0:
• {

• ulong rx_data_size;
  

• /* Copy the MM SP response from the shared buffer to the communication buffer */
  

• rx_data_size = ((struct efi_mm_communicate_header *)virt_shared_buf)->message_len +
  

• 	sizeof(efi_guid_t) +
  

• 	sizeof(size_t);
  

• if (rx_data_size > comm_buf_size) {
  

• 	efi_memcpy_runtime(comm_buf, virt_shared_buf, comm_buf_size);
  

• 	unmap_sysmem(virt_shared_buf);
  

• 	return EFI_BUFFER_TOO_SMALL;
  

• }
  

• efi_memcpy_runtime(comm_buf, virt_shared_buf, rx_data_size);
  

• unmap_sysmem(virt_shared_buf);
  

• return EFI_SUCCESS;
  

• }
• case -EINVAL:

• return EFI_DEVICE_ERROR;
  

• case -EPERM:

• return EFI_INVALID_PARAMETER;
  

• case -EACCES:

• return EFI_ACCESS_DENIED;
  

• case -EBUSY:

• return EFI_OUT_OF_RESOURCES;
  

• default:

• return EFI_ACCESS_DENIED;
  

• }

+} +#endif

+/**

• • mm_communicate() - Adjust the communication buffer to the MM SP and send
  • it to OP-TEE

• • @comm_buf: locally allocted communcation buffer

• • @comm_buf: locally allocated communication buffer
  • @dsize: buffer size
• • The MM SP (also called partition) can be StandAlonneMM or smm-gateway.
• • The comm_buf format is the same for both partitions.
• • When using the u-boot OP-TEE driver, StandAlonneMM is supported.
• • When using the u-boot FF-A driver, StandAlonneMM and smm-gateway are supported.
• • Return: status code
  */

-static efi_status_t mm_communicate(u8 *comm_buf, efi_uintn_t dsize) +static efi_status_t __efi_runtime mm_communicate(u8 *comm_buf, efi_uintn_t dsize) { efi_status_t ret; struct efi_mm_communicate_header *mm_hdr; @@ -162,9 +432,16 @@ static efi_status_t mm_communicate(u8 *comm_buf, efi_uintn_t dsize) mm_hdr = (struct efi_mm_communicate_header *)comm_buf; var_hdr = (struct smm_variable_communicate_header *)mm_hdr->data;

• #if (IS_ENABLED(CONFIG_OPTEE)) ret = optee_mm_communicate(comm_buf, dsize);
• #elif (IS_ENABLED(CONFIG_ARM_FFA_TRANSPORT))
• ret = ffa_mm_communicate(comm_buf, dsize);
• #endif if (ret != EFI_SUCCESS) {

• log_err("%s failed!\n", __func__);
  

• /* No need for showing a log here. mm_communicate failure happens
  

•  * when getVariable() is called with data size set to 0.
  

•  * This is not expected so no log shown.
  

•  */
  

It can also fail on normal cases were the size is != 0

return ret;

}

@@ -258,6 +535,13 @@ efi_status_t EFIAPI get_max_payload(efi_uintn_t *size) goto out; } *size = var_payload->size;

• #if (IS_ENABLED(CONFIG_ARM_FFA_TRANSPORT))

• if (*size > FFA_SHARED_MM_BUFFER_SIZE)
  

• 	*size = FFA_SHARED_MM_BUFFER_SIZE - MM_COMMUNICATE_HEADER_SIZE	-
  

• 		MM_VARIABLE_COMMUNICATE_SIZE;
  

• #endif
• /*
  • There seems to be a bug in EDK2 miscalculating the boundaries and
  • size checks, so deduct 2 more bytes to fulfill this requirement. Fix

@@ -697,7 +981,7 @@ void efi_variables_boot_exit_notify(void) ret = EFI_NOT_FOUND;

if (ret != EFI_SUCCESS)

• log_err("Unable to notify StMM for ExitBootServices\n");
  

• log_err("Unable to notify the MM partition for ExitBootServices\n");
  

  free(comm_buf);

  /*

-- 2.17.1

Thanks /Ilias