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

26 Sep 2022

On Thu, Apr 14, 2022 at 10:54:08PM +0300, Ilias Apalodimas wrote:
...
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?
In the v4 patchset the MM SPs are discovered at runtime. All MM SPs have the same UUID
and use the same communication protocol. From v4, FF-A MM communication in u-boot is not tied to
any MM SP. Please check: https://lore.kernel.org/all/20220926101723.9965-10-abdellatif.elkhlifi@arm.c...
...
...
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?
no need to add the attribute packed for efi_mm_communicate_header and 
smm_variable_communicate_header structures because all fields sizes are
multiple of 8 bytes. This has been addressed in v4 patchset.
However, the smm_variable_access structure still needs to be packed
because attr is u32 and a 4 bytes padding is added by the compiler
which breaks the communication with the secure world.
...
...
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.
...
From patchset v4, when using the u-boot FF-A driver all MM SPs are supported
because the agreement is that they all use the same UUID.
Please check: https://lore.kernel.org/all/20220926101723.9965-10-abdellatif.elkhlifi@arm.c...
...
...
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 ?
FF-A MM communication is implemented by ffa_mm_communicate(). 
It allows exchanging EFI services data with  the MM partition 
using FF-A transport. The mechanism is based on a shared buffer 
and a door bell event.
When u-boot wants to send data to the MM partition the data is
written to the shared buffer and a door bell event is issued.
A door bell event means that there is data to read from the 
shared buffer.
The MM partition writes its response in the same shared buffer 
and u-boot reads the response from the shared buffer at 
ffa_mm_communicate() level.
The shared buffer address is specified by 
FFA_SHARED_MM_BUFFER_ADDR and should be defined by the board.
...
...



+#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.
This has been addressed from v2 patchset. Please refer to the latest work 
on v4: https://lore.kernel.org/all/20220926101723.9965-10-abdellatif.elkhlifi@arm.c...
...
...


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
This has been addressed on v4 patchset: https://lore.kernel.org/all/20220926101723.9965-10-abdellatif.elkhlifi@arm.c...
...
...
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

    