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::243; helo=mail-io0-x243.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x243.google.com (mail-io0-x243.google.com [IPv6:2607:f8b0:4001:c06::243]) (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 986B6211B63F0 for ; Thu, 7 Jun 2018 02:52:30 -0700 (PDT) Received: by mail-io0-x243.google.com with SMTP id g7-v6so11082860ioh.11 for ; Thu, 07 Jun 2018 02:52:30 -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=E/EsOvG8COOSX3mxNIUGN83pkJMo0DVlJ78qjTkipjQ=; b=RYtBQZH4iHanWWFpDe37RqdcBisHSOTope9eAieuR94XAYAencmKTpVKDpSJn/HnG/ R9M4gyEzkk/1JPrgQc+JOsse3MrDNZK2tqU79C261G3GG2hh0CfqEm1HJz8qDuL4hxpW FV8pGZ6GwXY77zvQdOYDrf+TCMKOy4u5iUJNU= 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=E/EsOvG8COOSX3mxNIUGN83pkJMo0DVlJ78qjTkipjQ=; b=cmAPaUX/nUynykVfGmYc50tLD3DSrsw9NN18OmfNVrjTHtuMK0DC1Beh+E0WSyVDEj wJkGRmVkK2e7ybMuUpCqyOfXpFJOi+eA1gbyHJQ4tSiKI0sjmD0Hs6cMVkOloVTpsfDR KgPaRus/Wuh3WmX4ZVHz+U3ZpUhBMhqlwxSF1eHJZ9ebLYkGWucw9q8TPvJ9cI3/WiYL jdylLQV29Syt82qw7kZbhCIiTA4ibv/O9if6dIXdInbQKg9DR5NFnz/aO0EmdoehNS9p X1S94zpXuzF4CY9u1YfMXMszAhUmqiv3LQ+nMvRoMwK4DKK2lJRnBXGIB7kQAUs7/XGT BsCw== X-Gm-Message-State: APt69E2UCAg8s2YwfEQ0o6pBoebPgySWxon03lRCglknK7ta27hvQ6+J zhrMHqqweHbjj9H1QhCsh1nZEJFt5ijzIEG7H9Ra+g== X-Google-Smtp-Source: ADUXVKJ/Kkxr2M0pGhQ9fQKnN6rGM/kpFUd9fv3ANibKp8Hf9+xJlPqX7MwJwQEy3hVL+kR0fDQ6NsGheaa3ut4zwic= X-Received: by 2002:a6b:dd0b:: with SMTP id f11-v6mr856763ioc.173.1528365149614; Thu, 07 Jun 2018 02:52:29 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bb86:0:0:0:0:0 with HTTP; Thu, 7 Jun 2018 02:52:29 -0700 (PDT) In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103BB54798@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> From: Ard Biesheuvel Date: Thu, 7 Jun 2018 11:52:29 +0200 Message-ID: To: "Zeng, Star" Cc: "Ni, Ruiyu" , "edk2-devel@lists.01.org" , "Gao, Liming" , "Yao, Jiewen" , Leif Lindholm , "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 09:52:30 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 time= ? > > 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 TRU= E, 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 n= eeds 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 Ar= d 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 <= leif.lindholm@linaro.org>; 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 Up= dateCapsule? UpdateCapsule? Consumer of capsule data? >> > > Unfortunately, since the spec does not mention it at all, we cannot rely = on the caller of UpdateCapsule() to ensure this. That would require a spec = change, and break backward compatibility with older revisions. > > The main issue here is that ARM does not provide the means to clean the e= ntire cache, the only architectural method is clean cacheline by virtual ad= dress. > > 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 address= in memory, and can perform the cache maintenance right before consuming th= e data. > > So unless anyone has any other suggestions, I think this approach is the = only feasible one. > > If there are any concerns about adding this for architectures, I can look= 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 virtual= address. >>> >> >> It has the virtual address of the capsules yes. But how about the data s= tructures 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 wro= te: >>>> 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 o= f UpdateCapsule(), since the ScatterGatherList structures are only identifi= ed by physical address, and at runtime, the firmware doesn't know whether a= nd where this memory is mapped, and cache maintenance requires a virtual ad= dress. >>> """ >>> >>>> --- >>>> 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, E= ITHER 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 MemoryResour= ce[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