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 CF283211435B7 for ; Mon, 1 Oct 2018 09:57:22 -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 2EBC730821F3; Mon, 1 Oct 2018 16:57:22 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-123-68.rdu2.redhat.com [10.10.123.68]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8D272601A8; Mon, 1 Oct 2018 16:57:17 +0000 (UTC) To: marcandre.lureau@redhat.com, edk2-devel@lists.01.org Cc: Jordan Justen , Ard Biesheuvel , Anthony Perard , Julien Grall References: <20181001114529.26741-1-marcandre.lureau@redhat.com> From: Laszlo Ersek Message-ID: <7693a2ef-873a-5ffa-a647-0e8ec4443376@redhat.com> Date: Mon, 1 Oct 2018 18:57:16 +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: <20181001114529.26741-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.47]); Mon, 01 Oct 2018 16:57:22 +0000 (UTC) Subject: Re: [PATCH v1 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: Mon, 01 Oct 2018 16:57:24 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 10/01/18 13:45, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau > > The TCG "Platform Reset Attack Mitigation Specification" requires to > clear the processor caches when the MOR bit is set at boot time. > > According to Paolo Bonzini, clearing the CPU cache takes only a few > hundred clock cycles, so it can be done unconditionally. > > Flush the cache on all logical processors, thanks to > EFI_PEI_MP_SERVICES_PPI, calling WBINVD "Write Back and Invalidate > Cache" x86 instruction. (1) Please update this paragraph (and the code, of course) as suggested by Michael. (I guess I should have known better, and suggested WriteBackInvalidateDataCache() myself. While in this particular case, there's not a big difference, because this code is X86 only, I agree it's better to call the more abstract interface.) > > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Anthony Perard > Cc: Julien Grall > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Marc-André Lureau > --- > OvmfPkg/PlatformPei/PlatformPei.inf | 1 + > OvmfPkg/PlatformPei/Platform.h | 5 + > OvmfPkg/PlatformPei/ClearCache.c | 110 ++++++++++++++++++++ > OvmfPkg/PlatformPei/Platform.c | 1 + > 4 files changed, 117 insertions(+) > > diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf > index 9c5ad9961c4a..9c9a95fb3fe5 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 > 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..a1fff8446d13 > --- /dev/null > +++ b/OvmfPkg/PlatformPei/ClearCache.c > @@ -0,0 +1,110 @@ > +/**@file > + Install a callback to clear cache on all processors. (2) Please fuse the first two paragraphs of the commit message into a short additional sentence here, such as: 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. > + > + 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 "Platform.h" > + > +/** > + 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. > +**/ (3) Please prepend a short sentence to the top of the comment block, stating what the callback does. > +STATIC > +VOID > +EFIAPI > +ClearCache ( > + IN OUT VOID *WorkSpace > + ) > +{ > + AsmWbinvd (); (4) This is where Mike's comment applies. > +} > + > +/** > + 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 > +OnMpServicesAvailable ( (5) Technically this is correct, but we should preferably avoid a function name collision even for debug log purposes, between this file, and "OvmfPkg/PlatformPei/FeatureControl.c". We already disambiguate the log message quite well, because we log gEfiCallerBaseName, not just __FUNCTION__; however, in this case, gEfiCallerBaseName will no longer be unique. So please rename the function (include ClearCache somehow in the name). > + 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; > + } (Once you rename this function, this message becomes unique too.) > + > + // > + // 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 > + OnMpServicesAvailable // 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 (); > Thanks! Laszlo