Re: [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches

[Laszlo Ersek <lersek@redhat.com>]

On 10/02/18 10:36, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@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.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> Cc: Kinney Michael D <michael.d.kinney@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marc-André Lureau <marcandre.lureau@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 ();
>