| Message ID | 1430395447-4868-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 30 April 2015 at 14:31, Laszlo Ersek <lersek@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.biesheuvel@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 ... ------------------------------------------------------------------------------ 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
On 4 May 2015 at 09:08, Laszlo Ersek <lersek@redhat.com> wrote: > On 04/30/15 14:45, Ard Biesheuvel wrote: >> On 30 April 2015 at 14:31, Laszlo Ersek <lersek@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.biesheuvel@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 <lersek@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
On 6 May 2015 at 05:40, Zeng, Star <star.zeng@intel.com> wrote: > 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."? > I am referring to the BaseTools change. Even if those are fixed so that only aligned addresses are emitted, we would still need either an ASSERT() or my changes at runtime to prevent an uncontrolled crash when InternalData inadvertently assumes an unaligned value. My preference would be the latter > 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. > Yes, please. But in the meantime, please consider which runtime change you would prefer: an ASSERT() or my changes.
On 6 May 2015 at 07:40, Zeng, Star <star.zeng@intel.com> wrote: > InternalData pointer is from PcdDb + Offset, Offset is from LocalTokenNumber & PCD_DATABASE_OFFSET_MASK, LocalTokenNumber is from TokenNumber. > PCD_DATABASE_OFFSET_MASK does not mask any low bits, and TokenNumber is an external input to the function that is [indirectly] called from generated code. So there is nothing in DxePcd itself preventing InternalData from being an unaligned quantity with respect to its type and size. > In SetWorker() of Service.c has below code to ensure correct size and then correct aligned internal data to be referenced. > > if (*Size != DxePcdGetSize (TokenNumber + 1)) { > return EFI_INVALID_PARAMETER; > } > This checks the size but not the alignment, so it relies on the correctness of the BaseTools. But even if you would be able to prove conclusively that InternalData is always correctly aligned, casting a UINT8* to a pointer to a wider type always needs either an ASSERT() or a ReadUnaligned()WriteUnaligned() to be 100% correct. Regards, Ard. > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: Wednesday, May 6, 2015 1:31 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 05:40, Zeng, Star <star.zeng@intel.com> wrote: >> 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."? >> > > I am referring to the BaseTools change. Even if those are fixed so that only aligned addresses are emitted, we would still need either an > ASSERT() or my changes at runtime to prevent an uncontrolled crash when InternalData inadvertently assumes an unaligned value. My preference would be the latter > >> 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. >> > > Yes, please. But in the meantime, please consider which runtime change you would prefer: an ASSERT() or my changes. > > -- > Ard. > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@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 <lersek@redhat.com> wrote: >>> On 04/30/15 14:45, Ard Biesheuvel wrote: >>>> On 30 April 2015 at 14:31, Laszlo Ersek <lersek@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.biesheuvel@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 <lersek@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
On 6 May 2015 at 10:13, Zeng, Star <star.zeng@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.
On 6 May 2015 at 10:31, Zeng, Star <star.zeng@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.biesheuvel@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.zeng@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
On 6 May 2015 at 10:41, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 6 May 2015 at 10:31, Zeng, Star <star.zeng@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.biesheuvel@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.zeng@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
On 6 May 2015 at 10:59, Zeng, Star <star.zeng@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.biesheuvel@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.biesheuvel@linaro.org> wrote: >> On 6 May 2015 at 10:31, Zeng, Star <star.zeng@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.biesheuvel@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.zeng@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
On 6 May 2015 at 11:09, Zeng, Star <star.zeng@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.biesheuvel@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.zeng@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.biesheuvel@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.biesheuvel@linaro.org> wrote: >>> On 6 May 2015 at 10:31, Zeng, Star <star.zeng@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.biesheuvel@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.zeng@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
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:
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.biesheuvel@linaro.org> --- MdeModulePkg/Universal/PCD/Dxe/Service.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)