I was trying to not discuss the detailed implementation of ReadUnaligned/WriteUnaligned for different arches. I am trying to think the problem at the view point the PCD database. The unaligned case is not expected by original design for the BaseTools generated PCD database. So ASSERT() could be helpful to identify the bug of BaseTools. Anyway, many thank you and Aaron found the bug, and now we could fix the bug.
Thanks, Star -----Original Message----- From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] Sent: Wednesday, May 6, 2015 4:41 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: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
