On 9 June 2015 at 15:55, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote:
> 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?
>
... or perhaps get rid of the special case entirely? ELILO is dead,
and I doubt anyone using IA64 in production using an affected version
of ELILO would be interested in running the latest Tianocore.
--
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
