From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3405CC4332F for ; Mon, 17 Oct 2022 14:15:33 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 91FFE84E3C; Mon, 17 Oct 2022 16:15:31 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: by phobos.denx.de (Postfix, from userid 109) id 5B21584E49; Mon, 17 Oct 2022 16:15:30 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by phobos.denx.de (Postfix) with ESMTP id 0BD9884DEB for ; Mon, 17 Oct 2022 16:15:25 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=abdellatif.elkhlifi@arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6DD3D1042; Mon, 17 Oct 2022 07:15:30 -0700 (PDT) Received: from e121910.cambridge.arm.com (unknown [10.57.1.180]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 519D63F792; Mon, 17 Oct 2022 07:15:23 -0700 (PDT) Date: Mon, 17 Oct 2022 15:15:20 +0100 From: Abdellatif El Khlifi To: Simon Glass Cc: u-boot@lists.denx.de, nd@arm.com Subject: Re: [PATCH v5 09/10] arm_ffa: introduce FF-A MM communication Message-ID: <20221017141520.GA9608@e121910.cambridge.arm.com> References: <20220926101723.9965-1-abdellatif.elkhlifi@arm.com> <20220926140827.15125-1-abdellatif.elkhlifi@arm.com> <20220926140827.15125-10-abdellatif.elkhlifi@arm.com> <20221014103828.GA18079@e121910.cambridge.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean 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 > 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 > > > > * > > > > + * (C) Copyright 2022 ARM Limited > > > > + * Abdellatif El Khlifi > > > > + * > > > > * 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 > > > > + * > > > > + * (C) Copyright 2022 ARM Limited > > > > + * Abdellatif El Khlifi > > > > */ > > > > > > > > #include > > > > #include > > > > +#include > > > > #include > > > > #include > > > > #include > > > > @@ -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