On 6 May 2015 at 10:59, Zeng, Star <star.z...@intel.com> wrote: > It is for VPD PCD data region access, that is another story. I already have > some explanation about it in the attached email. >
OK, thanks for the explanation. So when can we expect this fix for the BaseTools? > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > Sent: Wednesday, May 6, 2015 4:55 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: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
