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

[Laszlo Ersek <lersek@redhat.com>]

On 10/01/18 13:45, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> 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 <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  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 <Library/DebugLib.h>
> +#include <Library/PeiServicesLib.h>
> +#include <Ppi/MpServices.h>
> +
> +#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