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 74604211616A0 for ; Tue, 2 Oct 2018 04:16:12 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 33AC33002C97; Tue, 2 Oct 2018 11:16:12 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-142.rdu2.redhat.com [10.10.120.142]) by smtp.corp.redhat.com (Postfix) with ESMTP id DDF1F6058F; Tue, 2 Oct 2018 11:16:07 +0000 (UTC) To: marcandre.lureau@redhat.com, edk2-devel@lists.01.org Cc: Jordan Justen , Kinney Michael D , Anthony Perard References: <20181002083602.581-1-marcandre.lureau@redhat.com> From: Laszlo Ersek Message-ID: <2c63afe1-ee4f-e41a-4d2a-55f5abaa39ee@redhat.com> Date: Tue, 2 Oct 2018 13:16: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: <20181002083602.581-1-marcandre.lureau@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.46]); Tue, 02 Oct 2018 11:16:12 +0000 (UTC) Subject: Re: [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches 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, 02 Oct 2018 11:16:13 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 10/02/18 10:36, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau > > This is for conformance with the TCG "Platform Reset Attack Mitigation > Specification". Because clearing the CPU caches at boot doesn't impact > performance significantly, do it unconditionally, for simplicity's > sake. > > Flush the cache on all logical processors, thanks to > EFI_PEI_MP_SERVICES_PPI and CacheMaintenanceLib. The commit message looks OK to me, thanks. > > Cc: Anthony Perard > Cc: Ard Biesheuvel > Cc: Jordan Justen > Cc: Julien Grall > Cc: Kinney Michael D > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Marc-André Lureau > --- > > v2: > - use CacheMaintenanceLib, instead of WBINVD usage > - rename OnMpServicesAvailable->ClearCacheOnMpServicesAvailable > - commit message & comments update > > OvmfPkg/PlatformPei/PlatformPei.inf | 2 + > OvmfPkg/PlatformPei/Platform.h | 5 + > OvmfPkg/PlatformPei/ClearCache.c | 113 ++++++++++++++++++++ > OvmfPkg/PlatformPei/Platform.c | 1 + > 4 files changed, 121 insertions(+) > > diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf > index 9c5ad9961c4a..5c8dd0fe6d72 100644 > --- a/OvmfPkg/PlatformPei/PlatformPei.inf > +++ b/OvmfPkg/PlatformPei/PlatformPei.inf > @@ -30,6 +30,7 @@ > > [Sources] > AmdSev.c > + ClearCache.c > Cmos.c > Cmos.h > FeatureControl.c > @@ -54,6 +55,7 @@ > > [LibraryClasses] > BaseLib > + CacheMaintenanceLib > DebugLib > HobLib > IoLib OK. > diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h > index f942e61bb4f9..b12a5c1f5f78 100644 > --- a/OvmfPkg/PlatformPei/Platform.h > +++ b/OvmfPkg/PlatformPei/Platform.h > @@ -83,6 +83,11 @@ InstallFeatureControlCallback ( > VOID > ); > > +VOID > +InstallClearCacheCallback ( > + VOID > + ); > + > EFI_STATUS > InitializeXen ( > VOID > diff --git a/OvmfPkg/PlatformPei/ClearCache.c b/OvmfPkg/PlatformPei/ClearCache.c > new file mode 100644 > index 000000000000..d333b7dcdd88 > --- /dev/null > +++ b/OvmfPkg/PlatformPei/ClearCache.c > @@ -0,0 +1,113 @@ > +/**@file > + Install a callback to clear cache on all processors. The short explanation I requested in my previous point (2) is still missing. (I think you may have misunderstood my previous points (1) and (2). In (1), I asked for addressing Mike's observation in the commit message. That was basically about replacing WBINVD with a reference to CacheMaintenanceLib. In (2), I asked for the commit message to be fused into a short sentence *here*, in this source file. IOW, my point (2) wasn't about modifying the commit message itself; it was about distilling a file comment from the commit message. Anyway, I'm fine with the new commit message too, but the file comment is still missing.) > + > + Copyright (C) 2018, Red Hat, Inc. > + > + This program and the accompanying materials are licensed and made available > + under the terms and conditions of the BSD License which accompanies this > + distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT > + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +**/ > + > +#include > +#include > +#include > +#include You've kept the INF file sections sorted, thanks a lot for that; please keep this Library/* include list sorted as well. The rest looks good, thanks. I'll regression-test the patch (with SMP and UP guests) once you post v3. Thanks, Laszlo > + > +#include "Platform.h" > + > +/** > + Invalidate data & instruction caches. > + All APs execute this function in parallel. The BSP executes the function > + separately. > + > + @param[in,out] WorkSpace Pointer to the input/output argument workspace > + shared by all processors. > +**/ > +STATIC > +VOID > +EFIAPI > +ClearCache ( > + IN OUT VOID *WorkSpace > + ) > +{ > + WriteBackInvalidateDataCache (); > + InvalidateInstructionCache (); > +} > + > +/** > + Notification function called when EFI_PEI_MP_SERVICES_PPI becomes available. > + > + @param[in] PeiServices Indirect reference to the PEI Services Table. > + @param[in] NotifyDescriptor Address of the notification descriptor data > + structure. > + @param[in] Ppi Address of the PPI that was installed. > + > + @return Status of the notification. The status code returned from this > + function is ignored. > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +ClearCacheOnMpServicesAvailable ( > + IN EFI_PEI_SERVICES **PeiServices, > + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor, > + IN VOID *Ppi > + ) > +{ > + EFI_PEI_MP_SERVICES_PPI *MpServices; > + EFI_STATUS Status; > + > + DEBUG ((DEBUG_INFO, "%a: %a\n", gEfiCallerBaseName, __FUNCTION__)); > + > + // > + // Clear cache on all the APs in parallel. > + // > + MpServices = Ppi; > + Status = MpServices->StartupAllAPs ( > + (CONST EFI_PEI_SERVICES **)PeiServices, > + MpServices, > + ClearCache, // Procedure > + FALSE, // SingleThread > + 0, // TimeoutInMicroSeconds: inf. > + NULL // ProcedureArgument > + ); > + if (EFI_ERROR (Status) && Status != EFI_NOT_STARTED) { > + DEBUG ((DEBUG_ERROR, "%a: StartupAllAps(): %r\n", __FUNCTION__, Status)); > + return Status; > + } > + > + // > + // Now clear cache on the BSP too. > + // > + ClearCache (NULL); > + return EFI_SUCCESS; > +} > + > +// > +// Notification object for registering the callback, for when > +// EFI_PEI_MP_SERVICES_PPI becomes available. > +// > +STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR mMpServicesNotify = { > + EFI_PEI_PPI_DESCRIPTOR_NOTIFY_CALLBACK | // Flags > + EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST, > + &gEfiPeiMpServicesPpiGuid, // Guid > + ClearCacheOnMpServicesAvailable // Notify > +}; > + > +VOID > +InstallClearCacheCallback ( > + VOID > + ) > +{ > + EFI_STATUS Status; > + > + Status = PeiServicesNotifyPpi (&mMpServicesNotify); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: failed to set up MP Services callback: %r\n", > + __FUNCTION__, Status)); > + } > +} > diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c > index 5a78668126b4..22139a64cbf4 100644 > --- a/OvmfPkg/PlatformPei/Platform.c > +++ b/OvmfPkg/PlatformPei/Platform.c > @@ -672,6 +672,7 @@ InitializePlatform ( > NoexecDxeInitialization (); > } > > + InstallClearCacheCallback (); > AmdSevInitialize (); > MiscInitialization (); > InstallFeatureControlCallback (); >