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

[Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>]

On Fri, Oct 14, 2022 at 09:55:59AM -0600, Simon Glass wrote:
> Hi,
> 
> On Fri, 14 Oct 2022 at 04:38, Abdellatif El Khlifi
> <abdellatif.elkhlifi@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@phytium.com.cn>
> > > >   *
> > > > + * (C) Copyright 2022 ARM Limited
> > > > + * Abdellatif El Khlifi <abdellatif.elkhlifi@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@suse.de>
> > > > + *
> > > > + * (C) Copyright 2022 ARM Limited
> > > > + * Abdellatif El Khlifi <abdellatif.elkhlifi@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

Thanks Simon. The link above is not part of the linux mailing list.
It points to the mirror of the u-boot mailing list under lore.kernel.org

> __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