public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches
@ 2018-10-02  8:36 marcandre.lureau
  2018-10-02 10:55 ` Laszlo Ersek
  2018-10-02 11:16 ` Laszlo Ersek
  0 siblings, 2 replies; 10+ messages in thread
From: marcandre.lureau @ 2018-10-02  8:36 UTC (permalink / raw)
  To: edk2-devel
  Cc: Marc-André Lureau, Anthony Perard, Ard Biesheuvel,
	Jordan Justen, Julien Grall, Kinney Michael D, Laszlo Ersek

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.

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
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.
+
+  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>
+
+#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 ();
-- 
2.19.0.271.gfe8321ec05



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches
  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:16 ` Laszlo Ersek
  1 sibling, 1 reply; 10+ messages in thread
From: Laszlo Ersek @ 2018-10-02 10:55 UTC (permalink / raw)
  To: marcandre.lureau
  Cc: edk2-devel, Jordan Justen, Kinney Michael D, Anthony Perard

Hi Marc-André,

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.
>
> 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(+)

Please fix your git settings for your local edk2 clone. Your patch
contains context lines with LF (not CRLF) line endings, and that's not
correct for edk2.

I decoded your base64-encoded patch email manually, hence my statement
about the line endings. Here's a snippet from
"OvmfPkg/PlatformPei/PlatformPei.inf":

00000d90  64 69 66 66 20 2d 2d 67  69 74 20 61 2f 4f 76 6d  |diff --git a/Ovm|
00000da0  66 50 6b 67 2f 50 6c 61  74 66 6f 72 6d 50 65 69  |fPkg/PlatformPei|
00000db0  2f 50 6c 61 74 66 6f 72  6d 50 65 69 2e 69 6e 66  |/PlatformPei.inf|
00000dc0  20 62 2f 4f 76 6d 66 50  6b 67 2f 50 6c 61 74 66  | b/OvmfPkg/Platf|
00000dd0  6f 72 6d 50 65 69 2f 50  6c 61 74 66 6f 72 6d 50  |ormPei/PlatformP|
00000de0  65 69 2e 69 6e 66 0a 69  6e 64 65 78 20 39 63 35  |ei.inf.index 9c5|
00000df0  61 64 39 39 36 31 63 34  61 2e 2e 35 63 38 64 64  |ad9961c4a..5c8dd|
00000e00  30 66 65 36 64 37 32 20  31 30 30 36 34 34 0a 2d  |0fe6d72 100644.-|
00000e10  2d 2d 20 61 2f 4f 76 6d  66 50 6b 67 2f 50 6c 61  |-- a/OvmfPkg/Pla|
00000e20  74 66 6f 72 6d 50 65 69  2f 50 6c 61 74 66 6f 72  |tformPei/Platfor|
00000e30  6d 50 65 69 2e 69 6e 66  0a 2b 2b 2b 20 62 2f 4f  |mPei.inf.+++ b/O|
00000e40  76 6d 66 50 6b 67 2f 50  6c 61 74 66 6f 72 6d 50  |vmfPkg/PlatformP|
00000e50  65 69 2f 50 6c 61 74 66  6f 72 6d 50 65 69 2e 69  |ei/PlatformPei.i|
00000e60  6e 66 0a 40 40 20 2d 33  30 2c 36 20 2b 33 30 2c  |nf.@@ -30,6 +30,|
00000e70  37 20 40 40 0a 20 0a 20  5b 53 6f 75 72 63 65 73  |7 @@. . [Sources|
00000e80  5d 0a 20 20 20 41 6d 64  53 65 76 2e 63 0a 2b 20  |].   AmdSev.c.+ |
00000e90  20 43 6c 65 61 72 43 61  63 68 65 2e 63 0a 20 20  | ClearCache.c.  |
00000ea0  20 43 6d 6f 73 2e 63 0a  20 20 20 43 6d 6f 73 2e  | Cmos.c.   Cmos.|
00000eb0  68 0a 20 20 20 46 65 61  74 75 72 65 43 6f 6e 74  |h.   FeatureCont|
00000ec0  72 6f 6c 2e 63 0a 40 40  20 2d 35 34 2c 36 20 2b  |rol.c.@@ -54,6 +|
00000ed0  35 35 2c 37 20 40 40 0a  20 0a 20 5b 4c 69 62 72  |55,7 @@. . [Libr|
00000ee0  61 72 79 43 6c 61 73 73  65 73 5d 0a 20 20 20 42  |aryClasses].   B|
00000ef0  61 73 65 4c 69 62 0a 2b  20 20 43 61 63 68 65 4d  |aseLib.+  CacheM|
00000f00  61 69 6e 74 65 6e 61 6e  63 65 4c 69 62 0a 20 20  |aintenanceLib.  |

Due to this issue, git-am fails to apply the patch.

I mentioned this earlier (including a request to push your patches being
submitted to a personal repo/branch as well):

  http://mid.mail-archive.com/f5c15a1e-25c2-c2e4-e6db-374107e3b488@redhat.com

The wiki article at

  https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers

explains the git settings in order for git not to hide the CRLFs in the
working tree. In particular, the git concept for (a) representing the
source code internally with LFs, and (b) converting to/from CRLF on
checkout/commit, is *not* to be used.

Is this edk2 peculiarity broken, considering git standards and other
open source repositories? It sure is.

Can we change it? No, we can't.

Thanks,
Laszlo


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches
  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:16 ` Laszlo Ersek
  2018-10-02 11:37   ` Laszlo Ersek
  1 sibling, 1 reply; 10+ messages in thread
From: Laszlo Ersek @ 2018-10-02 11:16 UTC (permalink / raw)
  To: marcandre.lureau, edk2-devel
  Cc: Jordan Justen, Kinney Michael D, Anthony Perard

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 ();
> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches
  2018-10-02 11:16 ` Laszlo Ersek
