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. > -----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
