What is the meaning of "Even if someone decides later on that the PCD database format should use natural alignment for all values, it is still wrong to cast a pointer to void back to a pointer a larger type."?
About "Since this patch fixes an actual problem on 32-bit ARM running under virtualization, may I suggest that we keep it separate from the discussion regarding the PCD database alignment?", I think the coming BaseTools fix will fix the actual problem you said, we could have you CCed to have a try when the patch will be sent out. Thanks, Star -----Original Message----- From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] Sent: Monday, May 4, 2015 6:23 PM To: Laszlo Ersek Cc: edk2-devel@lists.sourceforge.net Subject: Re: [edk2] [PATCH] MdeModulePkg: avoid unaligned writes in PcdDxe driver On 4 May 2015 at 09:08, Laszlo Ersek <ler...@redhat.com> wrote: > On 04/30/15 14:45, Ard Biesheuvel wrote: >> On 30 April 2015 at 14:31, Laszlo Ersek <ler...@redhat.com> wrote: >>> On 04/30/15 14:04, Ard Biesheuvel wrote: >>>> InternalData may not be aligned to the size of the type we are >>>> writing, so use WriteUnalignedXX() instead. >>>> >>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org> >>>> --- >>>> MdeModulePkg/Universal/PCD/Dxe/Service.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/MdeModulePkg/Universal/PCD/Dxe/Service.c >>>> b/MdeModulePkg/Universal/PCD/Dxe/Service.c >>>> index 9b4701bdd749..f071fc58361b 100644 >>>> --- a/MdeModulePkg/Universal/PCD/Dxe/Service.c >>>> +++ b/MdeModulePkg/Universal/PCD/Dxe/Service.c >>>> @@ -1259,15 +1259,15 @@ SetWorker ( >>>> break; >>>> >>>> case sizeof(UINT16): >>>> - *((UINT16 *) InternalData) = *((UINT16 *) Data); >>>> + WriteUnaligned16 (InternalData, *((UINT16 *) Data)); >>>> break; >>>> >>>> case sizeof(UINT32): >>>> - *((UINT32 *) InternalData) = *((UINT32 *) Data); >>>> + WriteUnaligned32 (InternalData, *((UINT32 *) Data)); >>>> break; >>>> >>>> case sizeof(UINT64): >>>> - *((UINT64 *) InternalData) = *((UINT64 *) Data); >>>> + WriteUnaligned64 (InternalData, *((UINT64 *) Data)); >>>> break; >>>> >>>> default: >>>> >>> >>> What guarantees that you can dereference Data, for reading? >>> Shouldn't the pattern be: >>> >>> WriteUnalignedXX (InternalData, ReadUnalignedXX (Data)); >>> >>> (The reason why this isn't necessary could be obvious from the code, >>> but you surely don't expect me to look at the code, do you. :)) >>> >> >> The callers of SetWorker() are currently doing the right thing in >> this >> regard: Data is always aligned to *Size. >> I could add an ASSERT() to enforce that, if you like ... >> > > It's fine like this as far as I'm concerned. > I agree. Even if someone decides later on that the PCD database format should use natural alignment for all values, it is still wrong to cast a pointer to void back to a pointer a larger type. As Laszlo pointed out, this applies not only to InternalData but also to Data, but in the latter case, all call sites can easily be verified to do the right thing. Since this patch fixes an actual problem on 32-bit ARM running under virtualization, may I suggest that we keep it separate from the discussion regarding the PCD database alignment? > Acked-by: Laszlo Ersek <ler...@redhat.com> > Thanks, 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 ------------------------------------------------------------------------------ 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
