From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.31; helo=mga06.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9D7D92117D288 for ; Tue, 23 Oct 2018 02:20:13 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Oct 2018 02:20:12 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,415,1534834800"; d="scan'208";a="83705593" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.11]) ([10.239.9.11]) by orsmga008.jf.intel.com with ESMTP; 23 Oct 2018 02:20:07 -0700 To: Laszlo Ersek , edk2-devel@lists.01.org Cc: Michael Kinney , Jiewen Yao References: <20181022090333.95988-1-ruiyu.ni@intel.com> From: "Ni, Ruiyu" Message-ID: <61c42bbb-c446-a342-55b8-94128303e58d@Intel.com> Date: Tue, 23 Oct 2018 17:21:23 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Subject: Re: [PATCH] MdeModulePkg/PiSmmIpl: Do not reset SMRAM to UC when CPU driver runs X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Oct 2018 09:20:13 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/22/2018 10:30 PM, Laszlo Ersek wrote: > On 10/22/18 11:03, Ruiyu Ni wrote: >> Today's PiSmmIpl implementation initially sets SMRAM to WB to speed >> up the SMM core/modules loading before SMM CPU driver runs. >> When SMM CPU driver runs, PiSmmIpl resets the SMRAM to UC. It's done >> in SmmIplDxeDispatchEventNotify(). COMM_BUFFER_SMM_DISPATCH_RESTART >> is returned from SMM core that SMM CPU driver is just dispatched. >> >> Since now the SMRR is widely used to control the SMRAM cache setting. >> It's not needed to reset the SMRAM to UC anymore. >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Ruiyu Ni >> Cc: Jiewen Yao >> Cc: Michael Kinney >> --- >> MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 13 ------------- >> 1 file changed, 13 deletions(-) >> >> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c >> index f8cbe1704b..dc0d9a70b0 100644 >> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c >> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c >> @@ -672,19 +672,6 @@ SmmIplDxeDispatchEventNotify ( >> return; >> } >> >> - // >> - // Attempt to reset SMRAM cacheability to UC >> - // Assume CPU AP is available at this time >> - // >> - Status = gDS->SetMemorySpaceAttributes( >> - mSmramCacheBase, >> - mSmramCacheSize, >> - EFI_MEMORY_UC >> - ); >> - if (EFI_ERROR (Status)) { >> - DEBUG ((DEBUG_WARN, "SMM IPL failed to reset SMRAM window to EFI_MEMORY_UC\n")); >> - } >> - >> // >> // Close all SMRAM ranges to protect SMRAM >> // >> > > I vaguely remember this code from commit b07ea4c198a4 ("MdeModulePkg: > SmmIplEntry(): don't suppress SMM core startup failure", 2015-05-12). So > here I'm asking just out of curiosity -- because SMRR is not necessary > to handle on QEMU/KVM. > > I assume that the original UC setting was part of the SMRAM protection > that starts at the next line -- the comment is visible in the context, > saying "Close all SMRAM ranges to protect SMRAM". The commit message > suggests that explicit SMRR management has now taken that role. > > > (1) Can you add more details to the commit message where that explicit > management happens? I.e., what modules call what library interfaces? I > believe the relevant APIs are from SmmCpuFeaturesLib. Yes. SmmCpuFeaturesInitializeProcessor() from SmmCpuFeaturesLib enables SMRR > > > (2) Also, can you elaborate on "widely"? Does that mean > > in most edk2 modules for which SMRAM caching is relevant > > or > > in most edk2-based platform firmware that is shipped in production > > ? Most (ALL maybe) edk2-based platform firmware that is shipped in production. > > > (3) You forgot to mention the bugzilla in the commit message: > > https://bugzilla.tianocore.org/show_bug.cgi?id=1268 > > > (4) From commit b07ea4c198a4 that I mention above, it seems that we have > another instance of this UC setting in the SMM IPL runtime DXE driver; > namely in the SmmIplEntry() function. Should we remove that too? > > Now, I understand that the situation at that site is different, because > in that case, we are resetting the cacheability after *failure* to > launch the SMM Core, so there may have been no chance for any module to > manage SMRR. And so we have to roll back our own initial WB setting. Do > I understand that right? Yes. I don't want to change that piece of code. > > (5) I like that this patch removes a "CPU AP" comment, because the > comment is misleading. In this context, "CPU AP" means "CPU > Architectural Protocol" (which underlies > gDS->SetMemorySpaceAttributes()), and not "Application Processor". It's > good that the patch removes such a confusing comment. > > However, other potential abuses of the shorthand "CPU AP" remain in the > code. Would you consider submitting a separate patch that replaces all > remaining instances of "CPU AP" with "CPU Arch Protocol", in > "MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c"? Sure the "AP" also confused me in the beginning. I will post a separate patch to change it. > > > ... I originally intended to test this patch at once, but given my > question (4), you might want to modify the code as well, not just the > commit message, and so I figured I'd postpone the testing until your answer. Let me check Paul's mail then decide what to do next. > > Thanks! > Laszlo > -- Thanks, Ray