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:c0b::242; helo=mail-it0-x242.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x242.google.com (mail-it0-x242.google.com [IPv6:2607:f8b0:4001:c0b::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 2178F21190721 for ; Mon, 11 Jun 2018 14:24:44 -0700 (PDT) Received: by mail-it0-x242.google.com with SMTP id 76-v6so12145227itx.4 for ; Mon, 11 Jun 2018 14:24:44 -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=bxHeBMj7kr5cVXtzt31HRxvE/2QTLAy9F8ZohlrDv2U=; b=TGnFJfrP4xG/HY82DjjMJzjjWhu4RslyDaM3JqpkgKTngk6QTNhQH07K4RlyU1bXNS PJVcHxbnS/CtdEm1N5BDbUNypbZZqYHQdss13eQ7+FQA0nbrIFf1+w7WOWRB57NA+8Hg onDWEOVRPNW6k7Rnr5Q1Gafhwo0qeUNrk9WDw= 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=bxHeBMj7kr5cVXtzt31HRxvE/2QTLAy9F8ZohlrDv2U=; b=ngNPb86epOOv9bZZdN8KyD2XiHfS4erwFk1KHVV2ra2uH8R4CsTFzSDgQJ7DyCMxwN D746rvNCpsUcBhrW2UoewCH0HLkHLSxopXl1HKp+YeqtWKFBzHhAcsKBqtpJpHMY+TAS UhHi9dw1bpICqdgLCA9OWN0qjchMJfhi+bOtaGdZWFsWDiMWUp+Xqh5BLxBSYyT5jHyq CUwWYggjV3Ie4S9pWX00P0t4fJDQAPgNetSCpxs8N4ETJawW1f1F9M7hM3nRmjgPuAQr +W9rSdznSti8ZNBrJPMBjLUz6ZlEe1bqrh2Yybs/wVOioG8nkwocfB/QmuZAhVWVh8FN Fyag== X-Gm-Message-State: APt69E1Tg8wTbjFIUwNLz3jcX1KzbrtsiDbAdr4XBW5fjZOuUKGlMPau Tu77DO860SDyKeOjZEHEoVi/cuoKArWrkLpAR1th4RKe X-Google-Smtp-Source: ADUXVKLXCpHOj96P3CnfwA87Xq8FDcuYGlI4TLcfojebhwMy/nkuq029CH9DNxoeDgak0sKyqWzEa22lmKLBQvO00B0= X-Received: by 2002:a24:3105:: with SMTP id y5-v6mr694758ity.138.1528752283435; Mon, 11 Jun 2018 14:24:43 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bbc7:0:0:0:0:0 with HTTP; Mon, 11 Jun 2018 14:24:42 -0700 (PDT) In-Reply-To: <20180608065811.2065-2-ard.biesheuvel@linaro.org> References: <20180608065811.2065-1-ard.biesheuvel@linaro.org> <20180608065811.2065-2-ard.biesheuvel@linaro.org> From: Ard Biesheuvel Date: Mon, 11 Jun 2018 23:24:42 +0200 Message-ID: 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 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:24:45 -0000 Content-Type: text/plain; charset="UTF-8" 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 >