On 9 June 2015 at 15:34, Yao, Jiewen <jiewen....@intel.com> wrote:
> Thank you! Agree, I have similar concern on
> PeCoffLoaderGetPeHeaderMagicValue().
>
> I have seen duplicated code in below modules:
> 1)
> SecurityPkg\Library\DxeImageVerificationLib\DxeImageVerificationLib.c(363):
> if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 &&
> mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> 2)
> SecurityPkg\Library\DxeImageVerificationLib\DxeImageVerificationLib.c(1690):
> if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 &&
> mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> 3) SecurityPkg\Library\DxeTpmMeasureBootLib\DxeTpmMeasureBootLib.c(419): if
> (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 &&
> Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> 4) SecurityPkg\Tcg\TrEEDxe\MeasureBootPeCoff.c(114): if
> (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 &&
> Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> 5)
> SecurityPkg\VariableAuthenticated\SecureBootConfigDxe\SecureBootConfigImpl.c(1734):
> if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 &&
> mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>
> How I wish it is exposed!
>
Actually, I do think this is somewhat of a concern, considering that
is the SecurityPkg. Having these kinds of exceptions on all
architectures, and in various places in the code, just because one
architecture needs it is not very elegant, especially since only IA64
builds are ever expected to try and load IA64 images in the first
place.
Is there any way we could export PeCoffLoaderGetPeHeaderMagicValue ()
from BasePecoffLib, and special case the implementation so the hack
only gets included on IPF builds? Or will we violate some spec by
doing that?
Regards,
Ard.
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Tuesday, June 09, 2015 9:22 PM
> To: Yao, Jiewen
> Cc: edk2-devel@lists.sourceforge.net; Zeng, Star; Gao, Liming
> Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support,
> MdeModulePkg/DxeCore support UEFI2.5 properties table
>
> On 9 June 2015 at 14:42, Yao, Jiewen <jiewen....@intel.com> wrote:
>> Hi Ard
>> You are right. I happen to find a logic error, too.
>>
>> Attach patch for your review.
>>
>
> Yes, that looks fine to me, although it is unfortunate that can't just use
> PeCoffLoaderGetPeHeaderMagicValue () directly.
> It builds fine on GCC/AArch64
>
> Reviewed-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
>
> Thanks,
> Ard.
>
>
>
>>
>>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> Sent: Tuesday, June 09, 2015 8:28 PM
>> To: Yao, Jiewen
>> Cc: edk2-devel@lists.sourceforge.net; Zeng, Star; Gao, Liming
>> Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support,
>> MdeModulePkg/DxeCore support UEFI2.5 properties table
>>
>> On 5 June 2015 at 15:22, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote:
>>> On 5 June 2015 at 15:21, Yao, Jiewen <jiewen....@intel.com> wrote:
>>>> Hi Ard
>>>> Thanks to report this. I have fixed this in 17565.
>>>
>>> Wow, that was quick! Thanks!
>>>
>>
>> You have fixed the build, but the copy-pasted comments are still there,
>> which make no sense at all anymore, since they refer to the Magic variable,
>> which has been removed.
>>
>> Next time, could you please send your patches (including fixes like
>> these) to the mailing list first? That way, people have a chance to see
>> what's going in before its gets merged.
>>
>> Thanks,
>> Ard.
>>
>>
>>>
>>>> -----Original Message-----
>>>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>>>> Sent: Friday, June 05, 2015 9:12 PM
>>>> To: edk2-devel@lists.sourceforge.net; Yao, Jiewen; Zeng, Star; Gao,
>>>> Liming
>>>> Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support,
>>>> MdeModulePkg/DxeCore support UEFI2.5 properties table
>>>>
>>>> On 4 June 2015 at 16:34, Yao, Jiewen <jiewen....@intel.com> wrote:
>>>>> Hi
>>>>>
>>>>> Here is patch to support UEFI2.5, chapter 4, section 4.6 Properties
>>>>> Table feature.
>>>>>
>>>>>
>>>>>
>>>>> MdePkg – add Properties table definition.
>>>>>
>>>>> MdeModulePkg – add properties table support in DXE core.
>>>>>
>>>>> MdeModulePkg – add PropertiesTableAttributesDxe driver to set
>>>>> ACPINvs/Reserved memory type to be XP.
>>>>>
>>>>>
>>>>>
>>>>> Signed-off-by: Yao, Jiewen jiewen....@intel.com
>>>>>
>>>>> Reviewed-by: Zeng, Star star.z...@intel.com
>>>>>
>>>>> Reviewed-by: Gao, Liming liming....@intel.com
>>>>>
>>>>
>>>> This patch breaks the build for GCC on 64-bit ARM
>>>>
>>>> 13:03:43
>>>> /home/buildslave/workspace/leg-virt-tianocore-edk2-upstream/edk2/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c:
>>>> In function ‘InsertImageRecord’:
>>>> 13:03:43
>>>> /home/buildslave/workspace/leg-virt-tianocore-edk2-upstream/edk2/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c:1113:10:
>>>> error: variable ‘Magic’ set but not used [-Werror=unused-but-set-variable]
>>>> 13:03:43 UINT16 Magic;
>>>> 13:03:43 ^
>>>> 13:03:43
>>>> /home/buildslave/workspace/leg-virt-tianocore-edk2-upstream/edk2/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c:1110:10:
>>>> error: variable ‘ImageType’ set but not used
>>>> [-Werror=unused-but-set-variable]
>>>> 13:03:43 UINT16 ImageType;
>>>> 13:03:43 ^
>>>>
>>>>
>>>> and honestly, i can't be bothered to look at the code myself, considering
>>>> the way it was dumped into the mailing list and the repository.
>>>>
>>>> Please get this fixed
>>>>
>>>> Regards,
>>>> Ard.
>>>>
>>>>
>>>>>
>>>>> -------------------------------------------------------------------
>>>>> -
>>>>> --
>>>>> --------
>>>>>
>>>>> _______________________________________________
>>>>> edk2-devel mailing list
>>>>> edk2-devel@lists.sourceforge.net
>>>>> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>>>>>
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
