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

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 13434 bytes --]

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@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.
> +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
> 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?
>        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.
>    if (fbtag)
> diff --git a/include/grub/i386/multiboot.h b/include/grub/i386/multiboot.h
> index a1b9488..807a1de 100644
> --- a/include/grub/i386/multiboot.h
> +++ b/include/grub/i386/multiboot.h
> @@ -30,6 +30,17 @@
>  #define MULTIBOOT_MBI_REGISTER ebx
>  #define MULTIBOOT_ARCHITECTURE_CURRENT MULTIBOOT_ARCHITECTURE_I386
>  
> +#ifdef GRUB_MACHINE_EFI
> +#ifdef __x86_64__
> +#define MULTIBOOT_EFI_INITIAL_STATE  { .rax = MULTIBOOT_BOOTLOADER_MAGIC,	\
> +    .rcx = 0,									\
> +    .rdx = 0,									\
> +      }
> +#define MULTIBOOT_EFI_ENTRY_REGISTER rip
> +#define MULTIBOOT_EFI_MBI_REGISTER rbx
> +#endif
> +#endif
> +
>  #define MULTIBOOT_ELF32_MACHINE EM_386
>  #define MULTIBOOT_ELF64_MACHINE EM_X86_64
>  
> diff --git a/include/grub/i386/relocator.h b/include/grub/i386/relocator.h
> index 5f89a7e..2a56c3b 100644
> --- a/include/grub/i386/relocator.h
> +++ b/include/grub/i386/relocator.h
> @@ -65,6 +65,20 @@ struct grub_relocator64_state
>    grub_addr_t cr3;
>  };
>  
> +#ifdef GRUB_MACHINE_EFI
> +#ifdef __x86_64__
> +struct grub_relocator64_efi_state
> +{
> +  grub_uint64_t rax;
> +  grub_uint64_t rbx;
> +  grub_uint64_t rcx;
> +  grub_uint64_t rdx;
> +  grub_uint64_t rip;
> +  grub_uint64_t rsi;
> +};
> +#endif
> +#endif
> +
>  grub_err_t grub_relocator16_boot (struct grub_relocator *rel,
>  				  struct grub_relocator16_state state);
>  
> @@ -76,4 +90,11 @@ grub_err_t grub_relocator64_boot (struct grub_relocator *rel,
>  				  struct grub_relocator64_state state,
>  				  grub_addr_t min_addr, grub_addr_t max_addr);
>  
> +#ifdef GRUB_MACHINE_EFI
> +#ifdef __x86_64__
> +grub_err_t grub_relocator64_efi_boot (struct grub_relocator *rel,
> +				      struct grub_relocator64_efi_state state);
> +#endif
> +#endif
> +
>  #endif /* ! GRUB_RELOCATOR_CPU_HEADER */
> diff --git a/include/multiboot2.h b/include/multiboot2.h
> index 9d48627..b3977e3 100644
> --- a/include/multiboot2.h
> +++ b/include/multiboot2.h
> @@ -69,6 +69,7 @@
>  #define MULTIBOOT_HEADER_TAG_FRAMEBUFFER  5
>  #define MULTIBOOT_HEADER_TAG_MODULE_ALIGN  6
>  #define MULTIBOOT_HEADER_TAG_EFI_BS  7
> +#define MULTIBOOT_HEADER_TAG_ENTRY_ADDRESS_EFI64  9
>  
>  #define MULTIBOOT_ARCHITECTURE_I386  0
>  #define MULTIBOOT_ARCHITECTURE_MIPS32  4
> @@ -133,6 +134,14 @@ struct multiboot_header_tag_entry_address
>    multiboot_uint32_t entry_addr;
>  };
>  
> +struct multiboot_header_tag_entry_address_efi64
> +{
> +  multiboot_uint16_t type;
> +  multiboot_uint16_t flags;
> +  multiboot_uint32_t size;
> +  multiboot_uint32_t entry_addr;
> +};
> +
Ditto
>  struct multiboot_header_tag_console_flags
>  {
>    multiboot_uint16_t type;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]