@ 2018-10-02 11:37   ` Laszlo Ersek
  0 siblings, 0 replies; 10+ messages in thread
From: Laszlo Ersek @ 2018-10-02 11:37 UTC (permalink / raw)
  To: Anthony Perard, Julien Grall, Gary Ching-Pang Lin
  Cc: marcandre.lureau, edk2-devel, Kinney Michael D, Jordan Justen,
	Ard Biesheuvel

Anthony, Julien, Gary,

On 10/02/18 13:16, Laszlo Ersek wrote:
> On 10/02/18 10:36, marcandre.lureau@redhat.com wrote:

[...]

> The rest looks good, thanks. I'll regression-test the patch (with SMP
> and UP guests) once you post v3.

can one of you guys please regression-test v3 as well, in a Xen guest?
(UP and SMP.) This patch is different from the FeatureControl stuff in
that this one is not gated by fw_cfg. That implies CpuMpPei will be put
to use in Xen guests as well. We had better check whether it works
before we push the patch.

I'd also like to receive an ACK from Mike, for v3.

Thanks!
Laszlo


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches
  2018-10-02 10:55 ` Laszlo Ersek
@ 2018-10-02 11:37   ` Marc-André Lureau
  2018-10-02 11:55     ` Laszlo Ersek
  0 siblings, 1 reply; 10+ messages in thread
From: Marc-André Lureau @ 2018-10-02 11:37 UTC (permalink / raw)
  To: Laszlo Ersek
  Cc: michael.d.kinney, jordan.l.justen, edk2-devel, Anthony Perard

[-- Attachment #1: Type: text/plain, Size: 5106 bytes --]

Hi

On Tue, Oct 2, 2018 at 2:55 PM Laszlo Ersek <lersek@redhat.com> wrote:
>
> Hi Marc-André,
>
> 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.
> >
> > 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(+)
>
> Please fix your git settings for your local edk2 clone. Your patch
> contains context lines with LF (not CRLF) line endings, and that's not
> correct for edk2.

Shouldn't it be catched by: python BaseTools/Scripts/PatchCheck.py -1 ?

>
> I decoded your base64-encoded patch email manually, hence my statement
> about the line endings. Here's a snippet from
> "OvmfPkg/PlatformPei/PlatformPei.inf":
>
> 00000d90  64 69 66 66 20 2d 2d 67  69 74 20 61 2f 4f 76 6d  |diff --git a/Ovm|
> 00000da0  66 50 6b 67 2f 50 6c 61  74 66 6f 72 6d 50 65 69  |fPkg/PlatformPei|
> 00000db0  2f 50 6c 61 74 66 6f 72  6d 50 65 69 2e 69 6e 66  |/PlatformPei.inf|
> 00000dc0  20 62 2f 4f 76 6d 66 50  6b 67 2f 50 6c 61 74 66  | b/OvmfPkg/Platf|
> 00000dd0  6f 72 6d 50 65 69 2f 50  6c 61 74 66 6f 72 6d 50  |ormPei/PlatformP|
> 00000de0  65 69 2e 69 6e 66 0a 69  6e 64 65 78 20 39 63 35  |ei.inf.index 9c5|
> 00000df0  61 64 39 39 36 31 63 34  61 2e 2e 35 63 38 64 64  |ad9961c4a..5c8dd|
> 00000e00  30 66 65 36 64 37 32 20  31 30 30 36 34 34 0a 2d  |0fe6d72 100644.-|
> 00000e10  2d 2d 20 61 2f 4f 76 6d  66 50 6b 67 2f 50 6c 61  |-- a/OvmfPkg/Pla|
> 00000e20  74 66 6f 72 6d 50 65 69  2f 50 6c 61 74 66 6f 72  |tformPei/Platfor|
> 00000e30  6d 50 65 69 2e 69 6e 66  0a 2b 2b 2b 20 62 2f 4f  |mPei.inf.+++ b/O|
> 00000e40  76 6d 66 50 6b 67 2f 50  6c 61 74 66 6f 72 6d 50  |vmfPkg/PlatformP|
> 00000e50  65 69 2f 50 6c 61 74 66  6f 72 6d 50 65 69 2e 69  |ei/PlatformPei.i|
> 00000e60  6e 66 0a 40 40 20 2d 33  30 2c 36 20 2b 33 30 2c  |nf.@@ -30,6 +30,|
> 00000e70  37 20 40 40 0a 20 0a 20  5b 53 6f 75 72 63 65 73  |7 @@. . [Sources|
> 00000e80  5d 0a 20 20 20 41 6d 64  53 65 76 2e 63 0a 2b 20  |].   AmdSev.c.+ |
> 00000e90  20 43 6c 65 61 72 43 61  63 68 65 2e 63 0a 20 20  | ClearCache.c.  |
> 00000ea0  20 43 6d 6f 73 2e 63 0a  20 20 20 43 6d 6f 73 2e  | Cmos.c.   Cmos.|
> 00000eb0  68 0a 20 20 20 46 65 61  74 75 72 65 43 6f 6e 74  |h.   FeatureCont|
> 00000ec0  72 6f 6c 2e 63 0a 40 40  20 2d 35 34 2c 36 20 2b  |rol.c.@@ -54,6 +|
> 00000ed0  35 35 2c 37 20 40 40 0a  20 0a 20 5b 4c 69 62 72  |55,7 @@. . [Libr|
> 00000ee0  61 72 79 43 6c 61 73 73  65 73 5d 0a 20 20 20 42  |aryClasses].   B|
> 00000ef0  61 73 65 4c 69 62 0a 2b  20 20 43 61 63 68 65 4d  |aseLib.+  CacheM|
> 00000f00  61 69 6e 74 65 6e 61 6e  63 65 4c 69 62 0a 20 20  |aintenanceLib.  |
>
> Due to this issue, git-am fails to apply the patch.
>
> I mentioned this earlier (including a request to push your patches being
> submitted to a personal repo/branch as well):
>
>   http://mid.mail-archive.com/f5c15a1e-25c2-c2e4-e6db-374107e3b488@redhat.com
>
> The wiki article at
>
>   https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers
>
> explains the git settings in order for git not to hide the CRLFs in the
> working tree. In particular, the git concept for (a) representing the
> source code internally with LFs, and (b) converting to/from CRLF on
> checkout/commit, is *not* to be used.
>
> Is this edk2 peculiarity broken, considering git standards and other
> open source repositories? It sure is.
>
> Can we change it? No, we can't.

Hmm, this is weird, the original patch has crlf (see attach file).
send-email or the mail server somehow stripped it?

I use your recommend git settings, I don't know what I am missing here.

>
> Thanks,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel



-- 
Marc-André Lureau

[-- Attachment #2: v2-0001-OvmfPkg-PlatformPei-clear-CPU-caches.patch --]
[-- Type: text/x-patch, Size: 6588 bytes --]

From ec72ca9774cef9f89fbe721fc77d750107b332c2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= <marcandre.lureau@redhat.com>
Date: Mon, 1 Oct 2018 15:26:40 +0400
Subject: [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.

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
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.
+
+  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>
+
+#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 ();
-- 
2.19.0.271.gfe8321ec05


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches
  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
  0 siblings, 2 replies; 10+ messages in thread
From: Laszlo Ersek @ 2018-10-02 11:55 UTC (permalink / raw)
  To: Marc-André Lureau
  Cc: michael.d.kinney, jordan.l.justen, edk2-devel, Anthony Perard

On 10/02/18 13:37, Marc-André Lureau wrote:
> On Tue, Oct 2, 2018 at 2:55 PM Laszlo Ersek <lersek@redhat.com> wrote:

>> Please fix your git settings for your local edk2 clone. Your patch
>> contains context lines with LF (not CRLF) line endings, and that's not
>> correct for edk2.
> 
> Shouldn't it be catched by: python BaseTools/Scripts/PatchCheck.py -1 ?

Yes, it should be caught by "PatchCheck.py". The "force_crlf" member is
set iff the pathname doesn't end in ".sh", and then the
check_added_line() method verifies CRLF.

I don't know why it doesn't work in practice. Can you submit a TianoCore
BZ about it?

(Your patch uses LF on both context lines and new (added) lines, so the
current script logic should complain, yes.)

> Hmm, this is weird, the original patch has crlf (see attach file).
> send-email or the mail server somehow stripped it?
> 
> I use your recommend git settings, I don't know what I am missing here.

You are right, the attachment looks fine.

... Can you resend the v2 patch (just to me directly, off-list) with

  --transfer-encoding=quoted-printable

? My guess is that the base64 encoding in git-send-email includes an
automatic CR stripping phase.

If indeed bas64 is the culprit, I'll update the wiki article to specify
quoted-printable.

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches
  2018-10-02 11:55     ` Laszlo Ersek
@ 2018-10-02 11:57       ` Laszlo Ersek
  2018-10-02 12:10       ` Marc-André Lureau
  1 sibling, 0 replies; 10+ messages in thread
From: Laszlo Ersek @ 2018-10-02 11:57 UTC (permalink / raw)
  To: Marc-André Lureau
  Cc: michael.d.kinney, jordan.l.justen, edk2-devel, Anthony Perard

On 10/02/18 13:55, Laszlo Ersek wrote:
> On 10/02/18 13:37, Marc-André Lureau wrote:
>> On Tue, Oct 2, 2018 at 2:55 PM Laszlo Ersek <lersek@redhat.com> wrote:
> 
>>> Please fix your git settings for your local edk2 clone. Your patch
>>> contains context lines with LF (not CRLF) line endings, and that's not
>>> correct for edk2.
>>
>> Shouldn't it be catched by: python BaseTools/Scripts/PatchCheck.py -1 ?
> 
> Yes, it should be caught by "PatchCheck.py". The "force_crlf" member is
> set iff the pathname doesn't end in ".sh", and then the
> check_added_line() method verifies CRLF.
> 
> I don't know why it doesn't work in practice. Can you submit a TianoCore
> BZ about it?

Nevermind:

> You are right, the attachment looks fine.

... so that's the reason "PatchCheck.py" does not complain.

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches
  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
  1 sibling, 1 reply; 10+ messages in thread
From: Marc-André Lureau @ 2018-10-02 12:10 UTC (permalink / raw)
  To: Laszlo Ersek
  Cc: marcandre, michael.d.kinney, jordan.l.justen, edk2-devel,
	anthony.perard

Hi

On Tue, Oct 2, 2018 at 3:55 PM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 10/02/18 13:37, Marc-André Lureau wrote:
> > On Tue, Oct 2, 2018 at 2:55 PM Laszlo Ersek <lersek@redhat.com> wrote:
>
> >> Please fix your git settings for your local edk2 clone. Your patch
> >> contains context lines with LF (not CRLF) line endings, and that's not
> >> correct for edk2.
> >
> > Shouldn't it be catched by: python BaseTools/Scripts/PatchCheck.py -1 ?
>
> Yes, it should be caught by "PatchCheck.py". The "force_crlf" member is
> set iff the pathname doesn't end in ".sh", and then the
> check_added_line() method verifies CRLF.
>
> I don't know why it doesn't work in practice. Can you submit a TianoCore
> BZ about it?
>
> (Your patch uses LF on both context lines and new (added) lines, so the
> current script logic should complain, yes.)
>
> > Hmm, this is weird, the original patch has crlf (see attach file).
> > send-email or the mail server somehow stripped it?
> >
> > I use your recommend git settings, I don't know what I am missing here.
>
> You are right, the attachment looks fine.
>
> ... Can you resend the v2 patch (just to me directly, off-list) with
>
>   --transfer-encoding=quoted-printable
>
> ? My guess is that the base64 encoding in git-send-email includes an
> automatic CR stripping phase.

Hmm, actually it's my emacs settings that stripped the crlf from the patch.

I edited the patch manually to add some notes...

>
> If indeed bas64 is the culprit, I'll update the wiki article to specify
> quoted-printable.
>
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches
  2018-10-02 12:10       ` Marc-André Lureau
@ 2018-10-02 12:19         ` Laszlo Ersek
  2018-10-02 12:30           ` Marc-André Lureau
  0 siblings, 1 reply; 10+ messages in thread
From: Laszlo Ersek @ 2018-10-02 12:19 UTC (permalink / raw)
  To: Marc-André Lureau
  Cc: marcandre, michael.d.kinney, jordan.l.justen, edk2-devel,
	anthony.perard

On 10/02/18 14:10, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Oct 2, 2018 at 3:55 PM Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 10/02/18 13:37, Marc-André Lureau wrote:
>>> On Tue, Oct 2, 2018 at 2:55 PM Laszlo Ersek <lersek@redhat.com> wrote:
>>
>>>> Please fix your git settings for your local edk2 clone. Your patch
>>>> contains context lines with LF (not CRLF) line endings, and that's not
>>>> correct for edk2.
>>>
>>> Shouldn't it be catched by: python BaseTools/Scripts/PatchCheck.py -1 ?
>>
>> Yes, it should be caught by "PatchCheck.py". The "force_crlf" member is
>> set iff the pathname doesn't end in ".sh", and then the
>> check_added_line() method verifies CRLF.
>>
>> I don't know why it doesn't work in practice. Can you submit a TianoCore
>> BZ about it?
>>
>> (Your patch uses LF on both context lines and new (added) lines, so the
>> current script logic should complain, yes.)
>>
>>> Hmm, this is weird, the original patch has crlf (see attach file).
>>> send-email or the mail server somehow stripped it?
>>>
>>> I use your recommend git settings, I don't know what I am missing here.
>>
>> You are right, the attachment looks fine.
>>
>> ... Can you resend the v2 patch (just to me directly, off-list) with
>>
>>   --transfer-encoding=quoted-printable
>>
>> ? My guess is that the base64 encoding in git-send-email includes an
>> automatic CR stripping phase.
> 
> Hmm, actually it's my emacs settings that stripped the crlf from the patch.
> 
> I edited the patch manually to add some notes...

Haha, serves you right then ;)

I suggest never editing patches after formatting them. In the first
place, there's no reason to: you can add, edit and remove notes on
commits with "git notes" (without changing the commit hashes / git
history). And "git-format-patch" takes a "--notes" option. (Same for
"git show".)

I do mention git-notes in the "unkempt guide":

git config notes.rewriteRef       refs/notes/commits

git format-patch                               \
  --notes                                      \

git notes edit COMMIT_HASH_OF_THAT_PATCH


Thanks for tracking this down!
Laszlo


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches
  2018-10-02 12:19         ` Laszlo Ersek
@ 2018-10-02 12:30           ` Marc-André Lureau
  0 siblings, 0 replies; 10+ messages in thread
From: Marc-André Lureau @ 2018-10-02 12:30 UTC (permalink / raw)
  To: Laszlo Ersek
  Cc: marcandre, michael.d.kinney, jordan.l.justen, edk2-devel,
	anthony.perard

Hi

On Tue, Oct 2, 2018 at 4:19 PM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 10/02/18 14:10, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Oct 2, 2018 at 3:55 PM Laszlo Ersek <lersek@redhat.com> wrote:
> >>
> >> On 10/02/18 13:37, Marc-André Lureau wrote:
> >>> On Tue, Oct 2, 2018 at 2:55 PM Laszlo Ersek <lersek@redhat.com> wrote:
> >>
> >>>> Please fix your git settings for your local edk2 clone. Your patch
> >>>> contains context lines with LF (not CRLF) line endings, and that's not
> >>>> correct for edk2.
> >>>
> >>> Shouldn't it be catched by: python BaseTools/Scripts/PatchCheck.py -1 ?
> >>
> >> Yes, it should be caught by "PatchCheck.py". The "force_crlf" member is
> >> set iff the pathname doesn't end in ".sh", and then the
> >> check_added_line() method verifies CRLF.
> >>
> >> I don't know why it doesn't work in practice. Can you submit a TianoCore
> >> BZ about it?
> >>
> >> (Your patch uses LF on both context lines and new (added) lines, so the
> >> current script logic should complain, yes.)
> >>
> >>> Hmm, this is weird, the original patch has crlf (see attach file).
> >>> send-email or the mail server somehow stripped it?
> >>>
> >>> I use your recommend git settings, I don't know what I am missing here.
> >>
> >> You are right, the attachment looks fine.
> >>
> >> ... Can you resend the v2 patch (just to me directly, off-list) with
> >>
> >>   --transfer-encoding=quoted-printable
> >>
> >> ? My guess is that the base64 encoding in git-send-email includes an
> >> automatic CR stripping phase.
> >
> > Hmm, actually it's my emacs settings that stripped the crlf from the patch.
> >
> > I edited the patch manually to add some notes...
>
> Haha, serves you right then ;)
>
> I suggest never editing patches after formatting them. In the first
> place, there's no reason to: you can add, edit and remove notes on
> commits with "git notes" (without changing the commit hashes / git
> history). And "git-format-patch" takes a "--notes" option. (Same for
> "git show".)
>
> I do mention git-notes in the "unkempt guide":
>
> git config notes.rewriteRef       refs/notes/commits
>
> git format-patch                               \
>   --notes                                      \
>
> git notes edit COMMIT_HASH_OF_THAT_PATCH
>

Yes, somehow git-notes doesn't fit with my idea of how patch mail
notes for v2/v3... should look:

Notes:
    v3:
     - foo

    v2:
     - blablah

 If there is a way to avoid the "Notes:" quoting, that would be nice.


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-10-02 12:30 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
2018-10-02 11:37   ` Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox