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::243; helo=mail-it0-x243.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x243.google.com (mail-it0-x243.google.com [IPv6:2607:f8b0:4001:c0b::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 04FDD211DCDA3 for ; Thu, 7 Jun 2018 03:14:02 -0700 (PDT) Received: by mail-it0-x243.google.com with SMTP id 144-v6so12075047iti.5 for ; Thu, 07 Jun 2018 03:14:02 -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=pKCnUC5z7vW7ezxIYjCE11z+tjUvR9X82d4aWRpcfpE=; b=EnfUH5XM3URX7/dSjfxt3uUGhnCy5a74HfgZJAPkKdS63fWNgXnGJLZJiJH5pQ/8Kt qALo4gkz/11rDbmExsnIcKS/SDYvBb8XIC41E8U3FxWbhV0lHk5s95+IrwpCFBDGRhEq CLGl7ZCofzZqS96788A/c/jnMjX9PcTHs12sI= 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=pKCnUC5z7vW7ezxIYjCE11z+tjUvR9X82d4aWRpcfpE=; b=GRg9Dtddlz7E3I9G3Z2hTpE6Io9rZ/Oe57FpVUOl2J0xVjFLhM/80yIBxNRcG/Vb/6 XFpc3ktjRy6ZlHXravPtFfaSCbt9xBpxRXpzsZJg3zgv+GQXrMjc3pb3QzfvwTfzMnPx +peJ6xIWaKPSoEiFPA1dMpkwZ5YQFNgDP+JDb0qMnDAyqS5kLLbJofn9qKMGIR7ZZ8rI Yc+PmFja1pXbujLM/7iublguZpifx/a5AjsYA1yW6rL8ekvtMUxAgjr9r8W7jQ7p85tv CZbKsPVhcGIDtRNV61tm+ZMc3SlF8FeGzYEwTdjh6OHGgFBNvqb5TtXKld6yUJ684IQU yqnQ== X-Gm-Message-State: APt69E2dLbN3xwoBnUVM6mKjdLHYjvEnvhI3+o0JSP1HKTAdb2mQGcqd SdSff7m8AddfWNxgChv7OoTIjvsS9bVsycjwVyVlfQ== X-Google-Smtp-Source: ADUXVKI9pJiWbhZipIqy/irRJBfWMs58NOP07xCBv0wGSmYhQG3OH2pqiISaljaXyH/xnyj59zcLKe4Wy9vgxZB5Vn0= X-Received: by 2002:a24:e105:: with SMTP id n5-v6mr1262582ith.68.1528366442229; Thu, 07 Jun 2018 03:14:02 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bb86:0:0:0:0:0 with HTTP; Thu, 7 Jun 2018 03:14:01 -0700 (PDT) In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103BB547E7@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> From: Ard Biesheuvel Date: Thu, 7 Jun 2018 12:14:01 +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 10:14:03 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 that 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 <= leif.lindholm@linaro.org>; 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 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', >> 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 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 >> 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 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 >>> 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 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 >>>> 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 wr= ote: >>>>> 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 memo= ry 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