* [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules @ 2020-06-18 7:01 Dandan Bi 2020-06-19 12:48 ` Laszlo Ersek 2020-06-19 17:29 ` Brian J. Johnson 0 siblings, 2 replies; 11+ messages in thread From: Dandan Bi @ 2020-06-18 7:01 UTC (permalink / raw) To: devel@edk2.groups.io, rfc@edk2.groups.io Cc: Dong, Eric, Ni, Ray, Wang, Jian J, Wu, Hao A, Tan, Ming, Bi, Dandan [-- Attachment #1: Type: text/plain, Size: 2439 bytes --] 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 from MdeModulePkg/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 [-- Attachment #2: Type: text/html, Size: 10785 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules 2020-06-18 7:01 [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules Dandan Bi @ 2020-06-19 12:48 ` Laszlo Ersek 2020-06-22 4:57 ` Dandan Bi 2020-06-19 17:29 ` Brian J. Johnson 1 sibling, 1 reply; 11+ messages in thread From: Laszlo Ersek @ 2020-06-19 12:48 UTC (permalink / raw) To: rfc, dandan.bi, devel@edk2.groups.io Cc: Dong, Eric, Ni, Ray, Wang, Jian J, Wu, Hao A, Tan, Ming On 06/18/20 09:01, 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 from MdeModulePkg/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 > ... > } So I assume you're going to remove PcdStatusCodeUseSerial and PcdStatusCodeUseMemory, and when converting the existent platforms, the new NULL class resolutions in the DSC files will reflect the specific PCD values used in those DSC files until then. Is that right? I'm OK with it. Thanks Laszlo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules 2020-06-19 12:48 ` Laszlo Ersek @ 2020-06-22 4:57 ` Dandan Bi 2020-06-22 15:02 ` Laszlo Ersek 0 siblings, 1 reply; 11+ messages in thread From: Dandan Bi @ 2020-06-22 4:57 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com, rfc@edk2.groups.io Cc: Dong, Eric, Ni, Ray, Wang, Jian J, Wu, Hao A, Tan, Ming, Bi, Dandan > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo > Ersek > Sent: Friday, June 19, 2020 8:48 PM > To: rfc@edk2.groups.io; Bi, Dandan <dandan.bi@intel.com>; > devel@edk2.groups.io > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, > Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Tan, > Ming <ming.tan@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 06/18/20 09:01, 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 from MdeModulePkg/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.in > f { > > <LibraryClasses> > > > NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/PeiMemorySt > ausCode > > NULL|HandlerLib.inf > > > NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/PeiSerialStatusC > o > > NULL|deHandlerLib.inf > > ... > > } > > > > > MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHan > dlerRuntimeDxe.inf { > > <LibraryClasses> > > > NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/RuntimeDxeM > emorySt > > NULL|ausCodeHandlerLib.inf > > > NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/RuntimeDxeSeri > alS > > NULL|tatusCodeHandlerLib.inf > > ... > > } > > > > > MdeModulePkg/Universal/StatusCodeHandler/Smm/StatusCodeHandlerSm > m.inf { > > <LibraryClasses> > > > > > NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/SmmMemory > StausCode > > HandlerLib.inf > > > NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/SmmSerialStatus > Co > > NULL|deHandlerLib.inf > > ... > > } > > So I assume you're going to remove PcdStatusCodeUseSerial and > PcdStatusCodeUseMemory, and when converting the existent platforms, > the new NULL class resolutions in the DSC files will reflect the specific PCD > values used in those DSC files until then. Is that right? > Thanks for pointing out the PCD part which I miss in this RFC. This commit https://github.com/tianocore/edk2/commit/45bc28172fbf38ac21e2592c07189b55f57695e3 have updated PcdStatusCodeUseSerial and PcdStatusCodeUseMemory type. We plan to keep PcdStatusCodeUseSerial and PcdStatusCodeUseMemory. Through NULL class resolutions in the DSC can make the code handler code included or not, then we still can control handler enable/disable through the PCD dynamically if the handler is included. What do you think of this? Thanks, Dandan > I'm OK with it. > > Thanks > Laszlo > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules 2020-06-22 4:57 ` Dandan Bi @ 2020-06-22 15:02 ` Laszlo Ersek 0 siblings, 0 replies; 11+ messages in thread From: Laszlo Ersek @ 2020-06-22 15:02 UTC (permalink / raw) To: Bi, Dandan, devel@edk2.groups.io, rfc@edk2.groups.io Cc: Dong, Eric, Ni, Ray, Wang, Jian J, Wu, Hao A, Tan, Ming On 06/22/20 06:57, Bi, Dandan wrote: >> -----Original Message----- >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo >> Ersek >> Sent: Friday, June 19, 2020 8:48 PM >> To: rfc@edk2.groups.io; Bi, Dandan <dandan.bi@intel.com>; >> devel@edk2.groups.io >> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, >> Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Tan, >> Ming <ming.tan@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 06/18/20 09:01, 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 from MdeModulePkg/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.in >> f { >>> <LibraryClasses> >>> >> NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/PeiMemorySt >> ausCode >>> NULL|HandlerLib.inf >>> >> NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/PeiSerialStatusC >> o >>> NULL|deHandlerLib.inf >>> ... >>> } >>> >>> >> MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHan >> dlerRuntimeDxe.inf { >>> <LibraryClasses> >>> >> NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/RuntimeDxeM >> emorySt >>> NULL|ausCodeHandlerLib.inf >>> >> NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/RuntimeDxeSeri >> alS >>> NULL|tatusCodeHandlerLib.inf >>> ... >>> } >>> >>> >> MdeModulePkg/Universal/StatusCodeHandler/Smm/StatusCodeHandlerSm >> m.inf { >>> <LibraryClasses> >>> >>> >> NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/SmmMemory >> StausCode >>> HandlerLib.inf >>> >> NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/SmmSerialStatus >> Co >>> NULL|deHandlerLib.inf >>> ... >>> } >> >> So I assume you're going to remove PcdStatusCodeUseSerial and >> PcdStatusCodeUseMemory, and when converting the existent platforms, >> the new NULL class resolutions in the DSC files will reflect the specific PCD >> values used in those DSC files until then. Is that right? >> > Thanks for pointing out the PCD part which I miss in this RFC. > This commit https://github.com/tianocore/edk2/commit/45bc28172fbf38ac21e2592c07189b55f57695e3 have updated PcdStatusCodeUseSerial and PcdStatusCodeUseMemory type. > We plan to keep PcdStatusCodeUseSerial and PcdStatusCodeUseMemory. Through NULL class resolutions in the DSC can make the code handler code included or not, then we still can control handler enable/disable through the PCD dynamically if the handler is included. > What do you think of this? Hm... OK. Thanks Laszlo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules 2020-06-18 7:01 [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules Dandan Bi 2020-06-19 12:48 ` Laszlo Ersek @ 2020-06-19 17:29 ` Brian J. Johnson 2020-06-19 18:03 ` Andrew Fish 2020-06-20 4:30 ` Dong, Eric 1 sibling, 2 replies; 11+ messages in thread From: Brian J. Johnson @ 2020-06-19 17:29 UTC (permalink / raw) To: devel, dandan.bi, rfc@edk2.groups.io Cc: Dong, Eric, Ni, Ray, Wang, Jian J, Wu, Hao A, Tan, Ming [-- Attachment #1: Type: text/plain, Size: 3415 bytes --] On 6/18/20 2:01 AM, Dandan Bi wrote: > > Hi All, > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2816 > <https://bugzilla.tianocore.org/show_bug.cgi?id=2816> > > We plan to separate two kinds of NULL class libraries for Memory and > serial handlers from *MdeModulePkg/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. Thanks, -- *Brian J. Johnson *Enterprise X86 Lab Hewlett Packard Enterprise *hpe.com* <3D"hpe.com"> [-- Attachment #2: Type: text/html, Size: 12839 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules 2020-06-19 17:29 ` Brian J. Johnson @ 2020-06-19 18:03 ` Andrew Fish 2020-06-22 7:27 ` Dandan Bi 2020-06-20 4:30 ` Dong, Eric 1 sibling, 1 reply; 11+ messages in thread From: Andrew Fish @ 2020-06-19 18:03 UTC (permalink / raw) To: edk2-devel-groups-io, brian.johnson Cc: Bi, Dandan, rfc@edk2.groups.io, Dong, Eric, Ni, Ray, Wang, Jian J, Wu, Hao A, Tan, Ming [-- Attachment #1: Type: text/plain, Size: 4161 bytes --] > On Jun 19, 2020, at 10:29 AM, Brian J. Johnson <brian.johnson@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 <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> > [-- Attachment #2: Type: text/html, Size: 14975 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules 2020-06-19 18:03 ` Andrew Fish @ 2020-06-22 7:27 ` Dandan Bi 2020-06-24 20:24 ` Brian J. Johnson 0 siblings, 1 reply; 11+ messages in thread From: Dandan Bi @ 2020-06-22 7:27 UTC (permalink / raw) To: Andrew Fish, edk2-devel-groups-io, brian.johnson@hpe.com Cc: rfc@edk2.groups.io, Dong, Eric, Ni, Ray, Wang, Jian J, Wu, Hao A, Tan, Ming, Laszlo Ersek, Bi, Dandan [-- Attachment #1: Type: text/plain, Size: 5821 bytes --] 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 <afish@apple.com> Sent: Saturday, June 20, 2020 2:04 AM To: edk2-devel-groups-io <devel@edk2.groups.io>; brian.johnson@hpe.com Cc: Bi, Dandan <dandan.bi@intel.com>; rfc@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Tan, Ming <ming.tan@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.johnson@hpe.com<mailto:brian.johnson@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> [-- Attachment #2: Type: text/html, Size: 20093 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules 2020-06-22 7:27 ` Dandan Bi @ 2020-06-24 20:24 ` Brian J. Johnson 2020-06-25 15:41 ` Dong, Eric 0 siblings, 1 reply; 11+ messages in thread From: Brian J. Johnson @ 2020-06-24 20:24 UTC (permalink / raw) To: Bi, Dandan, Andrew Fish, edk2-devel-groups-io Cc: rfc@edk2.groups.io, Dong, Eric, Ni, Ray, Wang, Jian J, Wu, Hao A, Tan, Ming, Laszlo Ersek [-- Attachment #1: Type: text/plain, Size: 9671 bytes --] Dandan, The Status Code Protocol/PPI is the high-level interface which is being implemented. The ReportStatusCodeRouter module implements this in terms of the ReportStatusCodeHandler Protocol/PPI. That allows multiple ReportStatusCodeHandler modules to be used at once, so they can each react to different types of status codes, or report them through multiple channels. That sort of multiplexing seems like a useful feature. Now we're considering adding a mechanism which allows registering status code handlers via NULL libraries, rather than via the protocol/PPI. That sounds like exactly what ReportStatusCodeRouter is intended for. It wouldn't really change its scope, it would just make it more flexible. Adding this feature via a StatusCodeHandler module wouldn't improve modularity, it would just add complexity. As an OEM, adding a custom handler would look the same to me either way: I would have to add the NULL class library to a MdeModulePkg driver's entry in my .dsc file. It doesn't matter to me whether it's the ReportStatusCodeRouter or StatusCodeHandler module. And if I can do it in ReportStatusCodeRouter, then I don't need to include any StatusCodeHandler modules in the build at all. That saves code space and reduces the number of modules in the APRIORI list, which are both good things. ReportStatusCodeRouterPei already has to track registered handlers in PEI when running from ROM (it uses a HOB.) Tracking handlers from NULL libraries wouldn't be any harder. In fact, it looks like it could just export the Register() function to the NULL libraries, and they could call it in their constructors. I think using NULL libraries for status code handlers is a great idea. I'd just like to take that opportunity to reduce the complexity of the overall status code stack while we're at it. Thanks, *Brian J. Johnson *Enterprise X86 Lab Hewlett Packard Enterprise ------------------------------------------------------------------------ *From:* Bi, Dandan [mailto:dandan.bi@intel.com] *Sent:* Monday, June 22, 2020, 2:27 AM *To:* Andrew Fish <afish@apple.com>, edk2-devel-groups-io <devel@edk2.groups.io>, brian.johnson@hpe.com <brian.johnson@hpe.com> *Cc:* rfc@edk2.groups.io <rfc@edk2.groups.io>, Dong, Eric <eric.dong@intel.com>, Ni, Ray <ray.ni@intel.com>, Wang, Jian J <jian.j.wang@intel.com>, Wu, Hao A <hao.a.wu@intel.com>, Tan, Ming <ming.tan@intel.com>, Laszlo Ersek <lersek@redhat.com>, Bi, Dandan <dandan.bi@intel.com> *Subject:* [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules > 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. > > 2. 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. > > 3. 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 <afish@apple.com> > *Sent:* Saturday, June 20, 2020 2:04 AM > *To:* edk2-devel-groups-io <devel@edk2.groups.io>; brian.johnson@hpe.com > *Cc:* Bi, Dandan <dandan.bi@intel.com>; rfc@edk2.groups.io; Dong, Eric > <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Tan, Ming > <ming.tan@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.johnson@hpe.com <mailto:brian.johnson@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 > <https://bugzilla.tianocore.org/show_bug.cgi?id=2816> > > We plan to separate two kinds of NULL class libraries for > Memory and serial handlers > from*MdeModulePkg/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> > > > - [-- Attachment #2: Type: text/html, Size: 33661 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules 2020-06-24 20:24 ` Brian J. Johnson @ 2020-06-25 15:41 ` Dong, Eric 2020-06-25 22:26 ` Brian J. Johnson 0 siblings, 1 reply; 11+ messages in thread From: Dong, Eric @ 2020-06-25 15:41 UTC (permalink / raw) To: Brian J. Johnson, Bi, Dandan, Andrew Fish, edk2-devel-groups-io Cc: rfc@edk2.groups.io, Ni, Ray, Wang, Jian J, Wu, Hao A, Tan, Ming, Laszlo Ersek [-- Attachment #1: Type: text/plain, Size: 10243 bytes --] Hi Brian, In this new design, we still use register status code handler Protocol/Ppi to register the handler logic. We just want to change the StatusCodeHandler driver. We try to split the register logic to NULL library to make the code more modularity. We already created sample library in Edk2-Platforms repo https://github.com/tianocore/edk2-platforms/tree/master/Features/Intel/Debugging/PostCodeDebugFeaturePkg/Library/PostCodeStatusCodeHandlerLib. You can check this code to understand more about what we want to do. Thanks, Eric From: Brian J. Johnson <brian.johnson@hpe.com> Sent: Thursday, June 25, 2020 4:25 AM To: Bi, Dandan <dandan.bi@intel.com>; Andrew Fish <afish@apple.com>; edk2-devel-groups-io <devel@edk2.groups.io> Cc: rfc@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Tan, Ming <ming.tan@intel.com>; Laszlo Ersek <lersek@redhat.com> Subject: Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules Dandan, The Status Code Protocol/PPI is the high-level interface which is being implemented. The ReportStatusCodeRouter module implements this in terms of the ReportStatusCodeHandler Protocol/PPI. That allows multiple ReportStatusCodeHandler modules to be used at once, so they can each react to different types of status codes, or report them through multiple channels. That sort of multiplexing seems like a useful feature. Now we're considering adding a mechanism which allows registering status code handlers via NULL libraries, rather than via the protocol/PPI. That sounds like exactly what ReportStatusCodeRouter is intended for. It wouldn't really change its scope, it would just make it more flexible. Adding this feature via a StatusCodeHandler module wouldn't improve modularity, it would just add complexity. As an OEM, adding a custom handler would look the same to me either way: I would have to add the NULL class library to a MdeModulePkg driver's entry in my .dsc file. It doesn't matter to me whether it's the ReportStatusCodeRouter or StatusCodeHandler module. And if I can do it in ReportStatusCodeRouter, then I don't need to include any StatusCodeHandler modules in the build at all. That saves code space and reduces the number of modules in the APRIORI list, which are both good things. ReportStatusCodeRouterPei already has to track registered handlers in PEI when running from ROM (it uses a HOB.) Tracking handlers from NULL libraries wouldn't be any harder. In fact, it looks like it could just export the Register() function to the NULL libraries, and they could call it in their constructors. I think using NULL libraries for status code handlers is a great idea. I'd just like to take that opportunity to reduce the complexity of the overall status code stack while we're at it. Thanks, Brian J. Johnson Enterprise X86 Lab Hewlett Packard Enterprise ________________________________ From: Bi, Dandan [mailto:dandan.bi@intel.com] Sent: Monday, June 22, 2020, 2:27 AM To: Andrew Fish <afish@apple.com><mailto:afish@apple.com>, edk2-devel-groups-io <devel@edk2.groups.io><mailto:devel@edk2.groups.io>, brian.johnson@hpe.com<mailto:brian.johnson@hpe.com> <brian.johnson@hpe.com><mailto:brian.johnson@hpe.com> Cc: rfc@edk2.groups.io<mailto:rfc@edk2.groups.io> <rfc@edk2.groups.io><mailto:rfc@edk2.groups.io>, Dong, Eric <eric.dong@intel.com><mailto:eric.dong@intel.com>, Ni, Ray <ray.ni@intel.com><mailto:ray.ni@intel.com>, Wang, Jian J <jian.j.wang@intel.com><mailto:jian.j.wang@intel.com>, Wu, Hao A <hao.a.wu@intel.com><mailto:hao.a.wu@intel.com>, Tan, Ming <ming.tan@intel.com><mailto:ming.tan@intel.com>, Laszlo Ersek <lersek@redhat.com><mailto:lersek@redhat.com>, Bi, Dandan <dandan.bi@intel.com><mailto:dandan.bi@intel.com> Subject: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules 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 <afish@apple.com><mailto:afish@apple.com> Sent: Saturday, June 20, 2020 2:04 AM To: edk2-devel-groups-io <devel@edk2.groups.io><mailto:devel@edk2.groups.io>; brian.johnson@hpe.com<mailto:brian.johnson@hpe.com> Cc: Bi, Dandan <dandan.bi@intel.com><mailto:dandan.bi@intel.com>; rfc@edk2.groups.io<mailto:rfc@edk2.groups.io>; Dong, Eric <eric.dong@intel.com><mailto:eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com><mailto:ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com><mailto:jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com><mailto:hao.a.wu@intel.com>; Tan, Ming <ming.tan@intel.com><mailto:ming.tan@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.johnson@hpe.com<mailto:brian.johnson@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> - [-- Attachment #2: Type: text/html, Size: 27302 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules 2020-06-25 15:41 ` Dong, Eric @ 2020-06-25 22:26 ` Brian J. Johnson 0 siblings, 0 replies; 11+ messages in thread From: Brian J. Johnson @ 2020-06-25 22:26 UTC (permalink / raw) To: Dong, Eric, Bi, Dandan, Andrew Fish, edk2-devel-groups-io Cc: rfc@edk2.groups.io, Ni, Ray, Wang, Jian J, Wu, Hao A, Tan, Ming, Laszlo Ersek [-- Attachment #1: Type: text/plain, Size: 15250 bytes --] Thanks for the link. I agree that this change will make the StatusCodeHandler driver more modular, and is a step in the right direction. But I think it could go further, with almost no additional work, and simplify the overall Status Code mechanism, not just the StatusCodeHandler driver. Currently, the StatusCodeHandler driver entry routines run some initialization code, register callbacks (eg. for ExitBootServices and SetVirtualAddressMap), and call the RscHandler PPI/Protocol to register the worker routines. If I'm understanding the proposal correctly, all that code will be moved to the individual NULL libraries, since any particular library may or may not need any of it. Then the StatusCodeHandler modules will be left with no code of their own at all: they will only be wrappers for the NULL libraries. Their entry routines will do nothing except return EFI_SUCCESS! (1) It seems strange and wasteful to keep around empty modules like this. So I'm suggesting adding the NULL libraries to the StatusCodeRouter modules instead. They would need to export the protocol/PPI routines to the NULL libraries via a header file, so they could call them directly instead of looking up the protocol/PPI. But that's a minor change. Then you could remove the empty StatusCodeHandler modules entirely. The advantage would be that there would be fewer modules in the build, simplifying the .dsc and .fdf files slightly. It would also reduce code size a bit by sharing common library routines, such as BaseLib, with the StatusCodeRouter modules. If those don't seem like worthwhile advantages, that's OK with me. I don't want to belabor the point, or impede progress. If others are OK with the proposal as it stands, then I am too. Thanks, *Brian J. Johnson *Enterprise X86 Lab Hewlett Packard Enterprise (1) The StatusCodeHandlerRuntimeDxe driver also handles PcdStatusCodeReplayIn as part of its entry code. That code would probably have to stay in a separate module rather than being linked to StatusCodeRouter as a NULL library. That way it could be dispatched after the ReportStatusCode protocol is available. ------------------------------------------------------------------------ *From:* Dong, Eric [mailto:eric.dong@intel.com] *Sent:* Thursday, June 25, 2020, 10:41 AM *To:* Brian J. Johnson <brian.johnson@hpe.com>, Bi, Dandan <dandan.bi@intel.com>, Andrew Fish <afish@apple.com>, edk2-devel-groups-io <devel@edk2.groups.io> *Cc:* rfc@edk2.groups.io <rfc@edk2.groups.io>, Ni, Ray <ray.ni@intel.com>, Wang, Jian J <jian.j.wang@intel.com>, Wu, Hao A <hao.a.wu@intel.com>, Tan, Ming <ming.tan@intel.com>, Laszlo Ersek <lersek@redhat.com> *Subject:* [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules > Hi Brian, > > In this new design, we still use register status code handler > Protocol/Ppi to register the handler logic. We just want to change the > StatusCodeHandler driver. We try to split the register logic to NULL > library to make the code more modularity. We already created sample > library in Edk2-Platforms repo > https://github.com/tianocore/edk2-platforms/tree/master/Features/Intel/Debugging/PostCodeDebugFeaturePkg/Library/PostCodeStatusCodeHandlerLib. > You can check this code to understand more about what we want to do. > > Thanks, > > Eric > > *From:* Brian J. Johnson <brian.johnson@hpe.com> > *Sent:* Thursday, June 25, 2020 4:25 AM > *To:* Bi, Dandan <dandan.bi@intel.com>; Andrew Fish <afish@apple.com>; > edk2-devel-groups-io <devel@edk2.groups.io> > *Cc:* rfc@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Ni, Ray > <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A > <hao.a.wu@intel.com>; Tan, Ming <ming.tan@intel.com>; Laszlo Ersek > <lersek@redhat.com> > *Subject:* Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: > Separate NULL class libraries for Memory and serial handlers from > MdeModulePkg/Universal/StatusCodeHandler modules > > Dandan, > > The Status Code Protocol/PPI is the high-level interface which is > being implemented. The ReportStatusCodeRouter module implements this > in terms of the ReportStatusCodeHandler Protocol/PPI. That allows > multiple ReportStatusCodeHandler modules to be used at once, so they > can each react to different types of status codes, or report them > through multiple channels. That sort of multiplexing seems like a > useful feature. > > Now we're considering adding a mechanism which allows registering > status code handlers via NULL libraries, rather than via the > protocol/PPI. That sounds like exactly what ReportStatusCodeRouter is > intended for. It wouldn't really change its scope, it would just make > it more flexible. Adding this feature via a StatusCodeHandler module > wouldn't improve modularity, it would just add complexity. As an OEM, > adding a custom handler would look the same to me either way: I would > have to add the NULL class library to a MdeModulePkg driver's entry in > my .dsc file. It doesn't matter to me whether it's the > ReportStatusCodeRouter or StatusCodeHandler module. And if I can do > it in ReportStatusCodeRouter, then I don't need to include any > StatusCodeHandler modules in the build at all. That saves code space > and reduces the number of modules in the APRIORI list, which are both > good things. > > ReportStatusCodeRouterPei already has to track registered handlers in > PEI when running from ROM (it uses a HOB.) Tracking handlers from > NULL libraries wouldn't be any harder. In fact, it looks like it > could just export the Register() function to the NULL libraries, and > they could call it in their constructors. > > I think using NULL libraries for status code handlers is a great > idea. I'd just like to take that opportunity to reduce the complexity > of the overall status code stack while we're at it. > > Thanks, > > *Brian J. Johnson > *Enterprise X86 Lab > > Hewlett Packard Enterprise > > ------------------------------------------------------------------------ > > *From:* Bi, Dandan [mailto:dandan.bi@intel.com] > > *Sent:* Monday, June 22, 2020, 2:27 AM > > *To:* Andrew Fish <afish@apple.com> <mailto:afish@apple.com>, > edk2-devel-groups-io <devel@edk2.groups.io> > <mailto:devel@edk2.groups.io>, brian.johnson@hpe.com > <mailto:brian.johnson@hpe.com> <brian.johnson@hpe.com> > <mailto:brian.johnson@hpe.com> > > *Cc:* rfc@edk2.groups.io <mailto:rfc@edk2.groups.io> > <rfc@edk2.groups.io> <mailto:rfc@edk2.groups.io>, Dong, Eric > <eric.dong@intel.com> <mailto:eric.dong@intel.com>, Ni, Ray > <ray.ni@intel.com> <mailto:ray.ni@intel.com>, Wang, Jian J > <jian.j.wang@intel.com> <mailto:jian.j.wang@intel.com>, Wu, Hao A > <hao.a.wu@intel.com> <mailto:hao.a.wu@intel.com>, Tan, Ming > <ming.tan@intel.com> <mailto:ming.tan@intel.com>, Laszlo Ersek > <lersek@redhat.com> <mailto:lersek@redhat.com>, Bi, Dandan > <dandan.bi@intel.com> <mailto:dandan.bi@intel.com> > > *Subject:* [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: > Separate NULL class libraries for Memory and serial handlers from > MdeModulePkg/Universal/StatusCodeHandler modules > > 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. > > 2. 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. > > 3. 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 <afish@apple.com> <mailto:afish@apple.com> > *Sent:* Saturday, June 20, 2020 2:04 AM > *To:* edk2-devel-groups-io <devel@edk2.groups.io> > <mailto:devel@edk2.groups.io>; brian.johnson@hpe.com > <mailto:brian.johnson@hpe.com> > *Cc:* Bi, Dandan <dandan.bi@intel.com> > <mailto:dandan.bi@intel.com>; rfc@edk2.groups.io > <mailto:rfc@edk2.groups.io>; Dong, Eric <eric.dong@intel.com> > <mailto:eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com> > <mailto:ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com> > <mailto:jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com> > <mailto:hao.a.wu@intel.com>; Tan, Ming <ming.tan@intel.com> > <mailto:ming.tan@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.johnson@hpe.com <mailto:brian.johnson@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 > <https://bugzilla.tianocore.org/show_bug.cgi?id=2816> > > We plan to separate two kinds of NULL class libraries for > Memory and serial handlers > from*MdeModulePkg/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> > > > > - > [-- Attachment #2: Type: text/html, Size: 45978 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules 2020-06-19 17:29 ` Brian J. Johnson 2020-06-19 18:03 ` Andrew Fish @ 2020-06-20 4:30 ` Dong, Eric 1 sibling, 0 replies; 11+ messages in thread From: Dong, Eric @ 2020-06-20 4:30 UTC (permalink / raw) To: devel@edk2.groups.io, brian.johnson@hpe.com, Bi, Dandan, rfc@edk2.groups.io Cc: Ni, Ray, Wang, Jian J, Wu, Hao A, Tan, Ming [-- Attachment #1: Type: text/plain, Size: 4193 bytes --] From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Brian J. Johnson Sent: Saturday, June 20, 2020 1:29 AM To: devel@edk2.groups.io; Bi, Dandan <dandan.bi@intel.com>; rfc@edk2.groups.io Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Tan, Ming <ming.tan@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 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 from MdeModulePkg/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 Hi Brain, You are right. Current design truly has a lot of layers. The ReportStatusCodeRouter module provides the register logic and maintain the registered status code handlers. Now the platform may have more than one of drivers used to register the status code handler. This RFC used to resolve the platform has more than one status code handler drivers' issue. We expect the platform only need one wrapper driver in MdeModulePkg to let the status code handler library to register its handler on it. Thanks, Eric Thanks, -- Brian J. Johnson Enterprise X86 Lab Hewlett Packard Enterprise hpe.com<3D%22hpe.com%22> [-- Attachment #2: Type: text/html, Size: 11670 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-06-25 22:26 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-06-18 7:01 [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules Dandan Bi 2020-06-19 12:48 ` Laszlo Ersek 2020-06-22 4:57 ` Dandan Bi 2020-06-22 15:02 ` Laszlo Ersek 2020-06-19 17:29 ` Brian J. Johnson 2020-06-19 18:03 ` Andrew Fish 2020-06-22 7:27 ` Dandan Bi 2020-06-24 20:24 ` Brian J. Johnson 2020-06-25 15:41 ` Dong, Eric 2020-06-25 22:26 ` Brian J. Johnson 2020-06-20 4:30 ` Dong, Eric
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox