On 06/23/15 05:04, Zeng, Star wrote: > 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 for that (although the git mirror won't reflect that change, because in git you can't edit commit messages in-place -- in git you can rebase and reword the commit message, but then the commit hash changes for the commit, and all the child commits too). Thanks Laszlo > 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
