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::244; helo=mail-it0-x244.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x244.google.com (mail-it0-x244.google.com [IPv6:2607:f8b0:4001:c0b::244]) (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 046C22118AA8E for ; Thu, 7 Jun 2018 03:27:58 -0700 (PDT) Received: by mail-it0-x244.google.com with SMTP id u4-v6so12173763itg.0 for ; Thu, 07 Jun 2018 03:27: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:content-transfer-encoding; bh=cdlc4ooEth9eOhKG4KFeq76mY0CgPwScvjIo/H4Mmj4=; b=UPuPgzjNfRsrSifDetsSn0k6gOri6AgcD41c0Y5rVg1VcfX4znhHJJ4S+VRI0TsvGQ tXrbALphHz+S7CiOWO1vLXRLoXn84uT1bnzRaRTw0OjwQE/dilVkiEOPyYNQ3Yf56q2e oCs4KjAy/MjAttihwGt5AsXP+a13dz63RUKxs= 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:content-transfer-encoding; bh=cdlc4ooEth9eOhKG4KFeq76mY0CgPwScvjIo/H4Mmj4=; b=Asazd7+xanLdzkdc1sQr8xdy2TmOh4BwD74v7nTEGdb3kVIvhjpWwqk32Xg4rt6/Fl JpBSU69/STCCavHzzhU+E7VmwvcmqoBMxmRcu8cIkSrqJTayqkhFlY3SXRb35uoMLNJG vyeQhHXCxGwLt9URnM0OazbIHyli0p0hgjKtdXk0QaBgdgRhtOvfsascsj+P7iz7xyfY jnWUAISmsux/17hyUAYC3F14dUvX98yD9p+gGO6VhIsv/OODdrbx2859HI1uid9fSElb 8FMXgsNK7bvbmMX5tMbVr2l7PFnEygL16s9DnMCXdCiaUxn0qva0pfUf9kXvzyWn1mX9 1fDQ== X-Gm-Message-State: APt69E1I4rhU6N9bOI4gc31JZ8yDmfWSgmeTYfpHCn0g1HggVL23Ik+v WSLwXV/ZixzsdB3C8vNiWDd16by7qHdGtSor+MigCA== X-Google-Smtp-Source: ADUXVKJ/SHiB5vlB+dYF46PrafJGrZnds43+wFdJuUxdc5i6TVF5NmCMWxPnjaUixjF2Ca/6wzGwP7vm8cvXhcncNLQ= X-Received: by 2002:a24:534e:: with SMTP id n75-v6mr1308262itb.138.1528367277783; Thu, 07 Jun 2018 03:27:57 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bb86:0:0:0:0:0 with HTTP; Thu, 7 Jun 2018 03:27:57 -0700 (PDT) In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103BB5480F@shsmsx102.ccr.corp.intel.com> References: <20180606095235.20822-1-ard.biesheuvel@linaro.org> <0C09AFA07DD0434D9E2A0C6AEB0483103BB542D7@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103BB54543@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103BB54798@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103BB547E7@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103BB5480F@shsmsx102.ccr.corp.intel.com> From: Ard Biesheuvel Date: Thu, 7 Jun 2018 12:27:57 +0200 Message-ID: To: "Zeng, Star" Cc: "Ni, Ruiyu" , "edk2-devel@lists.01.org" , Leif Lindholm , "Yao, Jiewen" , "Gao, Liming" , "Kinney, Michael D" Subject: Re: [PATCH] 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: Thu, 07 Jun 2018 10:27:59 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On 7 June 2018 at 12:27, Zeng, Star wrote: > I think checking validity is other code's responsibility, after that the = new code to perform cache maintenance. They are separated. > I prefer to cover both paths. > OK, fair enough. > > Thanks, > Star > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ar= d Biesheuvel > Sent: Thursday, June 7, 2018 6:14 PM > To: Zeng, Star > Cc: Ni, Ruiyu ; edk2-devel@lists.01.org; Leif Lindhol= m ; Yao, Jiewen ; Gao, Limi= ng ; Kinney, Michael D > Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache before = consuming capsule data > > On 7 June 2018 at 12:12, Zeng, Star wrote: >> Since capsule data pointer may be invalid (for example, point to MMIO), = we enhanced code to validate the capsule by memory resource HOB, and *recom= mend* platform/silicon (memory reference code) to report memory resource HO= B before capsule coalescing. >> To consider and compatible with some platform may not report memory reso= urce HOB before capsule coalescing, then MemoryResource =3D=3D NULL and the= code thinks it is valid. >> > > OK. I think it is a bad idea to perform cache maintenance on an address t= hat may be invalid. So I'd prefer to keep the single invocation. > >> >> Thanks, >> Star >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: Thursday, June 7, 2018 5:52 PM >> To: Zeng, Star >> Cc: Ni, Ruiyu ; edk2-devel@lists.01.org; Gao, >> Liming ; Yao, Jiewen ; >> Leif Lindholm ; Kinney, Michael D >> >> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >> before consuming capsule data >> >> On 7 June 2018 at 11:46, Zeng, Star wrote: >>> Ok, I want to know whether others have some idea, so let's wait some ti= me? >>> >>> About the code change, I have three minor comments below. >>> 1. I suggest adding some code comment for the new line code. >> >> OK >> >>> 2. There are two paths in ValidateCapsuleByMemoryResource() to return T= RUE, should the new line code be added for both of them? >> >> Good question: In which circumstances is the MemoryResource =3D=3D NULL = case expected to occur? >> >>> 3. There is VS2015 building failure like below with the patch. The code= needs to typecast parameter 'Size'. >>> >>> warning C4244: 'function': conversion from 'UINT64' to 'UINTN', >>> possible loss of data >>> >> >> Will fix. >> >>> >>> Thanks, >>> Star >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf >>> Of Ard Biesheuvel >>> Sent: Thursday, June 7, 2018 2:00 PM >>> To: Zeng, Star >>> Cc: Ni, Ruiyu ; edk2-devel@lists.01.org; Gao, >>> Liming ; Yao, Jiewen ; >>> Leif Lindholm ; Kinney, Michael D >>> >>> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >>> before consuming capsule data >>> >>> On 7 June 2018 at 07:41, Zeng, Star wrote: >>>> Thanks, got the point. >>>> >>>> It seems vague that who to ensure the cache coherency. MMU? Caller of = UpdateCapsule? UpdateCapsule? Consumer of capsule data? >>>> >>> >>> Unfortunately, since the spec does not mention it at all, we cannot rel= y on the caller of UpdateCapsule() to ensure this. That would require a spe= c change, and break backward compatibility with older revisions. >>> >>> The main issue here is that ARM does not provide the means to clean the= entire cache, the only architectural method is clean cacheline by virtual = address. >>> >>> So that leaves: >>> - UpdateCapsule - problematic because it may be called at runtime and >>> the firmware has no means of translating the physical addresses >>> - ResetSystem - same as above: it is a runtime service, and so it >>> cannot rely on a mapping to exist for those physical addresses >>> - SEC - lacks the information about where the capsule resides >>> - CapsulePei - already extracts the information about the capsule addre= ss in memory, and can perform the cache maintenance right before consuming = the data. >>> >>> So unless anyone has any other suggestions, I think this approach is th= e only feasible one. >>> >>> If there are any concerns about adding this for architectures, I can lo= ok into refactoring CapsulePei and add it only for ARM/AARCH64. >>> >>> Thanks, >>> Ard. >>> >>> >>> >>>> -----Original Message----- >>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf >>>> Of Ard Biesheuvel >>>> Sent: Thursday, June 7, 2018 12:50 PM >>>> To: Zeng, Star >>>> Cc: Ni, Ruiyu ; edk2-devel@lists.01.org; Leif >>>> Lindholm ; Yao, Jiewen >>>> ; Gao, Liming ; Kinney, >>>> Michael D >>>> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >>>> before consuming capsule data >>>> >>>> On 7 June 2018 at 03:37, Zeng, Star wrote: >>>>> Hi Ard, >>>>> >>>>> The input parameter CapsuleHeaderArray of UpdateCapsule has the virtu= al address. >>>>> >>>> >>>> It has the virtual address of the capsules yes. But how about the data= structures passed as the ScatterGatherList? >>>> >>>> >>>>> CapsuleHeaderArray >>>>> Virtual pointer to an array of virtual pointers to the capsules >>>>> being passed into update capsule. Each capsules is assumed to >>>>> stored in contiguous virtual memory. The capsules in the >>>>> CapsuleHeaderArray must be the same capsules as the >>>>> ScatterGatherList. The CapsuleHeaderArray must have the >>>>> >>>>> >>>>> Thanks, >>>>> Star >>>>> -----Original Message----- >>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf >>>>> Of Ard Biesheuvel >>>>> Sent: Wednesday, June 6, 2018 8:10 PM >>>>> To: edk2-devel@lists.01.org >>>>> Cc: Ni, Ruiyu ; Ard Biesheuvel >>>>> ; Gao, Liming ; >>>>> Yao, Jiewen ; Leif Lindholm >>>>> ; Kinney, Michael D >>>>> ; Zeng, Star >>>>> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >>>>> before consuming capsule data >>>>> >>>>> On 6 June 2018 at 11:52, Ard Biesheuvel w= rote: >>>>>> 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. >>>>>> >>>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>>> Signed-off-by: Ard Biesheuvel >>>>> >>>>> Leif asked me to include a note why this cannot be done when >>>>> UpdateCapsule() is called. >>>>> >>>>> >>>>> """ >>>>> Note that this cache maintenance cannot be done during the invocation= of UpdateCapsule(), since the ScatterGatherList structures are only identi= fied 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. >>>>> """ >>>>> >>>>>> --- >>>>>> MdeModulePkg/Universal/CapsulePei/CapsulePei.inf | 1 + >>>>>> MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c | 4 >>>>>> ++++ >>>>>> 2 files changed, 5 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..1730f925adc5 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 >>>>>> @@ -283,6 +284,9 @@ ValidateCapsuleByMemoryResource ( >>>>>> DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryReso= urce[0x%x] - Start(0x%lx) Length(0x%lx)\n", >>>>>> Address, Size, >>>>>> Index, >>>>>> MemoryResource[Index].PhysicalStart, >>>>>> MemoryResource[Index].ResourceLength)); >>>>>> + >>>>>> + WriteBackDataCacheRange ((VOID *)(UINTN)Address, Size); >>>>>> + >>>>>> return TRUE; >>>>>> } >>>>>> } >>>>>> -- >>>>>> 2.17.0 >>>>>> >>>>> _______________________________________________ >>>>> edk2-devel mailing list >>>>> edk2-devel@lists.01.org >>>>> https://lists.01.org/mailman/listinfo/edk2-devel >>>> _______________________________________________ >>>> edk2-devel mailing list >>>> edk2-devel@lists.01.org >>>> https://lists.01.org/mailman/listinfo/edk2-devel >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel