Re: [edk2-devel] [PATCH 2/2 v5] StMMRpmb: Add support for building StandaloneMm image for OP-TEE

["Ilias Apalodimas" <ilias.apalodimas@linaro.org>]

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