Hi Brian, Personally, I prefer to add the NULL class Library to StatusCodeHandler modules.
1. I think we should make the functionality of each module clear and separated. It may also be why we added ReportStatusCodeRouter and StatusCodeHandler modules in edk2 repo before. ReportStatusCodeRouter modules are responsible for producing Status Code Protocol/PPI and Report Status Code Handler Protocol/PPI, and StatusCodeHandler modules are responsible for producing handlers (Handlers can be provided by NULL class Libraries in this RFC). So, that’s why I don’t want to add the NULL class Library to ReportStatusCodeRouter modules directly, which change the functionality scope of existing modules. 1. I agree that we have a lot of layers of indirection now, but what we may gain is the good modularity. And you also mentioned that one or more StatusCodeHandler Modules may be used. We also want to achieve that only the StatusCodeHandler modules in MdeModulePkg can be used after this separation, platform can only add its own handler Libs to meet its requirement. 1. As Andrew mentioned below, if add the libraries to ReportStatusCodeRouter, there will be some issue we need to fix, which seems also make the code logic a little tricky to me. Thanks, Dandan From: Andrew Fish <af...@apple.com> Sent: Saturday, June 20, 2020 2:04 AM To: edk2-devel-groups-io <devel@edk2.groups.io>; brian.john...@hpe.com Cc: Bi, Dandan <dandan...@intel.com>; r...@edk2.groups.io; Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>; Tan, Ming <ming....@intel.com> Subject: Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules On Jun 19, 2020, at 10:29 AM, Brian J. Johnson <brian.john...@hpe.com<mailto:brian.john...@hpe.com>> wrote: On 6/18/20 2:01 AM, Dandan Bi wrote: Hi All, REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2816 We plan to separate two kinds of NULL class libraries for Memory and serial handlers fromMdeModulePkg/Universal/StatusCodeHandler/…/ StatusCodeHandlerPei/RuntimeDxe/Smm modules. The benefit we want to gain from this separation is to 1) make the code clear and easy to maintain, 2) make platform flexible to choose any handler library they need, and it also can reduce image size since the unused handlers can be excluded. If you have any concern or comments for this separation, please let me know. We plan to add new separated NULL class library MemoryStausCodeHandlerLib and SerialStatusCodeHandlerLib with different phase implementation into MdeModulePkg\Library\ directory. The main tree structure may like below: MdeModulePkg\Library |------MemoryStausCodeHandlerLib |------|------ PeiMemoryStausCodeHandlerLib.inf |------|------ RuntimeDxeMemoryStatusCodeHandlerLib.inf |------|------ SmmMemoryStausCodeHandlerLib.inf |------SerialStatusCodeHandlerLib |------|------ PeiSerialStatusCodeHandlerLib.inf |------|------ RuntimeDxeSerialStatusCodeHandlerLib.inf |------|------ SmmSerialStatusCodeHandlerLib.inf We will update existing platform use cases in edk2 and edk2-platform repo to cover the new NULL class library to make sure this change doesn’t impact any platform. After this separation, StatusCodeHandler module usage will like below, and it’s also very flexible for platform to cover more handler libraries to meet their requirements. MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPei.inf { <LibraryClasses> NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/PeiMemoryStausCodeHandlerLib.inf NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/PeiSerialStatusCodeHandlerLib.inf … } MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf { <LibraryClasses> NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/RuntimeDxeMemoryStausCodeHandlerLib.inf NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/RuntimeDxeSerialStatusCodeHandlerLib.inf … } MdeModulePkg/Universal/StatusCodeHandler/Smm/StatusCodeHandlerSmm.inf { <LibraryClasses> NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/SmmMemoryStausCodeHandlerLib.inf NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/SmmSerialStatusCodeHandlerLib.inf … } Thanks, Dandan Dandan, We'll have a lot of layers of indirection.... The ReportStatusCodeRouter modules will call one or more StatusCodeHandlerModules, and the standard StatusCodeHandler modules will call multiple StatusCodeHandlerLib libraries. How about adding StatusCodeHandlerLib support directly to the ReportStatusCodeRouter modules? Then platforms could omit the StatusCodeHandler modules if they're only using the open-source code. That sounds like less overhead since fewer modules would be needed. I think the need to execute from ROM makes this tricky. It looks to me that it is easy to move from PCD to libs for the StatusCodeHandler since registration is basically `RscHandlerPpi->Register (SerialStatusCodeReportWorker);`. The issue I see is the ReportStatusCodeRouter publishes RscHandlerPpi after the PEIMs constructor has been called and if the PEIM. Given globals don’t work when running from ROM you would have to do something like publish a HOB in the library constructor and then have the GenericStatusCodePeiEntry() walk the HOBs and install the handlers. So I guess it is a little easier than I 1st thought when I started writing this mail, but it would require a new public API. Thanks, Andrew Fish Thanks, -- Brian J. Johnson Enterprise X86 Lab Hewlett Packard Enterprise hpe.com<x-msg://64/3D%22hpe.com%22> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#61551): https://edk2.groups.io/g/devel/message/61551 Mute This Topic: https://groups.io/mt/74953841/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
