On 6 May 2015 at 11:09, Zeng, Star <star.z...@intel.com> wrote: > Bob is working on that, we are doing more test and will CC you into the patch > review. > > If you want to add ASSERT like we discussed(ASSERT (((UINTN) InternalData & > (*Size - 1)) == 0)), you can send patch, but remember to also cover PEI PCD > module. > Or you want me to send the patch? >
I disagree about the ASSERT, I still think using WriteAligned() is better, simply because Internaldata is of type VOID* and is being cast to a wider type with alignment requirements. Whether or not the BaseTools are supposed to ensure externally that InternalData's alignment will be in line with the value of *Size is irrelevant imo. But if you want to add that, please go ahead. > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > Sent: Wednesday, May 6, 2015 5:04 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: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
