| Message ID | b886eb2c-cabc-f195-4996-aae1fc3c61d9@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/EFI: build adjustments | expand |
On Thu, Apr 01, 2021 at 11:45:38AM +0200, Jan Beulich wrote: > There's no need to link relocs-dummy.o into the ELF binary. The two > symbols needed can as well be provided by the linker script. Then our > mkreloc tool also doesn't need to put them in the generated assembler > source. Maybe I'm just dense today, but I think the message needs to be expanded a bit to mention that while the __base_relocs_{start,end} are not used when loaded as an EFI application, they are used by the EFI code in Xen when booted using the multiboot2 protocol for example, as they are used by efi_arch_relocate_image. I think relocation is not needed when natively loaded as an EFI application, as then the load address matches the one expected by Xen? I also wonder, at some point there where plans for providing a single binary that would work as multiboot{1,2} and also be capable of being loaded as an EFI application (ie: have a PE/COFF header also I assume together with the ELF one), won't the changes here make it more difficult to reach that goal or require reverting later on, as I feel they are adding more differences between the PE binary and the ELF one. The code LGTM, but I think at least I would like the commit message to be expanded. Thanks, Roger.
On 21.04.2021 11:46, Roger Pau Monné wrote: > On Thu, Apr 01, 2021 at 11:45:38AM +0200, Jan Beulich wrote: >> There's no need to link relocs-dummy.o into the ELF binary. The two >> symbols needed can as well be provided by the linker script. Then our >> mkreloc tool also doesn't need to put them in the generated assembler >> source. > > Maybe I'm just dense today, but I think the message needs to be > expanded a bit to mention that while the __base_relocs_{start,end} are > not used when loaded as an EFI application, they are used by the EFI > code in Xen when booted using the multiboot2 protocol for example, as > they are used by efi_arch_relocate_image. > > I think relocation is not needed when natively loaded as an EFI > application, as then the load address matches the one expected by > Xen? It's quite the other way around: The EFI loader applies relocations to put the binary at its loaded _physical_ address (the image gets linked for the final virtual address). Hence we need to apply the same relocations a 2nd time (undoing what the EFI loader did) before we can branch from the physical (identity mapped) address range where xen.efi was loaded to the intended virtual address range where we mean to run Xen from. For the ELF binary the symbols are needed solely to make ld happy. > I also wonder, at some point there where plans for providing a single > binary that would work as multiboot{1,2} and also be capable of being > loaded as an EFI application (ie: have a PE/COFF header also I assume > together with the ELF one), won't the changes here make it more > difficult to reach that goal or require reverting later on, as I feel > they are adding more differences between the PE binary and the ELF > one. There were such plans, yes, but from the last round of that series I seem to recall that there was at least one issue breaking this idea. So no, at this point I'm not intending to take precautions to make that work easier (or not further complicate it). This said, I don't think the change here complicates anything there. > The code LGTM, but I think at least I would like the commit message to > be expanded. Well, once I know what exactly you're missing there, I can certainly try to expand it. Jan
On Wed, Apr 21, 2021 at 12:44:13PM +0200, Jan Beulich wrote: > On 21.04.2021 11:46, Roger Pau Monné wrote: > > On Thu, Apr 01, 2021 at 11:45:38AM +0200, Jan Beulich wrote: > >> There's no need to link relocs-dummy.o into the ELF binary. The two > >> symbols needed can as well be provided by the linker script. Then our > >> mkreloc tool also doesn't need to put them in the generated assembler > >> source. > > > > Maybe I'm just dense today, but I think the message needs to be > > expanded a bit to mention that while the __base_relocs_{start,end} are > > not used when loaded as an EFI application, they are used by the EFI > > code in Xen when booted using the multiboot2 protocol for example, as > > they are used by efi_arch_relocate_image. > > > > I think relocation is not needed when natively loaded as an EFI > > application, as then the load address matches the one expected by > > Xen? > > It's quite the other way around: The EFI loader applies relocations > to put the binary at its loaded _physical_ address (the image gets > linked for the final virtual address). Hence we need to apply the > same relocations a 2nd time (undoing what the EFI loader did) > before we can branch from the physical (identity mapped) address > range where xen.efi was loaded to the intended virtual address > range where we mean to run Xen from. > > For the ELF binary the symbols are needed solely to make ld happy. > > > I also wonder, at some point there where plans for providing a single > > binary that would work as multiboot{1,2} and also be capable of being > > loaded as an EFI application (ie: have a PE/COFF header also I assume > > together with the ELF one), won't the changes here make it more > > difficult to reach that goal or require reverting later on, as I feel > > they are adding more differences between the PE binary and the ELF > > one. > > There were such plans, yes, but from the last round of that series > I seem to recall that there was at least one issue breaking this > idea. So no, at this point I'm not intending to take precautions to > make that work easier (or not further complicate it). This said, I > don't think the change here complicates anything there. > > > The code LGTM, but I think at least I would like the commit message to > > be expanded. > > Well, once I know what exactly you're missing there, I can certainly > try to expand it. OK, I think I now have a clearer view, the commit message is likely fine as it already mentions the ELF binary only needs the dummy __base_relocs_{start,end}, hence it's the EFI binary the one that requires the relocation symbols. Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
--- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -133,7 +133,6 @@ XEN_NO_PE_FIXUPS := $(if $(XEN_BUILD_EFI endif ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS) -EFI_OBJS-$(XEN_BUILD_EFI) := efi/relocs-dummy.o ifeq ($(CONFIG_LTO),y) # Gather all LTO objects together @@ -141,13 +140,13 @@ prelink_lto.o: $(ALL_OBJS) $(ALL_LIBS) $(LD_LTO) -r -o $@ $(filter-out %.a,$^) --start-group $(filter %.a,$^) --end-group # Link it with all the binary objects -prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $(EFI_OBJS-y) FORCE +prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o FORCE $(call if_changed,ld) prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o FORCE $(call if_changed,ld) else -prelink.o: $(ALL_OBJS) $(ALL_LIBS) $(EFI_OBJS-y) FORCE +prelink.o: $(ALL_OBJS) $(ALL_LIBS) FORCE $(call if_changed,ld) prelink-efi.o: $(ALL_OBJS) $(ALL_LIBS) FORCE --- a/xen/arch/x86/efi/mkreloc.c +++ b/xen/arch/x86/efi/mkreloc.c @@ -320,9 +320,7 @@ int main(int argc, char *argv[]) } puts("\t.section .reloc, \"a\", @progbits\n" - "\t.balign 4\n" - "\t.globl __base_relocs_start, __base_relocs_end\n" - "__base_relocs_start:"); + "\t.balign 4"); for ( i = 0; i < nsec; ++i ) { @@ -373,8 +371,6 @@ int main(int argc, char *argv[]) diff_sections(NULL, NULL, NULL, 0, 0, 0, 0); - puts("__base_relocs_end:"); - close(in1); close(in2); --- a/xen/arch/x86/efi/relocs-dummy.S +++ b/xen/arch/x86/efi/relocs-dummy.S @@ -1,10 +1,8 @@ .section .reloc, "a", @progbits .balign 4 -GLOBAL(__base_relocs_start) .long 0 .long 8 -GLOBAL(__base_relocs_end) .globl VIRT_START, ALT_START .equ VIRT_START, XEN_VIRT_START --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -170,18 +170,6 @@ SECTIONS #endif #endif -/* - * ELF builds are linked to a fixed virtual address, and in principle - * shouldn't have a .reloc section. However, due to the way EFI support is - * currently implemented, retaining the .reloc section is necessary. - */ -#if defined(XEN_BUILD_EFI) && !defined(EFI) - . = ALIGN(4); - DECL_SECTION(.reloc) { - *(.reloc) - } :text -#endif - _erodata = .; . = ALIGN(SECTION_ALIGN); @@ -319,18 +307,27 @@ SECTIONS __2M_rwdata_end = .; #ifdef EFI - . = ALIGN(4); - DECL_SECTION(.reloc) { + .reloc ALIGN(4) : { + __base_relocs_start = .; *(.reloc) + __base_relocs_end = .; } /* Trick the linker into setting the image size to exactly 16Mb. */ . = ALIGN(__section_alignment__); DECL_SECTION(.pad) { . = ALIGN(MB(16)); } -#endif - -#ifndef XEN_BUILD_EFI +#elif defined(XEN_BUILD_EFI) + /* + * Due to the way EFI support is currently implemented, these two symbols + * need to be defined. Their precise values shouldn't matter (the consuming + * function doesn't get called), but to be on the safe side both values would + * better match. Of course the need to be reachable by the relocations + * referencing them. + */ + PROVIDE(__base_relocs_start = .); + PROVIDE(__base_relocs_end = .); +#else efi = .; #endif
There's no need to link relocs-dummy.o into the ELF binary. The two symbols needed can as well be provided by the linker script. Then our mkreloc tool also doesn't need to put them in the generated assembler source. Signed-off-by: Jan Beulich <jbeulich@suse.com>