Re: [PATCH 6/8] x86/EFI: avoid use of GNU ld's --disable-reloc-section when possible

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.04.2021 12:21, Roger Pau Monné wrote:
> On Thu, Apr 01, 2021 at 11:46:44AM +0200, Jan Beulich wrote:
>> @@ -189,7 +208,11 @@ EFI_LDFLAGS += --no-insert-timestamp
>>  endif
>>  
>>  $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, 
>> A VIRT_START$$,,p')
>> +ifeq ($(MKRELOC),:)
>> +$(TARGET).efi: ALT_BASE :=
>> +else
>>  $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A 
>> ALT_START$$,,p')
> 
> Could you maybe check whether $(relocs-dummy) is set as the condition
> here and use it here instead of efi/relocs-dummy.o?

I can use it in the ifeq() if you think that's neater (the current way
is minimally shorter), but using it in the ALT_BASE assignment would
make this differ more from the VIRT_BASE one, which I'd like to avoid.

>> @@ -210,16 +233,16 @@ note_file_option ?= $(note_file)
>>  ifeq ($(XEN_BUILD_PE),y)
>>  $(TARGET).efi: prelink.o $(note_file) efi.lds efi/relocs-dummy.o efi/mkreloc
> 
> Do you need to also replace the target prerequisite to use $(relocs-dummy)?

No - without the dependency the file might not be generated (if this ends
up being the only real dependency on $(BASEDIR)/arch/x86/efi/built_in.o).
We can't rely on $(note_file) resolving to efi/buildid.o, and hence
recursing into $(BASEDIR)/arch/x86/efi/ may not otherwise get triggered.
Yet to calculate VIRT_BASE we need efi/relocs-dummy.o.

>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -86,10 +86,12 @@ static void __init efi_arch_relocate_ima
>>                  }
>>                  break;
>>              case PE_BASE_RELOC_DIR64:
>> -                if ( in_page_tables(addr) )
>> -                    blexit(L"Unexpected relocation type");
>>                  if ( delta )
>> +                {
>>                      *(u64 *)addr += delta;
>> +                    if ( in_page_tables(addr) )
>> +                        *(u64 *)addr += xen_phys_start;
> 
> Doesn't the in_page_tables check and modification also apply when
> delta == 0?

No, it would be wrong to do so: efi_arch_load_addr_check() sets
xen_phys_start, and subsequently (to still be able to produce human
visible output) we invoke efi_arch_relocate_image() with an argument
of 0. Later we'll invoke efi_arch_relocate_image() a 2nd time (when
having exited boot services already, and hence when we can't produce
output via EFI anymore, and we can't produce output yet via Xen's
normal mechanisms), with a non-zero argument. Thus we'd add in
xen_phys_start twice.

> Maybe you could just break on !delta to reduce indentation if none of
> this applies then?

Could be done, sure, and if you think this makes sufficiently much
of a difference I can add a patch. The purpose here though it to
have this and the preceding case block look as similar as possible,
yet also not re-format that earlier one (which would be an unrelated
change).

Jan