[PATCH v5 09/10] arm_ffa: introduce FF-A MM communication

Simon Glass sjg at chromium.org
Fri Oct 14 17:55:59 CEST 2022


Hi,

On Fri, 14 Oct 2022 at 04:38, Abdellatif El Khlifi
<abdellatif.elkhlifi at 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 <fenghua at phytium.com.cn>
> > >   *
> > > + * (C) Copyright 2022 ARM Limited
> > > + * Abdellatif El Khlifi <abdellatif.elkhlifi at 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 <agraf at suse.de>
> > > + *
> > > + * (C) Copyright 2022 ARM Limited
> > > + * Abdellatif El Khlifi <abdellatif.elkhlifi at 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.GA22382@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


More information about the U-Boot mailing list