[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 3/6] i386/relocator: Add grub_relocator64_efi relocator



On Mon, Nov 09, 2015 at 09:05:15PM +0100, Vladimir 'Ï-coder/phcoder' Serbinenko 
wrote:
> On 20.07.2015 16:35, Daniel Kiper wrote:
> > Add grub_relocator64_efi relocator. It will be used on EFI 64-bit platforms
> > when multiboot2 compatible image requests MULTIBOOT_TAG_TYPE_EFI_BS. 
> > Relocator
> > will set lower parts of %rax and %rbx accordingly to multiboot2 
> > specification.
> > On the other hand processor mode, just before jumping into loaded image, 
> > will
> > be set accordingly to Unified Extensible Firmware Interface Specification,
> > Version 2.4 Errata B, section 2.3.4, x64 Platforms, boot services. This way
> > loaded image will be able to use EFI boot services without any issues.
> >
> > If idea is accepted I will prepare grub_relocator32_efi relocator too.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
> > ---
> >  grub-core/Makefile.core.def          |    1 +
> >  grub-core/lib/i386/relocator.c       |   53 +++++++++++++++++++++++
> >  grub-core/lib/i386/relocator64_efi.S |   77 
> > ++++++++++++++++++++++++++++++++++
> >  grub-core/loader/multiboot.c         |   29 +++++++++++--
> >  grub-core/loader/multiboot_mbi2.c    |   19 +++++++--
> >  include/grub/i386/multiboot.h        |   11 +++++
> >  include/grub/i386/relocator.h        |   21 ++++++++++
> >  include/multiboot2.h                 |    9 ++++
> >  8 files changed, 213 insertions(+), 7 deletions(-)
> >  create mode 100644 grub-core/lib/i386/relocator64_efi.S
> >
> > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> > index a6101de..d583549 100644
> > --- a/grub-core/Makefile.core.def
> > +++ b/grub-core/Makefile.core.def
> > @@ -1519,6 +1519,7 @@ module = {
> >    x86 = lib/i386/relocator_common_c.c;
> >    ieee1275 = lib/ieee1275/relocator.c;
> >    efi = lib/efi/relocator.c;
> > +  x86_64_efi = lib/i386/relocator64_efi.S;
> >    mips = lib/mips/relocator_asm.S;
> >    mips = lib/mips/relocator.c;
> >    powerpc = lib/powerpc/relocator_asm.S;
> > diff --git a/grub-core/lib/i386/relocator.c b/grub-core/lib/i386/relocator.c
> > index 71dd4f0..459027e 100644
> > --- a/grub-core/lib/i386/relocator.c
> > +++ b/grub-core/lib/i386/relocator.c
> > @@ -69,6 +69,19 @@ extern grub_uint64_t grub_relocator64_rsi;
> >  extern grub_addr_t grub_relocator64_cr3;
> >  extern struct grub_i386_idt grub_relocator16_idt;
> >
> > +#ifdef GRUB_MACHINE_EFI
> > +#ifdef __x86_64__
> > +extern grub_uint8_t grub_relocator64_efi_start;
> > +extern grub_uint8_t grub_relocator64_efi_end;
> > +extern grub_uint64_t grub_relocator64_efi_rax;
> > +extern grub_uint64_t grub_relocator64_efi_rbx;
> > +extern grub_uint64_t grub_relocator64_efi_rcx;
> > +extern grub_uint64_t grub_relocator64_efi_rdx;
> > +extern grub_uint64_t grub_relocator64_efi_rip;
> > +extern grub_uint64_t grub_relocator64_efi_rsi;
> > +#endif
> > +#endif
> > +
> >  #define RELOCATOR_SIZEOF(x)        (&grub_relocator##x##_end - 
> > &grub_relocator##x##_start)
> >
> >  grub_err_t
> > @@ -214,3 +227,43 @@ grub_relocator64_boot (struct grub_relocator *rel,
> >    /* Not reached.  */
> >    return GRUB_ERR_NONE;
> >  }
> > +
> > +#ifdef GRUB_MACHINE_EFI
> > +#ifdef __x86_64__
> > +grub_err_t
> > +grub_relocator64_efi_boot (struct grub_relocator *rel,
> > +                      struct grub_relocator64_efi_state state)
> > +{
> > +  grub_err_t err;
> > +  void *relst;
> > +  grub_relocator_chunk_t ch;
> > +
> > +  err = grub_relocator_alloc_chunk_align (rel, &ch, 0,
> > +                                     0x40000000 - RELOCATOR_SIZEOF 
> > (64_efi),
> > +                                     RELOCATOR_SIZEOF (64_efi), 16,
> > +                                     GRUB_RELOCATOR_PREFERENCE_NONE, 1);
> > +  if (err)
> > +    return err;
> > +
> > +  grub_relocator64_efi_rax = state.rax;
> > +  grub_relocator64_efi_rbx = state.rbx;
> > +  grub_relocator64_efi_rcx = state.rcx;
> > +  grub_relocator64_efi_rdx = state.rdx;
> > +  grub_relocator64_efi_rip = state.rip;
> > +  grub_relocator64_efi_rsi = state.rsi;
> > +
> > +  grub_memmove (get_virtual_current_address (ch), 
> > &grub_relocator64_efi_start,
> > +           RELOCATOR_SIZEOF (64_efi));
> > +
> > +  err = grub_relocator_prepare_relocs (rel, get_physical_target_address 
> > (ch),
> > +                                  &relst, NULL);
> > +  if (err)
> > +    return err;
> > +
> > +  ((void (*) (void)) relst) ();
> > +
> > +  /* Not reached.  */
> > +  return GRUB_ERR_NONE;
> > +}
> > +#endif
> > +#endif
> > diff --git a/grub-core/lib/i386/relocator64_efi.S 
> > b/grub-core/lib/i386/relocator64_efi.S
> > new file mode 100644
> > index 0000000..fcd1964
> > --- /dev/null
> > +++ b/grub-core/lib/i386/relocator64_efi.S
> > @@ -0,0 +1,77 @@
> > +/*
> > + *  GRUB  --  GRand Unified Bootloader
> > + *  Copyright (C) 2009,2010  Free Software Foundation, Inc.
> > + *  Copyright (C) 2014,2015  Oracle Co.
> > + *      Author: Daniel Kiper
> > + *
> > + *  GRUB is free software: you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License as published by
> > + *  the Free Software Foundation, either version 3 of the License, or
> > + *  (at your option) any later version.
> > + *
> > + *  GRUB is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + *
> > + *  You should have received a copy of the GNU General Public License
> > + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include "relocator_common.S"
> > +
> > +   .p2align        4       /* force 16-byte alignment */
> > +
> > +VARIABLE(grub_relocator64_efi_start)
> > +   PREAMBLE
> > +
> > +   .code64
> > +
> > +   /* mov imm64, %rax */
> > +   .byte   0x48
> > +   .byte   0xb8
> > +VARIABLE(grub_relocator64_efi_rsi)
> > +   .quad   0
> > +
> > +   movq    %rax, %rsi
> > +
> > +   /* mov imm64, %rax */
> > +   .byte   0x48
> > +   .byte   0xb8
> > +VARIABLE(grub_relocator64_efi_rax)
> > +   .quad   0
> > +
> > +   /* mov imm64, %rbx */
> > +   .byte   0x48
> > +   .byte   0xbb
> > +VARIABLE(grub_relocator64_efi_rbx)
> > +   .quad   0
> > +
> > +   /* mov imm64, %rcx */
> > +   .byte   0x48
> > +   .byte   0xb9
> > +VARIABLE(grub_relocator64_efi_rcx)
> > +   .quad   0
> > +
> > +   /* mov imm64, %rdx */
> > +   .byte   0x48
> > +   .byte   0xba
> > +VARIABLE(grub_relocator64_efi_rdx)
> > +   .quad   0
> > +
> > +   /* Cleared direction flag is of no problem with any current
> > +      payload and makes this implementation easier.  */
> > +   cld
> > +
> > +#ifdef __APPLE__
> > +   .byte 0xff, 0x25
> > +   .quad 0
> > +#else
> > +   jmp *LOCAL(jump_addr) (%rip)
> > +#endif
> > +
> > +LOCAL(jump_addr):
> > +VARIABLE(grub_relocator64_efi_rip)
> > +   .quad   0
> > +
> This repeats relocator64 almost exactly. Can we avoid code duplication?
> It's fine to compile it twice but code duplication is bad.

I will try to do that.

> > +VARIABLE(grub_relocator64_efi_end)
> > diff --git a/grub-core/loader/multiboot.c b/grub-core/loader/multiboot.c
> > index fd8f28e..ca7154f 100644
> > --- a/grub-core/loader/multiboot.c
> > +++ b/grub-core/loader/multiboot.c
> > @@ -118,21 +118,44 @@ grub_multiboot_set_video_mode (void)
> >    return err;
> >  }
> >
> > +#ifdef GRUB_MACHINE_EFI
> > +#ifdef __x86_64__
> > +#define grub_relocator_efi_boot    grub_relocator64_efi_boot
> > +#endif
> > +#endif
> > +
> >  static grub_err_t
> >  grub_multiboot_boot (void)
> >  {
> >    grub_err_t err;
> > +  grub_uint32_t target;
> >    struct grub_relocator32_state state = MULTIBOOT_INITIAL_STATE;
> > +#ifdef GRUB_MACHINE_EFI
> > +#ifdef __x86_64__
> > +  struct grub_relocator64_efi_state state_efi = 
> > MULTIBOOT_EFI_INITIAL_STATE;
> > +#endif
> > +#endif
> >
> > -  state.MULTIBOOT_ENTRY_REGISTER = grub_multiboot_payload_eip;
> > -
> > -  err = grub_multiboot_make_mbi (&state.MULTIBOOT_MBI_REGISTER);
> > +  err = grub_multiboot_make_mbi (&target);
> >
> >    if (err)
> >      return err;
> >
> > +  state.MULTIBOOT_ENTRY_REGISTER = grub_multiboot_payload_eip;
> > +  state.MULTIBOOT_MBI_REGISTER = target;
> > +
> >  #if defined (__i386__) || defined (__x86_64__)
> > +#ifdef GRUB_MACHINE_EFI
> > +  state_efi.MULTIBOOT_EFI_ENTRY_REGISTER = grub_multiboot_payload_eip;
> > +  state_efi.MULTIBOOT_EFI_MBI_REGISTER = target;
> > +
> > +  if (grub_efi_is_finished)
> > +    grub_relocator32_boot (grub_multiboot_relocator, state, 0);
> > +  else
> > +    grub_relocator_efi_boot (grub_multiboot_relocator, state_efi);
> > +#else
> >    grub_relocator32_boot (grub_multiboot_relocator, state, 0);
> > +#endif
> >  #else
> >    grub_relocator32_boot (grub_multiboot_relocator, state);
> >  #endif
> This becomes hairy. I think it's time to split it into platform-specific
> functions and/or use tricks like
> #ifndef GRUB_MACHINE_EFI
> #define grub_efi_is_finished 0
> #endif

OK.

> > diff --git a/grub-core/loader/multiboot_mbi2.c 
> > b/grub-core/loader/multiboot_mbi2.c
> > index d7c19bc..8d66e3f 100644
> > --- a/grub-core/loader/multiboot_mbi2.c
> > +++ b/grub-core/loader/multiboot_mbi2.c
> > @@ -107,8 +107,8 @@ grub_multiboot_load (grub_file_t file, const char 
> > *filename)
> >    grub_err_t err;
> >    struct multiboot_header_tag *tag;
> >    struct multiboot_header_tag_address *addr_tag = NULL;
> > -  int entry_specified = 0;
> > -  grub_addr_t entry = 0;
> > +  int entry_specified = 0, efi_entry_specified = 0;
> > +  grub_addr_t entry = 0, efi_entry = 0;
> >    grub_uint32_t console_required = 0;
> >    struct multiboot_header_tag_framebuffer *fbtag = NULL;
> >    int accepted_consoles = GRUB_MULTIBOOT_CONSOLE_EGA_TEXT;
> > @@ -192,6 +192,13 @@ grub_multiboot_load (grub_file_t file, const char 
> > *filename)
> >     entry = ((struct multiboot_header_tag_entry_address *) tag)->entry_addr;
> >     break;
> >
> > +      case MULTIBOOT_HEADER_TAG_ENTRY_ADDRESS_EFI64:
> > +#if defined (GRUB_MACHINE_EFI) && defined (__x86_64__)
> > +   efi_entry_specified = 1;
> > +   efi_entry = ((struct multiboot_header_tag_entry_address_efi64 *) 
> > tag)->entry_addr;
> > +#endif
> > +   break;
> > +
> Why do we need separate handling of EFI entry point? Why can't we use
> the same structure?

Make sense.

> >        case MULTIBOOT_HEADER_TAG_CONSOLE_FLAGS:
> >     if (!(((struct multiboot_header_tag_console_flags *) tag)->console_flags
> >         & MULTIBOOT_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED))
> > @@ -211,7 +218,9 @@ grub_multiboot_load (grub_file_t file, const char 
> > *filename)
> >     break;
> >
> >        case MULTIBOOT_HEADER_TAG_EFI_BS:
> > +#ifdef GRUB_MACHINE_EFI
> >     keep_bs = 1;
> > +#endif
> >     break;
> >
> >        default:
> > @@ -224,7 +233,7 @@ grub_multiboot_load (grub_file_t file, const char 
> > *filename)
> >     break;
> >        }
> >
> > -  if (addr_tag && !entry_specified)
> > +  if (addr_tag && !entry_specified && !(keep_bs && efi_entry_specified))
> >      {
> >        grub_free (buffer);
> >        return grub_error (GRUB_ERR_UNKNOWN_OS,
> > @@ -287,7 +296,9 @@ grub_multiboot_load (grub_file_t file, const char 
> > *filename)
> >     }
> >      }
> >
> > -  if (entry_specified)
> > +  if (keep_bs && efi_entry_specified)
> > +    grub_multiboot_payload_eip = efi_entry;
> > +  else if (entry_specified)
> >      grub_multiboot_payload_eip = entry;
> >
> This seems redundant.

What is wrong here?

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.