Re: [Xen-devel] [PATCH v3 1/3] x86/linker: add a reloc section to ELF linker script

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/06/2019 12:07, Roger Pau Monné wrote:
> On Thu, Jun 27, 2019 at 11:59:46AM +0100, Andrew Cooper wrote:
>> On 27/06/2019 10:33, Roger Pau Monne wrote:
>>> if the hypervisor has been built with EFI support (ie: multiboot2).
>>> This allows to position the .reloc section correctly in the output
>>> binary.
>>>
>>> Note that for the ELF output format the .reloc section is moved before
>>> .bss because the data it contains is read-only, so it belongs with the
>>> other sections containing read-only data.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> I have to admit that I'm still confused as to why we need this in the
>> first place.
>>
>> The ELF build is linked to a fixed virtual address, irrespective of
>> whether grub loads it via MB1 or MB2 and/or with EFI details.
>>
>> i.e. the non-EFI build shouldn't have any remaining relocations by the
>> time it is fully linked.
>>
>> Or am I missing something?
> Right, but there's code that depends on the symbols defined in .reloc
> (__base_relocs_start/__base_relocs_end), so unless those symbols are
> defined the linker will throw a missing symbols error on the final
> link step.

Ok.  I can certainly accept that this is how the code currently functions.

> I could add .reloc to the discarded sections list and create the
> __base_relocs_start and __base_relocs_end symbols on the linker script
> maybe, but I'm not sure that's any better than just having the dummy
> .reloc section.
>
> Or another option would be to compile the units that use those symbols
> twice, one for the ELF build and one for the PE build, but again that
> doesn't seem much better IMO.

So the bug here is that efi_arch_relocate_image() isn't inside an #ifdef
EFI (or is it XEN_BUILD_EFI?)

And the reason #ifdef-ing it won't work is because we have a single pass
of extra objects to link into the main Xen to add EFI support.

I think the proper longterm fix is to have CONFIG_EFI (suitably guarded
on toolchain support), seeing as it is common across our two binaries,
and the extra bits for the real EFI build then become rather smaller.

However, it is definitely not fair to lump this fix on you for this
series, so given a comment explaining that this isn't used in the ELF
build, but needs to be present for compatibility with the EFI build,
I'll be ok with the patch in this form.

Again, a comment can be fixed up on commit if everyone is happy with
this approach.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel