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
