Ard: For Patch 5 & 6, Zimmer Vincent will clarify OS-Loader on Properties Table in USWG, then provide the further direction
Thanks Liming -----Original Message----- From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] Sent: Tuesday, July 7, 2015 4:32 PM To: Gao, Liming Cc: edk2-devel@lists.sourceforge.net; Kinney, Michael D; Yao, Jiewen; Justen, Jordan L; ler...@redhat.com; olivier.mar...@arm.com Subject: Re: [PATCH 0/6] fix runtime relocation of disjoint PE/COFF images On 7 July 2015 at 03:57, Gao, Liming <liming....@intel.com> wrote: > Ard: > So, in this case, PE image space will not be continuous. Its code and data > section are in the different region. I know RelocatImage is for the absolute > address data access. But, I am still curious how PE image handles the > relative address data access? > That is indeed the question. On non-PIC ARM and AARCH64 builds, only branch instructions are relative, and the ELF to PE/COFF conversion ensures that there is only a single .code segment. On other archs and toolchains, this may not be guaranteed at all. I guess we could enhance GenFw to disallow relative references between code and data sections, but that does not help the non-ELF based toolchains. Bottom line is that there is no 100% correct solution for this. Using ConvertPointer() instead of a single adjustment value is obviously correct for existing cases, and fixes the observed issues at least under ArmVirtQemu and OVMF under X86. > For patch 1 & 2, I agree those changes. OK > For patch 3 & 4, I need more time to review them. OK. Note that high/low relocations are probably never used in the first place, since the observed issues are so severe that they would probably have been noticed by now. This also means I could not test this code, unfortunately. > For patch 5, PeCoffLoaderRelocateImageForRuntime() API is changed. This is > an incompatible change. Now, it only impacts BasePeCoffLib and RuntimeDriver. > I am not sure whether there is any other code to consume/produce it. OK. We could retain the old version, and add PeCoffLoaderRelocateImageForRuntimeEx() if you prefer? > For patch 6, how about update RuntimeDriverConvertInternalPointer() for > this usage? And, remove unreferenced local variable VirtImageBase. > Yes, that would work. I will change that in the next version. -- Ard. > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > Sent: Monday, July 6, 2015 3:12 PM > To: Gao, Liming > Cc: edk2-devel@lists.sourceforge.net; Kinney, Michael D; Yao, Jiewen; Justen, > Jordan L; ler...@redhat.com; olivier.mar...@arm.com > Subject: Re: [PATCH 0/6] fix runtime relocation of disjoint PE/COFF images > > On 6 July 2015 at 08:25, Gao, Liming <liming....@intel.com> wrote: >> Ard: >> Have you GIT branch for those changes? Then, I can easily review those >> changes. >> > > Sure > > I pushed it here: > https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/shortlog/refs/heads/pecoff-runtime-relocation > > Thanks, > Ard. > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] >> Sent: Friday, July 03, 2015 5:40 PM >> To: edk2-devel@lists.sourceforge.net; Kinney, Michael D; Yao, Jiewen; Gao, >> Liming; Justen, Jordan L >> Cc: ler...@redhat.com; olivier.mar...@arm.com; Ard Biesheuvel >> Subject: [PATCH 0/6] fix runtime relocation of disjoint PE/COFF images >> >> This is a followup to the patch I sen yesterday: >> http://article.gmane.org/gmane.comp.bios.tianocore.devel/16647 >> >> I should know better than to propose changes to MdePkg, but in this case, I >> think the issues around the MemoryProtectionAttribute are severe enough to >> warrant drastic measures. >> >> Patches #1 and #2 are cleanup patches, they remove dead code that handles >> relocations that are already handled by the callers of the respective >> functions. >> >> Patch #3 fixes a bug in BasePeCoffLib where it fails to take into account >> that applying a EFI_IMAGE_REL_BASED_LOW relocation may result in a carry >> that needs to be taken into account when handling the corresponding >> EFI_IMAGE_REL_BASED_HIGH relocation. >> >> Patch #4 works around a bug in BasePeCoffLib where the runtime re-relocation >> may reapply EFI_IMAGE_REL_BASED_LOW or EFI_IMAGE_REL_BASED_HIGH relocations >> even if the target location has been updated programmatically after the >> image was loaded. >> >> Patch #5 replaces the 'Adjust' argument of >> PeCoffLoaderRelocateImageForRuntime >> with a ConvertPointer() callback function pointer so that the runtime >> relocation can handle disjoint PE/COFF images in virtual memory. >> >> Patch #6 updates RuntimeDxe to use the new >> PeCoffLoaderRelocateImageForRuntime >> prototype. >> >> Unfortunately, this series is not bisectable between #5 and #6. Working >> around that is non-trivial and probably not worth the hassle. >> >> Ard Biesheuvel (6): >> MdePkg/BasePeCoffLib: remove redundant PeCoffLoaderEx.c for AARCH64 >> MdePkg/BasePeCoffLib: remove redundant handling of >> EFI_IMAGE_REL_BASED_DIR64 >> MdePkg/BasePeCoffLib: account for carry in high/low relocation pairs >> MdePkg/BasePeCoffLib: fix handling of high/low relocation pairs >> MdePkg/BasePeCoffLib: relocate for runtime using ConvertPointer >> callback >> MdeModulePkg/RuntimeDxe: update to new RelocateImageForRuntime() >> prototype >> >> MdeModulePkg/Core/RuntimeDxe/Runtime.c | 12 +- >> MdePkg/Include/Library/PeCoffLib.h | 28 ++++- >> MdePkg/Library/BasePeCoffLib/AArch64/PeCoffLoaderEx.c | 127 >> -------------------- >> MdePkg/Library/BasePeCoffLib/Arm/PeCoffLoaderEx.c | 19 +-- >> MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 86 +++++++++---- >> MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf | 5 +- >> MdePkg/Library/BasePeCoffLib/BasePeCoffLibInternals.h | 16 +-- >> MdePkg/Library/BasePeCoffLib/Ipf/PeCoffLoaderEx.c | 42 +++---- >> MdePkg/Library/BasePeCoffLib/PeCoffLoaderEx.c | 20 +-- >> 9 files changed, 138 insertions(+), 217 deletions(-) delete mode 100644 >> MdePkg/Library/BasePeCoffLib/AArch64/PeCoffLoaderEx.c >> >> -- >> 1.9.1 >> ------------------------------------------------------------------------------ Don't Limit Your Business. Reach for the Cloud. GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business. Configured For All Businesses. Start Your Cloud Today. https://www.gigenetcloud.com/ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
