On 10/02/18 10:36, marcandre.lur...@redhat.com wrote: > From: Marc-André Lureau <marcandre.lur...@redhat.com> > > 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 <anthony.per...@citrix.com> > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> > Cc: Jordan Justen <jordan.l.jus...@intel.com> > Cc: Julien Grall <julien.gr...@linaro.org> > Cc: Kinney Michael D <michael.d.kin...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> > --- > > 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 <Library/DebugLib.h> > +#include <Library/PeiServicesLib.h> > +#include <Library/CacheMaintenanceLib.h> > +#include <Ppi/MpServices.h> 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 (); > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
