From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f44.google.com (mail-ej1-f44.google.com [209.85.218.44]) by mx.groups.io with SMTP id smtpd.web10.1101.1614799483907260515 for ; Wed, 03 Mar 2021 11:24:44 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=oYlXRfio; spf=pass (domain: linaro.org, ip: 209.85.218.44, mailfrom: ilias.apalodimas@linaro.org) Received: by mail-ej1-f44.google.com with SMTP id gt32so32908929ejc.6 for ; Wed, 03 Mar 2021 11:24:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=K8QyK9Wg+GLugCqqlLuGnY27o+cWvjeByWzhMAthNWw=; b=oYlXRfionD1BgtCXwL6atpBa0YI69n+hIGDJL14MC2zTUhxclzEF+1HygCfDsHkUkQ /EIEKPsNMoOigRGL2gZ+TbLP/Z+TDCeV9A9oMQhWsInl/BkPezFSbuTv6WOQSos6FoOS EHxrxivfgwGxzKfh2TJHwqndsmzc1QzBEce8RYfwLG861mEQEwR/eEKYkGDM4LnKIADY jYQrfhYT41pA4KcetJ+JpzW183lByB6DREF2DhdYuyQg2/QSHFTqdms0qvm9DhotdwGa q2+KTuTrBgMPGad3x9shAR8O6VgLWbpTOclnF5mw7TjtqhBnnevKX/5iqG7LUhTdtGer EJSw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=K8QyK9Wg+GLugCqqlLuGnY27o+cWvjeByWzhMAthNWw=; b=R5TSHjkCG4UBQ4zS6ghqKJXXfZFhSiCB/Vy0C2Z8S6mb9beP8syNJQpioDDGYC7bC3 +wYiSo5X9nutfzztqinh6SPgrPzvAKl96CDj4aq9AnF2w5QUpu+cb++pmOkbazwjRGA2 a40ORSKy4txKqfMx7wDlFbxXpDkXnDa35yK0VlFIf3u0u/euFxVvOoFOm/OlBPED2EZz INDY+Up+1wEQzGuPC2copnGUFapHkVwODtcquF7UN5JJLzRgcxHUjmjgoNMzgz7GW84h VwscKRAzaFB11+R0AV14d52qF0QSeHHID8DmhhKPvXgBAAEhg2swPaBDkE90wVKqhuEy JfRg== X-Gm-Message-State: AOAM533JCrQHz/SQGW//UNFL/KFbgdirUhPZauvVRn5UpSQ+2N9JrZ+q U9FKamif3gt20mnkIyMRPXDXzw== X-Google-Smtp-Source: ABdhPJwdJdHooSR8sewCKobom0Bpk1G7/kFKn9VK33KkPhrLP4jHszwsWlo/7csbLFH7ob6KMuVfvg== X-Received: by 2002:a17:906:a090:: with SMTP id q16mr381651ejy.236.1614799482393; Wed, 03 Mar 2021 11:24:42 -0800 (PST) Return-Path: Received: from apalos.home (ppp-94-64-113-158.home.otenet.gr. [94.64.113.158]) by smtp.gmail.com with ESMTPSA id u14sm21072953ejx.60.2021.03.03.11.24.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 03 Mar 2021 11:24:41 -0800 (PST) Date: Wed, 3 Mar 2021 21:24:39 +0200 From: "Ilias Apalodimas" To: Pierre Cc: devel@edk2.groups.io, Sami.Mujawar@arm.com Subject: Re: [edk2-devel] [PATCH 2/2 v5] StMMRpmb: Add support for building StandaloneMm image for OP-TEE Message-ID: References: <20210212173459.508205-1-ilias.apalodimas@linaro.org> <20210212173459.508205-3-ilias.apalodimas@linaro.org> <05ede192-6a8f-ec8e-ed97-bfe52c9fc9de@arm.com> MIME-Version: 1.0 In-Reply-To: <05ede192-6a8f-ec8e-ed97-bfe52c9fc9de@arm.com> Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit Hi Pierre, On Wed, Mar 03, 2021 at 10:18:57AM +0000, Pierre wrote: > Hello Ilias, > > I would have some (inlined) remarks about your patch, > > Regards, > > Pierre > > On 3/2/21 3:35 PM, Pierre Gondois wrote: > > +  PLATFORM_NAME                  = MmStandaloneRpmb [...] > > > > +  PLATFORM_GUID                  = A27A486E-D7B9-4D70-9F37-FED9ABE041A2 > > > > +  PLATFORM_VERSION               = 1.0 > > > > +  DSC_SPECIFICATION              = 0x00010011 > I think we are at the revision 0x0001001C, cf https://edk2-docs.gitbook.io/edk-ii-dsc-specification/3_edk_ii_dsc_file_format/35_-defines-_section > Probably, the paptch has been sitting in my trees for quite some time. I'll have a look and update it. > > > > +  OUTPUT_DIRECTORY               = Build/$(PLATFORM_NAME) > > > > +  SUPPORTED_ARCHITECTURES        = AARCH64 > > + StandaloneMmCoreEntryPoint|StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf [...] > > > > + StandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntryPoint/StandaloneMmDriverEntryPoint.inf > > > > + > > > > + StandaloneMmMmuLib|ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf > > > > + CacheMaintenanceLib|MdePkg/Library/BaseCacheMaintenanceLibNull/BaseCacheMaintenanceLibNull.inf > > > > + PeCoffExtraActionLib|StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf > > > > +  RngLib|MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf > > > > + > > > > + SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf > > > > + DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf > It seems in the previous patch you are using some 'DEBUG ()' and 'ASSERT () > statements. Wouldn't using BaseDebugLibNull and BaseSerialPortLibNull make > them useless for DEBUG and RELEASE build ? There's a reasson for that. So what happens right now is that OP-TEE creates (and maps) the device(s) StMM can access. In the current case you are correct, but noone prevents us from mapping the console and in the future have those ASSERT/DEBUG statements visible. So I figured it's worth keeping them in there even if they won't (currently) show up, since they offer some hints to code reading as well. In fact we do have patches that map the console and intend to upstream them into OP-TEE at some point (and this was used during development as well). > > > > + > > > > +  # > > > > +  # It is not possible to prevent the ARM compiler for generic > > intrinsic functions. > > > > +  # This library provides the intrinsic functions generate by a given > > compiler. > > > > +  # NULL means link this library into all ARM images. > > > > +  # > > > > + NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf > > > > + > > > > +[LibraryClasses.common.MM_STANDALONE] > > > > + HobLib|StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf > > > > + MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf > > > > + MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.inf > > > > + > > > > + IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf > > > > +  OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf > > > > + PlatformSecureLib|SecurityPkg/Library/PlatformSecureLibNull/PlatformSecureLibNull.inf > > > > + SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf > > > > + TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf > > > > +################################################################################ > > > > +# > > > > +# Pcd Section - list of all EDK II PCD Entries defined by this Platform > > > > +# > > > > +################################################################################ > > > Since this comment is for the PCD section, I think it would be better to > remove the empty line after the comment and add one at the top of this > comment. Sure [...] Thanks /Ilias