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.ki...@oracle.com> > > --- > > 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@lists.xen.org http://lists.xen.org/xen-devel