Hi, On Fri, 14 Oct 2022 at 04:38, Abdellatif El Khlifi <abdellatif.elkhl...@arm.com> wrote: > > On Thu, Sep 29, 2022 at 12:32:42PM +0300, Ilias Apalodimas wrote: > > Hi Abdellatif, > > > > > --- a/arch/arm/cpu/armv8/cache.S > > > +++ b/arch/arm/cpu/armv8/cache.S > > > @@ -3,6 +3,9 @@ > > > * (C) Copyright 2013 > > > * David Feng <feng...@phytium.com.cn> > > > * > > > + * (C) Copyright 2022 ARM Limited > > > + * Abdellatif El Khlifi <abdellatif.elkhl...@arm.com> > > > + * > > > * This file is based on sample code from ARMv8 ARM. > > > */ > > > > > > @@ -21,7 +24,11 @@ > > > * x1: 0 clean & invalidate, 1 invalidate only > > > * x2~x9: clobbered > > > */ > > > +#ifdef CONFIG_EFI_LOADER > > > +.pushsection .text.efi_runtime, "ax" > > > > Maybe we discussed this in the past and I forgot, but why would you need > > __asm_dcache_level, __asm_dcache_all, __asm_invalidate_dcache_alla etc in > > the runtime section ? > > Because cache invalidation needs to be done at ffa_mm_communicate() level (v4 > patchset). > That code is runtime compatible so all the called functions must be under > .text.efi_runtime > > However, since we agreed to drop EFI runtime support from the patchset, v6 > patchset takes > care of that. > > > > > > +#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 +72,11 @@ ENDPROC(__asm_dcache_level) > > > * > > > * flush or invalidate all data cache by SET/WAY. > > > */ > > > +#ifdef CONFIG_EFI_LOADER > > > +.pushsection .text.efi_runtime, "ax" > > > +#else > > > .pushsection .text.__asm_dcache_all, "ax" > > > +#endif > > > ENTRY(__asm_dcache_all) > > > mov x1, x0 > > > dsb sy > > > @@ -109,7 +120,11 @@ ENTRY(__asm_flush_dcache_all) > > > ENDPROC(__asm_flush_dcache_all) > > > .popsection > > > > > > +#ifdef CONFIG_EFI_LOADER > > > +.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 +197,11 @@ ENTRY(__asm_invalidate_icache_all) > > > ENDPROC(__asm_invalidate_icache_all) > > > .popsection > > > > > > +#ifdef CONFIG_EFI_LOADER > > > +.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 e4736e5643..45f57372c2 100644 > > > --- a/arch/arm/cpu/armv8/cache_v8.c > > > +++ b/arch/arm/cpu/armv8/cache_v8.c > > > @@ -5,10 +5,14 @@ > > > * > > > * (C) Copyright 2016 > > > * Alexander Graf <ag...@suse.de> > > > + * > > > + * (C) Copyright 2022 ARM Limited > > > + * Abdellatif El Khlifi <abdellatif.elkhl...@arm.com> > > > */ > > > > > > #include <common.h> > > > #include <cpu_func.h> > > > +#include <efi_loader.h> > > > #include <hang.h> > > > #include <log.h> > > > #include <asm/cache.h> > > > @@ -445,7 +449,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..fe9104c56d 100644 > > > --- a/include/mm_communication.h > > > > [...] > > > > > * always begin with efi_mm_communicate_header. > > > */ > > > -struct __packed efi_mm_communicate_header { > > > +struct efi_mm_communicate_header { > > > efi_guid_t header_guid; > > > size_t message_len; > > > u8 data[]; > > > @@ -145,7 +150,7 @@ struct smm_variable_communicate_header { > > > * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE. > > > * > > > */ > > > -struct smm_variable_access { > > > +struct __packed smm_variable_access { > > > > You are randomly adding and deleting __packed cwin both structs. But you > > can't do that. > > Those structs are defined in StandAloneMM. This is the reason each struct > > description has the corresponding EDK2 definition. > > Thanks for the comment. > > However, we are not setting randomly the __packed keyword. There is a good > reason for that. > It has been explained before in this reply [1]. Basically it's because of > data padding issues > breaking the communication between u-boot and secure world (Optee). > > When upgrading Optee to v3.18, no issues anymore. > > The __packed changes have been dropped in patchset v6. > > [1]: > https://lore.kernel.org/all/20220926105620.ga22...@e121910.cambridge.arm.com/
That is the Linux mailing list. I cannot see any reason to add __packed to this struct as it is already set up that way. Also efi_mm_communicate_header is really long. I suggest efi_mm_hdr or efi_mm_comms_hdr Why are you using SMM on ARM? Isn't that an Intel thing? [..] Regards, SImon