* [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data @ 2018-06-06 9:52 Ard Biesheuvel 2018-06-06 12:09 ` Ard Biesheuvel 0 siblings, 1 reply; 15+ messages in thread From: Ard Biesheuvel @ 2018-06-06 9:52 UTC (permalink / raw) To: edk2-devel Cc: leif.lindholm, jiewen.yao, ruiyu.ni, star.zeng, michael.d.kinney, liming.gao, Ard Biesheuvel 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 <ard.biesheuvel@linaro.org> --- 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 <Guid/CapsuleVendor.h> #include <Library/BaseMemoryLib.h> +#include <Library/CacheMaintenanceLib.h> #include <Library/DebugLib.h> #include <Library/PrintLib.h> #include <Library/BaseLib.h> @@ -283,6 +284,9 @@ ValidateCapsuleByMemoryResource ( 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)); + + WriteBackDataCacheRange ((VOID *)(UINTN)Address, Size); + return TRUE; } } -- 2.17.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data 2018-06-06 9:52 [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data Ard Biesheuvel @ 2018-06-06 12:09 ` Ard Biesheuvel 2018-06-06 13:59 ` Yao, Jiewen ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Ard Biesheuvel @ 2018-06-06 12:09 UTC (permalink / raw) To: edk2-devel@lists.01.org Cc: Leif Lindholm, Yao, Jiewen, Ruiyu Ni, Zeng, Star, Kinney, Michael D, Gao, Liming, Ard Biesheuvel On 6 June 2018 at 11:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> 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 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. """ > --- > 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 <Guid/CapsuleVendor.h> > > #include <Library/BaseMemoryLib.h> > +#include <Library/CacheMaintenanceLib.h> > #include <Library/DebugLib.h> > #include <Library/PrintLib.h> > #include <Library/BaseLib.h> > @@ -283,6 +284,9 @@ ValidateCapsuleByMemoryResource ( > 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)); > + > + WriteBackDataCacheRange ((VOID *)(UINTN)Address, Size); > + > return TRUE; > } > } > -- > 2.17.0 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data 2018-06-06 12:09 ` Ard Biesheuvel @ 2018-06-06 13:59 ` Yao, Jiewen 2018-06-06 16:07 ` Kinney, Michael D 2018-06-07 1:37 ` Zeng, Star 2 siblings, 0 replies; 15+ messages in thread From: Yao, Jiewen @ 2018-06-06 13:59 UTC (permalink / raw) To: Ard Biesheuvel, edk2-devel@lists.01.org Cc: Ni, Ruiyu, Gao, Liming, Leif Lindholm, Kinney, Michael D, Zeng, Star Thanks. Please also include the note in the final commit message. Reviewed-by: Jiewen.yao@intel.com > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard > Biesheuvel > Sent: Wednesday, June 6, 2018 5:10 AM > To: edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; > Gao, Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Leif > Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D > <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache before > consuming capsule data > > On 6 June 2018 at 11:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > 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 > 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. > """ > > > --- > > 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 <Guid/CapsuleVendor.h> > > > > #include <Library/BaseMemoryLib.h> > > +#include <Library/CacheMaintenanceLib.h> > > #include <Library/DebugLib.h> > > #include <Library/PrintLib.h> > > #include <Library/BaseLib.h> > > @@ -283,6 +284,9 @@ ValidateCapsuleByMemoryResource ( > > 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)); > > + > > + 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data 2018-06-06 12:09 ` Ard Biesheuvel 2018-06-06 13:59 ` Yao, Jiewen @ 2018-06-06 16:07 ` Kinney, Michael D 2018-06-06 16:13 ` Ard Biesheuvel 2018-06-07 1:37 ` Zeng, Star 2 siblings, 1 reply; 15+ messages in thread From: Kinney, Michael D @ 2018-06-06 16:07 UTC (permalink / raw) To: Ard Biesheuvel, edk2-devel@lists.01.org, Kinney, Michael D Cc: Leif Lindholm, Yao, Jiewen, Ni, Ruiyu, Zeng, Star, Gao, Liming Ard, Thanks for adding the note. I was thinking that this could be done just before ResetSystem(). It could also be done in SEC phase. Since capsules are just one use of warm reset, would it make more sense to flush all the caches in either warm reset of SEC instead of just the ranges used by capsules. Thanks, Mike > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: Wednesday, June 6, 2018 5:10 AM > To: edk2-devel@lists.01.org > Cc: Leif Lindholm <leif.lindholm@linaro.org>; Yao, Jiewen > <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; > Zeng, Star <star.zeng@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Gao, Liming > <liming.gao@intel.com>; Ard Biesheuvel > <ard.biesheuvel@linaro.org> > Subject: Re: [PATCH] MdeModulePkg/CapsulePei: clean Dcache > before consuming capsule data > > On 6 June 2018 at 11:52, Ard Biesheuvel > <ard.biesheuvel@linaro.org> 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. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > 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 > 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. > """ > > > --- > > 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 <Guid/CapsuleVendor.h> > > > > #include <Library/BaseMemoryLib.h> > > +#include <Library/CacheMaintenanceLib.h> > > #include <Library/DebugLib.h> > > #include <Library/PrintLib.h> > > #include <Library/BaseLib.h> > > @@ -283,6 +284,9 @@ ValidateCapsuleByMemoryResource ( > > 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)); > > + > > + WriteBackDataCacheRange ((VOID *)(UINTN)Address, > Size); > > + > > return TRUE; > > } > > } > > -- > > 2.17.0 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data 2018-06-06 16:07 ` Kinney, Michael D @ 2018-06-06 16:13 ` Ard Biesheuvel 0 siblings, 0 replies; 15+ messages in thread From: Ard Biesheuvel @ 2018-06-06 16:13 UTC (permalink / raw) To: Kinney, Michael D Cc: edk2-devel@lists.01.org, Leif Lindholm, Yao, Jiewen, Ni, Ruiyu, Zeng, Star, Gao, Liming On 6 June 2018 at 18:07, Kinney, Michael D <michael.d.kinney@intel.com> wrote: > Ard, > > Thanks for adding the note. I was thinking that this could be done > just before ResetSystem(). It could also be done in SEC phase. > > Since capsules are just one use of warm reset, would it make more sense > to flush all the caches in either warm reset of SEC instead of just > the ranges used by capsules. > The ARM architecture does not provide for that, unfortunately. The only architected cache maintenance that guarantees that dirty cachelines make it all the way to memory (point of coherency or PoC in ARM parlance) is to clean the caches by virtual address, and cleaning all of memory by VA is intractible. The architecture does provide clean by set/way operations, but those operate on each cache level individually, and only on architected cache levels. (The architecture permits so-called system caches that whose set/way maintenance is implementation defined, and only maintenance by VA is guaranteed to clean the data to main memory) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data 2018-06-06 12:09 ` Ard Biesheuvel 2018-06-06 13:59 ` Yao, Jiewen 2018-06-06 16:07 ` Kinney, Michael D @ 2018-06-07 1:37 ` Zeng, Star 2018-06-07 4:50 ` Ard Biesheuvel 2 siblings, 1 reply; 15+ messages in thread From: Zeng, Star @ 2018-06-07 1:37 UTC (permalink / raw) To: Ard Biesheuvel, edk2-devel@lists.01.org Cc: Ni, Ruiyu, Gao, Liming, Yao, Jiewen, Leif Lindholm, Kinney, Michael D, Zeng, Star Hi Ard, The input parameter CapsuleHeaderArray of UpdateCapsule has the virtual address. 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 <ruiyu.ni@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data On 6 June 2018 at 11:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> 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 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. """ > --- > 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 <Guid/CapsuleVendor.h> > > #include <Library/BaseMemoryLib.h> > +#include <Library/CacheMaintenanceLib.h> > #include <Library/DebugLib.h> > #include <Library/PrintLib.h> > #include <Library/BaseLib.h> > @@ -283,6 +284,9 @@ ValidateCapsuleByMemoryResource ( > 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)); > + > + 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data 2018-06-07 1:37 ` Zeng, Star @ 2018-06-07 4:50 ` Ard Biesheuvel 2018-06-07 5:41 ` Zeng, Star 0 siblings, 1 reply; 15+ messages in thread From: Ard Biesheuvel @ 2018-06-07 4:50 UTC (permalink / raw) To: Zeng, Star Cc: edk2-devel@lists.01.org, Ni, Ruiyu, Gao, Liming, Yao, Jiewen, Leif Lindholm, Kinney, Michael D On 7 June 2018 at 03:37, Zeng, Star <star.zeng@intel.com> 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 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 <ruiyu.ni@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data > > On 6 June 2018 at 11:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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. >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > 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 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. > """ > >> --- >> 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 <Guid/CapsuleVendor.h> >> >> #include <Library/BaseMemoryLib.h> >> +#include <Library/CacheMaintenanceLib.h> >> #include <Library/DebugLib.h> >> #include <Library/PrintLib.h> >> #include <Library/BaseLib.h> >> @@ -283,6 +284,9 @@ ValidateCapsuleByMemoryResource ( >> 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)); >> + >> + 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data 2018-06-07 4:50 ` Ard Biesheuvel @ 2018-06-07 5:41 ` Zeng, Star 2018-06-07 6:00 ` Ard Biesheuvel 0 siblings, 1 reply; 15+ messages in thread From: Zeng, Star @ 2018-06-07 5:41 UTC (permalink / raw) To: Ard Biesheuvel Cc: Ni, Ruiyu, edk2-devel@lists.01.org, Leif Lindholm, Yao, Jiewen, Gao, Liming, Kinney, Michael D, Zeng, Star Thanks, got the point. It seems vague that who to ensure the cache coherency. MMU? Caller of UpdateCapsule? UpdateCapsule? Consumer of capsule data? Thanks, Star -----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 <star.zeng@intel.com> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>; Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data On 7 June 2018 at 03:37, Zeng, Star <star.zeng@intel.com> 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 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 <ruiyu.ni@intel.com>; Ard Biesheuvel > <ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>; Yao, > Jiewen <jiewen.yao@intel.com>; Leif Lindholm > <leif.lindholm@linaro.org>; Kinney, Michael D > <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache > before consuming capsule data > > On 6 June 2018 at 11:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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. >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > 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 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. > """ > >> --- >> 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 <Guid/CapsuleVendor.h> >> >> #include <Library/BaseMemoryLib.h> >> +#include <Library/CacheMaintenanceLib.h> >> #include <Library/DebugLib.h> >> #include <Library/PrintLib.h> >> #include <Library/BaseLib.h> >> @@ -283,6 +284,9 @@ ValidateCapsuleByMemoryResource ( >> 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)); >> + >> + 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data 2018-06-07 5:41 ` Zeng, Star @ 2018-06-07 6:00 ` Ard Biesheuvel 2018-06-07 9:46 ` Zeng, Star 0 siblings, 1 reply; 15+ messages in thread From: Ard Biesheuvel @ 2018-06-07 6:00 UTC (permalink / raw) To: Zeng, Star Cc: Ni, Ruiyu, edk2-devel@lists.01.org, Leif Lindholm, Yao, Jiewen, Gao, Liming, Kinney, Michael D On 7 June 2018 at 07:41, Zeng, Star <star.zeng@intel.com> 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 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 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 address 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 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 <star.zeng@intel.com> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>; Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com> > Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data > > On 7 June 2018 at 03:37, Zeng, Star <star.zeng@intel.com> 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 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 <ruiyu.ni@intel.com>; Ard Biesheuvel >> <ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>; Yao, >> Jiewen <jiewen.yao@intel.com>; Leif Lindholm >> <leif.lindholm@linaro.org>; Kinney, Michael D >> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> >> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >> before consuming capsule data >> >> On 6 June 2018 at 11:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> >> 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 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. >> """ >> >>> --- >>> 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 <Guid/CapsuleVendor.h> >>> >>> #include <Library/BaseMemoryLib.h> >>> +#include <Library/CacheMaintenanceLib.h> >>> #include <Library/DebugLib.h> >>> #include <Library/PrintLib.h> >>> #include <Library/BaseLib.h> >>> @@ -283,6 +284,9 @@ ValidateCapsuleByMemoryResource ( >>> 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)); >>> + >>> + 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data 2018-06-07 6:00 ` Ard Biesheuvel @ 2018-06-07 9:46 ` Zeng, Star 2018-06-07 9:52 ` Ard Biesheuvel 0 siblings, 1 reply; 15+ messages in thread From: Zeng, Star @ 2018-06-07 9:46 UTC (permalink / raw) To: Ard Biesheuvel Cc: Ni, Ruiyu, edk2-devel@lists.01.org, Gao, Liming, Yao, Jiewen, Leif Lindholm, Kinney, Michael D, Zeng, Star 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. 2. There are two paths in ValidateCapsuleByMemoryResource() to return TRUE, should the new line code be added for both of them? 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 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 <star.zeng@intel.com> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data On 7 June 2018 at 07:41, Zeng, Star <star.zeng@intel.com> 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 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 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 address 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 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 <star.zeng@intel.com> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Leif > Lindholm <leif.lindholm@linaro.org>; Yao, Jiewen > <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, > Michael D <michael.d.kinney@intel.com> > Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache > before consuming capsule data > > On 7 June 2018 at 03:37, Zeng, Star <star.zeng@intel.com> 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 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 <ruiyu.ni@intel.com>; Ard Biesheuvel >> <ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>; Yao, >> Jiewen <jiewen.yao@intel.com>; Leif Lindholm >> <leif.lindholm@linaro.org>; Kinney, Michael D >> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> >> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >> before consuming capsule data >> >> On 6 June 2018 at 11:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> >> 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 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. >> """ >> >>> --- >>> 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 <Guid/CapsuleVendor.h> >>> >>> #include <Library/BaseMemoryLib.h> >>> +#include <Library/CacheMaintenanceLib.h> >>> #include <Library/DebugLib.h> >>> #include <Library/PrintLib.h> >>> #include <Library/BaseLib.h> >>> @@ -283,6 +284,9 @@ ValidateCapsuleByMemoryResource ( >>> 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)); >>> + >>> + 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data 2018-06-07 9:46 ` Zeng, Star @ 2018-06-07 9:52 ` Ard Biesheuvel 2018-06-07 10:12 ` Zeng, Star 0 siblings, 1 reply; 15+ messages in thread From: Ard Biesheuvel @ 2018-06-07 9:52 UTC (permalink / raw) To: Zeng, Star Cc: Ni, Ruiyu, edk2-devel@lists.01.org, Gao, Liming, Yao, Jiewen, Leif Lindholm, Kinney, Michael D On 7 June 2018 at 11:46, Zeng, Star <star.zeng@intel.com> 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 TRUE, should the new line code be added for both of them? Good question: In which circumstances is the MemoryResource == 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 <star.zeng@intel.com> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com> > Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data > > On 7 June 2018 at 07:41, Zeng, Star <star.zeng@intel.com> 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 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 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 address 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 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 <star.zeng@intel.com> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Leif >> Lindholm <leif.lindholm@linaro.org>; Yao, Jiewen >> <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, >> Michael D <michael.d.kinney@intel.com> >> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >> before consuming capsule data >> >> On 7 June 2018 at 03:37, Zeng, Star <star.zeng@intel.com> 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 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 <ruiyu.ni@intel.com>; Ard Biesheuvel >>> <ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>; Yao, >>> Jiewen <jiewen.yao@intel.com>; Leif Lindholm >>> <leif.lindholm@linaro.org>; Kinney, Michael D >>> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> >>> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >>> before consuming capsule data >>> >>> On 6 June 2018 at 11:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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. >>>> >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> >>> 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 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. >>> """ >>> >>>> --- >>>> 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 <Guid/CapsuleVendor.h> >>>> >>>> #include <Library/BaseMemoryLib.h> >>>> +#include <Library/CacheMaintenanceLib.h> >>>> #include <Library/DebugLib.h> >>>> #include <Library/PrintLib.h> >>>> #include <Library/BaseLib.h> >>>> @@ -283,6 +284,9 @@ ValidateCapsuleByMemoryResource ( >>>> 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)); >>>> + >>>> + 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data 2018-06-07 9:52 ` Ard Biesheuvel @ 2018-06-07 10:12 ` Zeng, Star 2018-06-07 10:14 ` Ard Biesheuvel 0 siblings, 1 reply; 15+ messages in thread From: Zeng, Star @ 2018-06-07 10:12 UTC (permalink / raw) To: Ard Biesheuvel Cc: Ni, Ruiyu, edk2-devel@lists.01.org, Gao, Liming, Yao, Jiewen, Leif Lindholm, Kinney, Michael D, Zeng, Star Since capsule data pointer may be invalid (for example, point to MMIO), we enhanced code to validate the capsule by memory resource HOB, and *recommend* platform/silicon (memory reference code) to report memory resource HOB before capsule coalescing. To consider and compatible with some platform may not report memory resource HOB before capsule coalescing, then MemoryResource == NULL and the code thinks it is valid. Thanks, Star -----Original Message----- From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] Sent: Thursday, June 7, 2018 5:52 PM To: Zeng, Star <star.zeng@intel.com> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data On 7 June 2018 at 11:46, Zeng, Star <star.zeng@intel.com> 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 TRUE, should the new line code be added for both of them? Good question: In which circumstances is the MemoryResource == 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 <star.zeng@intel.com> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Gao, > Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; > Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D > <michael.d.kinney@intel.com> > Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache > before consuming capsule data > > On 7 June 2018 at 07:41, Zeng, Star <star.zeng@intel.com> 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 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 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 address 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 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 <star.zeng@intel.com> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Leif >> Lindholm <leif.lindholm@linaro.org>; Yao, Jiewen >> <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, >> Michael D <michael.d.kinney@intel.com> >> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >> before consuming capsule data >> >> On 7 June 2018 at 03:37, Zeng, Star <star.zeng@intel.com> 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 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 <ruiyu.ni@intel.com>; Ard Biesheuvel >>> <ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>; >>> Yao, Jiewen <jiewen.yao@intel.com>; Leif Lindholm >>> <leif.lindholm@linaro.org>; Kinney, Michael D >>> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> >>> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >>> before consuming capsule data >>> >>> On 6 June 2018 at 11:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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. >>>> >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> >>> 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 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. >>> """ >>> >>>> --- >>>> 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 <Guid/CapsuleVendor.h> >>>> >>>> #include <Library/BaseMemoryLib.h> >>>> +#include <Library/CacheMaintenanceLib.h> >>>> #include <Library/DebugLib.h> >>>> #include <Library/PrintLib.h> >>>> #include <Library/BaseLib.h> >>>> @@ -283,6 +284,9 @@ ValidateCapsuleByMemoryResource ( >>>> 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)); >>>> + >>>> + 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data 2018-06-07 10:12 ` Zeng, Star @ 2018-06-07 10:14 ` Ard Biesheuvel 2018-06-07 10:27 ` Zeng, Star 0 siblings, 1 reply; 15+ messages in thread From: Ard Biesheuvel @ 2018-06-07 10:14 UTC (permalink / raw) To: Zeng, Star Cc: Ni, Ruiyu, edk2-devel@lists.01.org, Gao, Liming, Yao, Jiewen, Leif Lindholm, Kinney, Michael D On 7 June 2018 at 12:12, Zeng, Star <star.zeng@intel.com> 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 *recommend* platform/silicon (memory reference code) to report memory resource HOB before capsule coalescing. > To consider and compatible with some platform may not report memory resource HOB before capsule coalescing, then MemoryResource == 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 <star.zeng@intel.com> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com> > Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data > > On 7 June 2018 at 11:46, Zeng, Star <star.zeng@intel.com> 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 TRUE, should the new line code be added for both of them? > > Good question: In which circumstances is the MemoryResource == 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 <star.zeng@intel.com> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Gao, >> Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; >> Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D >> <michael.d.kinney@intel.com> >> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >> before consuming capsule data >> >> On 7 June 2018 at 07:41, Zeng, Star <star.zeng@intel.com> 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 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 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 address 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 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 <star.zeng@intel.com> >>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Leif >>> Lindholm <leif.lindholm@linaro.org>; Yao, Jiewen >>> <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, >>> Michael D <michael.d.kinney@intel.com> >>> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >>> before consuming capsule data >>> >>> On 7 June 2018 at 03:37, Zeng, Star <star.zeng@intel.com> 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 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 <ruiyu.ni@intel.com>; Ard Biesheuvel >>>> <ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>; >>>> Yao, Jiewen <jiewen.yao@intel.com>; Leif Lindholm >>>> <leif.lindholm@linaro.org>; Kinney, Michael D >>>> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> >>>> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >>>> before consuming capsule data >>>> >>>> On 6 June 2018 at 11:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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. >>>>> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>> >>>> 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 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. >>>> """ >>>> >>>>> --- >>>>> 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 <Guid/CapsuleVendor.h> >>>>> >>>>> #include <Library/BaseMemoryLib.h> >>>>> +#include <Library/CacheMaintenanceLib.h> >>>>> #include <Library/DebugLib.h> >>>>> #include <Library/PrintLib.h> >>>>> #include <Library/BaseLib.h> >>>>> @@ -283,6 +284,9 @@ ValidateCapsuleByMemoryResource ( >>>>> 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)); >>>>> + >>>>> + 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data 2018-06-07 10:14 ` Ard Biesheuvel @ 2018-06-07 10:27 ` Zeng, Star 2018-06-07 10:27 ` Ard Biesheuvel 0 siblings, 1 reply; 15+ messages in thread From: Zeng, Star @ 2018-06-07 10:27 UTC (permalink / raw) To: Ard Biesheuvel Cc: Ni, Ruiyu, edk2-devel@lists.01.org, Leif Lindholm, Yao, Jiewen, Gao, Liming, Kinney, Michael D, Zeng, Star 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. 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 <star.zeng@intel.com> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>; Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data On 7 June 2018 at 12:12, Zeng, Star <star.zeng@intel.com> 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 *recommend* platform/silicon (memory reference code) to report memory resource HOB before capsule coalescing. > To consider and compatible with some platform may not report memory resource HOB before capsule coalescing, then MemoryResource == 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 <star.zeng@intel.com> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Gao, > Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; > Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D > <michael.d.kinney@intel.com> > Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache > before consuming capsule data > > On 7 June 2018 at 11:46, Zeng, Star <star.zeng@intel.com> 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 TRUE, should the new line code be added for both of them? > > Good question: In which circumstances is the MemoryResource == 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 <star.zeng@intel.com> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Gao, >> Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; >> Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D >> <michael.d.kinney@intel.com> >> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >> before consuming capsule data >> >> On 7 June 2018 at 07:41, Zeng, Star <star.zeng@intel.com> 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 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 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 address 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 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 <star.zeng@intel.com> >>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Leif >>> Lindholm <leif.lindholm@linaro.org>; Yao, Jiewen >>> <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, >>> Michael D <michael.d.kinney@intel.com> >>> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >>> before consuming capsule data >>> >>> On 7 June 2018 at 03:37, Zeng, Star <star.zeng@intel.com> 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 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 <ruiyu.ni@intel.com>; Ard Biesheuvel >>>> <ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>; >>>> Yao, Jiewen <jiewen.yao@intel.com>; Leif Lindholm >>>> <leif.lindholm@linaro.org>; Kinney, Michael D >>>> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> >>>> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >>>> before consuming capsule data >>>> >>>> On 6 June 2018 at 11:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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. >>>>> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>> >>>> 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 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. >>>> """ >>>> >>>>> --- >>>>> 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 <Guid/CapsuleVendor.h> >>>>> >>>>> #include <Library/BaseMemoryLib.h> >>>>> +#include <Library/CacheMaintenanceLib.h> >>>>> #include <Library/DebugLib.h> >>>>> #include <Library/PrintLib.h> >>>>> #include <Library/BaseLib.h> >>>>> @@ -283,6 +284,9 @@ ValidateCapsuleByMemoryResource ( >>>>> 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)); >>>>> + >>>>> + 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data 2018-06-07 10:27 ` Zeng, Star @ 2018-06-07 10:27 ` Ard Biesheuvel 0 siblings, 0 replies; 15+ messages in thread From: Ard Biesheuvel @ 2018-06-07 10:27 UTC (permalink / raw) To: Zeng, Star Cc: Ni, Ruiyu, edk2-devel@lists.01.org, Leif Lindholm, Yao, Jiewen, Gao, Liming, Kinney, Michael D On 7 June 2018 at 12:27, Zeng, Star <star.zeng@intel.com> 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 Ard Biesheuvel > Sent: Thursday, June 7, 2018 6:14 PM > To: Zeng, Star <star.zeng@intel.com> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>; Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com> > Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data > > On 7 June 2018 at 12:12, Zeng, Star <star.zeng@intel.com> 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 *recommend* platform/silicon (memory reference code) to report memory resource HOB before capsule coalescing. >> To consider and compatible with some platform may not report memory resource HOB before capsule coalescing, then MemoryResource == 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 <star.zeng@intel.com> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Gao, >> Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; >> Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D >> <michael.d.kinney@intel.com> >> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >> before consuming capsule data >> >> On 7 June 2018 at 11:46, Zeng, Star <star.zeng@intel.com> 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 TRUE, should the new line code be added for both of them? >> >> Good question: In which circumstances is the MemoryResource == 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 <star.zeng@intel.com> >>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Gao, >>> Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; >>> Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D >>> <michael.d.kinney@intel.com> >>> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >>> before consuming capsule data >>> >>> On 7 June 2018 at 07:41, Zeng, Star <star.zeng@intel.com> 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 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 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 address 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 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 <star.zeng@intel.com> >>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Leif >>>> Lindholm <leif.lindholm@linaro.org>; Yao, Jiewen >>>> <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, >>>> Michael D <michael.d.kinney@intel.com> >>>> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >>>> before consuming capsule data >>>> >>>> On 7 June 2018 at 03:37, Zeng, Star <star.zeng@intel.com> 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 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 <ruiyu.ni@intel.com>; Ard Biesheuvel >>>>> <ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>; >>>>> Yao, Jiewen <jiewen.yao@intel.com>; Leif Lindholm >>>>> <leif.lindholm@linaro.org>; Kinney, Michael D >>>>> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com> >>>>> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >>>>> before consuming capsule data >>>>> >>>>> On 6 June 2018 at 11:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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. >>>>>> >>>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>> >>>>> 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 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. >>>>> """ >>>>> >>>>>> --- >>>>>> 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 <Guid/CapsuleVendor.h> >>>>>> >>>>>> #include <Library/BaseMemoryLib.h> >>>>>> +#include <Library/CacheMaintenanceLib.h> >>>>>> #include <Library/DebugLib.h> >>>>>> #include <Library/PrintLib.h> >>>>>> #include <Library/BaseLib.h> >>>>>> @@ -283,6 +284,9 @@ ValidateCapsuleByMemoryResource ( >>>>>> 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)); >>>>>> + >>>>>> + 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-06-07 10:27 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-06-06 9:52 [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data Ard Biesheuvel 2018-06-06 12:09 ` Ard Biesheuvel 2018-06-06 13:59 ` Yao, Jiewen 2018-06-06 16:07 ` Kinney, Michael D 2018-06-06 16:13 ` Ard Biesheuvel 2018-06-07 1:37 ` Zeng, Star 2018-06-07 4:50 ` Ard Biesheuvel 2018-06-07 5:41 ` Zeng, Star 2018-06-07 6:00 ` Ard Biesheuvel 2018-06-07 9:46 ` Zeng, Star 2018-06-07 9:52 ` Ard Biesheuvel 2018-06-07 10:12 ` Zeng, Star 2018-06-07 10:14 ` Ard Biesheuvel 2018-06-07 10:27 ` Zeng, Star 2018-06-07 10:27 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox