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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Fri, Jul 06, 2018 at 09:16:38AM -0600, Jan Beulich wrote:
> >>> 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.

Ugh... You are right. I forgot about that. Sadly the problem applies to
the EFI boot code in the xen.gz too. So, both things have to be fixed.
At first sight it seems to me that we can leave relocs in the xen-syms
and then attach them to the xen.mb.efi/xen.gz somehow. It would be nice
to do that just only for the EFI boot code. Should not we put it in
separate section then? Another question is the size of the .reloc section.
We do not know it in advance. So, probably we should build the code in
two steps as we do now. Or prealloc a static place and fill it later.
This way we would have just one phase build.

Or another option. Add Xen mappings in the early EFI boot code. However,
it seems crazy and may not work on all EFI implementations. Hmmm...
Have to check the UEFI spec...

By the way, I am not sure why mkreloc is executed twice. Could you
explain that?

Daniel

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