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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

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,
  - 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. 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.

> >> > 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.

Daniel

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