From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.136; helo=mga12.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id EC25B2118AA8C for ; Thu, 7 Jun 2018 03:27:15 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 Jun 2018 03:27:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,486,1520924400"; d="scan'208";a="45341722" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga007.fm.intel.com with ESMTP; 07 Jun 2018 03:27:15 -0700 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 7 Jun 2018 03:27:15 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx111.amr.corp.intel.com (10.18.116.5) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 7 Jun 2018 03:27:14 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.223]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.70]) with mapi id 14.03.0319.002; Thu, 7 Jun 2018 18:27:13 +0800 From: "Zeng, Star" To: Ard Biesheuvel CC: "Ni, Ruiyu" , "edk2-devel@lists.01.org" , Leif Lindholm , "Yao, Jiewen" , "Gao, Liming" , "Kinney, Michael D" , "Zeng, Star" Thread-Topic: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data Thread-Index: AQHT/Xws8+sQHdZJdE2R/MRK1zEavKRSnX+AgAFnCPD//7BugIAAkwhw//+AeYCAAMMU4P//fd+AABD6JVD//34zgP//dyeQ Date: Thu, 7 Jun 2018 10:27:12 +0000 Message-ID: <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> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 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:16 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable I think checking validity is other code's responsibility, after that the ne= w code to perform cache maintenance. They are separated. I prefer to cover both paths. Thanks, Star -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard = Biesheuvel Sent: Thursday, June 7, 2018 6:14 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 co= nsuming capsule data On 7 June 2018 at 12:12, Zeng, Star wrote: > Since capsule data pointer may be invalid (for example, point to MMIO), w= e enhanced code to validate the capsule by memory resource HOB, and *recomm= end* platform/silicon (memory reference code) to report memory resource HOB= before capsule coalescing. > To consider and compatible with some platform may not report memory resou= rce 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 tha= t 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,=20 > Liming ; Yao, Jiewen ;=20 > Leif Lindholm ; Kinney, Michael D=20 > > Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache=20 > 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 tim= e? >> >> 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 TR= UE, should the new line code be added for both of them? > > Good question: In which circumstances is the MemoryResource =3D=3D NULL c= ase 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',=20 >> possible loss of data >> > > Will fix. > >> >> Thanks, >> Star >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf=20 >> Of Ard Biesheuvel >> Sent: Thursday, June 7, 2018 2:00 PM >> To: Zeng, Star >> Cc: Ni, Ruiyu ; edk2-devel@lists.01.org; Gao,=20 >> Liming ; Yao, Jiewen ;=20 >> Leif Lindholm ; Kinney, Michael D=20 >> >> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache=20 >> 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 U= pdateCapsule? 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 = entire cache, the only architectural method is clean cacheline by virtual a= ddress. >> >> So that leaves: >> - UpdateCapsule - problematic because it may be called at runtime and=20 >> the firmware has no means of translating the physical addresses >> - ResetSystem - same as above: it is a runtime service, and so it=20 >> 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 addres= s in memory, and can perform the cache maintenance right before consuming t= he 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 loo= k 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=20 >>> Of Ard Biesheuvel >>> Sent: Thursday, June 7, 2018 12:50 PM >>> To: Zeng, Star >>> Cc: Ni, Ruiyu ; edk2-devel@lists.01.org; Leif=20 >>> Lindholm ; Yao, Jiewen=20 >>> ; Gao, Liming ; Kinney,=20 >>> Michael D >>> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache=20 >>> before consuming capsule data >>> >>> On 7 June 2018 at 03:37, Zeng, Star wrote: >>>> Hi Ard, >>>> >>>> The input parameter CapsuleHeaderArray of UpdateCapsule has the virtua= l 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=20 >>>> being passed into update capsule. Each capsules is assumed to=20 >>>> stored in contiguous virtual memory. The capsules in the=20 >>>> CapsuleHeaderArray must be the same capsules as the=20 >>>> ScatterGatherList. The CapsuleHeaderArray must have the >>>> >>>> >>>> Thanks, >>>> Star >>>> -----Original Message----- >>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf=20 >>>> Of Ard Biesheuvel >>>> Sent: Wednesday, June 6, 2018 8:10 PM >>>> To: edk2-devel@lists.01.org >>>> Cc: Ni, Ruiyu ; Ard Biesheuvel=20 >>>> ; Gao, Liming ;=20 >>>> Yao, Jiewen ; Leif Lindholm=20 >>>> ; Kinney, Michael D=20 >>>> ; Zeng, Star >>>> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache=20 >>>> before consuming capsule data >>>> >>>> On 6 June 2018 at 11:52, Ard Biesheuvel wr= ote: >>>>> When capsule updates are staged for processing after a warm=20 >>>>> reboot, they are copied into memory with the MMU and caches=20 >>>>> enabled. When the capsule PEI gets around to coalescing the=20 >>>>> capsule, the MMU and caches may still be disabled, and so on=20 >>>>> architectures where uncached accesses are incoherent with the=20 >>>>> 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 identif= ied by physical address, and at runtime, the firmware doesn't know whether = and where this memory is mapped, and cache maintenance requires a virtual a= ddress. >>>> """ >>>> >>>>> --- >>>>> 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 MemoryResou= rce[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