From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::242; helo=mail-io0-x242.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x242.google.com (mail-io0-x242.google.com [IPv6:2607:f8b0:4001:c06::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BD9D02119591C for ; Mon, 11 Jun 2018 14:28:58 -0700 (PDT) Received: by mail-io0-x242.google.com with SMTP id l25-v6so25603881ioh.12 for ; Mon, 11 Jun 2018 14:28:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=nC59xRVqXcAZBY9E38CQlH/Z/0uW7Yn3uyPm26h78oE=; b=JU/LHgNX1kmONFeFE32bYGjMSOty/0zTbUKdfCCNWvtHBFpBE72Z8iE64AR2CGBOgr 6LgadNRvGXfXrdtDnnDokAfY63dgqEaJCCBbRhUE0PFNIHuC0BJSRMqaiZFeAOhuCJsE SAm0XKlVmWcEPl2r1sC1l1ouCiQnKnQmNf5oQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=nC59xRVqXcAZBY9E38CQlH/Z/0uW7Yn3uyPm26h78oE=; b=clKCwkzJK2MApGvWdLbBPi7QjaHneYJDAl9b7HetUmDykYdOdue5PUYLR7hAo8QQKB fhKcRAZZesYJm2ZsDLFtXyXvoJXEnlLYesU8uMzqGGnTVVEaIsKx7uFrR79B5itb0huz I4CquD/AyJ/dINYAk0LDl/74v5FhC21z+6SZyoO3TTfvAsAtBzv8dcuDt07Rr2VJsYKa rvEa3CJdGn9vKOiaFtOb8G0/l5Cxyb5k504g+3/ghN2WHIVVKx38iUMq3rLwopWv0xkq XRocRj5fM6fTMjQIWghjWwoDUk2H5hpTP4dK5eoISre6tdBAxLXSYO8lk1slSnBmFths PTMw== X-Gm-Message-State: APt69E0Da4Wdpai+h+nPDizF5TIrC+aIk/5iAK/tCRt9Qbx5xfB8NSAo UO+ohAPxYYNv6qkKvF8+VxMdupVlNR3XJyY6WKOh+jT8 X-Google-Smtp-Source: ADUXVKKceV/YY0FADiYcocfxfXluW37Zu4zlSwGJVqRbnLhdloIdDTj0aIy0giK3po1gufyuwpXCBxHuIFVSLepRVM0= X-Received: by 2002:a6b:520d:: with SMTP id g13-v6mr941481iob.60.1528752537927; Mon, 11 Jun 2018 14:28:57 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bbc7:0:0:0:0:0 with HTTP; Mon, 11 Jun 2018 14:28:57 -0700 (PDT) In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503AC3DA25@shsmsx102.ccr.corp.intel.com> References: <20180608065811.2065-1-ard.biesheuvel@linaro.org> <20180608065811.2065-2-ard.biesheuvel@linaro.org> <74D8A39837DF1E4DA445A8C0B3885C503AC3DA25@shsmsx102.ccr.corp.intel.com> From: Ard Biesheuvel Date: Mon, 11 Jun 2018 23:28:57 +0200 Message-ID: To: "Yao, Jiewen" Cc: "edk2-devel@lists.01.org" , Leif Lindholm , "Zeng, Star" , "Kinney, Michael D" Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Jun 2018 21:28:59 -0000 Content-Type: text/plain; charset="UTF-8" On 11 June 2018 at 23:27, Yao, Jiewen 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 ; Zeng, Star ; >> Yao, Jiewen ; Kinney, Michael D >> ; Ard Biesheuvel >> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before >> consuming capsule data >> >> On 8 June 2018 at 08:58, Ard Biesheuvel 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 >> > Contributed-under: TianoCore Contribution Agreement 1.1 >> > Signed-off-by: Ard Biesheuvel >> >> 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 >> > >> > #include >> > +#include >> > #include >> > #include >> > #include >> > @@ -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 >> >