From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0b-002e3701.pphosted.com (mx0b-002e3701.pphosted.com [148.163.143.35]) by mx.groups.io with SMTP id smtpd.web11.1492.1593030291833100297 for ; Wed, 24 Jun 2020 13:24:52 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: hpe.com, ip: 148.163.143.35, mailfrom: prvs=0444cc5795=brian.johnson@hpe.com) Received: from pps.filterd (m0150244.ppops.net [127.0.0.1]) by mx0b-002e3701.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 05OKNjeo025572; Wed, 24 Jun 2020 20:24:49 GMT Received: from g4t3427.houston.hpe.com (g4t3427.houston.hpe.com [15.241.140.73]) by mx0b-002e3701.pphosted.com with ESMTP id 31ux3277hc-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 24 Jun 2020 20:24:48 +0000 Received: from g4t3433.houston.hpecorp.net (g4t3433.houston.hpecorp.net [16.208.49.245]) by g4t3427.houston.hpe.com (Postfix) with ESMTP id 4FA4E57; Wed, 24 Jun 2020 20:24:48 +0000 (UTC) Received: from [16.214.35.118] (unknown [16.214.35.118]) by g4t3433.houston.hpecorp.net (Postfix) with ESMTP id 64F0246; Wed, 24 Jun 2020 20:24:45 +0000 (UTC) Subject: Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules 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 References: From: "Brian J. Johnson" Message-ID: <17a085f3-424b-ad6f-b1c1-5dc6834c4e4a@hpe.com> Date: Wed, 24 Jun 2020 15:24:44 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 In-Reply-To: X-Proofpoint-UnRewURL: 14 URL's were un-rewritten MIME-Version: 1.0 X-HPE-SCL: -1 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.216,18.0.687 definitions=2020-06-24_16:2020-06-24,2020-06-24 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 cotscore=-2147483648 suspectscore=0 clxscore=1015 priorityscore=1501 bulkscore=0 mlxscore=0 malwarescore=0 mlxlogscore=999 impostorscore=0 spamscore=0 lowpriorityscore=0 phishscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2004280000 definitions=main-2006240132 Content-Type: multipart/alternative; boundary="------------3C865B7911B4194121207C65" Content-Language: en-US --------------3C865B7911B4194121207C65 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Dandan, The Status Code Protocol/PPI is the high-level interface which is being=20 implemented.=C2=A0 The ReportStatusCodeRouter module implements this in te= rms=20 of the ReportStatusCodeHandler Protocol/PPI.=C2=A0 That allows multiple=20 ReportStatusCodeHandler modules to be used at once, so they can each=20 react to different types of status codes, or report them through=20 multiple channels.=C2=A0 That sort of multiplexing seems like a useful fea= ture. Now we're considering adding a mechanism which allows registering status= =20 code handlers via NULL libraries, rather than via the protocol/PPI.=C2=A0= =20 That sounds like exactly what ReportStatusCodeRouter is intended for.=C2= =A0=20 It wouldn't really change its scope, it would just make it more=20 flexible.=C2=A0 Adding this feature via a StatusCodeHandler module wouldn'= t=20 improve modularity, it would just add complexity.=C2=A0 As an OEM, adding = a=20 custom handler would look the same to me either way:=C2=A0 I would have to= = =20 add the NULL class library to a MdeModulePkg driver's entry in my .dsc=20 file.=C2=A0 It doesn't matter to me whether it's the ReportStatusCodeRoute= r=20 or StatusCodeHandler module.=C2=A0 And if I can do it in=20 ReportStatusCodeRouter, then I don't need to include any=20 StatusCodeHandler modules in the build at all.=C2=A0 That saves code space= = =20 and reduces the number of modules in the APRIORI list, which are both=20 good things. ReportStatusCodeRouterPei already has to track registered handlers in=20 PEI when running from ROM (it uses a HOB.)=C2=A0 Tracking handlers from NU= LL=20 libraries wouldn't be any harder.=C2=A0 In fact, it looks like it could ju= st=20 export the Register() function to the NULL libraries, and they could=20 call it in their constructors. I think using NULL libraries for status code handlers is a great idea.=C2= =A0=20 I'd just like to take that opportunity to reduce the complexity of the=20 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 , edk2-devel-groups-io=20 , brian.johnson@hpe.com *Cc:* rfc@edk2.groups.io , Dong, Eric=20 , Ni, Ray , Wang, Jian J=20 , Wu, Hao A , Tan, Ming=20 , Laszlo Ersek , Bi, Dandan=20 *Subject:* [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler:=20 Separate NULL class libraries for Memory and serial handlers from=20 MdeModulePkg/Universal/StatusCodeHandler modules > Hi Brian, > > Personally, I prefer to add the NULL class Library to=20 > 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=20 > Code Protocol/PPI and Report Status Code Handler Protocol/PPI, and=20 > StatusCodeHandler modules are responsible for producing handlers=20 > (Handlers can be provided by NULL class Libraries in this RFC). > > =C2=A0So, that=E2=80=99s why I don=E2=80=99t want to add the NULL class = Library to=20 > ReportStatusCodeRouter modules directly, which change the=20 > 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 > *Sent:* Saturday, June 20, 2020 2:04 AM > *To:* edk2-devel-groups-io ; brian.johnson@hpe.com > *Cc:* Bi, Dandan ; rfc@edk2.groups.io; Dong, Eric= =20 > ; Ni, Ray ; Wang, Jian J=20 > ; Wu, Hao A ; Tan, Ming=20 > > *Subject:* Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler:= =20 > Separate NULL class libraries for Memory and serial handlers from=20 > MdeModulePkg/Universal/StatusCodeHandler modules > > > > On Jun 19, 2020, at 10:29 AM, Brian J. Johnson > wrote: > > On 6/18/20 2:01 AM, Dandan Bi wrote: > > Hi All, > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D2816 > > > We plan to separate two kinds of NULL class libraries for > Memory and serial handlers > from*MdeModulePkg/Universal/StatusCodeHandler/=E2=80=A6/ > 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***SerialStatusCodeHandlerL= ib*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=E2=80=99t impact any platform. > > After this separation, StatusCodeHandler module usage will > like below, and it=E2=80=99s also very flexible for platform to = cover > more handler libraries to meet their requirements. > > MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPe= i.inf > { > > > > NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/PeiMemorySta= usCodeHandlerLib.inf > > NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/PeiSerialSt= atusCodeHandlerLib.inf > > =E2=80=A6 > > } > > MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHa= ndlerRuntimeDxe.inf > { > > > > NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/RuntimeDxeMe= moryStausCodeHandlerLib.inf > > NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/RuntimeDxeS= erialStatusCodeHandlerLib.inf > > =E2=80=A6 > > } > > MdeModulePkg/Universal/StatusCodeHandler/Smm/StatusCodeHandlerSm= m.inf > { > > > > =C2=A0NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/SmmMem= oryStausCodeHandlerLib.inf > > NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/SmmSerialSt= atusCodeHandlerLib.inf > > =E2=80=A6 > > } > > Thanks, > > Dandan > > Dandan, > > We'll have a lot of layers of indirection....=C2=A0 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?=C2=A0 Then platforms could omit the > StatusCodeHandler modules if they're only using the open-source > code.=C2=A0 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=20 > StatusCodeHandler since registration is basically=20 > `RscHandlerPpi->Register (SerialStatusCodeReportWorker);`. The issue I= =20 > see is the ReportStatusCodeRouter publishes=C2=A0RscHandlerPpi after the= = =20 > PEIMs constructor has been called and if the PEIM. Given globals don=E2= =80=99t=20 > work when running from ROM you would have to do something like publish= =20 > a HOB in the library constructor and then have=20 > the=C2=A0GenericStatusCodePeiEntry() walk the HOBs and install the=20 > handlers. So I guess it is a little easier than I 1st thought when I=20 > 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* > > =20 > - --------------3C865B7911B4194121207C65 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
Dandan,

The Status Code Protocol/PPI is the high-level interface which is being implemented.=C2=A0 The ReportStatusCodeRouter modul= e implements this in terms of the ReportStatusCodeHandler Protocol/PPI.=C2=A0 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.=C2=A0 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.=C2=A0 That sounds like exactly what ReportStatusCodeRouter is intended for.=C2=A0 It wouldn't really change its scope, it would just make it more flexible.=C2=A0 Addin= g this feature via a StatusCodeHandler module wouldn't improve modularity, it would just add complexity.=C2=A0 As an OEM, adding = a custom handler would look the same to me either way:=C2=A0 I would have to add the NULL class library to a MdeModulePkg driver's entry in my .dsc file.=C2=A0 It doesn't matter to me whether it's = the ReportStatusCodeRouter or StatusCodeHandler module.=C2=A0 And if I can do it in ReportStatusCodeRouter, then I don't need to include any StatusCodeHandler modules in the build at all.=C2=A0 T= hat 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.)=C2=A0 Track= ing handlers from NULL libraries wouldn't be any harder.=C2=A0 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.=C2=A0 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
Subject: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

