Re: [Xen-devel] [PATCH v2 8/8] efi: drop original xen.efi code and build mechanism

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.07.18 at 16:46, <daniel.kiper@xxxxxxxxxx> wrote:
> On Thu, Jul 05, 2018 at 02:35:32AM -0600, Jan Beulich wrote:
>> >>> On 04.07.18 at 18:48, <daniel.kiper@xxxxxxxxxx> wrote:
>> > On Wed, Jul 04, 2018 at 09:34:09AM -0600, Jan Beulich wrote:
>> >> >>> On 04.07.18 at 16:25, <daniel.kiper@xxxxxxxxxx> wrote:
>> >> > On Thu, Jun 28, 2018 at 07:51:52AM -0600, Jan Beulich wrote:
>> >> >> >>> On 19.06.18 at 16:35, <daniel.kiper@xxxxxxxxxx> wrote:
>> >> >> > Then rename xen.mb.efi to xen.efi and drop all related
>> >> >> > differentiators in the code.
>> >> >>
>> >> >> For this you'll first of all need to convince me that the binary you 
>> >> >> build is
>> >> >> a drop-in replacement for xen.efi. As noted in the replies to earlier
>> >> >> patches, I'm getting the impression of this not being the case. A 
>> >> >> further
>> >> >> hint towards this is the outright deletion of 
>> >> >> xen/arch/x86/efi/mkreloc.c:
>> >> >> How is the Xen image going to be relocated that way, when loaded from
>> >> >> the EFI shell or boot loader?
>> >> >
>> >> > It works because all addressing is relative to %rip. To be precise, new
>> >> > xen.efi, earlier xen.mb.efi, contains exactly the same code as ELF does.
>> >> > So, if ELF works without any relocations why PE should not. Especially 
>> >> > on
>> >> > x86-64. Additionally, my tests showed that in general UEFI 
>> >> > implementations
>> >> > just require Base Relocation Table entry in PE Data Directories to 
>> >> > relocate
>> >> > the image.
>> >>
>> >> The thing I'm missing in the series is the generation of the relocations 
>> >> to
>> >> go into the section pointed to by this Data Directory entry.
>> >
>> > There is no such thing because xen.mb.efi does not need that.
>>
>> Well, fine. But you still owe me an answer to the "why" part here.
>>
>> >> > Even it can be empty. As it is in current patchset.
>> >>
>> >> No, it can't be empty. Just try removing the relocations from xen.efi and
>> >> see what you get.
>> >
>> > I am pretty sure that current xen.efi will not work without .reloc section.
>> > However, xen.mb.efi works without any issue. So, I am not sure where is the
>> > problem. +/- fake .reloc entry which I was talking about earlier.
>>
>> As per above: If this is the case - fine. But I want this to be explained,
>> not the least because I'd like to understand if I wasted effort back when
>> I added to code to produce and handle the relocations.
> 
> OK, xen.mb.efi does not need relocs because:
>   - we generate PE file from xen-syms file like we do with ELF output;
>     so, the code in the PE file is the same like in the ELF file;
>     hence, if ELF works why PE should not,
>   - all addressing is relative to %rip as it is in ELF file,

What are the several hundred base relocs in xen.efi doing then? Sure
some of them wouldn't really be needed, but I doubt that's true for
all of them. The first and foremost case of non-RIP-relative addressing
is data with static initializers pointing elsewhere in the image. These
need relocations applied to work.

Once again - a fundamental criteria is whether your binary can be used
in place of the current xen.efi. I can't convince myself that you've
actually tried that out. At the very least I'd expect the static array in
PrintErrMesg() to present problems here.

>   - page tables content is updated by Xen EFI boot code and
>     does not depend on .reloc section.
> 
> Hmmm... And after some thinking I realized that maybe you used relocs because
> probably it is not possible, or at least difficult, to have relocatable PE
> EFI ia32 binary without them. IIRC you did that work when 32bit Xen was
> supported. However, I am not sure Xen PE EFI ia32 was build or not.

We've never built a xen.efi that would work on a 32-bit EFI.

> Another
> factor maybe was that, IIRC, you derived Xen EFI boot from another internal
> SUSE EFI project. Hence, there is a chance that it is more or less copy
> from it.

There was no need for base relocations in that original project, because
there the binary really was just the OS loader, not the OS kernel itself.

>> >> > Though I am
>> >> > afraid about more picky UEFI stuff and considering addition of at least
>> >> > one .reloc entry, fake one, as Linux kernel does. And this rises another
>> >> > question: should not we add .bss section into PE header? Right now it is
>> >> > embedded/hidden in PE .text section.
>> >>
>> >> My xen.efi does have a .bss section.
>> >
>> > It is completely valid to have BSS embedded in .text section or living
>> > as a separate entity in PE section table. So, the question is: which one
>> > do we prefer? Currently existing xen.mb.efi layout is similar to ELF 
>> > layout.
>>
>> I'd prefer any binary to look as natural as possible. However, our ELF
>> binary doesn't, and hence I wouldn't insist on this to be the case for
>> the PE one. There's a reason for the ELF one to be the way it is, though.
> 
> My guess is that it is related, at least to some extent, to the
> compatibility with the Muliboot protocol. Commit b199c44 (efi: build xen.gz
> with EFI code) explains the problem quite well. However, AFAICT, it is
> possible to build ELF with .text and .bss sections which can be properly
> digested by the Multiboot protocols.
> 
>> If no similar reason exists for the PE one, then please have it have a
>> "normal" set of sections.
> 
> OK, I will add .bss and fake .reloc section.

As per above - minus "fake".

Jan


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