It is good practice for me to wait at least one day for more comments to the patches(In fact, I did follow it for most of the patches, but not for some urgent cases, it is my fault). Your words are always helpful for us(I means at least for me) to do community work, thanks for that.
BTW: I have edited the commit log of SVN R17659 to include more helpful information. Thanks, Star -----Original Message----- From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Tuesday, June 23, 2015 12:24 AM To: Zeng, Star Cc: edk2-devel list; Doran, Mark; Yao, Jiewen; Justen, Jordan L; Peter Jones; Zimmer, Vincent Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg PiSmmCore: Unregister end of dxe notification in SmmReadyToLock. On 06/18/15 16:44, Zeng, Star wrote: > Thanks for your reminder. > > To avoid potential malicious code to trigger end of dxe SMI handler many > times, then install SmmEndOfDxe protocol many times to exhaust SMRAM space. > The SMI handler needs to be unregistered at proper point. > PI spec says SmmReadyToLock protocol should be installed immediately after > EFI_END_OF_DXE_EVENT_GROUP_GUID with no intervening modules dispatched. > So we can register the end of dxe notification in SmmReadyToLock safely. When I asked for the more elaborate explanation of your patch, I didn't just ask for it to be sent to the list. The commit message on the patch should have been updated, and the patch should have been preferably reposted, before committing the patch. I wrote (but you can see it in the context at the end too), emphasis added now: Please provide a detailed description *in the commit message* about the bug that this patch fixes. I just pulled the repo, and SVN r17659 only has the original (very unhelpful) commit message. Now whoever looks at the patch in the repo might ask the same question as I did, and won't find your answer (which was otherwise very helpful). This is especially bad because your patch may have security impact, and security characteristics of patches must be spelled out *loud and clear*. Public patch review makes absolutely no sense if you just go ahead and commit stuff without addressing review notes. The time difference that I can see between the public posting: * Thu Jun 18 05:40:35 UTC 2015 and the commit timestamp * Thu Jun 18 09:27:42 UTC 2015 indicates you left fewer than 4 hours for edk2-devel subscribers to comment on your patch, before committing it. I was not aware, but when I asked for the explanation: * Thu Jun 18 14:04:14 UTC 2015 about 8-9 hours after your posting, the patch had already been committed. This is not how a "good open source citizen" project works. I can see you got Jiewen's Reviewed-by three minutes after posting the patch, but many subscribers are not in the timezone you two are in, and you should give them too a chance to comment. For example, I have a somewhat urgent patch on the list [edk2] [PATCH] OvmfPkg: PlatformPei: set SMBIOS entry point version dynamically I could have committed it immediately to SVN after getting Jordan's Reviewed-by, but I first pinged Gabriel too on IRC (because I knew he might be interested in it), after Cc'ing him initially, and I decided to wait at least one day for his feedback, while the Earth revolves once or so. I just got lucky that he responded very soon and now I can go ahead with both tags. Maybe it has not been clear that I care very much about SMM. I hope it's clear now. (I would have posted an 50+ part SMM-for-OVMF patchset more than a month ago, had it not been blocked by an embargoed security issue I reported for another Intel product.) Thus, when posting SMM patches in the future, please Cc me, and await my feedback. If I cannot provide timely feedback, I *always* say so, either publicly or in private, and then your work won't be stalled behind my queue. The open source development process means that you are willing to delay your own *internal* assignments for the sake of the *community*. I have been delaying my SMM work at Red Hat for more than five weeks, missing a minor RHEL-7 release, (partly) out of respect for Intel's security schedule. Please do reciprocate. Laszlo > > Thanks, > Star > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Thursday, June 18, 2015 10:04 PM > To: Zeng, Star > Cc: edk2-devel@lists.sourceforge.net > Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg PiSmmCore: Unregister end of dxe > notification in SmmReadyToLock. > > On 06/18/15 07:40, Star Zeng wrote: >> Cc: Jiewen Yao <jiewen....@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Star Zeng <star.z...@intel.com> >> --- >> MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 2 +- >> MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c >> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c >> index 852f8b9..c91f149 100644 >> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c >> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c >> @@ -80,7 +80,7 @@ SMM_CORE_SMI_HANDLERS mSmmCoreSmiHandlers[] = { >> { SmmLegacyBootHandler, &gEfiEventLegacyBootGuid, NULL, >> FALSE }, >> { SmmExitBootServicesHandler, &gEfiEventExitBootServicesGuid, NULL, >> FALSE }, >> { SmmReadyToBootHandler, &gEfiEventReadyToBootGuid, NULL, >> FALSE }, >> - { SmmEndOfDxeHandler, &gEfiEndOfDxeEventGroupGuid, NULL, >> FALSE }, >> + { SmmEndOfDxeHandler, &gEfiEndOfDxeEventGroupGuid, NULL, >> TRUE }, >> { NULL, NULL, NULL, >> FALSE } >> }; >> >> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c >> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c >> index 4dd1352..ebef741 100644 >> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c >> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c >> @@ -267,7 +267,7 @@ SMM_IPL_EVENT_NOTIFICATION mSmmIplEvents[] = { >> // the associated event is immediately signalled, so the notification >> function will be executed and the >> // SMM End Of Dxe Protocol will be found if it is already in the handle >> database. >> // >> - { FALSE, FALSE, &gEfiEndOfDxeEventGroupGuid, >> SmmIplGuidedEventNotify, &gEfiEndOfDxeEventGroupGuid, >> TPL_CALLBACK, NULL }, >> + { FALSE, TRUE, &gEfiEndOfDxeEventGroupGuid, >> SmmIplGuidedEventNotify, &gEfiEndOfDxeEventGroupGuid, >> TPL_CALLBACK, NULL }, >> // >> // Declare event notification on the DXE Dispatch Event Group. This >> event is signaled by the DXE Core >> // each time the DXE Core dispatcher has completed its work. When >> this event is signalled, the SMM Core >> > > Please provide a detailed description in the commit message about the bug > that this patch fixes. > > Thanks > Laszlo > ------------------------------------------------------------------------------ Monitor 25 network devices or servers for free with OpManager! OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