Hi Brian,

=C2=A0

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).

=C2=A0So, that=E2=80=99s why I don=E2=80=99t want to add the NULL class Li= brary to ReportStatusCodeRouter modules directly, which change the functionality scope of existing modules.

=C2= = =A0

  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.

=C2=A0

  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.

=C2=A0

=C2=A0

=C2=A0

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<= br> 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

=C2=A0

=C2=A0



On Jun 19, 2020, at 10:29 AM, Brian J. Johnson <brian.johnson@hpe.comwrote:

=C2=A0

On 6/18/20 2:01 AM, Dandan Bi wrote:<= /p>

Hi All,

=C2=A0

=C2=A0

We plan to separate two kinds of NULL class libraries for Memory and serial handlers fromMdeModulePkg/Universal/StatusCodeHan= dler/=E2=80=A6/ StatusCodeHandlerPei/RuntimeDxe/Smm=C2=A0modul= es.

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.

=C2=A0

We plan to add new separated NULL class library=C2=A0Me= moryStausCodeHandlerLib=C2=A0and=C2=A0SerialStatusCodeHandlerLib=C2=A0with different phase implementation into=C2=A0Md= eModulePkg\Library\=C2=A0direc= tory.

The main tree structure may like below:

MdeModulePkg\Library=

|------MemoryStausCodeHandle= rLib

|------|------ PeiMemoryStausCodeHandlerLib.inf

|------|------ RuntimeDxeMemoryStatusCodeHandlerLib.inf<= /p>

|------|------ SmmMemoryStausCodeHandlerLib.inf

|------SerialStatusCodeHandl= erLib

|------|------ PeiSerialStatusCodeHandlerLib.inf

|------|------ RuntimeDxeSerialStatusCodeHandlerLib.inf<= /p>

|------|------ SmmSerialStatusCodeHandlerLib.inf

=C2=A0

=C2=A0

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=E2=80=99t impact any platform.

After this separation, StatusCodeHandler module usage will like below, and it=E2=80=99s also very flexible for platform to = cover more handler libraries to meet their requirements.

MdeModulePkg/Universal/StatusCo= deHandler/Pei/StatusCodeHandlerPei.inf {

=C2=A0=C2=A0<L= ibraryClasses>

NU= LL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/PeiMemoryStausCodeHandler= Lib.inf

NU= LL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/PeiSerialStatusCodeHandl= erLib.inf

=C2=A0=C2=A0=C2=A0=C2=A0=E2= =80=A6

}

=C2=A0

MdeModulePkg/Universal/StatusCo= deHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf=C2=A0 {

=C2=A0=C2=A0<L= ibraryClasses>

NU= LL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/RuntimeDxeMemoryStausCode= HandlerLib.inf

NU= LL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/RuntimeDxeSerialStatusCo= deHandlerLib.inf

=C2=A0=C2=A0=C2=A0=C2=A0=E2= =80=A6

}

=C2=A0

MdeModulePkg/Universal/StatusCo= deHandler/Smm/StatusCodeHandlerSmm.inf {

=C2=A0=C2=A0<L= ibraryClasses>

=C2=A0=C2=A0 =C2=A0NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/SmmMemoryStausCo= deHandlerLib.inf

NU= LL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/SmmSerialStatusCodeHandl= erLib.inf

=C2=A0=C2=A0=C2=A0=C2=A0=E2= =80=A6

}

=C2=A0

=C2=A0

Thanks,

Dandan

=C2=A0

Dandan,

We'll have a lot of layers of indirection....=C2=A0 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?=C2=A0 Then platforms could omit the StatusCodeHandler modules if they're only using the open-source code.=C2=A0 That sounds like less overhead since fewer modules would be needed.

=C2=A0

=C2=A0

I think the need to execute from ROM makes this tricky.=C2=A0

=C2=A0

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=C2=A0RscHandlerPpi after th= e PEIMs constructor has been called and if the PEIM. Given globals don=E2=80=99t work when running from ROM you would h= ave to do something like publish a HOB in the library constructor and then have the=C2=A0GenericStatusCodePeiEntry() 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.=C2=A0

=C2=A0

Thanks,

=C2=A0

Andrew Fish

Thanks,

--=C2=A0=

Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise

hpe.com=

=C2=A0


-
--------------3C865B7911B4194121207C65--