Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 11 June 2018 at 23:27, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> Hi Ard
> May I suggest that we split the Capsule Dispatch patch from the cache maintenance patch?
>
> I think the former may require more time for design discussion.
>

Yes, that is fine. As I explained, it has mainly to do with
dispatching the capsule at a time when the console or GOP is available
to report progress. This is separate from the cache maintenance issue.


>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Monday, June 11, 2018 2:25 PM
>> To: edk2-devel@lists.01.org
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Zeng, Star <star.zeng@intel.com>;
>> Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before
>> consuming capsule data
>>
>> On 8 June 2018 at 08:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> > When capsule updates are staged for processing after a warm reboot,
>> > they are copied into memory with the MMU and caches enabled. When
>> > the capsule PEI gets around to coalescing the capsule, the MMU and
>> > caches may still be disabled, and so on architectures where uncached
>> > accesses are incoherent with the caches (such as ARM and AARCH64),
>> > we may read stale data if we don't clean the caches to memory first.
>> >
>> > Note that this cache maintenance cannot be done during the invocation
>> > of UpdateCapsule(), since the ScatterGatherList structures are only
>> > identified by physical address, and at runtime, the firmware doesn't
>> > know whether and where this memory is mapped, and cache maintenance
>> > requires a virtual address.
>> >
>> > Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
>> > Contributed-under: TianoCore Contribution Agreement 1.1
>> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> Star,
>>
>> If you are ok with this version of the patch, please let me know.
>>
>> This patch and the PsciResetSystemLib one are prerequisites for making
>> PersistAcrossReset capsules work at all on ARM systems. The remaining
>> patches are only relevant when using the new progress reporting APIs,
>> so those can wait, but I would like to merge this one as soon as it is
>> ready.
>>
>> Thanks,
>> Ard.
>>
>>
>> > ---
>> >  MdeModulePkg/Universal/CapsulePei/CapsulePei.inf           |  1 +
>> >  MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c | 38
>> ++++++++++++++------
>> >  2 files changed, 28 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
>> b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
>> > index c54bc21a95a8..594e110d1f8a 100644
>> > --- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
>> > +++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
>> > @@ -48,6 +48,7 @@ [Packages]
>> >
>> >  [LibraryClasses]
>> >    BaseLib
>> > +  CacheMaintenanceLib
>> >    HobLib
>> >    BaseMemoryLib
>> >    PeiServicesLib
>> > diff --git a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
>> b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
>> > index 3e7054cd38a9..52b80e30b479 100644
>> > --- a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
>> > +++ b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
>> > @@ -27,6 +27,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
>> KIND, EITHER EXPRESS OR IMPLIED.
>> >  #include <Guid/CapsuleVendor.h>
>> >
>> >  #include <Library/BaseMemoryLib.h>
>> > +#include <Library/CacheMaintenanceLib.h>
>> >  #include <Library/DebugLib.h>
>> >  #include <Library/PrintLib.h>
>> >  #include <Library/BaseLib.h>
>> > @@ -253,6 +254,7 @@ ValidateCapsuleByMemoryResource (
>> >    )
>> >  {
>> >    UINTN             Index;
>> > +  BOOLEAN           Valid;
>> >
>> >    //
>> >    // Sanity Check
>> > @@ -270,25 +272,39 @@ ValidateCapsuleByMemoryResource (
>> >      return FALSE;
>> >    }
>> >
>> > +  Valid = FALSE;
>> >    if (MemoryResource == NULL) {
>> >      //
>> >      // No memory resource descriptor reported in HOB list before capsule
>> Coalesce.
>> >      //
>> > -    return TRUE;
>> > +    Valid = TRUE;
>> > +  } else {
>> > +    for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {
>> > +      if ((Address >= MemoryResource[Index].PhysicalStart) &&
>> > +          ((Address + Size) <= (MemoryResource[Index].PhysicalStart +
>> MemoryResource[Index].ResourceLength))) {
>> > +        DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in
>> MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
>> > +                            Address, Size,
>> > +                            Index,
>> MemoryResource[Index].PhysicalStart,
>> MemoryResource[Index].ResourceLength));
>> > +        Valid = TRUE;
>> > +        break;
>> > +      }
>> > +    }
>> > +    if (!Valid) {
>> > +      DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in
>> any MemoryResource\n", Address, Size));
>> > +    }
>> >    }
>> >
>> > -  for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {
>> > -    if ((Address >= MemoryResource[Index].PhysicalStart) &&
>> > -        ((Address + Size) <= (MemoryResource[Index].PhysicalStart +
>> MemoryResource[Index].ResourceLength))) {
>> > -      DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in
>> MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
>> > -                          Address, Size,
>> > -                          Index, MemoryResource[Index].PhysicalStart,
>> MemoryResource[Index].ResourceLength));
>> > -      return TRUE;
>> > -    }
>> > +  if (Valid) {
>> > +    //
>> > +    // At this point, we may still be running with the MMU and caches
>> disabled,
>> > +    // and on architectures such as ARM or AARCH64, capsule [meta]data
>> loaded
>> > +    // into memory with the caches on is only guaranteed to be visible to the
>> > +    // CPU running with the caches off after performing an explicit
>> writeback.
>> > +    //
>> > +    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);
>> >    }
>> >
>> > -  DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any
>> MemoryResource\n", Address, Size));
>> > -  return FALSE;
>> > +  return Valid;
>> >  }
>> >
>> >  /**
>> > --
>> > 2.17.0
>> >