From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 DA9992194D3B8 for ; Mon, 22 Oct 2018 07:30:10 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BCFAF7F3E1; Mon, 22 Oct 2018 14:30:09 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-124-111.rdu2.redhat.com [10.10.124.111]) by smtp.corp.redhat.com (Postfix) with ESMTP id B9DF6608F3; Mon, 22 Oct 2018 14:30:07 +0000 (UTC) To: Ruiyu Ni , edk2-devel@lists.01.org Cc: Michael Kinney , Jiewen Yao References: <20181022090333.95988-1-ruiyu.ni@intel.com> From: Laszlo Ersek Message-ID: Date: Mon, 22 Oct 2018 16:30:06 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181022090333.95988-1-ruiyu.ni@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Mon, 22 Oct 2018 14:30:09 +0000 (UTC) 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: Mon, 22 Oct 2018 14:30:11 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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. (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 ? (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? (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"? ... 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. Thanks! Laszlo