From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: "Zeng, Star" <star.zeng@intel.com>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
"leif.lindholm@linaro.org" <leif.lindholm@linaro.org>,
"Kinney, Michael D" <michael.d.kinney@intel.com>,
"Yao, Jiewen" <jiewen.yao@intel.com>
Subject: Re: [PATCH 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data
Date: Fri, 8 Jun 2018 08:06:32 +0200 [thread overview]
Message-ID: <CAKv+Gu-ZnLu6Sar0=u5JyBN6gonNdSr9yH342eVMcznF=_dzOw@mail.gmail.com> (raw)
In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103BB54B3F@shsmsx102.ccr.corp.intel.com>
On 8 June 2018 at 04:53, Zeng, Star <star.zeng@intel.com> wrote:
> I suggest to use goto/adjust code to have one place for both paths to perform cache maintenance (with comments).
>
Something like this?
@@ -253,6 +254,7 @@ ValidateCapsuleByMemoryResource (
)
{
UINTN Index;
+ BOOLEAN Found;
//
// Sanity Check
@@ -274,19 +276,32 @@ ValidateCapsuleByMemoryResource (
//
// No memory resource descriptor reported in HOB list before
capsule Coalesce.
//
- return TRUE;
+ Found = TRUE;
+ } else {
+ Found = FALSE;
}
- for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {
+ for (Index = 0; !Found && 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;
+ Found = TRUE;
}
}
+ if (Found) {
+ //
+ // 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);
+ return TRUE;
+ }
+
DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any
MemoryResource\n", Address, Size));
return FALSE;
}
>
> Thanks,
> Star
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Thursday, June 7, 2018 7:08 PM
> To: edk2-devel@lists.01.org
> Cc: leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: [PATCH 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data
>
> 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>
> ---
> MdeModulePkg/Universal/CapsulePei/CapsulePei.inf | 1 +
> MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c | 10 ++++++++++
> 2 files changed, 11 insertions(+)
>
> 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..fb59f338f100 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>
> @@ -274,6 +275,7 @@ ValidateCapsuleByMemoryResource (
> //
> // No memory resource descriptor reported in HOB list before capsule Coalesce.
> //
> + WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);
> return TRUE;
> }
>
> @@ -283,6 +285,14 @@ ValidateCapsuleByMemoryResource (
> 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));
> +
> + //
> + // 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);
> return TRUE;
> }
> }
> --
> 2.17.0
>
next prev parent reply other threads:[~2018-06-08 6:06 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-07 11:08 [PATCH 0/5] MdeModulePkg ArmPkg: support for persistent capsules and progress reporting Ard Biesheuvel
2018-06-07 11:08 ` [PATCH 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data Ard Biesheuvel
2018-06-08 2:53 ` Zeng, Star
2018-06-08 6:06 ` Ard Biesheuvel [this message]
2018-06-08 6:21 ` Zeng, Star
2018-06-08 6:24 ` Ard Biesheuvel
2018-06-07 11:08 ` [PATCH 2/5] MdeModulePkg/DxeCapsuleLibFmp: permit ProcessCapsules () to be called once Ard Biesheuvel
2018-06-08 2:57 ` Zeng, Star
2018-06-08 6:29 ` Ard Biesheuvel
2018-06-07 11:08 ` [PATCH 3/5] MdeModulePkg/DxeCapsuleLibFmp: pass progress callback only if it works Ard Biesheuvel
2018-06-07 11:08 ` [PATCH 4/5] ArmPkg/PlatformBootManagerLib: call ProcessCapsules() only once Ard Biesheuvel
2018-06-07 15:04 ` Leif Lindholm
2018-06-07 11:08 ` [PATCH 5/5] ArmPkg/ArmSmcPsciResetSystemLib: implement fallback for warm reboot Ard Biesheuvel
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='CAKv+Gu-ZnLu6Sar0=u5JyBN6gonNdSr9yH342eVMcznF=_dzOw@mail.gmail.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