public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: marcandre.lureau@redhat.com, edk2-devel@lists.01.org
Cc: Jordan Justen <jordan.l.justen@intel.com>,
	Kinney Michael D <michael.d.kinney@intel.com>,
	Anthony Perard <anthony.perard@citrix.com>
Subject: Re: [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches
Date: Tue, 2 Oct 2018 13:16:06 +0200	[thread overview]
Message-ID: <2c63afe1-ee4f-e41a-4d2a-55f5abaa39ee@redhat.com> (raw)
In-Reply-To: <20181002083602.581-1-marcandre.lureau@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 ();
> 



  parent reply	other threads:[~2018-10-02 11:16 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-02  8:36 [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches marcandre.lureau
2018-10-02 10:55 ` Laszlo Ersek
2018-10-02 11:37   ` Marc-André Lureau
2018-10-02 11:55     ` Laszlo Ersek
2018-10-02 11:57       ` Laszlo Ersek
2018-10-02 12:10       ` Marc-André Lureau
2018-10-02 12:19         ` Laszlo Ersek
2018-10-02 12:30           ` Marc-André Lureau
2018-10-02 11:16 ` Laszlo Ersek [this message]
2018-10-02 11:37   ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2c63afe1-ee4f-e41a-4d2a-55f5abaa39ee@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox