Re: [PATCH 1/2] MdeModulePkg/CapsuleRuntimeDxe: Merge changes form arm to all ARCH

["Gao, Zhichao" <zhichao.gao@intel.com>]

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Wednesday, March 20, 2019 5:49 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>
> Cc: edk2-devel@lists.01.org; Wu, Hao A <hao.a.wu@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH 1/2] MdeModulePkg/CapsuleRuntimeDxe:
> Merge changes form arm to all ARCH
> 
> On Wed, 20 Mar 2019 at 02:43, Zhichao Gao <zhichao.gao@intel.com> wrote:
> >
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1462
> >
> > The arm ARCH has already to add a function CapsuleCacheWriteBack to
> > flush the cache data to DRAM. That is also required in IA32 ARCH. So
> > merge the changes. And this function do not support in runtime phase.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> 
> After this patch, platforms may return TRUE from
> QueryCapsuleCapabilities() while they returned FALSE before, i.e., at
> runtime. This behavior change will surely break ARM, but I don't see how it
> doesn't break IA32 as well if it now relies on this cache maintenance to occur
> in the context of UpdateCapsule().
> 

You are right. I forgot to consider the function QueryCapsuleCapabilities.
At runtime, maybe IA32 would catch the same issue on arm ARCH. That should be concerned.
For this patch set, we only want to add cache function before runtime. So it is better to keep its original logic and add this support.

I would fix the issue you mentioned to make sure it would not affect arm ARCH. But disable some function for IA32 ARCH should be another case.

Thanks,
Zhichao

> 
> 
> > ---
> >  .../Universal/CapsuleRuntimeDxe/CapsuleReset.c     | 21
> ++++++++++++++++
> >  .../{Arm/CapsuleReset.c => CapsuleResetNull.c}     | 29 +++-------------------
> >  .../CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf        | 14 +++++------
> >  3 files changed, 30 insertions(+), 34 deletions(-)  rename
> > MdeModulePkg/Universal/CapsuleRuntimeDxe/{Arm/CapsuleReset.c =>
> > CapsuleResetNull.c} (51%)
> >
> > diff --git a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleReset.c
> > b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleReset.c
> > index 353f6f2090..8c45f6665e 100644
> > --- a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleReset.c
> > +++ b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleReset.c
> > @@ -16,6 +16,8 @@
> >
> >  #include "CapsuleService.h"
> >
> > +#include <Library/CacheMaintenanceLib.h>
> > +
> >  /**
> >    Whether the platform supports capsules that persist across reset. Note
> that
> >    some platforms only support such capsules at boot time.
> > @@ -46,4 +48,23 @@ CapsuleCacheWriteBack (
> >    IN  EFI_PHYSICAL_ADDRESS    ScatterGatherList
> >    )
> >  {
> > +  EFI_CAPSULE_BLOCK_DESCRIPTOR    *Desc;
> > +
> > +  if (!EfiAtRuntime()) {
> > +    Desc = (EFI_CAPSULE_BLOCK_DESCRIPTOR *)(UINTN)ScatterGatherList;
> > +    do {
> > +      WriteBackDataCacheRange (Desc, sizeof *Desc);
> > +
> > +      if (Desc->Length > 0) {
> > +        WriteBackDataCacheRange ((VOID *)(UINTN)Desc->Union.DataBlock,
> > +                                 Desc->Length
> > +                                 );
> > +        Desc++;
> > +      } else if (Desc->Union.ContinuationPointer > 0) {
> > +        Desc = (EFI_CAPSULE_BLOCK_DESCRIPTOR *)(UINTN)Desc-
> >Union.ContinuationPointer;
> > +      }
> > +    } while (Desc->Length > 0 || Desc->Union.ContinuationPointer >
> > + 0);
> > +
> > +    WriteBackDataCacheRange (Desc, sizeof *Desc);  }
> >  }
> > diff --git
> > a/MdeModulePkg/Universal/CapsuleRuntimeDxe/Arm/CapsuleReset.c
> > b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleResetNull.c
> > similarity index 51%
> > rename from
> > MdeModulePkg/Universal/CapsuleRuntimeDxe/Arm/CapsuleReset.c
> > rename to
> MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleResetNull.c
> > index d79d2fc693..3c5cfc1a16 100644
> > --- a/MdeModulePkg/Universal/CapsuleRuntimeDxe/Arm/CapsuleReset.c
> > +++ b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleResetNull.c
> > @@ -1,8 +1,9 @@
> >  /** @file
> > -  ARM implementation of architecture specific routines related to
> > +  Default implementation of architecture specific routines related to
> >    PersistAcrossReset capsules
> >
> >    Copyright (c) 2018, Linaro, Ltd. All rights reserved.<BR>
> > +  Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> >
> >    This program and the accompanying materials are licensed and made
> available
> >    under the terms and conditions of the BSD License which accompanies
> > this @@ -31,14 +32,7 @@ IsPersistAcrossResetCapsuleSupported (
> >    VOID
> >    )
> >  {
> > -  //
> > -  // ARM requires the capsule payload to be cleaned to the point of
> > coherency
> > -  // (PoC), but only permits doing so using cache maintenance
> > instructions that
> > -  // operate on virtual addresses. Since at runtime, we don't know
> > the virtual
> > -  // addresses of the data structures that make up the scatter/gather
> > list, we
> > -  // cannot perform the maintenance, and all we can do is give up.
> > -  //
> > -  return FeaturePcdGet (PcdSupportUpdateCapsuleReset) &&
> > !EfiAtRuntime ();
> > +  return FeaturePcdGet (PcdSupportUpdateCapsuleReset);
> >  }
> >
> >  /**
> > @@ -55,21 +49,4 @@ CapsuleCacheWriteBack (
> >    IN  EFI_PHYSICAL_ADDRESS    ScatterGatherList
> >    )
> >  {
> > -  EFI_CAPSULE_BLOCK_DESCRIPTOR    *Desc;
> > -
> > -  Desc = (EFI_CAPSULE_BLOCK_DESCRIPTOR *)(UINTN)ScatterGatherList;
> > -  do {
> > -    WriteBackDataCacheRange (Desc, sizeof *Desc);
> > -
> > -    if (Desc->Length > 0) {
> > -      WriteBackDataCacheRange ((VOID *)(UINTN)Desc->Union.DataBlock,
> > -                               Desc->Length
> > -                               );
> > -      Desc++;
> > -    } else if (Desc->Union.ContinuationPointer > 0) {
> > -      Desc = (EFI_CAPSULE_BLOCK_DESCRIPTOR *)(UINTN)Desc-
> >Union.ContinuationPointer;
> > -    }
> > -  } while (Desc->Length > 0 || Desc->Union.ContinuationPointer > 0);
> > -
> > -  WriteBackDataCacheRange (Desc, sizeof *Desc);  } diff --git
> > a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
> > b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
> > index ad7af5fe62..c0bdf6db3d 100644
> > ---
> a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
> > +++
> b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
> > @@ -36,15 +36,15 @@
> >
> >  [Sources.Ia32, Sources.EBC, Sources.ARM, Sources.AARCH64]
> >    SaveLongModeContext.c
> > +
> > +[Sources.Ia32, Sources.X64, Sources.ARM, Sources.AARCH64]
> >    CapsuleReset.c
> >
> > +[Sources.EBC]
> > +  CapsuleResetNull.c
> > +
> >  [Sources.X64]
> >    X64/SaveLongModeContext.c
> > -  CapsuleReset.c
> > -
> > -[Sources.ARM, Sources.AARCH64]
> > -  SaveLongModeContext.c
> > -  Arm/CapsuleReset.c
> >
> >  [Packages]
> >    MdePkg/MdePkg.dec
> > @@ -61,14 +61,12 @@
> >    BaseLib
> >    PrintLib
> >    BaseMemoryLib
> > +  CacheMaintenanceLib
> >
> >  [LibraryClasses.X64]
> >    UefiLib
> >    BaseMemoryLib
> >
> > -[LibraryClasses.ARM, LibraryClasses.AARCH64]
> > -  CacheMaintenanceLib
> > -
> >  [Guids]
> >    ## SOMETIMES_PRODUCES   ## Variable:L"CapsuleUpdateData" #
> (Process across reset capsule image) for capsule updated data
> >    ## SOMETIMES_PRODUCES   ## Variable:L"CapsuleLongModeBuffer" #
> The long mode buffer used by IA32 Capsule PEIM to call X64 CapsuleCoalesce
> code to handle >4GB capsule blocks
> > --
> > 2.16.2.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel