On 6 May 2015 at 10:41, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote: > On 6 May 2015 at 10:31, Zeng, Star <star.z...@intel.com> wrote: >> You mean ASSERT (((UINTN) InternalData & (*Size - 1)) == 0)? >> > > Yes. > >> Do you want to use ReadUnaligned/WriteUnaligned for all the >> UINT16*/UINT32*/UINT64* data pointers even they are assured to be aligned? >> I prefer to use ASSERT() if I must select one. >> > > If you look at MdePkg/Library/BaseLib/Unaligned.c, you will notice > that most architectures do nothing interesting for WriteUnalignedXX(), > only IPF and ARM do something special. Combined with the fact that > writes to dynamic PCDs are hardly a bottleneck in the execution, I > think using ReadAligned/WriteAligned is mostly harmless. >
BTW, in the same driver, DxePcdGetXX() are also already using ReadAlignedXX() > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] >> Sent: Wednesday, May 6, 2015 4:19 PM >> To: Zeng, Star >> Cc: edk2-devel@lists.sourceforge.net; Laszlo Ersek; Feng, Bob C >> Subject: Re: [edk2] [PATCH] MdeModulePkg: avoid unaligned writes in PcdDxe >> driver >> >> On 6 May 2015 at 10:13, Zeng, Star <star.z...@intel.com> wrote: >>> Yes of course, it relies on the correctness of the BaseTools. Even the >>> whole BIOS image relies on the correctness of the BaseTools. ^_^ >>> >> >> ... which is exactly why there are ASSERT()s all over the place, isn't it? >> >>> The ASSERT() you mean is like below? I admit it is an approach to ensure >>> the correctness of BaseTools. >>> ASSERT (((UINTN) InternalData & (sizeof (UINT16) - 1)) == >>> 0); >>> >> >> No we need (*Size - 1) not a constant. >> >>> But the ASSERT check maybe a little redundant as it will be in every >>> 16/32/64 PcdSet. Do you think it could be a little more efficient to add >>> ASSERT check only for the start pointer of uninitialized data(must be >>> 8bytes aligned) as we found the bug only in the uninitialized data? you can >>> reference the analysis result about the root cause of the bug in the email >>> I attached in the previous email thread. >>> >> >> Well, the most efficient would be not to ASSERT() at all but use >> ReadUnaligned/WriteUnaligned, since those will be implemented according to >> the capabilities of the architecture, i.e., it may simply be an unaligned >> load/store, but in ARM's case, it may perform the access byte by byte >> (depending on which base architecture version is being targeted) >> >> I still think we need to approach this as two separate problems, and fix >> this one by using the unaligned accessors. That way, it is independent of >> whether/how the BaseTools get fixed. >> >> -- >> Ard. ------------------------------------------------------------------------------ One dashboard for servers and applications across Physical-Virtual-Cloud Widest out-of-the-box monitoring support with 50+ applications Performance metrics, stats and reports that give you Actionable Insights Deep dive visibility with transaction tracing using APM Insight. http://ad.doubleclick.net/ddm/clk/290420510;117567292;y _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
