xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions
@ 2016-03-02 16:51 Daniel Kiper
  2016-03-02 16:51 ` [GRUB2 PATCH v3 1/4] i386/relocator: Add grub_relocator64_efi relocator Daniel Kiper
                   ` (4 more replies)
  0 siblings, 5 replies; 30+ messages in thread
From: Daniel Kiper @ 2016-03-02 16:51 UTC (permalink / raw)
  To: xen-devel, grub-devel
  Cc: jgross, eric.snowberg, arvidjaar, andrew.cooper3,
	stefano.stabellini, cardoe, pgnet.dev, roy.franz, ning.sun,
	david.vrabel, jbeulich, phcoder, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei, seth.goldberg

Hi,

This patch series:
  - enables EFI boot services usage in loaded images
    by multiboot2 protocol,
  - add support for multiboot2 protocol compatible
    relocatable images.

Earlier versions of this patch series are extensively tested
and used internally at least in Oracle. It should be mentioned
that this release does not change any functionality introduced
by earlier releases. It just takes into account comments posted
by various people.

Hmmm... Ugh... Cough... Is it possible to get this stuff
into 2.02 train?

Daniel

 grub-core/lib/i386/relocator.c        |   48 +++++++++++++++++++
 grub-core/lib/i386/relocator64.S      |    3 ++
 grub-core/loader/i386/multiboot_mbi.c |    6 ++-
 grub-core/loader/multiboot.c          |   63 ++++++++++++++++++++----
 grub-core/loader/multiboot_elfxx.c    |   28 ++++++++---
 grub-core/loader/multiboot_mbi2.c     |  205 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------
 include/grub/i386/multiboot.h         |   11 +++++
 include/grub/i386/relocator.h         |   21 ++++++++
 include/grub/multiboot.h              |    4 +-
 include/multiboot2.h                  |   41 ++++++++++++++++
 10 files changed, 357 insertions(+), 73 deletions(-)

Daniel Kiper (4):
      i386/relocator: Add grub_relocator64_efi relocator
      multiboot2: Add tags used to pass ImageHandle to loaded image
      multiboot2: Do not pass memory maps to image if EFI boot services are enabled
      multiboot2: Add support for relocatable images


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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [GRUB2 PATCH v3 1/4] i386/relocator: Add grub_relocator64_efi relocator
  2016-03-02 16:51 [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions Daniel Kiper
@ 2016-03-02 16:51 ` Daniel Kiper
  2016-03-10 20:23   ` Vladimir 'phcoder' Serbinenko
  2016-03-02 16:51 ` [GRUB2 PATCH v3 2/4] multiboot2: Add tags used to pass ImageHandle to loaded image Daniel Kiper
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 30+ messages in thread
From: Daniel Kiper @ 2016-03-02 16:51 UTC (permalink / raw)
  To: xen-devel, grub-devel
  Cc: jgross, eric.snowberg, arvidjaar, andrew.cooper3,
	stefano.stabellini, cardoe, pgnet.dev, roy.franz, ning.sun,
	david.vrabel, jbeulich, phcoder, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei, seth.goldberg

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>
---
v3 - suggestions/fixes:
   - reuse grub-core/lib/i386/relocator64.S code
     instead of creating separate assembly file
     (suggested by Vladimir 'phcoder' Serbinenko),
   - grub_multiboot_boot() cleanup
     (suggested by Vladimir 'phcoder' Serbinenko),
   - reuse multiboot_header_tag_entry_address struct instead
     of creating new one for EFI 64-bit entry point
     (suggested by Vladimir 'phcoder' Serbinenko).
---
 grub-core/lib/i386/relocator.c    |   48 ++++++++++++++++++++++++++++++++++
 grub-core/lib/i386/relocator64.S  |    3 +++
 grub-core/loader/multiboot.c      |   51 +++++++++++++++++++++++++++++++++----
 grub-core/loader/multiboot_mbi2.c |   19 +++++++++++---
 include/grub/i386/multiboot.h     |   11 ++++++++
 include/grub/i386/relocator.h     |   21 +++++++++++++++
 include/multiboot2.h              |    1 +
 7 files changed, 145 insertions(+), 9 deletions(-)

diff --git a/grub-core/lib/i386/relocator.c b/grub-core/lib/i386/relocator.c
index 71dd4f0..2b0c260 100644
--- a/grub-core/lib/i386/relocator.c
+++ b/grub-core/lib/i386/relocator.c
@@ -69,6 +69,13 @@ 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;
+#endif
+#endif
+
 #define RELOCATOR_SIZEOF(x)	(&grub_relocator##x##_end - &grub_relocator##x##_start)
 
 grub_err_t
@@ -214,3 +221,44 @@ 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;
+
+  /* Do not touch %rsp! It points to EFI created stack. */
+  grub_relocator64_rax = state.rax;
+  grub_relocator64_rbx = state.rbx;
+  grub_relocator64_rcx = state.rcx;
+  grub_relocator64_rdx = state.rdx;
+  grub_relocator64_rip = state.rip;
+  grub_relocator64_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.S b/grub-core/lib/i386/relocator64.S
index e4648d8..7a06b16 100644
--- a/grub-core/lib/i386/relocator64.S
+++ b/grub-core/lib/i386/relocator64.S
@@ -73,6 +73,7 @@ VARIABLE(grub_relocator64_rsp)
 
 	movq	%rax, %rsp
 
+VARIABLE(grub_relocator64_efi_start)
 	/* mov imm64, %rax */
 	.byte 	0x48
 	.byte	0xb8
@@ -120,6 +121,8 @@ LOCAL(jump_addr):
 VARIABLE(grub_relocator64_rip)
 	.quad	0
 
+VARIABLE(grub_relocator64_efi_end)
+
 #ifndef __x86_64__
 	.p2align	4
 LOCAL(gdt):
diff --git a/grub-core/loader/multiboot.c b/grub-core/loader/multiboot.c
index 73aa0aa..18038fd 100644
--- a/grub-core/loader/multiboot.c
+++ b/grub-core/loader/multiboot.c
@@ -118,6 +118,48 @@ grub_multiboot_set_video_mode (void)
   return err;
 }
 
+#ifdef GRUB_MACHINE_EFI
+#ifdef __x86_64__
+#define grub_relocator_efi_boot		grub_relocator64_efi_boot
+#define grub_relocator_efi_state	grub_relocator64_efi_state
+#endif
+#endif
+
+#ifdef grub_relocator_efi_boot
+static void
+efi_boot (struct grub_relocator *rel,
+	  grub_uint32_t target)
+{
+  struct grub_relocator_efi_state state_efi = MULTIBOOT_EFI_INITIAL_STATE;
+
+  state_efi.MULTIBOOT_EFI_ENTRY_REGISTER = grub_multiboot_payload_eip;
+  state_efi.MULTIBOOT_EFI_MBI_REGISTER = target;
+
+  grub_relocator_efi_boot (rel, state_efi);
+}
+#else
+#define grub_efi_is_finished	1
+static void
+efi_boot (struct grub_relocator *rel __attribute__ ((unused)),
+	  grub_uint32_t target __attribute__ ((unused)))
+{
+}
+#endif
+
+#if defined (__i386__) || defined (__x86_64__)
+static void
+normal_boot (struct grub_relocator *rel, struct grub_relocator32_state state)
+{
+  grub_relocator32_boot (rel, state, 0);
+}
+#else
+static void
+normal_boot (struct grub_relocator *rel, struct grub_relocator32_state state)
+{
+  grub_relocator32_boot (rel, state);
+}
+#endif
+
 static grub_err_t
 grub_multiboot_boot (void)
 {
@@ -131,11 +173,10 @@ grub_multiboot_boot (void)
   if (err)
     return err;
 
-#if defined (__i386__) || defined (__x86_64__)
-  grub_relocator32_boot (grub_multiboot_relocator, state, 0);
-#else
-  grub_relocator32_boot (grub_multiboot_relocator, state);
-#endif
+  if (grub_efi_is_finished)
+    normal_boot (grub_multiboot_relocator, state);
+  else
+    efi_boot (grub_multiboot_relocator, state.MULTIBOOT_MBI_REGISTER);
 
   /* Not reached.  */
   return GRUB_ERR_NONE;
diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
index f147d67..a3dca90 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 *) tag)->entry_addr;
+#endif
+	break;
+
       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;
 
   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..d96aa40 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
-- 
1.7.10.4


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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [GRUB2 PATCH v3 2/4] multiboot2: Add tags used to pass ImageHandle to loaded image
  2016-03-02 16:51 [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions Daniel Kiper
  2016-03-02 16:51 ` [GRUB2 PATCH v3 1/4] i386/relocator: Add grub_relocator64_efi relocator Daniel Kiper
@ 2016-03-02 16:51 ` Daniel Kiper
  2016-03-10 20:26   ` Vladimir 'phcoder' Serbinenko
  2016-03-02 16:51 ` [GRUB2 PATCH v3 3/4] multiboot2: Do not pass memory maps to image if EFI boot services are enabled Daniel Kiper
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 30+ messages in thread
From: Daniel Kiper @ 2016-03-02 16:51 UTC (permalink / raw)
  To: xen-devel, grub-devel
  Cc: jgross, eric.snowberg, arvidjaar, andrew.cooper3,
	stefano.stabellini, cardoe, pgnet.dev, roy.franz, ning.sun,
	david.vrabel, jbeulich, phcoder, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei, seth.goldberg

Add tags used to pass ImageHandle to loaded image if requested.
It is used by at least ExitBootServices() function.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
v3 - suggestions/fixes:
   - mbi EFI related stuff size calculation
     should depend on target architecture
     (suggested by Konrad Rzeszutek Wilk),
   - use plain type instead of pointer
     dereference as sizeof() argument
     (suggested by Konrad Rzeszutek Wilk),
   - improve commit message
     (suggested by Konrad Rzeszutek Wilk).
---
 grub-core/loader/multiboot_mbi2.c |   50 ++++++++++++++++++++++++++++++-------
 include/multiboot2.h              |   16 ++++++++++++
 2 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
index a3dca90..7591edc 100644
--- a/grub-core/loader/multiboot_mbi2.c
+++ b/grub-core/loader/multiboot_mbi2.c
@@ -172,6 +172,8 @@ grub_multiboot_load (grub_file_t file, const char *filename)
 	      case MULTIBOOT_TAG_TYPE_NETWORK:
 	      case MULTIBOOT_TAG_TYPE_EFI_MMAP:
 	      case MULTIBOOT_TAG_TYPE_EFI_BS:
+	      case MULTIBOOT_TAG_TYPE_EFI32_IH:
+	      case MULTIBOOT_TAG_TYPE_EFI64_IH:
 		break;
 
 	      default:
@@ -407,16 +409,22 @@ grub_multiboot_get_mbi_size (void)
 		 + grub_get_multiboot_mmap_count ()
 		 * sizeof (struct multiboot_mmap_entry)), MULTIBOOT_TAG_ALIGN)
     + ALIGN_UP (sizeof (struct multiboot_tag_framebuffer), MULTIBOOT_TAG_ALIGN)
+#ifdef GRUB_MACHINE_EFI
+#ifdef __i386__
     + ALIGN_UP (sizeof (struct multiboot_tag_efi32), MULTIBOOT_TAG_ALIGN)
+    + ALIGN_UP (sizeof (struct multiboot_tag_efi32_ih), MULTIBOOT_TAG_ALIGN)
+#endif
+#ifdef __x86_64__
     + ALIGN_UP (sizeof (struct multiboot_tag_efi64), MULTIBOOT_TAG_ALIGN)
+    + ALIGN_UP (sizeof (struct multiboot_tag_efi64_ih), MULTIBOOT_TAG_ALIGN)
+#endif
+    + ALIGN_UP (sizeof (struct multiboot_tag_efi_mmap)
+		+ efi_mmap_size, MULTIBOOT_TAG_ALIGN)
+#endif
     + ALIGN_UP (sizeof (struct multiboot_tag_old_acpi)
 		+ sizeof (struct grub_acpi_rsdp_v10), MULTIBOOT_TAG_ALIGN)
     + acpiv2_size ()
     + net_size ()
-#ifdef GRUB_MACHINE_EFI
-    + ALIGN_UP (sizeof (struct multiboot_tag_efi_mmap)
-		+ efi_mmap_size, MULTIBOOT_TAG_ALIGN)
-#endif
     + sizeof (struct multiboot_tag_vbe) + MULTIBOOT_TAG_ALIGN - 1
     + sizeof (struct multiboot_tag_apm) + MULTIBOOT_TAG_ALIGN - 1;
 }
@@ -907,11 +915,35 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
 
   if (keep_bs)
     {
-      struct multiboot_tag *tag = (struct multiboot_tag *) ptrorig;
-      tag->type = MULTIBOOT_TAG_TYPE_EFI_BS;
-      tag->size = sizeof (struct multiboot_tag);
-      ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
-	/ sizeof (grub_properly_aligned_t);
+      {
+	struct multiboot_tag *tag = (struct multiboot_tag *) ptrorig;
+	tag->type = MULTIBOOT_TAG_TYPE_EFI_BS;
+	tag->size = sizeof (struct multiboot_tag);
+	ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
+	  / sizeof (grub_properly_aligned_t);
+      }
+
+#ifdef __i386__
+      {
+	struct multiboot_tag_efi32_ih *tag = (struct multiboot_tag_efi32_ih *) ptrorig;
+	tag->type = MULTIBOOT_TAG_TYPE_EFI32_IH;
+	tag->size = sizeof (struct multiboot_tag_efi32_ih);
+	tag->pointer = (grub_addr_t) grub_efi_image_handle;
+	ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
+	  / sizeof (grub_properly_aligned_t);
+      }
+#endif
+
+#ifdef __x86_64__
+      {
+	struct multiboot_tag_efi64_ih *tag = (struct multiboot_tag_efi64_ih *) ptrorig;
+	tag->type = MULTIBOOT_TAG_TYPE_EFI64_IH;
+	tag->size = sizeof (struct multiboot_tag_efi64_ih);
+	tag->pointer = (grub_addr_t) grub_efi_image_handle;
+	ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
+	  / sizeof (grub_properly_aligned_t);
+      }
+#endif
     }
 #endif
 
diff --git a/include/multiboot2.h b/include/multiboot2.h
index d96aa40..36a174f 100644
--- a/include/multiboot2.h
+++ b/include/multiboot2.h
@@ -60,6 +60,8 @@
 #define MULTIBOOT_TAG_TYPE_NETWORK           16
 #define MULTIBOOT_TAG_TYPE_EFI_MMAP          17
 #define MULTIBOOT_TAG_TYPE_EFI_BS            18
+#define MULTIBOOT_TAG_TYPE_EFI32_IH          19
+#define MULTIBOOT_TAG_TYPE_EFI64_IH          20
 
 #define MULTIBOOT_HEADER_TAG_END  0
 #define MULTIBOOT_HEADER_TAG_INFORMATION_REQUEST  1
@@ -371,6 +373,20 @@ struct multiboot_tag_efi_mmap
   multiboot_uint8_t efi_mmap[0];
 }; 
 
+struct multiboot_tag_efi32_ih
+{
+  multiboot_uint32_t type;
+  multiboot_uint32_t size;
+  multiboot_uint32_t pointer;
+};
+
+struct multiboot_tag_efi64_ih
+{
+  multiboot_uint32_t type;
+  multiboot_uint32_t size;
+  multiboot_uint64_t pointer;
+};
+
 #endif /* ! ASM_FILE */
 
 #endif /* ! MULTIBOOT_HEADER */
-- 
1.7.10.4


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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [GRUB2 PATCH v3 3/4] multiboot2: Do not pass memory maps to image if EFI boot services are enabled
  2016-03-02 16:51 [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions Daniel Kiper
  2016-03-02 16:51 ` [GRUB2 PATCH v3 1/4] i386/relocator: Add grub_relocator64_efi relocator Daniel Kiper
  2016-03-02 16:51 ` [GRUB2 PATCH v3 2/4] multiboot2: Add tags used to pass ImageHandle to loaded image Daniel Kiper
@ 2016-03-02 16:51 ` Daniel Kiper
  2016-03-10 20:28   ` Vladimir 'phcoder' Serbinenko
  2016-03-02 16:51 ` [GRUB2 PATCH v3 4/4] multiboot2: Add support for relocatable images Daniel Kiper
  2016-03-09 10:48 ` [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions Daniel Kiper
  4 siblings, 1 reply; 30+ messages in thread
From: Daniel Kiper @ 2016-03-02 16:51 UTC (permalink / raw)
  To: xen-devel, grub-devel
  Cc: jgross, eric.snowberg, arvidjaar, andrew.cooper3,
	stefano.stabellini, cardoe, pgnet.dev, roy.franz, ning.sun,
	david.vrabel, jbeulich, phcoder, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei, seth.goldberg

Do not pass memory maps to image if it asked for EFI boot services.
Main reason for not providing maps is because they will likely be
invalid. We do a few allocations after filling them, e.g. for relocator
needs. Usually we do not care as we would already finish boot services.
If we keep boot services then it is easier to not provide maps. However,
if image needs memory maps and they are not provided by bootloader then
it should get them itself just before ExitBootServices() call.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v3 - suggestions/fixes:
   - improve commit message
     (suggested by Konrad Rzeszutek Wilk and Vladimir 'phcoder' Serbinenko).
---
 grub-core/loader/multiboot_mbi2.c |   71 ++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 36 deletions(-)

diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
index 7591edc..ce68f48 100644
--- a/grub-core/loader/multiboot_mbi2.c
+++ b/grub-core/loader/multiboot_mbi2.c
@@ -390,7 +390,7 @@ static grub_size_t
 grub_multiboot_get_mbi_size (void)
 {
 #ifdef GRUB_MACHINE_EFI
-  if (!efi_mmap_size)
+  if (!keep_bs && !efi_mmap_size)
     find_efi_mmap_size ();    
 #endif
   return 2 * sizeof (grub_uint32_t) + sizeof (struct multiboot_tag)
@@ -759,12 +759,13 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
       }
   }
 
-  {
-    struct multiboot_tag_mmap *tag = (struct multiboot_tag_mmap *) ptrorig;
-    grub_fill_multiboot_mmap (tag);
-    ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
-      / sizeof (grub_properly_aligned_t);
-  }
+  if (!keep_bs)
+    {
+      struct multiboot_tag_mmap *tag = (struct multiboot_tag_mmap *) ptrorig;
+      grub_fill_multiboot_mmap (tag);
+      ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
+	/ sizeof (grub_properly_aligned_t);
+    }
 
   {
     struct multiboot_tag_elf_sections *tag
@@ -780,18 +781,19 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
       / sizeof (grub_properly_aligned_t);
   }
 
-  {
-    struct multiboot_tag_basic_meminfo *tag
-      = (struct multiboot_tag_basic_meminfo *) ptrorig;
-    tag->type = MULTIBOOT_TAG_TYPE_BASIC_MEMINFO;
-    tag->size = sizeof (struct multiboot_tag_basic_meminfo); 
+  if (!keep_bs)
+    {
+      struct multiboot_tag_basic_meminfo *tag
+	= (struct multiboot_tag_basic_meminfo *) ptrorig;
+      tag->type = MULTIBOOT_TAG_TYPE_BASIC_MEMINFO;
+      tag->size = sizeof (struct multiboot_tag_basic_meminfo);
 
-    /* Convert from bytes to kilobytes.  */
-    tag->mem_lower = grub_mmap_get_lower () / 1024;
-    tag->mem_upper = grub_mmap_get_upper () / 1024;
-    ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
-       / sizeof (grub_properly_aligned_t);
-  }
+      /* Convert from bytes to kilobytes.  */
+      tag->mem_lower = grub_mmap_get_lower () / 1024;
+      tag->mem_upper = grub_mmap_get_upper () / 1024;
+      ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
+	/ sizeof (grub_properly_aligned_t);
+    }
 
   {
     struct grub_net_network_level_interface *net;
@@ -890,27 +892,24 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
     grub_efi_uintn_t efi_desc_size;
     grub_efi_uint32_t efi_desc_version;
 
-    tag->type = MULTIBOOT_TAG_TYPE_EFI_MMAP;
-    tag->size = sizeof (*tag) + efi_mmap_size;
-
     if (!keep_bs)
-      err = grub_efi_finish_boot_services (&efi_mmap_size, tag->efi_mmap, NULL,
-					   &efi_desc_size, &efi_desc_version);
-    else
       {
-	if (grub_efi_get_memory_map (&efi_mmap_size, (void *) tag->efi_mmap,
-				     NULL,
-				     &efi_desc_size, &efi_desc_version) <= 0)
-	  err = grub_error (GRUB_ERR_IO, "couldn't retrieve memory map");
+	tag->type = MULTIBOOT_TAG_TYPE_EFI_MMAP;
+	tag->size = sizeof (*tag) + efi_mmap_size;
+
+	err = grub_efi_finish_boot_services (&efi_mmap_size, tag->efi_mmap, NULL,
+					     &efi_desc_size, &efi_desc_version);
+
+	if (err)
+	  return err;
+
+	tag->descr_size = efi_desc_size;
+	tag->descr_vers = efi_desc_version;
+	tag->size = sizeof (*tag) + efi_mmap_size;
+
+	ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
+	  / sizeof (grub_properly_aligned_t);
       }
-    if (err)
-      return err;
-    tag->descr_size = efi_desc_size;
-    tag->descr_vers = efi_desc_version;
-    tag->size = sizeof (*tag) + efi_mmap_size;
-
-    ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
-      / sizeof (grub_properly_aligned_t);
   }
 
   if (keep_bs)
-- 
1.7.10.4


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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [GRUB2 PATCH v3 4/4] multiboot2: Add support for relocatable images
  2016-03-02 16:51 [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions Daniel Kiper
                   ` (2 preceding siblings ...)
  2016-03-02 16:51 ` [GRUB2 PATCH v3 3/4] multiboot2: Do not pass memory maps to image if EFI boot services are enabled Daniel Kiper
@ 2016-03-02 16:51 ` Daniel Kiper
  2016-03-04  6:51   ` Juergen Gross
                     ` (2 more replies)
  2016-03-09 10:48 ` [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions Daniel Kiper
  4 siblings, 3 replies; 30+ messages in thread
From: Daniel Kiper @ 2016-03-02 16:51 UTC (permalink / raw)
  To: xen-devel, grub-devel
  Cc: jgross, eric.snowberg, arvidjaar, andrew.cooper3,
	stefano.stabellini, cardoe, pgnet.dev, roy.franz, ning.sun,
	david.vrabel, jbeulich, phcoder, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei, seth.goldberg

Currently multiboot2 protocol loads image exactly at address specified in
ELF or multiboot2 header. This solution works quite well on legacy BIOS
platforms. It is possible because memory regions are placed at predictable
addresses (though I was not able to find any spec which says that it is
strong requirement, so, it looks that it is just a goodwill of hardware
designers). However, EFI platforms are more volatile. Even if required
memory regions live at specific addresses then they are sometimes simply
not free (e.g. used by boot/runtime services on Dell PowerEdge R820 and
OVMF). This means that you are not able to simply set up final image
destination on build time. You have to provide method to relocate image
contents to real load address which is usually different than load address
specified in ELF and multiboot2 headers.

This patch provides all needed machinery to do self relocation in image code.
First of all GRUB2 reads min_addr (min. load addr), max_addr (max. load addr),
align (required image alignment), preference (it says which memory regions are
preferred by image, e.g. none, low, high) from multiboot_header_tag_relocatable
header tag contained in binary. Later loader tries to fulfill request (not only
that one) and if it succeeds then it informs image about real load address via
multiboot_tag_base_addr tag. At this stage GRUB2 role is finished. Starting
from now executable must cope with relocations itself using whole static
and dynamic knowledge provided by boot loader.

This patch does not provide functionality which could do relocations using
ELF relocation data. However, I was asked by Konrad Rzeszutek Wilk and Vladimir
'phcoder' Serbinenko to investigate that thing. It looks that relevant machinery
could be added to existing code (including this patch) without huge effort.
Additionally, ELF relocation could live in parallel with self relocation provided
by this patch. However, during research I realized that first of all we should
establish the details how ELF relocatable image should look like and how it should
be build. At least to build proper test/example files.

As I saw multiboot2 protocol is able to consume ET_EXEC and ET_DYN ELF files.
Potentially we can use ET_DYN file type. It can be build with gcc/ld -pie option.
However, it contains a lot of unneeded stuff (e.g. INTERP, DYNAMIC, GNU_EH_FRAME
program headers) and it could be quite difficult to drop them (Hmmm... Is it
possible to build it properly with custom ld script?). So, I have checked ET_EXEC
file type. Sadly in this case linker by default resolves all local symbol relocations
and removes relocation related sections. Fortunately it is possible to leave them
as is with simple -q/--emit-relocs ld option. However, output file is quite fragile
and any operation on it should be done with great care (e.g. strip should be called
with --strip-unneeded option). So, this solution is not perfect too. It means that
maybe we should look for better solution. However, I think that we should not use
any custom tools and focus on functionalities provided by compiler and binutils.
In this context ld scripts looks quite promising but maybe you have better solutions.
So, what do you think about that?

This patch was tested with Xen image which uses that functionality. However, this Xen
feature is still under development and new patchset will be released in about 3-4 weeks.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
v3 - suggestions/fixes:
   - reduce number of casts
     (suggested by Konrad Rzeszutek Wilk),
   - remove unneeded space at the end of line
     (suggested by Konrad Rzeszutek Wilk),
   - improve commit message
     (suggested by Konrad Rzeszutek Wilk).
---
 grub-core/loader/i386/multiboot_mbi.c |    6 ++-
 grub-core/loader/multiboot.c          |   12 ++++--
 grub-core/loader/multiboot_elfxx.c    |   28 ++++++++++----
 grub-core/loader/multiboot_mbi2.c     |   65 ++++++++++++++++++++++++++++++---
 include/grub/multiboot.h              |    4 +-
 include/multiboot2.h                  |   24 ++++++++++++
 6 files changed, 120 insertions(+), 19 deletions(-)

diff --git a/grub-core/loader/i386/multiboot_mbi.c b/grub-core/loader/i386/multiboot_mbi.c
index f60b702..4fc83ed 100644
--- a/grub-core/loader/i386/multiboot_mbi.c
+++ b/grub-core/loader/i386/multiboot_mbi.c
@@ -72,7 +72,8 @@ load_kernel (grub_file_t file, const char *filename,
   grub_err_t err;
   if (grub_multiboot_quirks & GRUB_MULTIBOOT_QUIRK_BAD_KLUDGE)
     {
-      err = grub_multiboot_load_elf (file, filename, buffer);
+      err = grub_multiboot_load_elf (file, filename, buffer, 0, 0, 0, 0,
+				     GRUB_RELOCATOR_PREFERENCE_NONE, NULL, 0);
       if (err == GRUB_ERR_NONE) {
 	return GRUB_ERR_NONE;
       }
@@ -121,7 +122,8 @@ load_kernel (grub_file_t file, const char *filename,
       return GRUB_ERR_NONE;
     }
 
-  return grub_multiboot_load_elf (file, filename, buffer);
+  return grub_multiboot_load_elf (file, filename, buffer, 0, 0, 0, 0,
+				  GRUB_RELOCATOR_PREFERENCE_NONE, NULL, 0);
 }
 
 static struct multiboot_header *
diff --git a/grub-core/loader/multiboot.c b/grub-core/loader/multiboot.c
index 18038fd..c0f51b6 100644
--- a/grub-core/loader/multiboot.c
+++ b/grub-core/loader/multiboot.c
@@ -208,12 +208,18 @@ static grub_uint64_t highest_load;
 /* Load ELF32 or ELF64.  */
 grub_err_t
 grub_multiboot_load_elf (grub_file_t file, const char *filename,
-			 void *buffer)
+			 void *buffer, int relocatable, grub_uint32_t min_addr,
+			 grub_uint32_t max_addr, grub_size_t align, grub_uint32_t preference,
+			 grub_uint32_t *base_addr, int avoid_efi_boot_services)
 {
   if (grub_multiboot_is_elf32 (buffer))
-    return grub_multiboot_load_elf32 (file, filename, buffer);
+    return grub_multiboot_load_elf32 (file, filename, buffer, relocatable,
+				      min_addr, max_addr, align, preference,
+				      base_addr, avoid_efi_boot_services);
   else if (grub_multiboot_is_elf64 (buffer))
-    return grub_multiboot_load_elf64 (file, filename, buffer);
+    return grub_multiboot_load_elf64 (file, filename, buffer, relocatable,
+				      min_addr, max_addr, align, preference,
+				      base_addr, avoid_efi_boot_services);
 
   return grub_error (GRUB_ERR_UNKNOWN_OS, N_("invalid arch-dependent ELF magic"));
 }
diff --git a/grub-core/loader/multiboot_elfxx.c b/grub-core/loader/multiboot_elfxx.c
index e3a39b6..0c01569 100644
--- a/grub-core/loader/multiboot_elfxx.c
+++ b/grub-core/loader/multiboot_elfxx.c
@@ -51,7 +51,10 @@ CONCAT(grub_multiboot_is_elf, XX) (void *buffer)
 }
 
 static grub_err_t
-CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char *filename, void *buffer)
+CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char *filename,
+				     void *buffer, int relocatable, grub_uint32_t min_addr,
+				     grub_uint32_t max_addr, grub_size_t align, grub_uint32_t preference,
+				     grub_uint32_t *base_addr, int avoid_efi_boot_services)
 {
   Elf_Ehdr *ehdr = (Elf_Ehdr *) buffer;
   char *phdr_base;
@@ -89,19 +92,30 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char *filename, voi
 	  if (phdr(i)->p_paddr + phdr(i)->p_memsz > highest_load)
 	    highest_load = phdr(i)->p_paddr + phdr(i)->p_memsz;
 
-	  grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx, memsz=0x%lx, vaddr=0x%lx\n",
-			i, (long) phdr(i)->p_paddr, (long) phdr(i)->p_memsz, (long) phdr(i)->p_vaddr);
+	  grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx, memsz=0x%lx, vaddr=0x%lx,"
+			"align=0x%lx, relocatable=%d, avoid_efi_boot_services=%d\n", i,
+			(long) phdr(i)->p_paddr, (long) phdr(i)->p_memsz, (long) phdr(i)->p_vaddr,
+			(long) align, relocatable, avoid_efi_boot_services);
 
 	  {
 	    grub_relocator_chunk_t ch;
-	    err = grub_relocator_alloc_chunk_addr (grub_multiboot_relocator, 
-						   &ch, phdr(i)->p_paddr,
-						   phdr(i)->p_memsz);
+
+	    if (relocatable)
+	      err = grub_relocator_alloc_chunk_align (grub_multiboot_relocator, &ch,
+						      min_addr, max_addr - phdr(i)->p_memsz,
+						      phdr(i)->p_memsz, align ? align : 1,
+						      preference, avoid_efi_boot_services);
+	    else
+	      err = grub_relocator_alloc_chunk_addr (grub_multiboot_relocator,
+						     &ch, phdr(i)->p_paddr,
+						     phdr(i)->p_memsz);
 	    if (err)
 	      {
 		grub_dprintf ("multiboot_loader", "Error loading phdr %d\n", i);
 		return err;
 	      }
+	    if (base_addr)
+	      *base_addr = get_physical_target_address (ch);
 	    source = get_virtual_current_address (ch);
 	  }
 
@@ -208,7 +222,7 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char *filename, voi
 						    + 1, sh->sh_size,
 						    sh->sh_addralign,
 						    GRUB_RELOCATOR_PREFERENCE_NONE,
-						    0);
+						    avoid_efi_boot_services);
 	    if (err)
 	      {
 		grub_dprintf ("multiboot_loader", "Error loading shdr %d\n", i);
diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
index ce68f48..03725a1 100644
--- a/grub-core/loader/multiboot_mbi2.c
+++ b/grub-core/loader/multiboot_mbi2.c
@@ -68,6 +68,7 @@ static grub_size_t elf_sec_num, elf_sec_entsize;
 static unsigned elf_sec_shstrndx;
 static void *elf_sections;
 static int keep_bs = 0;
+static grub_uint32_t base_addr = 0;
 
 void
 grub_multiboot_add_elfsyms (grub_size_t num, grub_size_t entsize,
@@ -107,11 +108,14 @@ 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, efi_entry_specified = 0;
+  struct multiboot_header_tag_relocatable *rel_tag;
+  int entry_specified = 0, efi_entry_specified = 0, relocatable = 0;
   grub_addr_t entry = 0, efi_entry = 0;
-  grub_uint32_t console_required = 0;
+  grub_uint32_t console_required = 0, min_addr = 0;
+  grub_uint32_t max_addr = 0, preference = GRUB_RELOCATOR_PREFERENCE_NONE;
   struct multiboot_header_tag_framebuffer *fbtag = NULL;
   int accepted_consoles = GRUB_MULTIBOOT_CONSOLE_EGA_TEXT;
+  grub_size_t align = 0;
 
   buffer = grub_malloc (MULTIBOOT_SEARCH);
   if (!buffer)
@@ -174,6 +178,7 @@ grub_multiboot_load (grub_file_t file, const char *filename)
 	      case MULTIBOOT_TAG_TYPE_EFI_BS:
 	      case MULTIBOOT_TAG_TYPE_EFI32_IH:
 	      case MULTIBOOT_TAG_TYPE_EFI64_IH:
+	      case MULTIBOOT_TAG_TYPE_BASE_ADDR:
 		break;
 
 	      default:
@@ -215,6 +220,27 @@ grub_multiboot_load (grub_file_t file, const char *filename)
 	accepted_consoles |= GRUB_MULTIBOOT_CONSOLE_FRAMEBUFFER;
 	break;
 
+      case MULTIBOOT_HEADER_TAG_RELOCATABLE:
+	relocatable = 1;
+	rel_tag = (struct multiboot_header_tag_relocatable *) tag;
+	min_addr = rel_tag->min_addr;
+	max_addr = rel_tag->max_addr;
+	align = rel_tag->align;
+	switch (rel_tag->preference)
+	  {
+	  case MULTIBOOT_LOAD_PREFERENCE_LOW:
+	    preference = GRUB_RELOCATOR_PREFERENCE_LOW;
+	    break;
+
+	  case MULTIBOOT_LOAD_PREFERENCE_HIGH:
+	    preference = GRUB_RELOCATOR_PREFERENCE_HIGH;
+	    break;
+
+	  default:
+	    preference = GRUB_RELOCATOR_PREFERENCE_NONE;
+	  }
+	break;
+
 	/* GRUB always page-aligns modules.  */
       case MULTIBOOT_HEADER_TAG_MODULE_ALIGN:
 	break;
@@ -260,15 +286,22 @@ grub_multiboot_load (grub_file_t file, const char *filename)
       else
 	code_size = load_size;
 
-      err = grub_relocator_alloc_chunk_addr (grub_multiboot_relocator, 
-					     &ch, load_addr,
-					     code_size);
+      if (relocatable)
+	err = grub_relocator_alloc_chunk_align (grub_multiboot_relocator, &ch,
+						min_addr, max_addr - code_size,
+						code_size, align ? align : 1,
+						preference, keep_bs);
+      else
+	err = grub_relocator_alloc_chunk_addr (grub_multiboot_relocator,
+					       &ch, load_addr,
+					       code_size);
       if (err)
 	{
 	  grub_dprintf ("multiboot_loader", "Error loading aout kludge\n");
 	  grub_free (buffer);
 	  return err;
 	}
+      base_addr = get_physical_target_address (ch);
       source = get_virtual_current_address (ch);
 
       if ((grub_file_seek (file, offset)) == (grub_off_t) -1)
@@ -290,7 +323,9 @@ grub_multiboot_load (grub_file_t file, const char *filename)
     }
   else
     {
-      err = grub_multiboot_load_elf (file, filename, buffer);
+      err = grub_multiboot_load_elf (file, filename, buffer,
+				     relocatable, min_addr, max_addr,
+				     align, preference, &base_addr, keep_bs);
       if (err)
 	{
 	  grub_free (buffer);
@@ -303,6 +338,14 @@ grub_multiboot_load (grub_file_t file, const char *filename)
   else if (entry_specified)
     grub_multiboot_payload_eip = entry;
 
+  if (relocatable)
+    {
+      if (base_addr > min_addr)
+	grub_multiboot_payload_eip += base_addr - min_addr;
+      else
+	grub_multiboot_payload_eip -= min_addr - base_addr;
+    }
+
   if (fbtag)
     err = grub_multiboot_set_console (GRUB_MULTIBOOT_CONSOLE_FRAMEBUFFER,
 				      accepted_consoles,
@@ -409,6 +452,7 @@ grub_multiboot_get_mbi_size (void)
 		 + grub_get_multiboot_mmap_count ()
 		 * sizeof (struct multiboot_mmap_entry)), MULTIBOOT_TAG_ALIGN)
     + ALIGN_UP (sizeof (struct multiboot_tag_framebuffer), MULTIBOOT_TAG_ALIGN)
+    + ALIGN_UP (sizeof (struct multiboot_tag_base_addr), MULTIBOOT_TAG_ALIGN)
 #ifdef GRUB_MACHINE_EFI
 #ifdef __i386__
     + ALIGN_UP (sizeof (struct multiboot_tag_efi32), MULTIBOOT_TAG_ALIGN)
@@ -698,6 +742,15 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
   ptrorig += (2 * sizeof (grub_uint32_t)) / sizeof (grub_properly_aligned_t);
 
   {
+    struct multiboot_tag_base_addr *tag = (struct multiboot_tag_base_addr *) ptrorig;
+    tag->type = MULTIBOOT_TAG_TYPE_BASE_ADDR;
+    tag->size = sizeof (struct multiboot_tag_base_addr);
+    tag->base_addr = base_addr;
+    ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
+       / sizeof (grub_properly_aligned_t);
+  }
+
+  {
     struct multiboot_tag_string *tag = (struct multiboot_tag_string *) ptrorig;
     tag->type = MULTIBOOT_TAG_TYPE_CMDLINE;
     tag->size = sizeof (struct multiboot_tag_string) + cmdline_size; 
diff --git a/include/grub/multiboot.h b/include/grub/multiboot.h
index e13c084..ec322b0 100644
--- a/include/grub/multiboot.h
+++ b/include/grub/multiboot.h
@@ -94,7 +94,9 @@ grub_multiboot_load (grub_file_t file, const char *filename);
 /* Load ELF32 or ELF64.  */
 grub_err_t
 grub_multiboot_load_elf (grub_file_t file, const char *filename,
-			 void *buffer);
+			 void *buffer, int relocatable, grub_uint32_t min_addr,
+			 grub_uint32_t max_addr, grub_size_t align, grub_uint32_t preference,
+			 grub_uint32_t *base_addr, int avoid_efi_boot_services);
 extern grub_size_t grub_multiboot_pure_size;
 extern grub_size_t grub_multiboot_alloc_mbi;
 extern grub_uint32_t grub_multiboot_payload_eip;
diff --git a/include/multiboot2.h b/include/multiboot2.h
index 36a174f..c09bdbc 100644
--- a/include/multiboot2.h
+++ b/include/multiboot2.h
@@ -62,6 +62,7 @@
 #define MULTIBOOT_TAG_TYPE_EFI_BS            18
 #define MULTIBOOT_TAG_TYPE_EFI32_IH          19
 #define MULTIBOOT_TAG_TYPE_EFI64_IH          20
+#define MULTIBOOT_TAG_TYPE_BASE_ADDR         21
 
 #define MULTIBOOT_HEADER_TAG_END  0
 #define MULTIBOOT_HEADER_TAG_INFORMATION_REQUEST  1
@@ -72,11 +73,16 @@
 #define MULTIBOOT_HEADER_TAG_MODULE_ALIGN  6
 #define MULTIBOOT_HEADER_TAG_EFI_BS  7
 #define MULTIBOOT_HEADER_TAG_ENTRY_ADDRESS_EFI64  9
+#define MULTIBOOT_HEADER_TAG_RELOCATABLE  10
 
 #define MULTIBOOT_ARCHITECTURE_I386  0
 #define MULTIBOOT_ARCHITECTURE_MIPS32  4
 #define MULTIBOOT_HEADER_TAG_OPTIONAL 1
 
+#define MULTIBOOT_LOAD_PREFERENCE_NONE 0
+#define MULTIBOOT_LOAD_PREFERENCE_LOW 1
+#define MULTIBOOT_LOAD_PREFERENCE_HIGH 2
+
 #define MULTIBOOT_CONSOLE_FLAGS_CONSOLE_REQUIRED 1
 #define MULTIBOOT_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED 2
 
@@ -161,6 +167,17 @@ struct multiboot_header_tag_module_align
   multiboot_uint32_t size;
 };
 
+struct multiboot_header_tag_relocatable
+{
+  multiboot_uint16_t type;
+  multiboot_uint16_t flags;
+  multiboot_uint32_t size;
+  multiboot_uint32_t min_addr;
+  multiboot_uint32_t max_addr;
+  multiboot_uint32_t align;
+  multiboot_uint32_t preference;
+};
+
 struct multiboot_color
 {
   multiboot_uint8_t red;
@@ -387,6 +404,13 @@ struct multiboot_tag_efi64_ih
   multiboot_uint64_t pointer;
 };
 
+struct multiboot_tag_base_addr
+{
+  multiboot_uint32_t type;
+  multiboot_uint32_t size;
+  multiboot_uint32_t base_addr;
+};
+
 #endif /* ! ASM_FILE */
 
 #endif /* ! MULTIBOOT_HEADER */
-- 
1.7.10.4


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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 4/4] multiboot2: Add support for relocatable images
  2016-03-02 16:51 ` [GRUB2 PATCH v3 4/4] multiboot2: Add support for relocatable images Daniel Kiper
@ 2016-03-04  6:51   ` Juergen Gross
  2016-03-10 20:42     ` Vladimir 'phcoder' Serbinenko
  2016-03-10 20:41   ` Vladimir 'phcoder' Serbinenko
  2016-03-10 20:44   ` Vladimir 'phcoder' Serbinenko
  2 siblings, 1 reply; 30+ messages in thread
From: Juergen Gross @ 2016-03-04  6:51 UTC (permalink / raw)
  To: Daniel Kiper, xen-devel, grub-devel
  Cc: eric.snowberg, arvidjaar, andrew.cooper3, stefano.stabellini,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	phcoder, qiaowei.ren, richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg

On 02/03/16 17:51, Daniel Kiper wrote:
> Currently multiboot2 protocol loads image exactly at address specified in
> ELF or multiboot2 header. This solution works quite well on legacy BIOS
> platforms. It is possible because memory regions are placed at predictable
> addresses (though I was not able to find any spec which says that it is
> strong requirement, so, it looks that it is just a goodwill of hardware
> designers). However, EFI platforms are more volatile. Even if required
> memory regions live at specific addresses then they are sometimes simply
> not free (e.g. used by boot/runtime services on Dell PowerEdge R820 and
> OVMF). This means that you are not able to simply set up final image
> destination on build time. You have to provide method to relocate image
> contents to real load address which is usually different than load address
> specified in ELF and multiboot2 headers.
> 
> This patch provides all needed machinery to do self relocation in image code.
> First of all GRUB2 reads min_addr (min. load addr), max_addr (max. load addr),
> align (required image alignment), preference (it says which memory regions are
> preferred by image, e.g. none, low, high) from multiboot_header_tag_relocatable
> header tag contained in binary. Later loader tries to fulfill request (not only
> that one) and if it succeeds then it informs image about real load address via
> multiboot_tag_base_addr tag. At this stage GRUB2 role is finished. Starting
> from now executable must cope with relocations itself using whole static
> and dynamic knowledge provided by boot loader.
> 
> This patch does not provide functionality which could do relocations using
> ELF relocation data. However, I was asked by Konrad Rzeszutek Wilk and Vladimir
> 'phcoder' Serbinenko to investigate that thing. It looks that relevant machinery
> could be added to existing code (including this patch) without huge effort.
> Additionally, ELF relocation could live in parallel with self relocation provided
> by this patch. However, during research I realized that first of all we should
> establish the details how ELF relocatable image should look like and how it should
> be build. At least to build proper test/example files.
> 
> As I saw multiboot2 protocol is able to consume ET_EXEC and ET_DYN ELF files.
> Potentially we can use ET_DYN file type. It can be build with gcc/ld -pie option.
> However, it contains a lot of unneeded stuff (e.g. INTERP, DYNAMIC, GNU_EH_FRAME
> program headers) and it could be quite difficult to drop them (Hmmm... Is it
> possible to build it properly with custom ld script?). So, I have checked ET_EXEC
> file type. Sadly in this case linker by default resolves all local symbol relocations
> and removes relocation related sections. Fortunately it is possible to leave them
> as is with simple -q/--emit-relocs ld option. However, output file is quite fragile
> and any operation on it should be done with great care (e.g. strip should be called
> with --strip-unneeded option). So, this solution is not perfect too. It means that
> maybe we should look for better solution. However, I think that we should not use
> any custom tools and focus on functionalities provided by compiler and binutils.
> In this context ld scripts looks quite promising but maybe you have better solutions.
> So, what do you think about that?
> 
> This patch was tested with Xen image which uses that functionality. However, this Xen
> feature is still under development and new patchset will be released in about 3-4 weeks.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
> v3 - suggestions/fixes:
>    - reduce number of casts
>      (suggested by Konrad Rzeszutek Wilk),
>    - remove unneeded space at the end of line
>      (suggested by Konrad Rzeszutek Wilk),
>    - improve commit message
>      (suggested by Konrad Rzeszutek Wilk).
> ---
>  grub-core/loader/i386/multiboot_mbi.c |    6 ++-
>  grub-core/loader/multiboot.c          |   12 ++++--
>  grub-core/loader/multiboot_elfxx.c    |   28 ++++++++++----
>  grub-core/loader/multiboot_mbi2.c     |   65 ++++++++++++++++++++++++++++++---
>  include/grub/multiboot.h              |    4 +-
>  include/multiboot2.h                  |   24 ++++++++++++
>  6 files changed, 120 insertions(+), 19 deletions(-)
> 
> diff --git a/grub-core/loader/i386/multiboot_mbi.c b/grub-core/loader/i386/multiboot_mbi.c
> index f60b702..4fc83ed 100644
> --- a/grub-core/loader/i386/multiboot_mbi.c
> +++ b/grub-core/loader/i386/multiboot_mbi.c
> @@ -72,7 +72,8 @@ load_kernel (grub_file_t file, const char *filename,
>    grub_err_t err;
>    if (grub_multiboot_quirks & GRUB_MULTIBOOT_QUIRK_BAD_KLUDGE)
>      {
> -      err = grub_multiboot_load_elf (file, filename, buffer);
> +      err = grub_multiboot_load_elf (file, filename, buffer, 0, 0, 0, 0,
> +				     GRUB_RELOCATOR_PREFERENCE_NONE, NULL, 0);

Uuh, really? You are adding 7 parameters for the relocatable case.
Wouldn't it make more sense to have some kind of structure containing
the data you need for relocatable images and pass the pointer to that
structure or NULL in the case of non-relocatable images (or have a
default structure for the non-relocatable case)?


Juergen

>        if (err == GRUB_ERR_NONE) {
>  	return GRUB_ERR_NONE;
>        }
> @@ -121,7 +122,8 @@ load_kernel (grub_file_t file, const char *filename,
>        return GRUB_ERR_NONE;
>      }
>  
> -  return grub_multiboot_load_elf (file, filename, buffer);
> +  return grub_multiboot_load_elf (file, filename, buffer, 0, 0, 0, 0,
> +				  GRUB_RELOCATOR_PREFERENCE_NONE, NULL, 0);
>  }
>  
>  static struct multiboot_header *
> diff --git a/grub-core/loader/multiboot.c b/grub-core/loader/multiboot.c
> index 18038fd..c0f51b6 100644
> --- a/grub-core/loader/multiboot.c
> +++ b/grub-core/loader/multiboot.c
> @@ -208,12 +208,18 @@ static grub_uint64_t highest_load;
>  /* Load ELF32 or ELF64.  */
>  grub_err_t
>  grub_multiboot_load_elf (grub_file_t file, const char *filename,
> -			 void *buffer)
> +			 void *buffer, int relocatable, grub_uint32_t min_addr,
> +			 grub_uint32_t max_addr, grub_size_t align, grub_uint32_t preference,
> +			 grub_uint32_t *base_addr, int avoid_efi_boot_services)
>  {
>    if (grub_multiboot_is_elf32 (buffer))
> -    return grub_multiboot_load_elf32 (file, filename, buffer);
> +    return grub_multiboot_load_elf32 (file, filename, buffer, relocatable,
> +				      min_addr, max_addr, align, preference,
> +				      base_addr, avoid_efi_boot_services);
>    else if (grub_multiboot_is_elf64 (buffer))
> -    return grub_multiboot_load_elf64 (file, filename, buffer);
> +    return grub_multiboot_load_elf64 (file, filename, buffer, relocatable,
> +				      min_addr, max_addr, align, preference,
> +				      base_addr, avoid_efi_boot_services);
>  
>    return grub_error (GRUB_ERR_UNKNOWN_OS, N_("invalid arch-dependent ELF magic"));
>  }
> diff --git a/grub-core/loader/multiboot_elfxx.c b/grub-core/loader/multiboot_elfxx.c
> index e3a39b6..0c01569 100644
> --- a/grub-core/loader/multiboot_elfxx.c
> +++ b/grub-core/loader/multiboot_elfxx.c
> @@ -51,7 +51,10 @@ CONCAT(grub_multiboot_is_elf, XX) (void *buffer)
>  }
>  
>  static grub_err_t
> -CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char *filename, void *buffer)
> +CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char *filename,
> +				     void *buffer, int relocatable, grub_uint32_t min_addr,
> +				     grub_uint32_t max_addr, grub_size_t align, grub_uint32_t preference,
> +				     grub_uint32_t *base_addr, int avoid_efi_boot_services)
>  {
>    Elf_Ehdr *ehdr = (Elf_Ehdr *) buffer;
>    char *phdr_base;
> @@ -89,19 +92,30 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char *filename, voi
>  	  if (phdr(i)->p_paddr + phdr(i)->p_memsz > highest_load)
>  	    highest_load = phdr(i)->p_paddr + phdr(i)->p_memsz;
>  
> -	  grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx, memsz=0x%lx, vaddr=0x%lx\n",
> -			i, (long) phdr(i)->p_paddr, (long) phdr(i)->p_memsz, (long) phdr(i)->p_vaddr);
> +	  grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx, memsz=0x%lx, vaddr=0x%lx,"
> +			"align=0x%lx, relocatable=%d, avoid_efi_boot_services=%d\n", i,
> +			(long) phdr(i)->p_paddr, (long) phdr(i)->p_memsz, (long) phdr(i)->p_vaddr,
> +			(long) align, relocatable, avoid_efi_boot_services);
>  
>  	  {
>  	    grub_relocator_chunk_t ch;
> -	    err = grub_relocator_alloc_chunk_addr (grub_multiboot_relocator, 
> -						   &ch, phdr(i)->p_paddr,
> -						   phdr(i)->p_memsz);
> +
> +	    if (relocatable)
> +	      err = grub_relocator_alloc_chunk_align (grub_multiboot_relocator, &ch,
> +						      min_addr, max_addr - phdr(i)->p_memsz,
> +						      phdr(i)->p_memsz, align ? align : 1,
> +						      preference, avoid_efi_boot_services);
> +	    else
> +	      err = grub_relocator_alloc_chunk_addr (grub_multiboot_relocator,
> +						     &ch, phdr(i)->p_paddr,
> +						     phdr(i)->p_memsz);
>  	    if (err)
>  	      {
>  		grub_dprintf ("multiboot_loader", "Error loading phdr %d\n", i);
>  		return err;
>  	      }
> +	    if (base_addr)
> +	      *base_addr = get_physical_target_address (ch);
>  	    source = get_virtual_current_address (ch);
>  	  }
>  
> @@ -208,7 +222,7 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char *filename, voi
>  						    + 1, sh->sh_size,
>  						    sh->sh_addralign,
>  						    GRUB_RELOCATOR_PREFERENCE_NONE,
> -						    0);
> +						    avoid_efi_boot_services);
>  	    if (err)
>  	      {
>  		grub_dprintf ("multiboot_loader", "Error loading shdr %d\n", i);
> diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
> index ce68f48..03725a1 100644
> --- a/grub-core/loader/multiboot_mbi2.c
> +++ b/grub-core/loader/multiboot_mbi2.c
> @@ -68,6 +68,7 @@ static grub_size_t elf_sec_num, elf_sec_entsize;
>  static unsigned elf_sec_shstrndx;
>  static void *elf_sections;
>  static int keep_bs = 0;
> +static grub_uint32_t base_addr = 0;
>  
>  void
>  grub_multiboot_add_elfsyms (grub_size_t num, grub_size_t entsize,
> @@ -107,11 +108,14 @@ 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, efi_entry_specified = 0;
> +  struct multiboot_header_tag_relocatable *rel_tag;
> +  int entry_specified = 0, efi_entry_specified = 0, relocatable = 0;
>    grub_addr_t entry = 0, efi_entry = 0;
> -  grub_uint32_t console_required = 0;
> +  grub_uint32_t console_required = 0, min_addr = 0;
> +  grub_uint32_t max_addr = 0, preference = GRUB_RELOCATOR_PREFERENCE_NONE;
>    struct multiboot_header_tag_framebuffer *fbtag = NULL;
>    int accepted_consoles = GRUB_MULTIBOOT_CONSOLE_EGA_TEXT;
> +  grub_size_t align = 0;
>  
>    buffer = grub_malloc (MULTIBOOT_SEARCH);
>    if (!buffer)
> @@ -174,6 +178,7 @@ grub_multiboot_load (grub_file_t file, const char *filename)
>  	      case MULTIBOOT_TAG_TYPE_EFI_BS:
>  	      case MULTIBOOT_TAG_TYPE_EFI32_IH:
>  	      case MULTIBOOT_TAG_TYPE_EFI64_IH:
> +	      case MULTIBOOT_TAG_TYPE_BASE_ADDR:
>  		break;
>  
>  	      default:
> @@ -215,6 +220,27 @@ grub_multiboot_load (grub_file_t file, const char *filename)
>  	accepted_consoles |= GRUB_MULTIBOOT_CONSOLE_FRAMEBUFFER;
>  	break;
>  
> +      case MULTIBOOT_HEADER_TAG_RELOCATABLE:
> +	relocatable = 1;
> +	rel_tag = (struct multiboot_header_tag_relocatable *) tag;
> +	min_addr = rel_tag->min_addr;
> +	max_addr = rel_tag->max_addr;
> +	align = rel_tag->align;
> +	switch (rel_tag->preference)
> +	  {
> +	  case MULTIBOOT_LOAD_PREFERENCE_LOW:
> +	    preference = GRUB_RELOCATOR_PREFERENCE_LOW;
> +	    break;
> +
> +	  case MULTIBOOT_LOAD_PREFERENCE_HIGH:
> +	    preference = GRUB_RELOCATOR_PREFERENCE_HIGH;
> +	    break;
> +
> +	  default:
> +	    preference = GRUB_RELOCATOR_PREFERENCE_NONE;
> +	  }
> +	break;
> +
>  	/* GRUB always page-aligns modules.  */
>        case MULTIBOOT_HEADER_TAG_MODULE_ALIGN:
>  	break;
> @@ -260,15 +286,22 @@ grub_multiboot_load (grub_file_t file, const char *filename)
>        else
>  	code_size = load_size;
>  
> -      err = grub_relocator_alloc_chunk_addr (grub_multiboot_relocator, 
> -					     &ch, load_addr,
> -					     code_size);
> +      if (relocatable)
> +	err = grub_relocator_alloc_chunk_align (grub_multiboot_relocator, &ch,
> +						min_addr, max_addr - code_size,
> +						code_size, align ? align : 1,
> +						preference, keep_bs);
> +      else
> +	err = grub_relocator_alloc_chunk_addr (grub_multiboot_relocator,
> +					       &ch, load_addr,
> +					       code_size);
>        if (err)
>  	{
>  	  grub_dprintf ("multiboot_loader", "Error loading aout kludge\n");
>  	  grub_free (buffer);
>  	  return err;
>  	}
> +      base_addr = get_physical_target_address (ch);
>        source = get_virtual_current_address (ch);
>  
>        if ((grub_file_seek (file, offset)) == (grub_off_t) -1)
> @@ -290,7 +323,9 @@ grub_multiboot_load (grub_file_t file, const char *filename)
>      }
>    else
>      {
> -      err = grub_multiboot_load_elf (file, filename, buffer);
> +      err = grub_multiboot_load_elf (file, filename, buffer,
> +				     relocatable, min_addr, max_addr,
> +				     align, preference, &base_addr, keep_bs);
>        if (err)
>  	{
>  	  grub_free (buffer);
> @@ -303,6 +338,14 @@ grub_multiboot_load (grub_file_t file, const char *filename)
>    else if (entry_specified)
>      grub_multiboot_payload_eip = entry;
>  
> +  if (relocatable)
> +    {
> +      if (base_addr > min_addr)
> +	grub_multiboot_payload_eip += base_addr - min_addr;
> +      else
> +	grub_multiboot_payload_eip -= min_addr - base_addr;
> +    }
> +
>    if (fbtag)
>      err = grub_multiboot_set_console (GRUB_MULTIBOOT_CONSOLE_FRAMEBUFFER,
>  				      accepted_consoles,
> @@ -409,6 +452,7 @@ grub_multiboot_get_mbi_size (void)
>  		 + grub_get_multiboot_mmap_count ()
>  		 * sizeof (struct multiboot_mmap_entry)), MULTIBOOT_TAG_ALIGN)
>      + ALIGN_UP (sizeof (struct multiboot_tag_framebuffer), MULTIBOOT_TAG_ALIGN)
> +    + ALIGN_UP (sizeof (struct multiboot_tag_base_addr), MULTIBOOT_TAG_ALIGN)
>  #ifdef GRUB_MACHINE_EFI
>  #ifdef __i386__
>      + ALIGN_UP (sizeof (struct multiboot_tag_efi32), MULTIBOOT_TAG_ALIGN)
> @@ -698,6 +742,15 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
>    ptrorig += (2 * sizeof (grub_uint32_t)) / sizeof (grub_properly_aligned_t);
>  
>    {
> +    struct multiboot_tag_base_addr *tag = (struct multiboot_tag_base_addr *) ptrorig;
> +    tag->type = MULTIBOOT_TAG_TYPE_BASE_ADDR;
> +    tag->size = sizeof (struct multiboot_tag_base_addr);
> +    tag->base_addr = base_addr;
> +    ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> +       / sizeof (grub_properly_aligned_t);
> +  }
> +
> +  {
>      struct multiboot_tag_string *tag = (struct multiboot_tag_string *) ptrorig;
>      tag->type = MULTIBOOT_TAG_TYPE_CMDLINE;
>      tag->size = sizeof (struct multiboot_tag_string) + cmdline_size; 
> diff --git a/include/grub/multiboot.h b/include/grub/multiboot.h
> index e13c084..ec322b0 100644
> --- a/include/grub/multiboot.h
> +++ b/include/grub/multiboot.h
> @@ -94,7 +94,9 @@ grub_multiboot_load (grub_file_t file, const char *filename);
>  /* Load ELF32 or ELF64.  */
>  grub_err_t
>  grub_multiboot_load_elf (grub_file_t file, const char *filename,
> -			 void *buffer);
> +			 void *buffer, int relocatable, grub_uint32_t min_addr,
> +			 grub_uint32_t max_addr, grub_size_t align, grub_uint32_t preference,
> +			 grub_uint32_t *base_addr, int avoid_efi_boot_services);
>  extern grub_size_t grub_multiboot_pure_size;
>  extern grub_size_t grub_multiboot_alloc_mbi;
>  extern grub_uint32_t grub_multiboot_payload_eip;
> diff --git a/include/multiboot2.h b/include/multiboot2.h
> index 36a174f..c09bdbc 100644
> --- a/include/multiboot2.h
> +++ b/include/multiboot2.h
> @@ -62,6 +62,7 @@
>  #define MULTIBOOT_TAG_TYPE_EFI_BS            18
>  #define MULTIBOOT_TAG_TYPE_EFI32_IH          19
>  #define MULTIBOOT_TAG_TYPE_EFI64_IH          20
> +#define MULTIBOOT_TAG_TYPE_BASE_ADDR         21
>  
>  #define MULTIBOOT_HEADER_TAG_END  0
>  #define MULTIBOOT_HEADER_TAG_INFORMATION_REQUEST  1
> @@ -72,11 +73,16 @@
>  #define MULTIBOOT_HEADER_TAG_MODULE_ALIGN  6
>  #define MULTIBOOT_HEADER_TAG_EFI_BS  7
>  #define MULTIBOOT_HEADER_TAG_ENTRY_ADDRESS_EFI64  9
> +#define MULTIBOOT_HEADER_TAG_RELOCATABLE  10
>  
>  #define MULTIBOOT_ARCHITECTURE_I386  0
>  #define MULTIBOOT_ARCHITECTURE_MIPS32  4
>  #define MULTIBOOT_HEADER_TAG_OPTIONAL 1
>  
> +#define MULTIBOOT_LOAD_PREFERENCE_NONE 0
> +#define MULTIBOOT_LOAD_PREFERENCE_LOW 1
> +#define MULTIBOOT_LOAD_PREFERENCE_HIGH 2
> +
>  #define MULTIBOOT_CONSOLE_FLAGS_CONSOLE_REQUIRED 1
>  #define MULTIBOOT_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED 2
>  
> @@ -161,6 +167,17 @@ struct multiboot_header_tag_module_align
>    multiboot_uint32_t size;
>  };
>  
> +struct multiboot_header_tag_relocatable
> +{
> +  multiboot_uint16_t type;
> +  multiboot_uint16_t flags;
> +  multiboot_uint32_t size;
> +  multiboot_uint32_t min_addr;
> +  multiboot_uint32_t max_addr;
> +  multiboot_uint32_t align;
> +  multiboot_uint32_t preference;
> +};
> +
>  struct multiboot_color
>  {
>    multiboot_uint8_t red;
> @@ -387,6 +404,13 @@ struct multiboot_tag_efi64_ih
>    multiboot_uint64_t pointer;
>  };
>  
> +struct multiboot_tag_base_addr
> +{
> +  multiboot_uint32_t type;
> +  multiboot_uint32_t size;
> +  multiboot_uint32_t base_addr;
> +};
> +
>  #endif /* ! ASM_FILE */
>  
>  #endif /* ! MULTIBOOT_HEADER */
> 


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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions
  2016-03-02 16:51 [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions Daniel Kiper
                   ` (3 preceding siblings ...)
  2016-03-02 16:51 ` [GRUB2 PATCH v3 4/4] multiboot2: Add support for relocatable images Daniel Kiper
@ 2016-03-09 10:48 ` Daniel Kiper
  2016-03-11 12:27   ` Vladimir 'phcoder' Serbinenko
  4 siblings, 1 reply; 30+ messages in thread
From: Daniel Kiper @ 2016-03-09 10:48 UTC (permalink / raw)
  To: xen-devel, grub-devel
  Cc: jgross, eric.snowberg, arvidjaar, andrew.cooper3,
	stefano.stabellini, cardoe, pgnet.dev, roy.franz, ning.sun,
	david.vrabel, jbeulich, phcoder, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei, seth.goldberg

On Wed, Mar 02, 2016 at 05:51:36PM +0100, Daniel Kiper wrote:
> Hi,
>
> This patch series:
>   - enables EFI boot services usage in loaded images
>     by multiboot2 protocol,
>   - add support for multiboot2 protocol compatible
>     relocatable images.
>
> Earlier versions of this patch series are extensively tested
> and used internally at least in Oracle. It should be mentioned
> that this release does not change any functionality introduced
> by earlier releases. It just takes into account comments posted
> by various people.
>
> Hmmm... Ugh... Cough... Is it possible to get this stuff
> into 2.02 train?

Andrei, Vladimir, ping?

Daniel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 1/4] i386/relocator: Add grub_relocator64_efi relocator
  2016-03-02 16:51 ` [GRUB2 PATCH v3 1/4] i386/relocator: Add grub_relocator64_efi relocator Daniel Kiper
@ 2016-03-10 20:23   ` Vladimir 'phcoder' Serbinenko
  2016-03-11 13:23     ` Daniel Kiper
  0 siblings, 1 reply; 30+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2016-03-10 20:23 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com


[-- Attachment #1.1: Type: text/plain, Size: 11340 bytes --]

On Wednesday, March 2, 2016, Daniel Kiper <daniel.kiper@oracle.com> 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 <javascript:;>>
> ---
> v3 - suggestions/fixes:
>    - reuse grub-core/lib/i386/relocator64.S code
>      instead of creating separate assembly file
>      (suggested by Vladimir 'phcoder' Serbinenko),
>    - grub_multiboot_boot() cleanup
>      (suggested by Vladimir 'phcoder' Serbinenko),
>    - reuse multiboot_header_tag_entry_address struct instead
>      of creating new one for EFI 64-bit entry point
>      (suggested by Vladimir 'phcoder' Serbinenko).
> ---
>  grub-core/lib/i386/relocator.c    |   48
> ++++++++++++++++++++++++++++++++++
>  grub-core/lib/i386/relocator64.S  |    3 +++
>  grub-core/loader/multiboot.c      |   51
> +++++++++++++++++++++++++++++++++----
>  grub-core/loader/multiboot_mbi2.c |   19 +++++++++++---
>  include/grub/i386/multiboot.h     |   11 ++++++++
>  include/grub/i386/relocator.h     |   21 +++++++++++++++
>  include/multiboot2.h              |    1 +
>  7 files changed, 145 insertions(+), 9 deletions(-)
>
> diff --git a/grub-core/lib/i386/relocator.c
> b/grub-core/lib/i386/relocator.c
> index 71dd4f0..2b0c260 100644
> --- a/grub-core/lib/i386/relocator.c
> +++ b/grub-core/lib/i386/relocator.c
> @@ -69,6 +69,13 @@ 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;
> +#endif
> +#endif
> +
>
Can we move this and all to a separate file to avoid too many #ifdef ?


>  #define RELOCATOR_SIZEOF(x)    (&grub_relocator##x##_end -
> &grub_relocator##x##_start)
>
>  grub_err_t
> @@ -214,3 +221,44 @@ 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;
> +
> +  /* Do not touch %rsp! It points to EFI created stack. */
> +  grub_relocator64_rax = state.rax;
> +  grub_relocator64_rbx = state.rbx;
> +  grub_relocator64_rcx = state.rcx;
> +  grub_relocator64_rdx = state.rdx;
> +  grub_relocator64_rip = state.rip;
> +  grub_relocator64_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.S
> b/grub-core/lib/i386/relocator64.S
> index e4648d8..7a06b16 100644
> --- a/grub-core/lib/i386/relocator64.S
> +++ b/grub-core/lib/i386/relocator64.S
> @@ -73,6 +73,7 @@ VARIABLE(grub_relocator64_rsp)
>
>         movq    %rax, %rsp
>
> +VARIABLE(grub_relocator64_efi_start)
>
Very nice code reuse. Can you add a comment that whoever modifies this file
needs to take care to preserve this function-in function structure
correctly.

>         /* mov imm64, %rax */
>         .byte   0x48
>         .byte   0xb8
> @@ -120,6 +121,8 @@ LOCAL(jump_addr):
>  VARIABLE(grub_relocator64_rip)
>         .quad   0
>
> +VARIABLE(grub_relocator64_efi_end)
> +
>  #ifndef __x86_64__
>         .p2align        4
>  LOCAL(gdt):
> diff --git a/grub-core/loader/multiboot.c b/grub-core/loader/multiboot.c
> index 73aa0aa..18038fd 100644
> --- a/grub-core/loader/multiboot.c
> +++ b/grub-core/loader/multiboot.c
> @@ -118,6 +118,48 @@ grub_multiboot_set_video_mode (void)
>    return err;
>  }
>
> +#ifdef GRUB_MACHINE_EFI
> +#ifdef __x86_64__
> +#define grub_relocator_efi_boot                grub_relocator64_efi_boot
> +#define grub_relocator_efi_state       grub_relocator64_efi_state
> +#endif
> +#endif
> +
> +#ifdef grub_relocator_efi_boot
> +static void
> +efi_boot (struct grub_relocator *rel,
> +         grub_uint32_t target)
> +{
> +  struct grub_relocator_efi_state state_efi = MULTIBOOT_EFI_INITIAL_STATE;
> +
> +  state_efi.MULTIBOOT_EFI_ENTRY_REGISTER = grub_multiboot_payload_eip;
> +  state_efi.MULTIBOOT_EFI_MBI_REGISTER = target;
> +
> +  grub_relocator_efi_boot (rel, state_efi);
> +}
> +#else
> +#define grub_efi_is_finished   1
> +static void
> +efi_boot (struct grub_relocator *rel __attribute__ ((unused)),
> +         grub_uint32_t target __attribute__ ((unused)))
> +{
> +}
> +#endif
> +
> +#if defined (__i386__) || defined (__x86_64__)
> +static void
> +normal_boot (struct grub_relocator *rel, struct grub_relocator32_state
> state)
> +{
> +  grub_relocator32_boot (rel, state, 0);
> +}
> +#else
> +static void
> +normal_boot (struct grub_relocator *rel, struct grub_relocator32_state
> state)
> +{
> +  grub_relocator32_boot (rel, state);
> +}
> +#endif
> +
>  static grub_err_t
>  grub_multiboot_boot (void)
>  {
> @@ -131,11 +173,10 @@ grub_multiboot_boot (void)
>    if (err)
>      return err;
>
> -#if defined (__i386__) || defined (__x86_64__)
> -  grub_relocator32_boot (grub_multiboot_relocator, state, 0);
> -#else
> -  grub_relocator32_boot (grub_multiboot_relocator, state);
> -#endif
> +  if (grub_efi_is_finished)
> +    normal_boot (grub_multiboot_relocator, state);
> +  else
> +    efi_boot (grub_multiboot_relocator, state.MULTIBOOT_MBI_REGISTER);
>
>    /* Not reached.  */
>    return GRUB_ERR_NONE;
> diff --git a/grub-core/loader/multiboot_mbi2.c
> b/grub-core/loader/multiboot_mbi2.c
> index f147d67..a3dca90 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 *)
> tag)->entry_addr;
> +#endif
> +       break;
> +
>        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;
>
>    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..d96aa40 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
> --
> 1.7.10.4
>
>

-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #1.2: Type: text/html, Size: 13609 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 2/4] multiboot2: Add tags used to pass ImageHandle to loaded image
  2016-03-02 16:51 ` [GRUB2 PATCH v3 2/4] multiboot2: Add tags used to pass ImageHandle to loaded image Daniel Kiper
@ 2016-03-10 20:26   ` Vladimir 'phcoder' Serbinenko
  2016-03-11 13:27     ` Daniel Kiper
  0 siblings, 1 reply; 30+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2016-03-10 20:26 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com


[-- Attachment #1.1: Type: text/plain, Size: 5488 bytes --]

On Wednesday, March 2, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:

> Add tags used to pass ImageHandle to loaded image if requested.
> It is used by at least ExitBootServices() function.
>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com <javascript:;>>
> ---
> v3 - suggestions/fixes:
>    - mbi EFI related stuff size calculation
>      should depend on target architecture
>      (suggested by Konrad Rzeszutek Wilk),
>    - use plain type instead of pointer
>      dereference as sizeof() argument
>      (suggested by Konrad Rzeszutek Wilk),
>    - improve commit message
>      (suggested by Konrad Rzeszutek Wilk).
> ---
>  grub-core/loader/multiboot_mbi2.c |   50
> ++++++++++++++++++++++++++++++-------
>  include/multiboot2.h              |   16 ++++++++++++
>  2 files changed, 57 insertions(+), 9 deletions(-)
>
> diff --git a/grub-core/loader/multiboot_mbi2.c
> b/grub-core/loader/multiboot_mbi2.c
> index a3dca90..7591edc 100644
> --- a/grub-core/loader/multiboot_mbi2.c
> +++ b/grub-core/loader/multiboot_mbi2.c
> @@ -172,6 +172,8 @@ grub_multiboot_load (grub_file_t file, const char
> *filename)
>               case MULTIBOOT_TAG_TYPE_NETWORK:
>               case MULTIBOOT_TAG_TYPE_EFI_MMAP:
>               case MULTIBOOT_TAG_TYPE_EFI_BS:
> +             case MULTIBOOT_TAG_TYPE_EFI32_IH:
> +             case MULTIBOOT_TAG_TYPE_EFI64_IH:
>                 break;
>
>               default:
> @@ -407,16 +409,22 @@ grub_multiboot_get_mbi_size (void)
>                  + grub_get_multiboot_mmap_count ()
>                  * sizeof (struct multiboot_mmap_entry)),
> MULTIBOOT_TAG_ALIGN)
>      + ALIGN_UP (sizeof (struct multiboot_tag_framebuffer),
> MULTIBOOT_TAG_ALIGN)
> +#ifdef GRUB_MACHINE_EFI
> +#ifdef __i386__
>      + ALIGN_UP (sizeof (struct multiboot_tag_efi32), MULTIBOOT_TAG_ALIGN)
> +    + ALIGN_UP (sizeof (struct multiboot_tag_efi32_ih),
> MULTIBOOT_TAG_ALIGN)
> +#endif
> +#ifdef __x86_64__
>      + ALIGN_UP (sizeof (struct multiboot_tag_efi64), MULTIBOOT_TAG_ALIGN)
> +    + ALIGN_UP (sizeof (struct multiboot_tag_efi64_ih),
> MULTIBOOT_TAG_ALIGN)
> +#endif
> +    + ALIGN_UP (sizeof (struct multiboot_tag_efi_mmap)
> +               + efi_mmap_size, MULTIBOOT_TAG_ALIGN)
> +#endif
>
It doesn't need to be exact. It's just an upper bound. Feel free to
simplify knowing this, removing few #ifdef.

>      + ALIGN_UP (sizeof (struct multiboot_tag_old_acpi)
>                 + sizeof (struct grub_acpi_rsdp_v10), MULTIBOOT_TAG_ALIGN)
>      + acpiv2_size ()
>      + net_size ()
> -#ifdef GRUB_MACHINE_EFI
> -    + ALIGN_UP (sizeof (struct multiboot_tag_efi_mmap)
> -               + efi_mmap_size, MULTIBOOT_TAG_ALIGN)
> -#endif
>      + sizeof (struct multiboot_tag_vbe) + MULTIBOOT_TAG_ALIGN - 1
>      + sizeof (struct multiboot_tag_apm) + MULTIBOOT_TAG_ALIGN - 1;
>  }
> @@ -907,11 +915,35 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
>
>    if (keep_bs)
>      {
> -      struct multiboot_tag *tag = (struct multiboot_tag *) ptrorig;
> -      tag->type = MULTIBOOT_TAG_TYPE_EFI_BS;
> -      tag->size = sizeof (struct multiboot_tag);
> -      ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> -       / sizeof (grub_properly_aligned_t);
> +      {
> +       struct multiboot_tag *tag = (struct multiboot_tag *) ptrorig;
> +       tag->type = MULTIBOOT_TAG_TYPE_EFI_BS;
> +       tag->size = sizeof (struct multiboot_tag);
> +       ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> +         / sizeof (grub_properly_aligned_t);
> +      }
> +
> +#ifdef __i386__
> +      {
> +       struct multiboot_tag_efi32_ih *tag = (struct
> multiboot_tag_efi32_ih *) ptrorig;
> +       tag->type = MULTIBOOT_TAG_TYPE_EFI32_IH;
> +       tag->size = sizeof (struct multiboot_tag_efi32_ih);
> +       tag->pointer = (grub_addr_t) grub_efi_image_handle;
> +       ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> +         / sizeof (grub_properly_aligned_t);
> +      }
> +#endif
> +
> +#ifdef __x86_64__
> +      {
> +       struct multiboot_tag_efi64_ih *tag = (struct
> multiboot_tag_efi64_ih *) ptrorig;
> +       tag->type = MULTIBOOT_TAG_TYPE_EFI64_IH;
> +       tag->size = sizeof (struct multiboot_tag_efi64_ih);
> +       tag->pointer = (grub_addr_t) grub_efi_image_handle;
> +       ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> +         / sizeof (grub_properly_aligned_t);
> +      }
> +#endif
>      }
>  #endif
>
> diff --git a/include/multiboot2.h b/include/multiboot2.h
> index d96aa40..36a174f 100644
> --- a/include/multiboot2.h
> +++ b/include/multiboot2.h
> @@ -60,6 +60,8 @@
>  #define MULTIBOOT_TAG_TYPE_NETWORK           16
>  #define MULTIBOOT_TAG_TYPE_EFI_MMAP          17
>  #define MULTIBOOT_TAG_TYPE_EFI_BS            18
> +#define MULTIBOOT_TAG_TYPE_EFI32_IH          19
> +#define MULTIBOOT_TAG_TYPE_EFI64_IH          20
>
Is there a followup to define those in texi doc as well?

>
>  #define MULTIBOOT_HEADER_TAG_END  0
>  #define MULTIBOOT_HEADER_TAG_INFORMATION_REQUEST  1
> @@ -371,6 +373,20 @@ struct multiboot_tag_efi_mmap
>    multiboot_uint8_t efi_mmap[0];
>  };
>
> +struct multiboot_tag_efi32_ih
> +{
> +  multiboot_uint32_t type;
> +  multiboot_uint32_t size;
> +  multiboot_uint32_t pointer;
> +};
> +
> +struct multiboot_tag_efi64_ih
> +{
> +  multiboot_uint32_t type;
> +  multiboot_uint32_t size;
> +  multiboot_uint64_t pointer;
> +};
> +
>  #endif /* ! ASM_FILE */
>
>  #endif /* ! MULTIBOOT_HEADER */
> --
> 1.7.10.4
>
>

-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #1.2: Type: text/html, Size: 6800 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 3/4] multiboot2: Do not pass memory maps to image if EFI boot services are enabled
  2016-03-02 16:51 ` [GRUB2 PATCH v3 3/4] multiboot2: Do not pass memory maps to image if EFI boot services are enabled Daniel Kiper
@ 2016-03-10 20:28   ` Vladimir 'phcoder' Serbinenko
  2016-03-11 13:30     ` Daniel Kiper
  0 siblings, 1 reply; 30+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2016-03-10 20:28 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com


[-- Attachment #1.1: Type: text/plain, Size: 5225 bytes --]

On Wednesday, March 2, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:

> Do not pass memory maps to image if it asked for EFI boot services.
> Main reason for not providing maps is because they will likely be
> invalid. We do a few allocations after filling them, e.g. for relocator
> needs. Usually we do not care as we would already finish boot services.
> If we keep boot services then it is easier to not provide maps. However,
> if image needs memory maps and they are not provided by bootloader then
> it should get them itself just before ExitBootServices() call.
>
Patch gets too cluttered. Can you provide 2 variants: normal (for commit)
and with ignoring whitespaces (for review)

>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com <javascript:;>>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com <javascript:;>>
> ---
> v3 - suggestions/fixes:
>    - improve commit message
>      (suggested by Konrad Rzeszutek Wilk and Vladimir 'phcoder'
> Serbinenko).
> ---
>  grub-core/loader/multiboot_mbi2.c |   71
> ++++++++++++++++++-------------------
>  1 file changed, 35 insertions(+), 36 deletions(-)
>
> diff --git a/grub-core/loader/multiboot_mbi2.c
> b/grub-core/loader/multiboot_mbi2.c
> index 7591edc..ce68f48 100644
> --- a/grub-core/loader/multiboot_mbi2.c
> +++ b/grub-core/loader/multiboot_mbi2.c
> @@ -390,7 +390,7 @@ static grub_size_t
>  grub_multiboot_get_mbi_size (void)
>  {
>  #ifdef GRUB_MACHINE_EFI
> -  if (!efi_mmap_size)
> +  if (!keep_bs && !efi_mmap_size)
>      find_efi_mmap_size ();
>  #endif
>    return 2 * sizeof (grub_uint32_t) + sizeof (struct multiboot_tag)
> @@ -759,12 +759,13 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
>        }
>    }
>
> -  {
> -    struct multiboot_tag_mmap *tag = (struct multiboot_tag_mmap *)
> ptrorig;
> -    grub_fill_multiboot_mmap (tag);
> -    ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> -      / sizeof (grub_properly_aligned_t);
> -  }
> +  if (!keep_bs)
> +    {
> +      struct multiboot_tag_mmap *tag = (struct multiboot_tag_mmap *)
> ptrorig;
> +      grub_fill_multiboot_mmap (tag);
> +      ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> +       / sizeof (grub_properly_aligned_t);
> +    }
>
>    {
>      struct multiboot_tag_elf_sections *tag
> @@ -780,18 +781,19 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
>        / sizeof (grub_properly_aligned_t);
>    }
>
> -  {
> -    struct multiboot_tag_basic_meminfo *tag
> -      = (struct multiboot_tag_basic_meminfo *) ptrorig;
> -    tag->type = MULTIBOOT_TAG_TYPE_BASIC_MEMINFO;
> -    tag->size = sizeof (struct multiboot_tag_basic_meminfo);
> +  if (!keep_bs)
> +    {
> +      struct multiboot_tag_basic_meminfo *tag
> +       = (struct multiboot_tag_basic_meminfo *) ptrorig;
> +      tag->type = MULTIBOOT_TAG_TYPE_BASIC_MEMINFO;
> +      tag->size = sizeof (struct multiboot_tag_basic_meminfo);
>
> -    /* Convert from bytes to kilobytes.  */
> -    tag->mem_lower = grub_mmap_get_lower () / 1024;
> -    tag->mem_upper = grub_mmap_get_upper () / 1024;
> -    ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> -       / sizeof (grub_properly_aligned_t);
> -  }
> +      /* Convert from bytes to kilobytes.  */
> +      tag->mem_lower = grub_mmap_get_lower () / 1024;
> +      tag->mem_upper = grub_mmap_get_upper () / 1024;
> +      ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> +       / sizeof (grub_properly_aligned_t);
> +    }
>
>    {
>      struct grub_net_network_level_interface *net;
> @@ -890,27 +892,24 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
>      grub_efi_uintn_t efi_desc_size;
>      grub_efi_uint32_t efi_desc_version;
>
> -    tag->type = MULTIBOOT_TAG_TYPE_EFI_MMAP;
> -    tag->size = sizeof (*tag) + efi_mmap_size;
> -
>      if (!keep_bs)
> -      err = grub_efi_finish_boot_services (&efi_mmap_size, tag->efi_mmap,
> NULL,
> -                                          &efi_desc_size,
> &efi_desc_version);
> -    else
>        {
> -       if (grub_efi_get_memory_map (&efi_mmap_size, (void *)
> tag->efi_mmap,
> -                                    NULL,
> -                                    &efi_desc_size, &efi_desc_version) <=
> 0)
> -         err = grub_error (GRUB_ERR_IO, "couldn't retrieve memory map");
> +       tag->type = MULTIBOOT_TAG_TYPE_EFI_MMAP;
> +       tag->size = sizeof (*tag) + efi_mmap_size;
> +
> +       err = grub_efi_finish_boot_services (&efi_mmap_size,
> tag->efi_mmap, NULL,
> +                                            &efi_desc_size,
> &efi_desc_version);
> +
> +       if (err)
> +         return err;
> +
> +       tag->descr_size = efi_desc_size;
> +       tag->descr_vers = efi_desc_version;
> +       tag->size = sizeof (*tag) + efi_mmap_size;
> +
> +       ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> +         / sizeof (grub_properly_aligned_t);
>        }
> -    if (err)
> -      return err;
> -    tag->descr_size = efi_desc_size;
> -    tag->descr_vers = efi_desc_version;
> -    tag->size = sizeof (*tag) + efi_mmap_size;
> -
> -    ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> -      / sizeof (grub_properly_aligned_t);
>    }
>
>    if (keep_bs)
> --
> 1.7.10.4
>
>

-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #1.2: Type: text/html, Size: 6556 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 4/4] multiboot2: Add support for relocatable images
  2016-03-02 16:51 ` [GRUB2 PATCH v3 4/4] multiboot2: Add support for relocatable images Daniel Kiper
  2016-03-04  6:51   ` Juergen Gross
@ 2016-03-10 20:41   ` Vladimir 'phcoder' Serbinenko
  2016-03-11 16:06     ` Daniel Kiper
  2016-03-10 20:44   ` Vladimir 'phcoder' Serbinenko
  2 siblings, 1 reply; 30+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2016-03-10 20:41 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com


[-- Attachment #1.1: Type: text/plain, Size: 19432 bytes --]

On Wednesday, March 2, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:

> Currently multiboot2 protocol loads image exactly at address specified in
> ELF or multiboot2 header. This solution works quite well on legacy BIOS
> platforms. It is possible because memory regions are placed at predictable
> addresses (though I was not able to find any spec which says that it is
> strong requirement, so, it looks that it is just a goodwill of hardware
> designers). However, EFI platforms are more volatile. Even if required
> memory regions live at specific addresses then they are sometimes simply
> not free (e.g. used by boot/runtime services on Dell PowerEdge R820 and
> OVMF). This means that you are not able to simply set up final image
> destination on build time. You have to provide method to relocate image
> contents to real load address which is usually different than load address
> specified in ELF and multiboot2 headers.
>
> This patch provides all needed machinery to do self relocation in image
> code.
> First of all GRUB2 reads min_addr (min. load addr), max_addr (max. load
> addr),
> align (required image alignment), preference (it says which memory regions
> are
> preferred by image, e.g. none, low, high) from
> multiboot_header_tag_relocatable
> header tag contained in binary. Later loader tries to fulfill request (not
> only
> that one) and if it succeeds then it informs image about real load address
> via
> multiboot_tag_base_addr tag. At this stage GRUB2 role is finished. Starting
> from now executable must cope with relocations itself using whole static
> and dynamic knowledge provided by boot loader.
>
> This patch does not provide functionality which could do relocations using
> ELF relocation data. However, I was asked by Konrad Rzeszutek Wilk and
> Vladimir
> 'phcoder' Serbinenko to investigate that thing. It looks that relevant
> machinery
> could be added to existing code (including this patch) without huge effort.
> Additionally, ELF relocation could live in parallel with self relocation
> provided
> by this patch. However, during research I realized that first of all we
> should
> establish the details how ELF relocatable image should look like and how
> it should
> be build. At least to build proper test/example files.
>
> As I saw multiboot2 protocol is able to consume ET_EXEC and ET_DYN ELF
> files.
> Potentially we can use ET_DYN file type. It can be build with gcc/ld -pie
> option.
> However, it contains a lot of unneeded stuff (e.g. INTERP, DYNAMIC,
> GNU_EH_FRAME
> program headers) and it could be quite difficult to drop them (Hmmm... Is
> it
> possible to build it properly with custom ld script?).

How big are they? Are they a real problem?

> So, I have checked ET_EXEC
> file type. Sadly in this case linker by default resolves all local symbol
> relocations
> and removes relocation related sections. Fortunately it is possible to
> leave them
> as is with simple -q/--emit-relocs ld option. However, output file is
> quite fragile
> and any operation on it should be done with great care (e.g. strip should
> be called
> with --strip-unneeded option). So, this solution is not perfect too. It
> means that
> maybe we should look for better solution. However, I think that we should
> not use
> any custom tools and focus on functionalities provided by compiler and
> binutils.
> In this context ld scripts looks quite promising but maybe you have better
> solutions.
> So, what do you think about that?
>
 Another possibility is to use intermediary .o files like we do for modules
and like Linux does for modules AFAIR.

>
> This patch was tested with Xen image which uses that functionality.
> However, this Xen
> feature is still under development and new patchset will be released in
> about 3-4 weeks.
>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com <javascript:;>>
> ---
> v3 - suggestions/fixes:
>    - reduce number of casts
>      (suggested by Konrad Rzeszutek Wilk),
>    - remove unneeded space at the end of line
>      (suggested by Konrad Rzeszutek Wilk),
>    - improve commit message
>      (suggested by Konrad Rzeszutek Wilk).
> ---
>  grub-core/loader/i386/multiboot_mbi.c |    6 ++-
>  grub-core/loader/multiboot.c          |   12 ++++--
>  grub-core/loader/multiboot_elfxx.c    |   28 ++++++++++----
>  grub-core/loader/multiboot_mbi2.c     |   65
> ++++++++++++++++++++++++++++++---
>  include/grub/multiboot.h              |    4 +-
>  include/multiboot2.h                  |   24 ++++++++++++
>  6 files changed, 120 insertions(+), 19 deletions(-)
>
> diff --git a/grub-core/loader/i386/multiboot_mbi.c
> b/grub-core/loader/i386/multiboot_mbi.c
> index f60b702..4fc83ed 100644
> --- a/grub-core/loader/i386/multiboot_mbi.c
> +++ b/grub-core/loader/i386/multiboot_mbi.c
> @@ -72,7 +72,8 @@ load_kernel (grub_file_t file, const char *filename,
>    grub_err_t err;
>    if (grub_multiboot_quirks & GRUB_MULTIBOOT_QUIRK_BAD_KLUDGE)
>      {
> -      err = grub_multiboot_load_elf (file, filename, buffer);
> +      err = grub_multiboot_load_elf (file, filename, buffer, 0, 0, 0, 0,
> +                                    GRUB_RELOCATOR_PREFERENCE_NONE, NULL,
> 0);
>        if (err == GRUB_ERR_NONE) {
>         return GRUB_ERR_NONE;
>        }
> @@ -121,7 +122,8 @@ load_kernel (grub_file_t file, const char *filename,
>        return GRUB_ERR_NONE;
>      }
>
> -  return grub_multiboot_load_elf (file, filename, buffer);
> +  return grub_multiboot_load_elf (file, filename, buffer, 0, 0, 0, 0,
> +                                 GRUB_RELOCATOR_PREFERENCE_NONE, NULL, 0);
>  }
>
>  static struct multiboot_header *
> diff --git a/grub-core/loader/multiboot.c b/grub-core/loader/multiboot.c
> index 18038fd..c0f51b6 100644
> --- a/grub-core/loader/multiboot.c
> +++ b/grub-core/loader/multiboot.c
> @@ -208,12 +208,18 @@ static grub_uint64_t highest_load;
>  /* Load ELF32 or ELF64.  */
>  grub_err_t
>  grub_multiboot_load_elf (grub_file_t file, const char *filename,
> -                        void *buffer)
> +                        void *buffer, int relocatable, grub_uint32_t
> min_addr,
> +                        grub_uint32_t max_addr, grub_size_t align,
> grub_uint32_t preference,
> +                        grub_uint32_t *base_addr, int
> avoid_efi_boot_services)
>  {
>    if (grub_multiboot_is_elf32 (buffer))
> -    return grub_multiboot_load_elf32 (file, filename, buffer);
> +    return grub_multiboot_load_elf32 (file, filename, buffer, relocatable,
> +                                     min_addr, max_addr, align,
> preference,
> +                                     base_addr, avoid_efi_boot_services);
>    else if (grub_multiboot_is_elf64 (buffer))
> -    return grub_multiboot_load_elf64 (file, filename, buffer);
> +    return grub_multiboot_load_elf64 (file, filename, buffer, relocatable,
> +                                     min_addr, max_addr, align,
> preference,
> +                                     base_addr, avoid_efi_boot_services);
>
>    return grub_error (GRUB_ERR_UNKNOWN_OS, N_("invalid arch-dependent ELF
> magic"));
>  }
> diff --git a/grub-core/loader/multiboot_elfxx.c
> b/grub-core/loader/multiboot_elfxx.c
> index e3a39b6..0c01569 100644
> --- a/grub-core/loader/multiboot_elfxx.c
> +++ b/grub-core/loader/multiboot_elfxx.c
> @@ -51,7 +51,10 @@ CONCAT(grub_multiboot_is_elf, XX) (void *buffer)
>  }
>
>  static grub_err_t
> -CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char
> *filename, void *buffer)
> +CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char
> *filename,
> +                                    void *buffer, int relocatable,
> grub_uint32_t min_addr,
> +                                    grub_uint32_t max_addr, grub_size_t
> align, grub_uint32_t preference,
> +                                    grub_uint32_t *base_addr, int
> avoid_efi_boot_services)
>  {
>    Elf_Ehdr *ehdr = (Elf_Ehdr *) buffer;
>    char *phdr_base;
> @@ -89,19 +92,30 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file,
> const char *filename, voi
>           if (phdr(i)->p_paddr + phdr(i)->p_memsz > highest_load)
>             highest_load = phdr(i)->p_paddr + phdr(i)->p_memsz;
>
> -         grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx,
> memsz=0x%lx, vaddr=0x%lx\n",
> -                       i, (long) phdr(i)->p_paddr, (long)
> phdr(i)->p_memsz, (long) phdr(i)->p_vaddr);
> +         grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx,
> memsz=0x%lx, vaddr=0x%lx,"
> +                       "align=0x%lx, relocatable=%d,
> avoid_efi_boot_services=%d\n", i,
> +                       (long) phdr(i)->p_paddr, (long) phdr(i)->p_memsz,
> (long) phdr(i)->p_vaddr,
> +                       (long) align, relocatable,
> avoid_efi_boot_services);
>
>           {
>             grub_relocator_chunk_t ch;
> -           err = grub_relocator_alloc_chunk_addr
> (grub_multiboot_relocator,
> -                                                  &ch, phdr(i)->p_paddr,
> -                                                  phdr(i)->p_memsz);
> +
> +           if (relocatable)
> +             err = grub_relocator_alloc_chunk_align
> (grub_multiboot_relocator, &ch,
> +                                                     min_addr, max_addr -
> phdr(i)->p_memsz,
> +                                                     phdr(i)->p_memsz,
> align ? align : 1,
> +                                                     preference,
> avoid_efi_boot_services);
> +           else
> +             err = grub_relocator_alloc_chunk_addr
> (grub_multiboot_relocator,
> +                                                    &ch, phdr(i)->p_paddr,
> +                                                    phdr(i)->p_memsz);
>             if (err)
>               {
>                 grub_dprintf ("multiboot_loader", "Error loading phdr
> %d\n", i);
>                 return err;
>               }
> +           if (base_addr)
> +             *base_addr = get_physical_target_address (ch);
>             source = get_virtual_current_address (ch);
>           }
>
> @@ -208,7 +222,7 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file,
> const char *filename, voi
>                                                     + 1, sh->sh_size,
>                                                     sh->sh_addralign,
>
> GRUB_RELOCATOR_PREFERENCE_NONE,
> -                                                   0);
> +
>  avoid_efi_boot_services);
>             if (err)
>               {
>                 grub_dprintf ("multiboot_loader", "Error loading shdr
> %d\n", i);
> diff --git a/grub-core/loader/multiboot_mbi2.c
> b/grub-core/loader/multiboot_mbi2.c
> index ce68f48..03725a1 100644
> --- a/grub-core/loader/multiboot_mbi2.c
> +++ b/grub-core/loader/multiboot_mbi2.c
> @@ -68,6 +68,7 @@ static grub_size_t elf_sec_num, elf_sec_entsize;
>  static unsigned elf_sec_shstrndx;
>  static void *elf_sections;
>  static int keep_bs = 0;
> +static grub_uint32_t base_addr = 0;
>
>  void
>  grub_multiboot_add_elfsyms (grub_size_t num, grub_size_t entsize,
> @@ -107,11 +108,14 @@ 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, efi_entry_specified = 0;
> +  struct multiboot_header_tag_relocatable *rel_tag;
> +  int entry_specified = 0, efi_entry_specified = 0, relocatable = 0;
>    grub_addr_t entry = 0, efi_entry = 0;
> -  grub_uint32_t console_required = 0;
> +  grub_uint32_t console_required = 0, min_addr = 0;
> +  grub_uint32_t max_addr = 0, preference = GRUB_RELOCATOR_PREFERENCE_NONE;
>    struct multiboot_header_tag_framebuffer *fbtag = NULL;
>    int accepted_consoles = GRUB_MULTIBOOT_CONSOLE_EGA_TEXT;
> +  grub_size_t align = 0;
>
>    buffer = grub_malloc (MULTIBOOT_SEARCH);
>    if (!buffer)
> @@ -174,6 +178,7 @@ grub_multiboot_load (grub_file_t file, const char
> *filename)
>               case MULTIBOOT_TAG_TYPE_EFI_BS:
>               case MULTIBOOT_TAG_TYPE_EFI32_IH:
>               case MULTIBOOT_TAG_TYPE_EFI64_IH:
> +             case MULTIBOOT_TAG_TYPE_BASE_ADDR:
>                 break;
>
>               default:
> @@ -215,6 +220,27 @@ grub_multiboot_load (grub_file_t file, const char
> *filename)
>         accepted_consoles |= GRUB_MULTIBOOT_CONSOLE_FRAMEBUFFER;
>         break;
>
> +      case MULTIBOOT_HEADER_TAG_RELOCATABLE:
> +       relocatable = 1;
> +       rel_tag = (struct multiboot_header_tag_relocatable *) tag;
> +       min_addr = rel_tag->min_addr;
> +       max_addr = rel_tag->max_addr;
> +       align = rel_tag->align;
> +       switch (rel_tag->preference)
> +         {
> +         case MULTIBOOT_LOAD_PREFERENCE_LOW:
> +           preference = GRUB_RELOCATOR_PREFERENCE_LOW;
> +           break;
> +
> +         case MULTIBOOT_LOAD_PREFERENCE_HIGH:
> +           preference = GRUB_RELOCATOR_PREFERENCE_HIGH;
> +           break;
> +
> +         default:
> +           preference = GRUB_RELOCATOR_PREFERENCE_NONE;
> +         }
> +       break;
> +
>         /* GRUB always page-aligns modules.  */
>        case MULTIBOOT_HEADER_TAG_MODULE_ALIGN:
>         break;
> @@ -260,15 +286,22 @@ grub_multiboot_load (grub_file_t file, const char
> *filename)
>        else
>         code_size = load_size;
>
> -      err = grub_relocator_alloc_chunk_addr (grub_multiboot_relocator,
> -                                            &ch, load_addr,
> -                                            code_size);
> +      if (relocatable)
> +       err = grub_relocator_alloc_chunk_align (grub_multiboot_relocator,
> &ch,
> +                                               min_addr, max_addr -
> code_size,
> +                                               code_size, align ? align :
> 1,
> +                                               preference, keep_bs);
> +      else
> +       err = grub_relocator_alloc_chunk_addr (grub_multiboot_relocator,
> +                                              &ch, load_addr,
> +                                              code_size);
>        if (err)
>         {
>           grub_dprintf ("multiboot_loader", "Error loading aout kludge\n");
>           grub_free (buffer);
>           return err;
>         }
> +      base_addr = get_physical_target_address (ch);
>        source = get_virtual_current_address (ch);
>
>        if ((grub_file_seek (file, offset)) == (grub_off_t) -1)
> @@ -290,7 +323,9 @@ grub_multiboot_load (grub_file_t file, const char
> *filename)
>      }
>    else
>      {
> -      err = grub_multiboot_load_elf (file, filename, buffer);
> +      err = grub_multiboot_load_elf (file, filename, buffer,
> +                                    relocatable, min_addr, max_addr,
> +                                    align, preference, &base_addr,
> keep_bs);
>        if (err)
>         {
>           grub_free (buffer);
> @@ -303,6 +338,14 @@ grub_multiboot_load (grub_file_t file, const char
> *filename)
>    else if (entry_specified)
>      grub_multiboot_payload_eip = entry;
>
> +  if (relocatable)
> +    {
> +      if (base_addr > min_addr)
> +       grub_multiboot_payload_eip += base_addr - min_addr;
> +      else
> +       grub_multiboot_payload_eip -= min_addr - base_addr;
> +    }
> +
>
Why is it relative to min_addr? Sounds like it should be just an offset
from base addr. What do ET_DYN files use?

>    if (fbtag)
>      err = grub_multiboot_set_console (GRUB_MULTIBOOT_CONSOLE_FRAMEBUFFER,
>                                       accepted_consoles,
> @@ -409,6 +452,7 @@ grub_multiboot_get_mbi_size (void)
>                  + grub_get_multiboot_mmap_count ()
>                  * sizeof (struct multiboot_mmap_entry)),
> MULTIBOOT_TAG_ALIGN)
>      + ALIGN_UP (sizeof (struct multiboot_tag_framebuffer),
> MULTIBOOT_TAG_ALIGN)
> +    + ALIGN_UP (sizeof (struct multiboot_tag_base_addr),
> MULTIBOOT_TAG_ALIGN)
>  #ifdef GRUB_MACHINE_EFI
>  #ifdef __i386__
>      + ALIGN_UP (sizeof (struct multiboot_tag_efi32), MULTIBOOT_TAG_ALIGN)
> @@ -698,6 +742,15 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
>    ptrorig += (2 * sizeof (grub_uint32_t)) / sizeof
> (grub_properly_aligned_t);
>
>    {
> +    struct multiboot_tag_base_addr *tag = (struct multiboot_tag_base_addr
> *) ptrorig;
> +    tag->type = MULTIBOOT_TAG_TYPE_BASE_ADDR;
> +    tag->size = sizeof (struct multiboot_tag_base_addr);
> +    tag->base_addr = base_addr;
> +    ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> +       / sizeof (grub_properly_aligned_t);
> +  }
> +
> +  {
>      struct multiboot_tag_string *tag = (struct multiboot_tag_string *)
> ptrorig;
>      tag->type = MULTIBOOT_TAG_TYPE_CMDLINE;
>      tag->size = sizeof (struct multiboot_tag_string) + cmdline_size;
> diff --git a/include/grub/multiboot.h b/include/grub/multiboot.h
> index e13c084..ec322b0 100644
> --- a/include/grub/multiboot.h
> +++ b/include/grub/multiboot.h
> @@ -94,7 +94,9 @@ grub_multiboot_load (grub_file_t file, const char
> *filename);
>  /* Load ELF32 or ELF64.  */
>  grub_err_t
>  grub_multiboot_load_elf (grub_file_t file, const char *filename,
> -                        void *buffer);
> +                        void *buffer, int relocatable, grub_uint32_t
> min_addr,
> +                        grub_uint32_t max_addr, grub_size_t align,
> grub_uint32_t preference,
> +                        grub_uint32_t *base_addr, int
> avoid_efi_boot_services);
>  extern grub_size_t grub_multiboot_pure_size;
>  extern grub_size_t grub_multiboot_alloc_mbi;
>  extern grub_uint32_t grub_multiboot_payload_eip;
> diff --git a/include/multiboot2.h b/include/multiboot2.h
> index 36a174f..c09bdbc 100644
> --- a/include/multiboot2.h
> +++ b/include/multiboot2.h
> @@ -62,6 +62,7 @@
>  #define MULTIBOOT_TAG_TYPE_EFI_BS            18
>  #define MULTIBOOT_TAG_TYPE_EFI32_IH          19
>  #define MULTIBOOT_TAG_TYPE_EFI64_IH          20
> +#define MULTIBOOT_TAG_TYPE_BASE_ADDR         21
>
>  #define MULTIBOOT_HEADER_TAG_END  0
>  #define MULTIBOOT_HEADER_TAG_INFORMATION_REQUEST  1
> @@ -72,11 +73,16 @@
>  #define MULTIBOOT_HEADER_TAG_MODULE_ALIGN  6
>  #define MULTIBOOT_HEADER_TAG_EFI_BS  7
>  #define MULTIBOOT_HEADER_TAG_ENTRY_ADDRESS_EFI64  9
> +#define MULTIBOOT_HEADER_TAG_RELOCATABLE  10
>
>  #define MULTIBOOT_ARCHITECTURE_I386  0
>  #define MULTIBOOT_ARCHITECTURE_MIPS32  4
>  #define MULTIBOOT_HEADER_TAG_OPTIONAL 1
>
> +#define MULTIBOOT_LOAD_PREFERENCE_NONE 0
> +#define MULTIBOOT_LOAD_PREFERENCE_LOW 1
> +#define MULTIBOOT_LOAD_PREFERENCE_HIGH 2
> +
>  #define MULTIBOOT_CONSOLE_FLAGS_CONSOLE_REQUIRED 1
>  #define MULTIBOOT_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED 2
>
> @@ -161,6 +167,17 @@ struct multiboot_header_tag_module_align
>    multiboot_uint32_t size;
>  };
>
> +struct multiboot_header_tag_relocatable
> +{
> +  multiboot_uint16_t type;
> +  multiboot_uint16_t flags;
> +  multiboot_uint32_t size;
> +  multiboot_uint32_t min_addr;
> +  multiboot_uint32_t max_addr;
> +  multiboot_uint32_t align;
> +  multiboot_uint32_t preference;
> +};
> +
>  struct multiboot_color
>  {
>    multiboot_uint8_t red;
> @@ -387,6 +404,13 @@ struct multiboot_tag_efi64_ih
>    multiboot_uint64_t pointer;
>  };
>
> +struct multiboot_tag_base_addr
> +{
> +  multiboot_uint32_t type;
> +  multiboot_uint32_t size;
> +  multiboot_uint32_t base_addr;
> +};
> +
>  #endif /* ! ASM_FILE */
>
>  #endif /* ! MULTIBOOT_HEADER */
> --
> 1.7.10.4
>
>

-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #1.2: Type: text/html, Size: 23046 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 4/4] multiboot2: Add support for relocatable images
  2016-03-04  6:51   ` Juergen Gross
@ 2016-03-10 20:42     ` Vladimir 'phcoder' Serbinenko
  0 siblings, 0 replies; 30+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2016-03-10 20:42 UTC (permalink / raw)
  To: Juergen Gross
  Cc: grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	Daniel Kiper, cardoe, pgnet.dev, roy.franz, ning.sun,
	david.vrabel, jbeulich, stefano.stabellini, xen-devel,
	qiaowei.ren, richard.l.maliszewski, gang.wei, fu.wei


[-- Attachment #1.1: Type: text/plain, Size: 20211 bytes --]

On Friday, March 4, 2016, Juergen Gross <jgross@suse.com> wrote:

> On 02/03/16 17:51, Daniel Kiper wrote:
> > Currently multiboot2 protocol loads image exactly at address specified in
> > ELF or multiboot2 header. This solution works quite well on legacy BIOS
> > platforms. It is possible because memory regions are placed at
> predictable
> > addresses (though I was not able to find any spec which says that it is
> > strong requirement, so, it looks that it is just a goodwill of hardware
> > designers). However, EFI platforms are more volatile. Even if required
> > memory regions live at specific addresses then they are sometimes simply
> > not free (e.g. used by boot/runtime services on Dell PowerEdge R820 and
> > OVMF). This means that you are not able to simply set up final image
> > destination on build time. You have to provide method to relocate image
> > contents to real load address which is usually different than load
> address
> > specified in ELF and multiboot2 headers.
> >
> > This patch provides all needed machinery to do self relocation in image
> code.
> > First of all GRUB2 reads min_addr (min. load addr), max_addr (max. load
> addr),
> > align (required image alignment), preference (it says which memory
> regions are
> > preferred by image, e.g. none, low, high) from
> multiboot_header_tag_relocatable
> > header tag contained in binary. Later loader tries to fulfill request
> (not only
> > that one) and if it succeeds then it informs image about real load
> address via
> > multiboot_tag_base_addr tag. At this stage GRUB2 role is finished.
> Starting
> > from now executable must cope with relocations itself using whole static
> > and dynamic knowledge provided by boot loader.
> >
> > This patch does not provide functionality which could do relocations
> using
> > ELF relocation data. However, I was asked by Konrad Rzeszutek Wilk and
> Vladimir
> > 'phcoder' Serbinenko to investigate that thing. It looks that relevant
> machinery
> > could be added to existing code (including this patch) without huge
> effort.
> > Additionally, ELF relocation could live in parallel with self relocation
> provided
> > by this patch. However, during research I realized that first of all we
> should
> > establish the details how ELF relocatable image should look like and how
> it should
> > be build. At least to build proper test/example files.
> >
> > As I saw multiboot2 protocol is able to consume ET_EXEC and ET_DYN ELF
> files.
> > Potentially we can use ET_DYN file type. It can be build with gcc/ld
> -pie option.
> > However, it contains a lot of unneeded stuff (e.g. INTERP, DYNAMIC,
> GNU_EH_FRAME
> > program headers) and it could be quite difficult to drop them (Hmmm...
> Is it
> > possible to build it properly with custom ld script?). So, I have
> checked ET_EXEC
> > file type. Sadly in this case linker by default resolves all local
> symbol relocations
> > and removes relocation related sections. Fortunately it is possible to
> leave them
> > as is with simple -q/--emit-relocs ld option. However, output file is
> quite fragile
> > and any operation on it should be done with great care (e.g. strip
> should be called
> > with --strip-unneeded option). So, this solution is not perfect too. It
> means that
> > maybe we should look for better solution. However, I think that we
> should not use
> > any custom tools and focus on functionalities provided by compiler and
> binutils.
> > In this context ld scripts looks quite promising but maybe you have
> better solutions.
> > So, what do you think about that?
> >
> > This patch was tested with Xen image which uses that functionality.
> However, this Xen
> > feature is still under development and new patchset will be released in
> about 3-4 weeks.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com <javascript:;>>
> > ---
> > v3 - suggestions/fixes:
> >    - reduce number of casts
> >      (suggested by Konrad Rzeszutek Wilk),
> >    - remove unneeded space at the end of line
> >      (suggested by Konrad Rzeszutek Wilk),
> >    - improve commit message
> >      (suggested by Konrad Rzeszutek Wilk).
> > ---
> >  grub-core/loader/i386/multiboot_mbi.c |    6 ++-
> >  grub-core/loader/multiboot.c          |   12 ++++--
> >  grub-core/loader/multiboot_elfxx.c    |   28 ++++++++++----
> >  grub-core/loader/multiboot_mbi2.c     |   65
> ++++++++++++++++++++++++++++++---
> >  include/grub/multiboot.h              |    4 +-
> >  include/multiboot2.h                  |   24 ++++++++++++
> >  6 files changed, 120 insertions(+), 19 deletions(-)
> >
> > diff --git a/grub-core/loader/i386/multiboot_mbi.c
> b/grub-core/loader/i386/multiboot_mbi.c
> > index f60b702..4fc83ed 100644
> > --- a/grub-core/loader/i386/multiboot_mbi.c
> > +++ b/grub-core/loader/i386/multiboot_mbi.c
> > @@ -72,7 +72,8 @@ load_kernel (grub_file_t file, const char *filename,
> >    grub_err_t err;
> >    if (grub_multiboot_quirks & GRUB_MULTIBOOT_QUIRK_BAD_KLUDGE)
> >      {
> > -      err = grub_multiboot_load_elf (file, filename, buffer);
> > +      err = grub_multiboot_load_elf (file, filename, buffer, 0, 0, 0, 0,
> > +                                  GRUB_RELOCATOR_PREFERENCE_NONE, NULL,
> 0);
>
> Uuh, really? You are adding 7 parameters for the relocatable case.
> Wouldn't it make more sense to have some kind of structure containing
> the data you need for relocatable images and pass the pointer to that
> structure or NULL in the case of non-relocatable images (or have a
> default structure for the non-relocatable case)?
>
I second this. Feel free to encapsulate more image-related stuff into same
structure

>
>
> Juergen
>
> >        if (err == GRUB_ERR_NONE) {
> >       return GRUB_ERR_NONE;
> >        }
> > @@ -121,7 +122,8 @@ load_kernel (grub_file_t file, const char *filename,
> >        return GRUB_ERR_NONE;
> >      }
> >
> > -  return grub_multiboot_load_elf (file, filename, buffer);
> > +  return grub_multiboot_load_elf (file, filename, buffer, 0, 0, 0, 0,
> > +                               GRUB_RELOCATOR_PREFERENCE_NONE, NULL, 0);
> >  }
> >
> >  static struct multiboot_header *
> > diff --git a/grub-core/loader/multiboot.c b/grub-core/loader/multiboot.c
> > index 18038fd..c0f51b6 100644
> > --- a/grub-core/loader/multiboot.c
> > +++ b/grub-core/loader/multiboot.c
> > @@ -208,12 +208,18 @@ static grub_uint64_t highest_load;
> >  /* Load ELF32 or ELF64.  */
> >  grub_err_t
> >  grub_multiboot_load_elf (grub_file_t file, const char *filename,
> > -                      void *buffer)
> > +                      void *buffer, int relocatable, grub_uint32_t
> min_addr,
> > +                      grub_uint32_t max_addr, grub_size_t align,
> grub_uint32_t preference,
> > +                      grub_uint32_t *base_addr, int
> avoid_efi_boot_services)
> >  {
> >    if (grub_multiboot_is_elf32 (buffer))
> > -    return grub_multiboot_load_elf32 (file, filename, buffer);
> > +    return grub_multiboot_load_elf32 (file, filename, buffer,
> relocatable,
> > +                                   min_addr, max_addr, align,
> preference,
> > +                                   base_addr, avoid_efi_boot_services);
> >    else if (grub_multiboot_is_elf64 (buffer))
> > -    return grub_multiboot_load_elf64 (file, filename, buffer);
> > +    return grub_multiboot_load_elf64 (file, filename, buffer,
> relocatable,
> > +                                   min_addr, max_addr, align,
> preference,
> > +                                   base_addr, avoid_efi_boot_services);
> >
> >    return grub_error (GRUB_ERR_UNKNOWN_OS, N_("invalid arch-dependent
> ELF magic"));
> >  }
> > diff --git a/grub-core/loader/multiboot_elfxx.c
> b/grub-core/loader/multiboot_elfxx.c
> > index e3a39b6..0c01569 100644
> > --- a/grub-core/loader/multiboot_elfxx.c
> > +++ b/grub-core/loader/multiboot_elfxx.c
> > @@ -51,7 +51,10 @@ CONCAT(grub_multiboot_is_elf, XX) (void *buffer)
> >  }
> >
> >  static grub_err_t
> > -CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char
> *filename, void *buffer)
> > +CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char
> *filename,
> > +                                  void *buffer, int relocatable,
> grub_uint32_t min_addr,
> > +                                  grub_uint32_t max_addr, grub_size_t
> align, grub_uint32_t preference,
> > +                                  grub_uint32_t *base_addr, int
> avoid_efi_boot_services)
> >  {
> >    Elf_Ehdr *ehdr = (Elf_Ehdr *) buffer;
> >    char *phdr_base;
> > @@ -89,19 +92,30 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t
> file, const char *filename, voi
> >         if (phdr(i)->p_paddr + phdr(i)->p_memsz > highest_load)
> >           highest_load = phdr(i)->p_paddr + phdr(i)->p_memsz;
> >
> > -       grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx,
> memsz=0x%lx, vaddr=0x%lx\n",
> > -                     i, (long) phdr(i)->p_paddr, (long)
> phdr(i)->p_memsz, (long) phdr(i)->p_vaddr);
> > +       grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx,
> memsz=0x%lx, vaddr=0x%lx,"
> > +                     "align=0x%lx, relocatable=%d,
> avoid_efi_boot_services=%d\n", i,
> > +                     (long) phdr(i)->p_paddr, (long) phdr(i)->p_memsz,
> (long) phdr(i)->p_vaddr,
> > +                     (long) align, relocatable,
> avoid_efi_boot_services);
> >
> >         {
> >           grub_relocator_chunk_t ch;
> > -         err = grub_relocator_alloc_chunk_addr
> (grub_multiboot_relocator,
> > -                                                &ch, phdr(i)->p_paddr,
> > -                                                phdr(i)->p_memsz);
> > +
> > +         if (relocatable)
> > +           err = grub_relocator_alloc_chunk_align
> (grub_multiboot_relocator, &ch,
> > +                                                   min_addr, max_addr -
> phdr(i)->p_memsz,
> > +                                                   phdr(i)->p_memsz,
> align ? align : 1,
> > +                                                   preference,
> avoid_efi_boot_services);
> > +         else
> > +           err = grub_relocator_alloc_chunk_addr
> (grub_multiboot_relocator,
> > +                                                  &ch, phdr(i)->p_paddr,
> > +                                                  phdr(i)->p_memsz);
> >           if (err)
> >             {
> >               grub_dprintf ("multiboot_loader", "Error loading phdr
> %d\n", i);
> >               return err;
> >             }
> > +         if (base_addr)
> > +           *base_addr = get_physical_target_address (ch);
> >           source = get_virtual_current_address (ch);
> >         }
> >
> > @@ -208,7 +222,7 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t
> file, const char *filename, voi
> >                                                   + 1, sh->sh_size,
> >                                                   sh->sh_addralign,
> >
>  GRUB_RELOCATOR_PREFERENCE_NONE,
> > -                                                 0);
> > +
>  avoid_efi_boot_services);
> >           if (err)
> >             {
> >               grub_dprintf ("multiboot_loader", "Error loading shdr
> %d\n", i);
> > diff --git a/grub-core/loader/multiboot_mbi2.c
> b/grub-core/loader/multiboot_mbi2.c
> > index ce68f48..03725a1 100644
> > --- a/grub-core/loader/multiboot_mbi2.c
> > +++ b/grub-core/loader/multiboot_mbi2.c
> > @@ -68,6 +68,7 @@ static grub_size_t elf_sec_num, elf_sec_entsize;
> >  static unsigned elf_sec_shstrndx;
> >  static void *elf_sections;
> >  static int keep_bs = 0;
> > +static grub_uint32_t base_addr = 0;
> >
> >  void
> >  grub_multiboot_add_elfsyms (grub_size_t num, grub_size_t entsize,
> > @@ -107,11 +108,14 @@ 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, efi_entry_specified = 0;
> > +  struct multiboot_header_tag_relocatable *rel_tag;
> > +  int entry_specified = 0, efi_entry_specified = 0, relocatable = 0;
> >    grub_addr_t entry = 0, efi_entry = 0;
> > -  grub_uint32_t console_required = 0;
> > +  grub_uint32_t console_required = 0, min_addr = 0;
> > +  grub_uint32_t max_addr = 0, preference =
> GRUB_RELOCATOR_PREFERENCE_NONE;
> >    struct multiboot_header_tag_framebuffer *fbtag = NULL;
> >    int accepted_consoles = GRUB_MULTIBOOT_CONSOLE_EGA_TEXT;
> > +  grub_size_t align = 0;
> >
> >    buffer = grub_malloc (MULTIBOOT_SEARCH);
> >    if (!buffer)
> > @@ -174,6 +178,7 @@ grub_multiboot_load (grub_file_t file, const char
> *filename)
> >             case MULTIBOOT_TAG_TYPE_EFI_BS:
> >             case MULTIBOOT_TAG_TYPE_EFI32_IH:
> >             case MULTIBOOT_TAG_TYPE_EFI64_IH:
> > +           case MULTIBOOT_TAG_TYPE_BASE_ADDR:
> >               break;
> >
> >             default:
> > @@ -215,6 +220,27 @@ grub_multiboot_load (grub_file_t file, const char
> *filename)
> >       accepted_consoles |= GRUB_MULTIBOOT_CONSOLE_FRAMEBUFFER;
> >       break;
> >
> > +      case MULTIBOOT_HEADER_TAG_RELOCATABLE:
> > +     relocatable = 1;
> > +     rel_tag = (struct multiboot_header_tag_relocatable *) tag;
> > +     min_addr = rel_tag->min_addr;
> > +     max_addr = rel_tag->max_addr;
> > +     align = rel_tag->align;
> > +     switch (rel_tag->preference)
> > +       {
> > +       case MULTIBOOT_LOAD_PREFERENCE_LOW:
> > +         preference = GRUB_RELOCATOR_PREFERENCE_LOW;
> > +         break;
> > +
> > +       case MULTIBOOT_LOAD_PREFERENCE_HIGH:
> > +         preference = GRUB_RELOCATOR_PREFERENCE_HIGH;
> > +         break;
> > +
> > +       default:
> > +         preference = GRUB_RELOCATOR_PREFERENCE_NONE;
> > +       }
> > +     break;
> > +
> >       /* GRUB always page-aligns modules.  */
> >        case MULTIBOOT_HEADER_TAG_MODULE_ALIGN:
> >       break;
> > @@ -260,15 +286,22 @@ grub_multiboot_load (grub_file_t file, const char
> *filename)
> >        else
> >       code_size = load_size;
> >
> > -      err = grub_relocator_alloc_chunk_addr (grub_multiboot_relocator,
> > -                                          &ch, load_addr,
> > -                                          code_size);
> > +      if (relocatable)
> > +     err = grub_relocator_alloc_chunk_align (grub_multiboot_relocator,
> &ch,
> > +                                             min_addr, max_addr -
> code_size,
> > +                                             code_size, align ? align :
> 1,
> > +                                             preference, keep_bs);
> > +      else
> > +     err = grub_relocator_alloc_chunk_addr (grub_multiboot_relocator,
> > +                                            &ch, load_addr,
> > +                                            code_size);
> >        if (err)
> >       {
> >         grub_dprintf ("multiboot_loader", "Error loading aout kludge\n");
> >         grub_free (buffer);
> >         return err;
> >       }
> > +      base_addr = get_physical_target_address (ch);
> >        source = get_virtual_current_address (ch);
> >
> >        if ((grub_file_seek (file, offset)) == (grub_off_t) -1)
> > @@ -290,7 +323,9 @@ grub_multiboot_load (grub_file_t file, const char
> *filename)
> >      }
> >    else
> >      {
> > -      err = grub_multiboot_load_elf (file, filename, buffer);
> > +      err = grub_multiboot_load_elf (file, filename, buffer,
> > +                                  relocatable, min_addr, max_addr,
> > +                                  align, preference, &base_addr,
> keep_bs);
> >        if (err)
> >       {
> >         grub_free (buffer);
> > @@ -303,6 +338,14 @@ grub_multiboot_load (grub_file_t file, const char
> *filename)
> >    else if (entry_specified)
> >      grub_multiboot_payload_eip = entry;
> >
> > +  if (relocatable)
> > +    {
> > +      if (base_addr > min_addr)
> > +     grub_multiboot_payload_eip += base_addr - min_addr;
> > +      else
> > +     grub_multiboot_payload_eip -= min_addr - base_addr;
> > +    }
> > +
> >    if (fbtag)
> >      err = grub_multiboot_set_console
> (GRUB_MULTIBOOT_CONSOLE_FRAMEBUFFER,
> >                                     accepted_consoles,
> > @@ -409,6 +452,7 @@ grub_multiboot_get_mbi_size (void)
> >                + grub_get_multiboot_mmap_count ()
> >                * sizeof (struct multiboot_mmap_entry)),
> MULTIBOOT_TAG_ALIGN)
> >      + ALIGN_UP (sizeof (struct multiboot_tag_framebuffer),
> MULTIBOOT_TAG_ALIGN)
> > +    + ALIGN_UP (sizeof (struct multiboot_tag_base_addr),
> MULTIBOOT_TAG_ALIGN)
> >  #ifdef GRUB_MACHINE_EFI
> >  #ifdef __i386__
> >      + ALIGN_UP (sizeof (struct multiboot_tag_efi32),
> MULTIBOOT_TAG_ALIGN)
> > @@ -698,6 +742,15 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
> >    ptrorig += (2 * sizeof (grub_uint32_t)) / sizeof
> (grub_properly_aligned_t);
> >
> >    {
> > +    struct multiboot_tag_base_addr *tag = (struct
> multiboot_tag_base_addr *) ptrorig;
> > +    tag->type = MULTIBOOT_TAG_TYPE_BASE_ADDR;
> > +    tag->size = sizeof (struct multiboot_tag_base_addr);
> > +    tag->base_addr = base_addr;
> > +    ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> > +       / sizeof (grub_properly_aligned_t);
> > +  }
> > +
> > +  {
> >      struct multiboot_tag_string *tag = (struct multiboot_tag_string *)
> ptrorig;
> >      tag->type = MULTIBOOT_TAG_TYPE_CMDLINE;
> >      tag->size = sizeof (struct multiboot_tag_string) + cmdline_size;
> > diff --git a/include/grub/multiboot.h b/include/grub/multiboot.h
> > index e13c084..ec322b0 100644
> > --- a/include/grub/multiboot.h
> > +++ b/include/grub/multiboot.h
> > @@ -94,7 +94,9 @@ grub_multiboot_load (grub_file_t file, const char
> *filename);
> >  /* Load ELF32 or ELF64.  */
> >  grub_err_t
> >  grub_multiboot_load_elf (grub_file_t file, const char *filename,
> > -                      void *buffer);
> > +                      void *buffer, int relocatable, grub_uint32_t
> min_addr,
> > +                      grub_uint32_t max_addr, grub_size_t align,
> grub_uint32_t preference,
> > +                      grub_uint32_t *base_addr, int
> avoid_efi_boot_services);
> >  extern grub_size_t grub_multiboot_pure_size;
> >  extern grub_size_t grub_multiboot_alloc_mbi;
> >  extern grub_uint32_t grub_multiboot_payload_eip;
> > diff --git a/include/multiboot2.h b/include/multiboot2.h
> > index 36a174f..c09bdbc 100644
> > --- a/include/multiboot2.h
> > +++ b/include/multiboot2.h
> > @@ -62,6 +62,7 @@
> >  #define MULTIBOOT_TAG_TYPE_EFI_BS            18
> >  #define MULTIBOOT_TAG_TYPE_EFI32_IH          19
> >  #define MULTIBOOT_TAG_TYPE_EFI64_IH          20
> > +#define MULTIBOOT_TAG_TYPE_BASE_ADDR         21
> >
> >  #define MULTIBOOT_HEADER_TAG_END  0
> >  #define MULTIBOOT_HEADER_TAG_INFORMATION_REQUEST  1
> > @@ -72,11 +73,16 @@
> >  #define MULTIBOOT_HEADER_TAG_MODULE_ALIGN  6
> >  #define MULTIBOOT_HEADER_TAG_EFI_BS  7
> >  #define MULTIBOOT_HEADER_TAG_ENTRY_ADDRESS_EFI64  9
> > +#define MULTIBOOT_HEADER_TAG_RELOCATABLE  10
> >
> >  #define MULTIBOOT_ARCHITECTURE_I386  0
> >  #define MULTIBOOT_ARCHITECTURE_MIPS32  4
> >  #define MULTIBOOT_HEADER_TAG_OPTIONAL 1
> >
> > +#define MULTIBOOT_LOAD_PREFERENCE_NONE 0
> > +#define MULTIBOOT_LOAD_PREFERENCE_LOW 1
> > +#define MULTIBOOT_LOAD_PREFERENCE_HIGH 2
> > +
> >  #define MULTIBOOT_CONSOLE_FLAGS_CONSOLE_REQUIRED 1
> >  #define MULTIBOOT_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED 2
> >
> > @@ -161,6 +167,17 @@ struct multiboot_header_tag_module_align
> >    multiboot_uint32_t size;
> >  };
> >
> > +struct multiboot_header_tag_relocatable
> > +{
> > +  multiboot_uint16_t type;
> > +  multiboot_uint16_t flags;
> > +  multiboot_uint32_t size;
> > +  multiboot_uint32_t min_addr;
> > +  multiboot_uint32_t max_addr;
> > +  multiboot_uint32_t align;
> > +  multiboot_uint32_t preference;
> > +};
> > +
> >  struct multiboot_color
> >  {
> >    multiboot_uint8_t red;
> > @@ -387,6 +404,13 @@ struct multiboot_tag_efi64_ih
> >    multiboot_uint64_t pointer;
> >  };
> >
> > +struct multiboot_tag_base_addr
> > +{
> > +  multiboot_uint32_t type;
> > +  multiboot_uint32_t size;
> > +  multiboot_uint32_t base_addr;
> > +};
> > +
> >  #endif /* ! ASM_FILE */
> >
> >  #endif /* ! MULTIBOOT_HEADER */
> >
>
>

-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #1.2: Type: text/html, Size: 24674 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 4/4] multiboot2: Add support for relocatable images
  2016-03-02 16:51 ` [GRUB2 PATCH v3 4/4] multiboot2: Add support for relocatable images Daniel Kiper
  2016-03-04  6:51   ` Juergen Gross
  2016-03-10 20:41   ` Vladimir 'phcoder' Serbinenko
@ 2016-03-10 20:44   ` Vladimir 'phcoder' Serbinenko
  2016-03-11 16:23     ` Daniel Kiper
  2 siblings, 1 reply; 30+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2016-03-10 20:44 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com


[-- Attachment #1.1: Type: text/plain, Size: 19286 bytes --]

On Wednesday, March 2, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:

> Currently multiboot2 protocol loads image exactly at address specified in
> ELF or multiboot2 header. This solution works quite well on legacy BIOS
> platforms. It is possible because memory regions are placed at predictable
> addresses (though I was not able to find any spec which says that it is
> strong requirement, so, it looks that it is just a goodwill of hardware
> designers). However, EFI platforms are more volatile. Even if required
> memory regions live at specific addresses then they are sometimes simply
> not free (e.g. used by boot/runtime services on Dell PowerEdge R820 and
> OVMF). This means that you are not able to simply set up final image
> destination on build time. You have to provide method to relocate image
> contents to real load address which is usually different than load address
> specified in ELF and multiboot2 headers.
>
> This patch provides all needed machinery to do self relocation in image
> code.
> First of all GRUB2 reads min_addr (min. load addr), max_addr (max. load
> addr),
> align (required image alignment), preference (it says which memory regions
> are
> preferred by image, e.g. none, low, high) from
> multiboot_header_tag_relocatable
> header tag contained in binary. Later loader tries to fulfill request (not
> only
> that one) and if it succeeds then it informs image about real load address
> via
> multiboot_tag_base_addr tag. At this stage GRUB2 role is finished. Starting
> from now executable must cope with relocations itself using whole static
> and dynamic knowledge provided by boot loader.
>
> This patch does not provide functionality which could do relocations using
> ELF relocation data.

Can you add a check that image doesn't have any relocation entries? So that
we fail nicely rather than loading half-working binary?

> However, I was asked by Konrad Rzeszutek Wilk and Vladimir
> 'phcoder' Serbinenko to investigate that thing. It looks that relevant
> machinery
> could be added to existing code (including this patch) without huge effort.
> Additionally, ELF relocation could live in parallel with self relocation
> provided
> by this patch. However, during research I realized that first of all we
> should
> establish the details how ELF relocatable image should look like and how
> it should
> be build. At least to build proper test/example files.
>
> As I saw multiboot2 protocol is able to consume ET_EXEC and ET_DYN ELF
> files.
> Potentially we can use ET_DYN file type. It can be build with gcc/ld -pie
> option.
> However, it contains a lot of unneeded stuff (e.g. INTERP, DYNAMIC,
> GNU_EH_FRAME
> program headers) and it could be quite difficult to drop them (Hmmm... Is
> it
> possible to build it properly with custom ld script?). So, I have checked
> ET_EXEC
> file type. Sadly in this case linker by default resolves all local symbol
> relocations
> and removes relocation related sections. Fortunately it is possible to
> leave them
> as is with simple -q/--emit-relocs ld option. However, output file is
> quite fragile
> and any operation on it should be done with great care (e.g. strip should
> be called
> with --strip-unneeded option). So, this solution is not perfect too. It
> means that
> maybe we should look for better solution. However, I think that we should
> not use
> any custom tools and focus on functionalities provided by compiler and
> binutils.
> In this context ld scripts looks quite promising but maybe you have better
> solutions.
> So, what do you think about that?
>
> This patch was tested with Xen image which uses that functionality.
> However, this Xen
> feature is still under development and new patchset will be released in
> about 3-4 weeks.
>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com <javascript:;>>
> ---
> v3 - suggestions/fixes:
>    - reduce number of casts
>      (suggested by Konrad Rzeszutek Wilk),
>    - remove unneeded space at the end of line
>      (suggested by Konrad Rzeszutek Wilk),
>    - improve commit message
>      (suggested by Konrad Rzeszutek Wilk).
> ---
>  grub-core/loader/i386/multiboot_mbi.c |    6 ++-
>  grub-core/loader/multiboot.c          |   12 ++++--
>  grub-core/loader/multiboot_elfxx.c    |   28 ++++++++++----
>  grub-core/loader/multiboot_mbi2.c     |   65
> ++++++++++++++++++++++++++++++---
>  include/grub/multiboot.h              |    4 +-
>  include/multiboot2.h                  |   24 ++++++++++++
>  6 files changed, 120 insertions(+), 19 deletions(-)
>
> diff --git a/grub-core/loader/i386/multiboot_mbi.c
> b/grub-core/loader/i386/multiboot_mbi.c
> index f60b702..4fc83ed 100644
> --- a/grub-core/loader/i386/multiboot_mbi.c
> +++ b/grub-core/loader/i386/multiboot_mbi.c
> @@ -72,7 +72,8 @@ load_kernel (grub_file_t file, const char *filename,
>    grub_err_t err;
>    if (grub_multiboot_quirks & GRUB_MULTIBOOT_QUIRK_BAD_KLUDGE)
>      {
> -      err = grub_multiboot_load_elf (file, filename, buffer);
> +      err = grub_multiboot_load_elf (file, filename, buffer, 0, 0, 0, 0,
> +                                    GRUB_RELOCATOR_PREFERENCE_NONE, NULL,
> 0);
>        if (err == GRUB_ERR_NONE) {
>         return GRUB_ERR_NONE;
>        }
> @@ -121,7 +122,8 @@ load_kernel (grub_file_t file, const char *filename,
>        return GRUB_ERR_NONE;
>      }
>
> -  return grub_multiboot_load_elf (file, filename, buffer);
> +  return grub_multiboot_load_elf (file, filename, buffer, 0, 0, 0, 0,
> +                                 GRUB_RELOCATOR_PREFERENCE_NONE, NULL, 0);
>  }
>
>  static struct multiboot_header *
> diff --git a/grub-core/loader/multiboot.c b/grub-core/loader/multiboot.c
> index 18038fd..c0f51b6 100644
> --- a/grub-core/loader/multiboot.c
> +++ b/grub-core/loader/multiboot.c
> @@ -208,12 +208,18 @@ static grub_uint64_t highest_load;
>  /* Load ELF32 or ELF64.  */
>  grub_err_t
>  grub_multiboot_load_elf (grub_file_t file, const char *filename,
> -                        void *buffer)
> +                        void *buffer, int relocatable, grub_uint32_t
> min_addr,
> +                        grub_uint32_t max_addr, grub_size_t align,
> grub_uint32_t preference,
> +                        grub_uint32_t *base_addr, int
> avoid_efi_boot_services)
>  {
>    if (grub_multiboot_is_elf32 (buffer))
> -    return grub_multiboot_load_elf32 (file, filename, buffer);
> +    return grub_multiboot_load_elf32 (file, filename, buffer, relocatable,
> +                                     min_addr, max_addr, align,
> preference,
> +                                     base_addr, avoid_efi_boot_services);
>    else if (grub_multiboot_is_elf64 (buffer))
> -    return grub_multiboot_load_elf64 (file, filename, buffer);
> +    return grub_multiboot_load_elf64 (file, filename, buffer, relocatable,
> +                                     min_addr, max_addr, align,
> preference,
> +                                     base_addr, avoid_efi_boot_services);
>
>    return grub_error (GRUB_ERR_UNKNOWN_OS, N_("invalid arch-dependent ELF
> magic"));
>  }
> diff --git a/grub-core/loader/multiboot_elfxx.c
> b/grub-core/loader/multiboot_elfxx.c
> index e3a39b6..0c01569 100644
> --- a/grub-core/loader/multiboot_elfxx.c
> +++ b/grub-core/loader/multiboot_elfxx.c
> @@ -51,7 +51,10 @@ CONCAT(grub_multiboot_is_elf, XX) (void *buffer)
>  }
>
>  static grub_err_t
> -CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char
> *filename, void *buffer)
> +CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char
> *filename,
> +                                    void *buffer, int relocatable,
> grub_uint32_t min_addr,
> +                                    grub_uint32_t max_addr, grub_size_t
> align, grub_uint32_t preference,
> +                                    grub_uint32_t *base_addr, int
> avoid_efi_boot_services)
>  {
>    Elf_Ehdr *ehdr = (Elf_Ehdr *) buffer;
>    char *phdr_base;
> @@ -89,19 +92,30 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file,
> const char *filename, voi
>           if (phdr(i)->p_paddr + phdr(i)->p_memsz > highest_load)
>             highest_load = phdr(i)->p_paddr + phdr(i)->p_memsz;
>
> -         grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx,
> memsz=0x%lx, vaddr=0x%lx\n",
> -                       i, (long) phdr(i)->p_paddr, (long)
> phdr(i)->p_memsz, (long) phdr(i)->p_vaddr);
> +         grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx,
> memsz=0x%lx, vaddr=0x%lx,"
> +                       "align=0x%lx, relocatable=%d,
> avoid_efi_boot_services=%d\n", i,
> +                       (long) phdr(i)->p_paddr, (long) phdr(i)->p_memsz,
> (long) phdr(i)->p_vaddr,
> +                       (long) align, relocatable,
> avoid_efi_boot_services);
>
>           {
>             grub_relocator_chunk_t ch;
> -           err = grub_relocator_alloc_chunk_addr
> (grub_multiboot_relocator,
> -                                                  &ch, phdr(i)->p_paddr,
> -                                                  phdr(i)->p_memsz);
> +
> +           if (relocatable)
> +             err = grub_relocator_alloc_chunk_align
> (grub_multiboot_relocator, &ch,
> +                                                     min_addr, max_addr -
> phdr(i)->p_memsz,
> +                                                     phdr(i)->p_memsz,
> align ? align : 1,
> +                                                     preference,
> avoid_efi_boot_services);
> +           else
> +             err = grub_relocator_alloc_chunk_addr
> (grub_multiboot_relocator,
> +                                                    &ch, phdr(i)->p_paddr,
> +                                                    phdr(i)->p_memsz);
>             if (err)
>               {
>                 grub_dprintf ("multiboot_loader", "Error loading phdr
> %d\n", i);
>                 return err;
>               }
> +           if (base_addr)
> +             *base_addr = get_physical_target_address (ch);
>             source = get_virtual_current_address (ch);
>           }
>
> @@ -208,7 +222,7 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file,
> const char *filename, voi
>                                                     + 1, sh->sh_size,
>                                                     sh->sh_addralign,
>
> GRUB_RELOCATOR_PREFERENCE_NONE,
> -                                                   0);
> +
>  avoid_efi_boot_services);
>             if (err)
>               {
>                 grub_dprintf ("multiboot_loader", "Error loading shdr
> %d\n", i);
> diff --git a/grub-core/loader/multiboot_mbi2.c
> b/grub-core/loader/multiboot_mbi2.c
> index ce68f48..03725a1 100644
> --- a/grub-core/loader/multiboot_mbi2.c
> +++ b/grub-core/loader/multiboot_mbi2.c
> @@ -68,6 +68,7 @@ static grub_size_t elf_sec_num, elf_sec_entsize;
>  static unsigned elf_sec_shstrndx;
>  static void *elf_sections;
>  static int keep_bs = 0;
> +static grub_uint32_t base_addr = 0;
>
>  void
>  grub_multiboot_add_elfsyms (grub_size_t num, grub_size_t entsize,
> @@ -107,11 +108,14 @@ 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, efi_entry_specified = 0;
> +  struct multiboot_header_tag_relocatable *rel_tag;
> +  int entry_specified = 0, efi_entry_specified = 0, relocatable = 0;
>    grub_addr_t entry = 0, efi_entry = 0;
> -  grub_uint32_t console_required = 0;
> +  grub_uint32_t console_required = 0, min_addr = 0;
> +  grub_uint32_t max_addr = 0, preference = GRUB_RELOCATOR_PREFERENCE_NONE;
>    struct multiboot_header_tag_framebuffer *fbtag = NULL;
>    int accepted_consoles = GRUB_MULTIBOOT_CONSOLE_EGA_TEXT;
> +  grub_size_t align = 0;
>
>    buffer = grub_malloc (MULTIBOOT_SEARCH);
>    if (!buffer)
> @@ -174,6 +178,7 @@ grub_multiboot_load (grub_file_t file, const char
> *filename)
>               case MULTIBOOT_TAG_TYPE_EFI_BS:
>               case MULTIBOOT_TAG_TYPE_EFI32_IH:
>               case MULTIBOOT_TAG_TYPE_EFI64_IH:
> +             case MULTIBOOT_TAG_TYPE_BASE_ADDR:
>                 break;
>
>               default:
> @@ -215,6 +220,27 @@ grub_multiboot_load (grub_file_t file, const char
> *filename)
>         accepted_consoles |= GRUB_MULTIBOOT_CONSOLE_FRAMEBUFFER;
>         break;
>
> +      case MULTIBOOT_HEADER_TAG_RELOCATABLE:
> +       relocatable = 1;
> +       rel_tag = (struct multiboot_header_tag_relocatable *) tag;
> +       min_addr = rel_tag->min_addr;
> +       max_addr = rel_tag->max_addr;
> +       align = rel_tag->align;
> +       switch (rel_tag->preference)
> +         {
> +         case MULTIBOOT_LOAD_PREFERENCE_LOW:
> +           preference = GRUB_RELOCATOR_PREFERENCE_LOW;
> +           break;
> +
> +         case MULTIBOOT_LOAD_PREFERENCE_HIGH:
> +           preference = GRUB_RELOCATOR_PREFERENCE_HIGH;
> +           break;
> +
> +         default:
> +           preference = GRUB_RELOCATOR_PREFERENCE_NONE;
> +         }
> +       break;
> +
>         /* GRUB always page-aligns modules.  */
>        case MULTIBOOT_HEADER_TAG_MODULE_ALIGN:
>         break;
> @@ -260,15 +286,22 @@ grub_multiboot_load (grub_file_t file, const char
> *filename)
>        else
>         code_size = load_size;
>
> -      err = grub_relocator_alloc_chunk_addr (grub_multiboot_relocator,
> -                                            &ch, load_addr,
> -                                            code_size);
> +      if (relocatable)
> +       err = grub_relocator_alloc_chunk_align (grub_multiboot_relocator,
> &ch,
> +                                               min_addr, max_addr -
> code_size,
> +                                               code_size, align ? align :
> 1,
> +                                               preference, keep_bs);
> +      else
> +       err = grub_relocator_alloc_chunk_addr (grub_multiboot_relocator,
> +                                              &ch, load_addr,
> +                                              code_size);
>        if (err)
>         {
>           grub_dprintf ("multiboot_loader", "Error loading aout kludge\n");
>           grub_free (buffer);
>           return err;
>         }
> +      base_addr = get_physical_target_address (ch);
>        source = get_virtual_current_address (ch);
>
>        if ((grub_file_seek (file, offset)) == (grub_off_t) -1)
> @@ -290,7 +323,9 @@ grub_multiboot_load (grub_file_t file, const char
> *filename)
>      }
>    else
>      {
> -      err = grub_multiboot_load_elf (file, filename, buffer);
> +      err = grub_multiboot_load_elf (file, filename, buffer,
> +                                    relocatable, min_addr, max_addr,
> +                                    align, preference, &base_addr,
> keep_bs);
>        if (err)
>         {
>           grub_free (buffer);
> @@ -303,6 +338,14 @@ grub_multiboot_load (grub_file_t file, const char
> *filename)
>    else if (entry_specified)
>      grub_multiboot_payload_eip = entry;
>
> +  if (relocatable)
> +    {
> +      if (base_addr > min_addr)
> +       grub_multiboot_payload_eip += base_addr - min_addr;
> +      else
> +       grub_multiboot_payload_eip -= min_addr - base_addr;
> +    }
> +
>    if (fbtag)
>      err = grub_multiboot_set_console (GRUB_MULTIBOOT_CONSOLE_FRAMEBUFFER,
>                                       accepted_consoles,
> @@ -409,6 +452,7 @@ grub_multiboot_get_mbi_size (void)
>                  + grub_get_multiboot_mmap_count ()
>                  * sizeof (struct multiboot_mmap_entry)),
> MULTIBOOT_TAG_ALIGN)
>      + ALIGN_UP (sizeof (struct multiboot_tag_framebuffer),
> MULTIBOOT_TAG_ALIGN)
> +    + ALIGN_UP (sizeof (struct multiboot_tag_base_addr),
> MULTIBOOT_TAG_ALIGN)
>  #ifdef GRUB_MACHINE_EFI
>  #ifdef __i386__
>      + ALIGN_UP (sizeof (struct multiboot_tag_efi32), MULTIBOOT_TAG_ALIGN)
> @@ -698,6 +742,15 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
>    ptrorig += (2 * sizeof (grub_uint32_t)) / sizeof
> (grub_properly_aligned_t);
>
>    {
> +    struct multiboot_tag_base_addr *tag = (struct multiboot_tag_base_addr
> *) ptrorig;
> +    tag->type = MULTIBOOT_TAG_TYPE_BASE_ADDR;
> +    tag->size = sizeof (struct multiboot_tag_base_addr);
> +    tag->base_addr = base_addr;
> +    ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> +       / sizeof (grub_properly_aligned_t);
> +  }
> +
> +  {
>      struct multiboot_tag_string *tag = (struct multiboot_tag_string *)
> ptrorig;
>      tag->type = MULTIBOOT_TAG_TYPE_CMDLINE;
>      tag->size = sizeof (struct multiboot_tag_string) + cmdline_size;
> diff --git a/include/grub/multiboot.h b/include/grub/multiboot.h
> index e13c084..ec322b0 100644
> --- a/include/grub/multiboot.h
> +++ b/include/grub/multiboot.h
> @@ -94,7 +94,9 @@ grub_multiboot_load (grub_file_t file, const char
> *filename);
>  /* Load ELF32 or ELF64.  */
>  grub_err_t
>  grub_multiboot_load_elf (grub_file_t file, const char *filename,
> -                        void *buffer);
> +                        void *buffer, int relocatable, grub_uint32_t
> min_addr,
> +                        grub_uint32_t max_addr, grub_size_t align,
> grub_uint32_t preference,
> +                        grub_uint32_t *base_addr, int
> avoid_efi_boot_services);
>  extern grub_size_t grub_multiboot_pure_size;
>  extern grub_size_t grub_multiboot_alloc_mbi;
>  extern grub_uint32_t grub_multiboot_payload_eip;
> diff --git a/include/multiboot2.h b/include/multiboot2.h
> index 36a174f..c09bdbc 100644
> --- a/include/multiboot2.h
> +++ b/include/multiboot2.h
> @@ -62,6 +62,7 @@
>  #define MULTIBOOT_TAG_TYPE_EFI_BS            18
>  #define MULTIBOOT_TAG_TYPE_EFI32_IH          19
>  #define MULTIBOOT_TAG_TYPE_EFI64_IH          20
> +#define MULTIBOOT_TAG_TYPE_BASE_ADDR         21
>
>  #define MULTIBOOT_HEADER_TAG_END  0
>  #define MULTIBOOT_HEADER_TAG_INFORMATION_REQUEST  1
> @@ -72,11 +73,16 @@
>  #define MULTIBOOT_HEADER_TAG_MODULE_ALIGN  6
>  #define MULTIBOOT_HEADER_TAG_EFI_BS  7
>  #define MULTIBOOT_HEADER_TAG_ENTRY_ADDRESS_EFI64  9
> +#define MULTIBOOT_HEADER_TAG_RELOCATABLE  10
>
>  #define MULTIBOOT_ARCHITECTURE_I386  0
>  #define MULTIBOOT_ARCHITECTURE_MIPS32  4
>  #define MULTIBOOT_HEADER_TAG_OPTIONAL 1
>
> +#define MULTIBOOT_LOAD_PREFERENCE_NONE 0
> +#define MULTIBOOT_LOAD_PREFERENCE_LOW 1
> +#define MULTIBOOT_LOAD_PREFERENCE_HIGH 2
> +
>  #define MULTIBOOT_CONSOLE_FLAGS_CONSOLE_REQUIRED 1
>  #define MULTIBOOT_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED 2
>
> @@ -161,6 +167,17 @@ struct multiboot_header_tag_module_align
>    multiboot_uint32_t size;
>  };
>
> +struct multiboot_header_tag_relocatable
> +{
> +  multiboot_uint16_t type;
> +  multiboot_uint16_t flags;
> +  multiboot_uint32_t size;
> +  multiboot_uint32_t min_addr;
> +  multiboot_uint32_t max_addr;
> +  multiboot_uint32_t align;
> +  multiboot_uint32_t preference;
> +};
> +
>  struct multiboot_color
>  {
>    multiboot_uint8_t red;
> @@ -387,6 +404,13 @@ struct multiboot_tag_efi64_ih
>    multiboot_uint64_t pointer;
>  };
>
> +struct multiboot_tag_base_addr
> +{
> +  multiboot_uint32_t type;
> +  multiboot_uint32_t size;
> +  multiboot_uint32_t base_addr;
> +};
> +
>  #endif /* ! ASM_FILE */
>
>  #endif /* ! MULTIBOOT_HEADER */
> --
> 1.7.10.4
>
>

-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #1.2: Type: text/html, Size: 22657 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions
  2016-03-09 10:48 ` [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions Daniel Kiper
@ 2016-03-11 12:27   ` Vladimir 'phcoder' Serbinenko
  2016-03-11 13:14     ` Daniel Kiper
  0 siblings, 1 reply; 30+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2016-03-11 12:27 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com


[-- Attachment #1.1: Type: text/plain, Size: 1035 bytes --]

On Wednesday, March 9, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:

> On Wed, Mar 02, 2016 at 05:51:36PM +0100, Daniel Kiper wrote:
> > Hi,
> >
> > This patch series:
> >   - enables EFI boot services usage in loaded images
> >     by multiboot2 protocol,
> >   - add support for multiboot2 protocol compatible
> >     relocatable images.
> >
> > Earlier versions of this patch series are extensively tested
> > and used internally at least in Oracle. It should be mentioned
> > that this release does not change any functionality introduced
> > by earlier releases. It just takes into account comments posted
> > by various people.
> >
> > Hmmm... Ugh... Cough... Is it possible to get this stuff
> > into 2.02 train?
>
> Andrei, Vladimir, ping?
>
They look reasonable for 2.02. Strictly speaking they're features but code
is well isolated and the alternative is painful. I answered the individual
mails with review. Is there a followup with updating multiboot2 docs?

>
> Daniel
>


-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #1.2: Type: text/html, Size: 1489 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions
  2016-03-11 12:27   ` Vladimir 'phcoder' Serbinenko
@ 2016-03-11 13:14     ` Daniel Kiper
  2016-03-11 15:44       ` Vladimir 'phcoder' Serbinenko
  0 siblings, 1 reply; 30+ messages in thread
From: Daniel Kiper @ 2016-03-11 13:14 UTC (permalink / raw)
  To: Vladimir 'phcoder' Serbinenko
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com

On Fri, Mar 11, 2016 at 01:27:32PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> On Wednesday, March 9, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> > On Wed, Mar 02, 2016 at 05:51:36PM +0100, Daniel Kiper wrote:
> > > Hi,
> > >
> > > This patch series:
> > >   - enables EFI boot services usage in loaded images
> > >     by multiboot2 protocol,
> > >   - add support for multiboot2 protocol compatible
> > >     relocatable images.
> > >
> > > Earlier versions of this patch series are extensively tested
> > > and used internally at least in Oracle. It should be mentioned
> > > that this release does not change any functionality introduced
> > > by earlier releases. It just takes into account comments posted
> > > by various people.
> > >
> > > Hmmm... Ugh... Cough... Is it possible to get this stuff
> > > into 2.02 train?
> >
> > Andrei, Vladimir, ping?
> >
> They look reasonable for 2.02. Strictly speaking they're features but code

Great! Thanks a lot!

> is well isolated and the alternative is painful. I answered the individual
> mails with review.

I have some questions which I send you shortly.

> Is there a followup with updating multiboot2 docs?

Do you want to have updated docs in 2.02?

By the way, where is multiboot2 doc file, which I should update, in git tree?

Daniel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 1/4] i386/relocator: Add grub_relocator64_efi relocator
  2016-03-10 20:23   ` Vladimir 'phcoder' Serbinenko
@ 2016-03-11 13:23     ` Daniel Kiper
  2016-03-11 15:41       ` Vladimir 'phcoder' Serbinenko
  2016-03-11 15:42       ` Vladimir 'phcoder' Serbinenko
  0 siblings, 2 replies; 30+ messages in thread
From: Daniel Kiper @ 2016-03-11 13:23 UTC (permalink / raw)
  To: Vladimir 'phcoder' Serbinenko
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com

On Thu, Mar 10, 2016 at 09:23:23PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> On Wednesday, March 2, 2016, Daniel Kiper <daniel.kiper@oracle.com> 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.

OK, as I can see idea in general is accepted. Do you want
grub_relocator32_efi in 2.02 or 2.03 is OK?

> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com <javascript:;>>
> > ---
> > v3 - suggestions/fixes:
> >    - reuse grub-core/lib/i386/relocator64.S code
> >      instead of creating separate assembly file
> >      (suggested by Vladimir 'phcoder' Serbinenko),
> >    - grub_multiboot_boot() cleanup
> >      (suggested by Vladimir 'phcoder' Serbinenko),
> >    - reuse multiboot_header_tag_entry_address struct instead
> >      of creating new one for EFI 64-bit entry point
> >      (suggested by Vladimir 'phcoder' Serbinenko).
> > ---
> >  grub-core/lib/i386/relocator.c    |   48
> > ++++++++++++++++++++++++++++++++++
> >  grub-core/lib/i386/relocator64.S  |    3 +++
> >  grub-core/loader/multiboot.c      |   51
> > +++++++++++++++++++++++++++++++++----
> >  grub-core/loader/multiboot_mbi2.c |   19 +++++++++++---
> >  include/grub/i386/multiboot.h     |   11 ++++++++
> >  include/grub/i386/relocator.h     |   21 +++++++++++++++
> >  include/multiboot2.h              |    1 +
> >  7 files changed, 145 insertions(+), 9 deletions(-)
> >
> > diff --git a/grub-core/lib/i386/relocator.c
> > b/grub-core/lib/i386/relocator.c
> > index 71dd4f0..2b0c260 100644
> > --- a/grub-core/lib/i386/relocator.c
> > +++ b/grub-core/lib/i386/relocator.c
> > @@ -69,6 +69,13 @@ 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;
> > +#endif
> > +#endif
> > +
> >
> Can we move this and all to a separate file to avoid too many #ifdef ?

Do you think about grub-core/lib/i386/relocator-efi.c or something like that?
If yes then I do not think it is a problem.

Daniel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 2/4] multiboot2: Add tags used to pass ImageHandle to loaded image
  2016-03-10 20:26   ` Vladimir 'phcoder' Serbinenko
@ 2016-03-11 13:27     ` Daniel Kiper
  2016-03-11 15:39       ` Vladimir 'phcoder' Serbinenko
  0 siblings, 1 reply; 30+ messages in thread
From: Daniel Kiper @ 2016-03-11 13:27 UTC (permalink / raw)
  To: Vladimir 'phcoder' Serbinenko
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com

On Thu, Mar 10, 2016 at 09:26:06PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> On Wednesday, March 2, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> > Add tags used to pass ImageHandle to loaded image if requested.
> > It is used by at least ExitBootServices() function.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com <javascript:;>>
> > ---
> > v3 - suggestions/fixes:
> >    - mbi EFI related stuff size calculation
> >      should depend on target architecture
> >      (suggested by Konrad Rzeszutek Wilk),
> >    - use plain type instead of pointer
> >      dereference as sizeof() argument
> >      (suggested by Konrad Rzeszutek Wilk),
> >    - improve commit message
> >      (suggested by Konrad Rzeszutek Wilk).
> > ---
> >  grub-core/loader/multiboot_mbi2.c |   50
> > ++++++++++++++++++++++++++++++-------
> >  include/multiboot2.h              |   16 ++++++++++++
> >  2 files changed, 57 insertions(+), 9 deletions(-)
> >
> > diff --git a/grub-core/loader/multiboot_mbi2.c
> > b/grub-core/loader/multiboot_mbi2.c
> > index a3dca90..7591edc 100644
> > --- a/grub-core/loader/multiboot_mbi2.c
> > +++ b/grub-core/loader/multiboot_mbi2.c
> > @@ -172,6 +172,8 @@ grub_multiboot_load (grub_file_t file, const char
> > *filename)
> >               case MULTIBOOT_TAG_TYPE_NETWORK:
> >               case MULTIBOOT_TAG_TYPE_EFI_MMAP:
> >               case MULTIBOOT_TAG_TYPE_EFI_BS:
> > +             case MULTIBOOT_TAG_TYPE_EFI32_IH:
> > +             case MULTIBOOT_TAG_TYPE_EFI64_IH:
> >                 break;
> >
> >               default:
> > @@ -407,16 +409,22 @@ grub_multiboot_get_mbi_size (void)
> >                  + grub_get_multiboot_mmap_count ()
> >                  * sizeof (struct multiboot_mmap_entry)),
> > MULTIBOOT_TAG_ALIGN)
> >      + ALIGN_UP (sizeof (struct multiboot_tag_framebuffer),
> > MULTIBOOT_TAG_ALIGN)
> > +#ifdef GRUB_MACHINE_EFI
> > +#ifdef __i386__
> >      + ALIGN_UP (sizeof (struct multiboot_tag_efi32), MULTIBOOT_TAG_ALIGN)
> > +    + ALIGN_UP (sizeof (struct multiboot_tag_efi32_ih),
> > MULTIBOOT_TAG_ALIGN)
> > +#endif
> > +#ifdef __x86_64__
> >      + ALIGN_UP (sizeof (struct multiboot_tag_efi64), MULTIBOOT_TAG_ALIGN)
> > +    + ALIGN_UP (sizeof (struct multiboot_tag_efi64_ih),
> > MULTIBOOT_TAG_ALIGN)
> > +#endif
> > +    + ALIGN_UP (sizeof (struct multiboot_tag_efi_mmap)
> > +               + efi_mmap_size, MULTIBOOT_TAG_ALIGN)
> > +#endif
> >
> It doesn't need to be exact. It's just an upper bound. Feel free to
> simplify knowing this, removing few #ifdef.

OK.

> >      + ALIGN_UP (sizeof (struct multiboot_tag_old_acpi)
> >                 + sizeof (struct grub_acpi_rsdp_v10), MULTIBOOT_TAG_ALIGN)
> >      + acpiv2_size ()
> >      + net_size ()
> > -#ifdef GRUB_MACHINE_EFI
> > -    + ALIGN_UP (sizeof (struct multiboot_tag_efi_mmap)
> > -               + efi_mmap_size, MULTIBOOT_TAG_ALIGN)
> > -#endif
> >      + sizeof (struct multiboot_tag_vbe) + MULTIBOOT_TAG_ALIGN - 1
> >      + sizeof (struct multiboot_tag_apm) + MULTIBOOT_TAG_ALIGN - 1;
> >  }
> > @@ -907,11 +915,35 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
> >
> >    if (keep_bs)
> >      {
> > -      struct multiboot_tag *tag = (struct multiboot_tag *) ptrorig;
> > -      tag->type = MULTIBOOT_TAG_TYPE_EFI_BS;
> > -      tag->size = sizeof (struct multiboot_tag);
> > -      ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> > -       / sizeof (grub_properly_aligned_t);
> > +      {
> > +       struct multiboot_tag *tag = (struct multiboot_tag *) ptrorig;
> > +       tag->type = MULTIBOOT_TAG_TYPE_EFI_BS;
> > +       tag->size = sizeof (struct multiboot_tag);
> > +       ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> > +         / sizeof (grub_properly_aligned_t);
> > +      }
> > +
> > +#ifdef __i386__
> > +      {
> > +       struct multiboot_tag_efi32_ih *tag = (struct
> > multiboot_tag_efi32_ih *) ptrorig;
> > +       tag->type = MULTIBOOT_TAG_TYPE_EFI32_IH;
> > +       tag->size = sizeof (struct multiboot_tag_efi32_ih);
> > +       tag->pointer = (grub_addr_t) grub_efi_image_handle;
> > +       ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> > +         / sizeof (grub_properly_aligned_t);
> > +      }
> > +#endif
> > +
> > +#ifdef __x86_64__
> > +      {
> > +       struct multiboot_tag_efi64_ih *tag = (struct
> > multiboot_tag_efi64_ih *) ptrorig;
> > +       tag->type = MULTIBOOT_TAG_TYPE_EFI64_IH;
> > +       tag->size = sizeof (struct multiboot_tag_efi64_ih);
> > +       tag->pointer = (grub_addr_t) grub_efi_image_handle;
> > +       ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> > +         / sizeof (grub_properly_aligned_t);
> > +      }
> > +#endif
> >      }
> >  #endif
> >
> > diff --git a/include/multiboot2.h b/include/multiboot2.h
> > index d96aa40..36a174f 100644
> > --- a/include/multiboot2.h
> > +++ b/include/multiboot2.h
> > @@ -60,6 +60,8 @@
> >  #define MULTIBOOT_TAG_TYPE_NETWORK           16
> >  #define MULTIBOOT_TAG_TYPE_EFI_MMAP          17
> >  #define MULTIBOOT_TAG_TYPE_EFI_BS            18
> > +#define MULTIBOOT_TAG_TYPE_EFI32_IH          19
> > +#define MULTIBOOT_TAG_TYPE_EFI64_IH          20
> >
> Is there a followup to define those in texi doc as well?

No problem. Which one should I update?

Daniel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 3/4] multiboot2: Do not pass memory maps to image if EFI boot services are enabled
  2016-03-10 20:28   ` Vladimir 'phcoder' Serbinenko
@ 2016-03-11 13:30     ` Daniel Kiper
  0 siblings, 0 replies; 30+ messages in thread
From: Daniel Kiper @ 2016-03-11 13:30 UTC (permalink / raw)
  To: Vladimir 'phcoder' Serbinenko
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com

On Thu, Mar 10, 2016 at 09:28:25PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> On Wednesday, March 2, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> > Do not pass memory maps to image if it asked for EFI boot services.
> > Main reason for not providing maps is because they will likely be
> > invalid. We do a few allocations after filling them, e.g. for relocator
> > needs. Usually we do not care as we would already finish boot services.
> > If we keep boot services then it is easier to not provide maps. However,
> > if image needs memory maps and they are not provided by bootloader then
> > it should get them itself just before ExitBootServices() call.
> >
> Patch gets too cluttered. Can you provide 2 variants: normal (for commit)
> and with ignoring whitespaces (for review)

Sure thing! I will prepare whole updated patch series next week.
I hope that it is OK for you.

Daniel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 2/4] multiboot2: Add tags used to pass ImageHandle to loaded image
  2016-03-11 13:27     ` Daniel Kiper
@ 2016-03-11 15:39       ` Vladimir 'phcoder' Serbinenko
  0 siblings, 0 replies; 30+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2016-03-11 15:39 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com


[-- Attachment #1.1: Type: text/plain, Size: 5817 bytes --]

On Friday, March 11, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:

> On Thu, Mar 10, 2016 at 09:26:06PM +0100, Vladimir 'phcoder' Serbinenko
> wrote:
> > On Wednesday, March 2, 2016, Daniel Kiper <daniel.kiper@oracle.com
> <javascript:;>> wrote:
> >
> > > Add tags used to pass ImageHandle to loaded image if requested.
> > > It is used by at least ExitBootServices() function.
> > >
> > > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com <javascript:;>
> <javascript:;>>
> > > ---
> > > v3 - suggestions/fixes:
> > >    - mbi EFI related stuff size calculation
> > >      should depend on target architecture
> > >      (suggested by Konrad Rzeszutek Wilk),
> > >    - use plain type instead of pointer
> > >      dereference as sizeof() argument
> > >      (suggested by Konrad Rzeszutek Wilk),
> > >    - improve commit message
> > >      (suggested by Konrad Rzeszutek Wilk).
> > > ---
> > >  grub-core/loader/multiboot_mbi2.c |   50
> > > ++++++++++++++++++++++++++++++-------
> > >  include/multiboot2.h              |   16 ++++++++++++
> > >  2 files changed, 57 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/grub-core/loader/multiboot_mbi2.c
> > > b/grub-core/loader/multiboot_mbi2.c
> > > index a3dca90..7591edc 100644
> > > --- a/grub-core/loader/multiboot_mbi2.c
> > > +++ b/grub-core/loader/multiboot_mbi2.c
> > > @@ -172,6 +172,8 @@ grub_multiboot_load (grub_file_t file, const char
> > > *filename)
> > >               case MULTIBOOT_TAG_TYPE_NETWORK:
> > >               case MULTIBOOT_TAG_TYPE_EFI_MMAP:
> > >               case MULTIBOOT_TAG_TYPE_EFI_BS:
> > > +             case MULTIBOOT_TAG_TYPE_EFI32_IH:
> > > +             case MULTIBOOT_TAG_TYPE_EFI64_IH:
> > >                 break;
> > >
> > >               default:
> > > @@ -407,16 +409,22 @@ grub_multiboot_get_mbi_size (void)
> > >                  + grub_get_multiboot_mmap_count ()
> > >                  * sizeof (struct multiboot_mmap_entry)),
> > > MULTIBOOT_TAG_ALIGN)
> > >      + ALIGN_UP (sizeof (struct multiboot_tag_framebuffer),
> > > MULTIBOOT_TAG_ALIGN)
> > > +#ifdef GRUB_MACHINE_EFI
> > > +#ifdef __i386__
> > >      + ALIGN_UP (sizeof (struct multiboot_tag_efi32),
> MULTIBOOT_TAG_ALIGN)
> > > +    + ALIGN_UP (sizeof (struct multiboot_tag_efi32_ih),
> > > MULTIBOOT_TAG_ALIGN)
> > > +#endif
> > > +#ifdef __x86_64__
> > >      + ALIGN_UP (sizeof (struct multiboot_tag_efi64),
> MULTIBOOT_TAG_ALIGN)
> > > +    + ALIGN_UP (sizeof (struct multiboot_tag_efi64_ih),
> > > MULTIBOOT_TAG_ALIGN)
> > > +#endif
> > > +    + ALIGN_UP (sizeof (struct multiboot_tag_efi_mmap)
> > > +               + efi_mmap_size, MULTIBOOT_TAG_ALIGN)
> > > +#endif
> > >
> > It doesn't need to be exact. It's just an upper bound. Feel free to
> > simplify knowing this, removing few #ifdef.
>
> OK.
>
> > >      + ALIGN_UP (sizeof (struct multiboot_tag_old_acpi)
> > >                 + sizeof (struct grub_acpi_rsdp_v10),
> MULTIBOOT_TAG_ALIGN)
> > >      + acpiv2_size ()
> > >      + net_size ()
> > > -#ifdef GRUB_MACHINE_EFI
> > > -    + ALIGN_UP (sizeof (struct multiboot_tag_efi_mmap)
> > > -               + efi_mmap_size, MULTIBOOT_TAG_ALIGN)
> > > -#endif
> > >      + sizeof (struct multiboot_tag_vbe) + MULTIBOOT_TAG_ALIGN - 1
> > >      + sizeof (struct multiboot_tag_apm) + MULTIBOOT_TAG_ALIGN - 1;
> > >  }
> > > @@ -907,11 +915,35 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
> > >
> > >    if (keep_bs)
> > >      {
> > > -      struct multiboot_tag *tag = (struct multiboot_tag *) ptrorig;
> > > -      tag->type = MULTIBOOT_TAG_TYPE_EFI_BS;
> > > -      tag->size = sizeof (struct multiboot_tag);
> > > -      ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> > > -       / sizeof (grub_properly_aligned_t);
> > > +      {
> > > +       struct multiboot_tag *tag = (struct multiboot_tag *) ptrorig;
> > > +       tag->type = MULTIBOOT_TAG_TYPE_EFI_BS;
> > > +       tag->size = sizeof (struct multiboot_tag);
> > > +       ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> > > +         / sizeof (grub_properly_aligned_t);
> > > +      }
> > > +
> > > +#ifdef __i386__
> > > +      {
> > > +       struct multiboot_tag_efi32_ih *tag = (struct
> > > multiboot_tag_efi32_ih *) ptrorig;
> > > +       tag->type = MULTIBOOT_TAG_TYPE_EFI32_IH;
> > > +       tag->size = sizeof (struct multiboot_tag_efi32_ih);
> > > +       tag->pointer = (grub_addr_t) grub_efi_image_handle;
> > > +       ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> > > +         / sizeof (grub_properly_aligned_t);
> > > +      }
> > > +#endif
> > > +
> > > +#ifdef __x86_64__
> > > +      {
> > > +       struct multiboot_tag_efi64_ih *tag = (struct
> > > multiboot_tag_efi64_ih *) ptrorig;
> > > +       tag->type = MULTIBOOT_TAG_TYPE_EFI64_IH;
> > > +       tag->size = sizeof (struct multiboot_tag_efi64_ih);
> > > +       tag->pointer = (grub_addr_t) grub_efi_image_handle;
> > > +       ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
> > > +         / sizeof (grub_properly_aligned_t);
> > > +      }
> > > +#endif
> > >      }
> > >  #endif
> > >
> > > diff --git a/include/multiboot2.h b/include/multiboot2.h
> > > index d96aa40..36a174f 100644
> > > --- a/include/multiboot2.h
> > > +++ b/include/multiboot2.h
> > > @@ -60,6 +60,8 @@
> > >  #define MULTIBOOT_TAG_TYPE_NETWORK           16
> > >  #define MULTIBOOT_TAG_TYPE_EFI_MMAP          17
> > >  #define MULTIBOOT_TAG_TYPE_EFI_BS            18
> > > +#define MULTIBOOT_TAG_TYPE_EFI32_IH          19
> > > +#define MULTIBOOT_TAG_TYPE_EFI64_IH          20
> > >
> > Is there a followup to define those in texi doc as well?
>
> No problem. Which one should I update?
>
It's in the branch 'multiboot2' in git:
http://git.savannah.gnu.org/cgit/grub.git/tree/doc/multiboot.texi?h=multiboot2


>
> Daniel
>


-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #1.2: Type: text/html, Size: 7935 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 1/4] i386/relocator: Add grub_relocator64_efi relocator
  2016-03-11 13:23     ` Daniel Kiper
@ 2016-03-11 15:41       ` Vladimir 'phcoder' Serbinenko
  2016-03-11 15:42       ` Vladimir 'phcoder' Serbinenko
  1 sibling, 0 replies; 30+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2016-03-11 15:41 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com


[-- Attachment #1.1: Type: text/plain, Size: 3115 bytes --]

On Friday, March 11, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:

> On Thu, Mar 10, 2016 at 09:23:23PM +0100, Vladimir 'phcoder' Serbinenko
> wrote:
> > On Wednesday, March 2, 2016, Daniel Kiper <daniel.kiper@oracle.com
> <javascript:;>> 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.
>
> OK, as I can see idea in general is accepted. Do you want
> grub_relocator32_efi in 2.02 or 2.03 is OK?
>
> > > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com <javascript:;>
> <javascript:;>>
> > > ---
> > > v3 - suggestions/fixes:
> > >    - reuse grub-core/lib/i386/relocator64.S code
> > >      instead of creating separate assembly file
> > >      (suggested by Vladimir 'phcoder' Serbinenko),
> > >    - grub_multiboot_boot() cleanup
> > >      (suggested by Vladimir 'phcoder' Serbinenko),
> > >    - reuse multiboot_header_tag_entry_address struct instead
> > >      of creating new one for EFI 64-bit entry point
> > >      (suggested by Vladimir 'phcoder' Serbinenko).
> > > ---
> > >  grub-core/lib/i386/relocator.c    |   48
> > > ++++++++++++++++++++++++++++++++++
> > >  grub-core/lib/i386/relocator64.S  |    3 +++
> > >  grub-core/loader/multiboot.c      |   51
> > > +++++++++++++++++++++++++++++++++----
> > >  grub-core/loader/multiboot_mbi2.c |   19 +++++++++++---
> > >  include/grub/i386/multiboot.h     |   11 ++++++++
> > >  include/grub/i386/relocator.h     |   21 +++++++++++++++
> > >  include/multiboot2.h              |    1 +
> > >  7 files changed, 145 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/grub-core/lib/i386/relocator.c
> > > b/grub-core/lib/i386/relocator.c
> > > index 71dd4f0..2b0c260 100644
> > > --- a/grub-core/lib/i386/relocator.c
> > > +++ b/grub-core/lib/i386/relocator.c
> > > @@ -69,6 +69,13 @@ 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;
> > > +#endif
> > > +#endif
> > > +
> > >
> > Can we move this and all to a separate file to avoid too many #ifdef ?
>
> Do you think about grub-core/lib/i386/relocator-efi.c or something like
> that?
>
 grub-core/lib/x86_64/efi/relocator.c would probably fit in naming scheme
the best

> If yes then I do not think it is a problem.
>
> Daniel
>


-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #1.2: Type: text/html, Size: 4225 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 1/4] i386/relocator: Add grub_relocator64_efi relocator
  2016-03-11 13:23     ` Daniel Kiper
  2016-03-11 15:41       ` Vladimir 'phcoder' Serbinenko
@ 2016-03-11 15:42       ` Vladimir 'phcoder' Serbinenko
  2016-03-11 16:40         ` Daniel Kiper
  1 sibling, 1 reply; 30+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2016-03-11 15:42 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com


[-- Attachment #1.1: Type: text/plain, Size: 3112 bytes --]

On Friday, March 11, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:

> On Thu, Mar 10, 2016 at 09:23:23PM +0100, Vladimir 'phcoder' Serbinenko
> wrote:
> > On Wednesday, March 2, 2016, Daniel Kiper <daniel.kiper@oracle.com
> <javascript:;>> 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.
>
> OK, as I can see idea in general is accepted. Do you want
> grub_relocator32_efi in 2.02 or 2.03 is OK?
>
Is there already a user for it? Or someone who is expected to adopt it
shortly?

>
> > > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com <javascript:;>
> <javascript:;>>
> > > ---
> > > v3 - suggestions/fixes:
> > >    - reuse grub-core/lib/i386/relocator64.S code
> > >      instead of creating separate assembly file
> > >      (suggested by Vladimir 'phcoder' Serbinenko),
> > >    - grub_multiboot_boot() cleanup
> > >      (suggested by Vladimir 'phcoder' Serbinenko),
> > >    - reuse multiboot_header_tag_entry_address struct instead
> > >      of creating new one for EFI 64-bit entry point
> > >      (suggested by Vladimir 'phcoder' Serbinenko).
> > > ---
> > >  grub-core/lib/i386/relocator.c    |   48
> > > ++++++++++++++++++++++++++++++++++
> > >  grub-core/lib/i386/relocator64.S  |    3 +++
> > >  grub-core/loader/multiboot.c      |   51
> > > +++++++++++++++++++++++++++++++++----
> > >  grub-core/loader/multiboot_mbi2.c |   19 +++++++++++---
> > >  include/grub/i386/multiboot.h     |   11 ++++++++
> > >  include/grub/i386/relocator.h     |   21 +++++++++++++++
> > >  include/multiboot2.h              |    1 +
> > >  7 files changed, 145 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/grub-core/lib/i386/relocator.c
> > > b/grub-core/lib/i386/relocator.c
> > > index 71dd4f0..2b0c260 100644
> > > --- a/grub-core/lib/i386/relocator.c
> > > +++ b/grub-core/lib/i386/relocator.c
> > > @@ -69,6 +69,13 @@ 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;
> > > +#endif
> > > +#endif
> > > +
> > >
> > Can we move this and all to a separate file to avoid too many #ifdef ?
>
> Do you think about grub-core/lib/i386/relocator-efi.c or something like
> that?
> If yes then I do not think it is a problem.
>
> Daniel
>


-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #1.2: Type: text/html, Size: 4223 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions
  2016-03-11 13:14     ` Daniel Kiper
@ 2016-03-11 15:44       ` Vladimir 'phcoder' Serbinenko
  2016-03-11 16:32         ` Daniel Kiper
  0 siblings, 1 reply; 30+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2016-03-11 15:44 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com


[-- Attachment #1.1: Type: text/plain, Size: 1566 bytes --]

On Friday, March 11, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:

> On Fri, Mar 11, 2016 at 01:27:32PM +0100, Vladimir 'phcoder' Serbinenko
> wrote:
> > On Wednesday, March 9, 2016, Daniel Kiper <daniel.kiper@oracle.com
> <javascript:;>> wrote:
> >
> > > On Wed, Mar 02, 2016 at 05:51:36PM +0100, Daniel Kiper wrote:
> > > > Hi,
> > > >
> > > > This patch series:
> > > >   - enables EFI boot services usage in loaded images
> > > >     by multiboot2 protocol,
> > > >   - add support for multiboot2 protocol compatible
> > > >     relocatable images.
> > > >
> > > > Earlier versions of this patch series are extensively tested
> > > > and used internally at least in Oracle. It should be mentioned
> > > > that this release does not change any functionality introduced
> > > > by earlier releases. It just takes into account comments posted
> > > > by various people.
> > > >
> > > > Hmmm... Ugh... Cough... Is it possible to get this stuff
> > > > into 2.02 train?
> > >
> > > Andrei, Vladimir, ping?
> > >
> > They look reasonable for 2.02. Strictly speaking they're features but
> code
>
> Great! Thanks a lot!
>
> > is well isolated and the alternative is painful. I answered the
> individual
> > mails with review.
>
> I have some questions which I send you shortly.
>
> > Is there a followup with updating multiboot2 docs?
>
> Do you want to have updated docs in 2.02?
>
multiboot2 spec is a rolling update.

>
> By the way, where is multiboot2 doc file, which I should update, in git
> tree?
>
> Daniel
>


-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #1.2: Type: text/html, Size: 2304 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 4/4] multiboot2: Add support for relocatable images
  2016-03-10 20:41   ` Vladimir 'phcoder' Serbinenko
@ 2016-03-11 16:06     ` Daniel Kiper
  2016-03-11 16:13       ` Vladimir 'phcoder' Serbinenko
  0 siblings, 1 reply; 30+ messages in thread
From: Daniel Kiper @ 2016-03-11 16:06 UTC (permalink / raw)
  To: Vladimir 'phcoder' Serbinenko
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com

On Thu, Mar 10, 2016 at 09:41:26PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> On Wednesday, March 2, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> > Currently multiboot2 protocol loads image exactly at address specified in
> > ELF or multiboot2 header. This solution works quite well on legacy BIOS
> > platforms. It is possible because memory regions are placed at predictable
> > addresses (though I was not able to find any spec which says that it is
> > strong requirement, so, it looks that it is just a goodwill of hardware
> > designers). However, EFI platforms are more volatile. Even if required
> > memory regions live at specific addresses then they are sometimes simply
> > not free (e.g. used by boot/runtime services on Dell PowerEdge R820 and
> > OVMF). This means that you are not able to simply set up final image
> > destination on build time. You have to provide method to relocate image
> > contents to real load address which is usually different than load address
> > specified in ELF and multiboot2 headers.
> >
> > This patch provides all needed machinery to do self relocation in image
> > code.
> > First of all GRUB2 reads min_addr (min. load addr), max_addr (max. load
> > addr),
> > align (required image alignment), preference (it says which memory regions
> > are
> > preferred by image, e.g. none, low, high) from
> > multiboot_header_tag_relocatable
> > header tag contained in binary. Later loader tries to fulfill request (not
> > only
> > that one) and if it succeeds then it informs image about real load address
> > via
> > multiboot_tag_base_addr tag. At this stage GRUB2 role is finished. Starting
> > from now executable must cope with relocations itself using whole static
> > and dynamic knowledge provided by boot loader.
> >
> > This patch does not provide functionality which could do relocations using
> > ELF relocation data. However, I was asked by Konrad Rzeszutek Wilk and
> > Vladimir
> > 'phcoder' Serbinenko to investigate that thing. It looks that relevant
> > machinery
> > could be added to existing code (including this patch) without huge effort.
> > Additionally, ELF relocation could live in parallel with self relocation
> > provided
> > by this patch. However, during research I realized that first of all we
> > should
> > establish the details how ELF relocatable image should look like and how
> > it should
> > be build. At least to build proper test/example files.
> >
> > As I saw multiboot2 protocol is able to consume ET_EXEC and ET_DYN ELF
> > files.
> > Potentially we can use ET_DYN file type. It can be build with gcc/ld -pie
> > option.
> > However, it contains a lot of unneeded stuff (e.g. INTERP, DYNAMIC,
> > GNU_EH_FRAME
> > program headers) and it could be quite difficult to drop them (Hmmm... Is
> > it
> > possible to build it properly with custom ld script?).
>
> How big are they? Are they a real problem?

ET_DYN file is ~2.5 times bigger then normal ET_EXEC (I has checked multiboot2.elf
from GRUB2). There is a chance that we can ignore most of stuff in ET_DYN, however,
it does not look nice. IMO, image should have only what is needed by loader.

> > So, I have checked ET_EXEC
> > file type. Sadly in this case linker by default resolves all local symbol
> > relocations
> > and removes relocation related sections. Fortunately it is possible to
> > leave them
> > as is with simple -q/--emit-relocs ld option. However, output file is
> > quite fragile
> > and any operation on it should be done with great care (e.g. strip should
> > be called
> > with --strip-unneeded option). So, this solution is not perfect too. It
> > means that
> > maybe we should look for better solution. However, I think that we should
> > not use
> > any custom tools and focus on functionalities provided by compiler and
> > binutils.
> > In this context ld scripts looks quite promising but maybe you have better
> > solutions.
> > So, what do you think about that?
> >
>  Another possibility is to use intermediary .o files like we do for modules
> and like Linux does for modules AFAIR.

Correct but I think that it would be better to have normal ET_EXEC or ET_DYN file.

> > This patch was tested with Xen image which uses that functionality.
> > However, this Xen
> > feature is still under development and new patchset will be released in
> > about 3-4 weeks.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com <javascript:;>>
> > ---
> > v3 - suggestions/fixes:
> >    - reduce number of casts
> >      (suggested by Konrad Rzeszutek Wilk),
> >    - remove unneeded space at the end of line
> >      (suggested by Konrad Rzeszutek Wilk),
> >    - improve commit message
> >      (suggested by Konrad Rzeszutek Wilk).
> > ---
> >  grub-core/loader/i386/multiboot_mbi.c |    6 ++-
> >  grub-core/loader/multiboot.c          |   12 ++++--
> >  grub-core/loader/multiboot_elfxx.c    |   28 ++++++++++----
> >  grub-core/loader/multiboot_mbi2.c     |   65
> > ++++++++++++++++++++++++++++++---
> >  include/grub/multiboot.h              |    4 +-
> >  include/multiboot2.h                  |   24 ++++++++++++
> >  6 files changed, 120 insertions(+), 19 deletions(-)
> >
> > diff --git a/grub-core/loader/i386/multiboot_mbi.c
> > b/grub-core/loader/i386/multiboot_mbi.c
> > index f60b702..4fc83ed 100644
> > --- a/grub-core/loader/i386/multiboot_mbi.c
> > +++ b/grub-core/loader/i386/multiboot_mbi.c
> > @@ -72,7 +72,8 @@ load_kernel (grub_file_t file, const char *filename,
> >    grub_err_t err;
> >    if (grub_multiboot_quirks & GRUB_MULTIBOOT_QUIRK_BAD_KLUDGE)
> >      {
> > -      err = grub_multiboot_load_elf (file, filename, buffer);
> > +      err = grub_multiboot_load_elf (file, filename, buffer, 0, 0, 0, 0,
> > +                                    GRUB_RELOCATOR_PREFERENCE_NONE, NULL,
> > 0);
> >        if (err == GRUB_ERR_NONE) {
> >         return GRUB_ERR_NONE;
> >        }
> > @@ -121,7 +122,8 @@ load_kernel (grub_file_t file, const char *filename,
> >        return GRUB_ERR_NONE;
> >      }
> >
> > -  return grub_multiboot_load_elf (file, filename, buffer);
> > +  return grub_multiboot_load_elf (file, filename, buffer, 0, 0, 0, 0,
> > +                                 GRUB_RELOCATOR_PREFERENCE_NONE, NULL, 0);
> >  }
> >
> >  static struct multiboot_header *
> > diff --git a/grub-core/loader/multiboot.c b/grub-core/loader/multiboot.c
> > index 18038fd..c0f51b6 100644
> > --- a/grub-core/loader/multiboot.c
> > +++ b/grub-core/loader/multiboot.c
> > @@ -208,12 +208,18 @@ static grub_uint64_t highest_load;
> >  /* Load ELF32 or ELF64.  */
> >  grub_err_t
> >  grub_multiboot_load_elf (grub_file_t file, const char *filename,
> > -                        void *buffer)
> > +                        void *buffer, int relocatable, grub_uint32_t
> > min_addr,
> > +                        grub_uint32_t max_addr, grub_size_t align,
> > grub_uint32_t preference,
> > +                        grub_uint32_t *base_addr, int
> > avoid_efi_boot_services)
> >  {
> >    if (grub_multiboot_is_elf32 (buffer))
> > -    return grub_multiboot_load_elf32 (file, filename, buffer);
> > +    return grub_multiboot_load_elf32 (file, filename, buffer, relocatable,
> > +                                     min_addr, max_addr, align,
> > preference,
> > +                                     base_addr, avoid_efi_boot_services);
> >    else if (grub_multiboot_is_elf64 (buffer))
> > -    return grub_multiboot_load_elf64 (file, filename, buffer);
> > +    return grub_multiboot_load_elf64 (file, filename, buffer, relocatable,
> > +                                     min_addr, max_addr, align,
> > preference,
> > +                                     base_addr, avoid_efi_boot_services);
> >
> >    return grub_error (GRUB_ERR_UNKNOWN_OS, N_("invalid arch-dependent ELF
> > magic"));
> >  }
> > diff --git a/grub-core/loader/multiboot_elfxx.c
> > b/grub-core/loader/multiboot_elfxx.c
> > index e3a39b6..0c01569 100644
> > --- a/grub-core/loader/multiboot_elfxx.c
> > +++ b/grub-core/loader/multiboot_elfxx.c
> > @@ -51,7 +51,10 @@ CONCAT(grub_multiboot_is_elf, XX) (void *buffer)
> >  }
> >
> >  static grub_err_t
> > -CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char
> > *filename, void *buffer)
> > +CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char
> > *filename,
> > +                                    void *buffer, int relocatable,
> > grub_uint32_t min_addr,
> > +                                    grub_uint32_t max_addr, grub_size_t
> > align, grub_uint32_t preference,
> > +                                    grub_uint32_t *base_addr, int
> > avoid_efi_boot_services)
> >  {
> >    Elf_Ehdr *ehdr = (Elf_Ehdr *) buffer;
> >    char *phdr_base;
> > @@ -89,19 +92,30 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file,
> > const char *filename, voi
> >           if (phdr(i)->p_paddr + phdr(i)->p_memsz > highest_load)
> >             highest_load = phdr(i)->p_paddr + phdr(i)->p_memsz;
> >
> > -         grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx,
> > memsz=0x%lx, vaddr=0x%lx\n",
> > -                       i, (long) phdr(i)->p_paddr, (long)
> > phdr(i)->p_memsz, (long) phdr(i)->p_vaddr);
> > +         grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx,
> > memsz=0x%lx, vaddr=0x%lx,"
> > +                       "align=0x%lx, relocatable=%d,
> > avoid_efi_boot_services=%d\n", i,
> > +                       (long) phdr(i)->p_paddr, (long) phdr(i)->p_memsz,
> > (long) phdr(i)->p_vaddr,
> > +                       (long) align, relocatable,
> > avoid_efi_boot_services);
> >
> >           {
> >             grub_relocator_chunk_t ch;
> > -           err = grub_relocator_alloc_chunk_addr
> > (grub_multiboot_relocator,
> > -                                                  &ch, phdr(i)->p_paddr,
> > -                                                  phdr(i)->p_memsz);
> > +
> > +           if (relocatable)
> > +             err = grub_relocator_alloc_chunk_align
> > (grub_multiboot_relocator, &ch,
> > +                                                     min_addr, max_addr -
> > phdr(i)->p_memsz,
> > +                                                     phdr(i)->p_memsz,
> > align ? align : 1,
> > +                                                     preference,
> > avoid_efi_boot_services);
> > +           else
> > +             err = grub_relocator_alloc_chunk_addr
> > (grub_multiboot_relocator,
> > +                                                    &ch, phdr(i)->p_paddr,
> > +                                                    phdr(i)->p_memsz);
> >             if (err)
> >               {
> >                 grub_dprintf ("multiboot_loader", "Error loading phdr
> > %d\n", i);
> >                 return err;
> >               }
> > +           if (base_addr)
> > +             *base_addr = get_physical_target_address (ch);
> >             source = get_virtual_current_address (ch);
> >           }
> >
> > @@ -208,7 +222,7 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file,
> > const char *filename, voi
> >                                                     + 1, sh->sh_size,
> >                                                     sh->sh_addralign,
> >
> > GRUB_RELOCATOR_PREFERENCE_NONE,
> > -                                                   0);
> > +
> >  avoid_efi_boot_services);
> >             if (err)
> >               {
> >                 grub_dprintf ("multiboot_loader", "Error loading shdr
> > %d\n", i);
> > diff --git a/grub-core/loader/multiboot_mbi2.c
> > b/grub-core/loader/multiboot_mbi2.c
> > index ce68f48..03725a1 100644
> > --- a/grub-core/loader/multiboot_mbi2.c
> > +++ b/grub-core/loader/multiboot_mbi2.c
> > @@ -68,6 +68,7 @@ static grub_size_t elf_sec_num, elf_sec_entsize;
> >  static unsigned elf_sec_shstrndx;
> >  static void *elf_sections;
> >  static int keep_bs = 0;
> > +static grub_uint32_t base_addr = 0;
> >
> >  void
> >  grub_multiboot_add_elfsyms (grub_size_t num, grub_size_t entsize,
> > @@ -107,11 +108,14 @@ 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, efi_entry_specified = 0;
> > +  struct multiboot_header_tag_relocatable *rel_tag;
> > +  int entry_specified = 0, efi_entry_specified = 0, relocatable = 0;
> >    grub_addr_t entry = 0, efi_entry = 0;
> > -  grub_uint32_t console_required = 0;
> > +  grub_uint32_t console_required = 0, min_addr = 0;
> > +  grub_uint32_t max_addr = 0, preference = GRUB_RELOCATOR_PREFERENCE_NONE;
> >    struct multiboot_header_tag_framebuffer *fbtag = NULL;
> >    int accepted_consoles = GRUB_MULTIBOOT_CONSOLE_EGA_TEXT;
> > +  grub_size_t align = 0;
> >
> >    buffer = grub_malloc (MULTIBOOT_SEARCH);
> >    if (!buffer)
> > @@ -174,6 +178,7 @@ grub_multiboot_load (grub_file_t file, const char
> > *filename)
> >               case MULTIBOOT_TAG_TYPE_EFI_BS:
> >               case MULTIBOOT_TAG_TYPE_EFI32_IH:
> >               case MULTIBOOT_TAG_TYPE_EFI64_IH:
> > +             case MULTIBOOT_TAG_TYPE_BASE_ADDR:
> >                 break;
> >
> >               default:
> > @@ -215,6 +220,27 @@ grub_multiboot_load (grub_file_t file, const char
> > *filename)
> >         accepted_consoles |= GRUB_MULTIBOOT_CONSOLE_FRAMEBUFFER;
> >         break;
> >
> > +      case MULTIBOOT_HEADER_TAG_RELOCATABLE:
> > +       relocatable = 1;
> > +       rel_tag = (struct multiboot_header_tag_relocatable *) tag;
> > +       min_addr = rel_tag->min_addr;
> > +       max_addr = rel_tag->max_addr;
> > +       align = rel_tag->align;
> > +       switch (rel_tag->preference)
> > +         {
> > +         case MULTIBOOT_LOAD_PREFERENCE_LOW:
> > +           preference = GRUB_RELOCATOR_PREFERENCE_LOW;
> > +           break;
> > +
> > +         case MULTIBOOT_LOAD_PREFERENCE_HIGH:
> > +           preference = GRUB_RELOCATOR_PREFERENCE_HIGH;
> > +           break;
> > +
> > +         default:
> > +           preference = GRUB_RELOCATOR_PREFERENCE_NONE;
> > +         }
> > +       break;
> > +
> >         /* GRUB always page-aligns modules.  */
> >        case MULTIBOOT_HEADER_TAG_MODULE_ALIGN:
> >         break;
> > @@ -260,15 +286,22 @@ grub_multiboot_load (grub_file_t file, const char
> > *filename)
> >        else
> >         code_size = load_size;
> >
> > -      err = grub_relocator_alloc_chunk_addr (grub_multiboot_relocator,
> > -                                            &ch, load_addr,
> > -                                            code_size);
> > +      if (relocatable)
> > +       err = grub_relocator_alloc_chunk_align (grub_multiboot_relocator,
> > &ch,
> > +                                               min_addr, max_addr -
> > code_size,
> > +                                               code_size, align ? align :
> > 1,
> > +                                               preference, keep_bs);
> > +      else
> > +       err = grub_relocator_alloc_chunk_addr (grub_multiboot_relocator,
> > +                                              &ch, load_addr,
> > +                                              code_size);
> >        if (err)
> >         {
> >           grub_dprintf ("multiboot_loader", "Error loading aout kludge\n");
> >           grub_free (buffer);
> >           return err;
> >         }
> > +      base_addr = get_physical_target_address (ch);
> >        source = get_virtual_current_address (ch);
> >
> >        if ((grub_file_seek (file, offset)) == (grub_off_t) -1)
> > @@ -290,7 +323,9 @@ grub_multiboot_load (grub_file_t file, const char
> > *filename)
> >      }
> >    else
> >      {
> > -      err = grub_multiboot_load_elf (file, filename, buffer);
> > +      err = grub_multiboot_load_elf (file, filename, buffer,
> > +                                    relocatable, min_addr, max_addr,
> > +                                    align, preference, &base_addr,
> > keep_bs);
> >        if (err)
> >         {
> >           grub_free (buffer);
> > @@ -303,6 +338,14 @@ grub_multiboot_load (grub_file_t file, const char
> > *filename)
> >    else if (entry_specified)
> >      grub_multiboot_payload_eip = entry;
> >
> > +  if (relocatable)
> > +    {
> > +      if (base_addr > min_addr)
> > +       grub_multiboot_payload_eip += base_addr - min_addr;
> > +      else
> > +       grub_multiboot_payload_eip -= min_addr - base_addr;
> > +    }
> > +
> >
> Why is it relative to min_addr? Sounds like it should be just an offset

Ugh... IIRC, it has meaning but I forgot what. I will check it.
However, this means that I must put comment here.

> from base addr. What do ET_DYN files use?

I will take a look.

Daniel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 4/4] multiboot2: Add support for relocatable images
  2016-03-11 16:06     ` Daniel Kiper
@ 2016-03-11 16:13       ` Vladimir 'phcoder' Serbinenko
  2016-03-14 11:38         ` Daniel Kiper
  0 siblings, 1 reply; 30+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2016-03-11 16:13 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com


[-- Attachment #1.1: Type: text/plain, Size: 759 bytes --]

>
>
> > > +  if (relocatable)
> > > +    {
> > > +      if (base_addr > min_addr)
> > > +       grub_multiboot_payload_eip += base_addr - min_addr;
> > > +      else
> > > +       grub_multiboot_payload_eip -= min_addr - base_addr;
> > > +    }
> > > +
> > >
> > Why is it relative to min_addr? Sounds like it should be just an offset
>
> Ugh... IIRC, it has meaning but I forgot what. I will check it.
> However, this means that I must put comment here.
>

Is it possible that you have confused link address and minimal loading
address? How is entry usually specified in ELF? How do you suggest it
should be done in mb headers?

>
> > from base addr. What do ET_DYN files use?
>
> I will take a look.
>
> Daniel
>


-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #1.2: Type: text/html, Size: 1180 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 4/4] multiboot2: Add support for relocatable images
  2016-03-10 20:44   ` Vladimir 'phcoder' Serbinenko
@ 2016-03-11 16:23     ` Daniel Kiper
  0 siblings, 0 replies; 30+ messages in thread
From: Daniel Kiper @ 2016-03-11 16:23 UTC (permalink / raw)
  To: Vladimir 'phcoder' Serbinenko
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com

On Thu, Mar 10, 2016 at 09:44:31PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> On Wednesday, March 2, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> > Currently multiboot2 protocol loads image exactly at address specified in
> > ELF or multiboot2 header. This solution works quite well on legacy BIOS
> > platforms. It is possible because memory regions are placed at predictable
> > addresses (though I was not able to find any spec which says that it is
> > strong requirement, so, it looks that it is just a goodwill of hardware
> > designers). However, EFI platforms are more volatile. Even if required
> > memory regions live at specific addresses then they are sometimes simply
> > not free (e.g. used by boot/runtime services on Dell PowerEdge R820 and
> > OVMF). This means that you are not able to simply set up final image
> > destination on build time. You have to provide method to relocate image
> > contents to real load address which is usually different than load address
> > specified in ELF and multiboot2 headers.
> >
> > This patch provides all needed machinery to do self relocation in image
> > code.
> > First of all GRUB2 reads min_addr (min. load addr), max_addr (max. load
> > addr),
> > align (required image alignment), preference (it says which memory regions
> > are
> > preferred by image, e.g. none, low, high) from
> > multiboot_header_tag_relocatable
> > header tag contained in binary. Later loader tries to fulfill request (not
> > only
> > that one) and if it succeeds then it informs image about real load address
> > via
> > multiboot_tag_base_addr tag. At this stage GRUB2 role is finished. Starting
> > from now executable must cope with relocations itself using whole static
> > and dynamic knowledge provided by boot loader.
> >
> > This patch does not provide functionality which could do relocations using
> > ELF relocation data.
>
> Can you add a check that image doesn't have any relocation entries? So that
> we fail nicely rather than loading half-working binary?

Make sense. I will do that.

Daniel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions
  2016-03-11 15:44       ` Vladimir 'phcoder' Serbinenko
@ 2016-03-11 16:32         ` Daniel Kiper
  2016-03-11 17:33           ` Vladimir 'phcoder' Serbinenko
  0 siblings, 1 reply; 30+ messages in thread
From: Daniel Kiper @ 2016-03-11 16:32 UTC (permalink / raw)
  To: Vladimir 'phcoder' Serbinenko
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com

On Fri, Mar 11, 2016 at 04:44:00PM +0100, Vladimir 'phcoder' Serbinenko wrote:

[...]

> multiboot2 spec is a rolling update.

OK. So, I think that we should do this in that way:
  - apply this patch series after polishing;
    I hope that it will happen next week,
  - update multiboot2 doc,
  - add grub_relocator32_efi;
    should it be 2.02 goal?
  - agree and add support for ELF relocations;
    should it be 2.02 goal?

Does it make sense?

Daniel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 1/4] i386/relocator: Add grub_relocator64_efi relocator
  2016-03-11 15:42       ` Vladimir 'phcoder' Serbinenko
@ 2016-03-11 16:40         ` Daniel Kiper
  0 siblings, 0 replies; 30+ messages in thread
From: Daniel Kiper @ 2016-03-11 16:40 UTC (permalink / raw)
  To: Vladimir 'phcoder' Serbinenko
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com

On Fri, Mar 11, 2016 at 04:42:27PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> On Friday, March 11, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> > On Thu, Mar 10, 2016 at 09:23:23PM +0100, Vladimir 'phcoder' Serbinenko
> > wrote:
> > > On Wednesday, March 2, 2016, Daniel Kiper <daniel.kiper@oracle.com
> > <javascript:;>> 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.
> >
> > OK, as I can see idea in general is accepted. Do you want
> > grub_relocator32_efi in 2.02 or 2.03 is OK?
> >
> Is there already a user for it? Or someone who is expected to adopt it
> shortly?

I have not heard anything about that. So, safely we can defer it to 2.03.
We should just reserve relevant numbers for required constants and say
somewhere that this feature is not implemented yet.

Daniel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions
  2016-03-11 16:32         ` Daniel Kiper
@ 2016-03-11 17:33           ` Vladimir 'phcoder' Serbinenko
  2016-03-11 17:49             ` Daniel Kiper
  0 siblings, 1 reply; 30+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2016-03-11 17:33 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com


[-- Attachment #1.1: Type: text/plain, Size: 688 bytes --]

On Friday, March 11, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:

> On Fri, Mar 11, 2016 at 04:44:00PM +0100, Vladimir 'phcoder' Serbinenko
> wrote:
>
> [...]
>
> > multiboot2 spec is a rolling update.
>
> OK. So, I think that we should do this in that way:
>   - apply this patch series after polishing;
>     I hope that it will happen next week,

  - update multiboot2 doc,
>   - add grub_relocator32_efi;
>     should it be 2.02 goal?
>
Only if it's small

>   - agree and add support for ELF relocations;
>     should it be 2.02 goal?
>
Full ELF relocations are definitely not for 2.02

>
> Does it make sense?
>
yes

>
> Daniel
>


-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #1.2: Type: text/html, Size: 1459 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions
  2016-03-11 17:33           ` Vladimir 'phcoder' Serbinenko
@ 2016-03-11 17:49             ` Daniel Kiper
  0 siblings, 0 replies; 30+ messages in thread
From: Daniel Kiper @ 2016-03-11 17:49 UTC (permalink / raw)
  To: Vladimir 'phcoder' Serbinenko
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com

On Fri, Mar 11, 2016 at 06:33:15PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> On Friday, March 11, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> > On Fri, Mar 11, 2016 at 04:44:00PM +0100, Vladimir 'phcoder' Serbinenko
> > wrote:
> >
> > [...]
> >
> > > multiboot2 spec is a rolling update.
> >
> > OK. So, I think that we should do this in that way:
> >   - apply this patch series after polishing;
> >     I hope that it will happen next week,
>
>   - update multiboot2 doc,
> >   - add grub_relocator32_efi;
> >     should it be 2.02 goal?
> >
> Only if it's small
>
> >   - agree and add support for ELF relocations;
> >     should it be 2.02 goal?
> >
> Full ELF relocations are definitely not for 2.02
>
> >
> > Does it make sense?
> >
> yes

Great! Stay tuned... I will polish and repost this patch series next week.

Daniel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [GRUB2 PATCH v3 4/4] multiboot2: Add support for relocatable images
  2016-03-11 16:13       ` Vladimir 'phcoder' Serbinenko
@ 2016-03-14 11:38         ` Daniel Kiper
  0 siblings, 0 replies; 30+ messages in thread
From: Daniel Kiper @ 2016-03-14 11:38 UTC (permalink / raw)
  To: Vladimir 'phcoder' Serbinenko
  Cc: jgross, grub-devel, eric.snowberg, arvidjaar, andrew.cooper3,
	cardoe, pgnet.dev, roy.franz, ning.sun, david.vrabel, jbeulich,
	stefano.stabellini, xen-devel, qiaowei.ren,
	richard.l.maliszewski, gang.wei, fu.wei,
	seth.goldberg@oracle.com

On Fri, Mar 11, 2016 at 05:13:19PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> > > > +  if (relocatable)
> > > > +    {
> > > > +      if (base_addr > min_addr)
> > > > +       grub_multiboot_payload_eip += base_addr - min_addr;
> > > > +      else
> > > > +       grub_multiboot_payload_eip -= min_addr - base_addr;
> > > > +    }
> > > > +
> > > >
> > > Why is it relative to min_addr? Sounds like it should be just an offset
> >
> > Ugh... IIRC, it has meaning but I forgot what. I will check it.
> > However, this means that I must put comment here.
> >
>
> Is it possible that you have confused link address and minimal loading address?

Yep, you are right. Fortunately it is quite easy to fix and probably do not
require any changes in Xen image.

> How is entry usually specified in ELF?

IIRC, there is no such thing per se. However, I think that
we should calculate link base address using following formula:

link_base_addr = ~0;

for (i = 0; i < ehdr->e_phnum; i++)
  link_base_addr = min(link_base_addr, phdr(i)->p_paddr);

> How do you suggest it should be done in mb headers?

I think that we can use multiboot_header_tag_address.load_addr
as link_base_addr.

Daniel

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2016-03-14 11:39 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-02 16:51 [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions Daniel Kiper
2016-03-02 16:51 ` [GRUB2 PATCH v3 1/4] i386/relocator: Add grub_relocator64_efi relocator Daniel Kiper
2016-03-10 20:23   ` Vladimir 'phcoder' Serbinenko
2016-03-11 13:23     ` Daniel Kiper
2016-03-11 15:41       ` Vladimir 'phcoder' Serbinenko
2016-03-11 15:42       ` Vladimir 'phcoder' Serbinenko
2016-03-11 16:40         ` Daniel Kiper
2016-03-02 16:51 ` [GRUB2 PATCH v3 2/4] multiboot2: Add tags used to pass ImageHandle to loaded image Daniel Kiper
2016-03-10 20:26   ` Vladimir 'phcoder' Serbinenko
2016-03-11 13:27     ` Daniel Kiper
2016-03-11 15:39       ` Vladimir 'phcoder' Serbinenko
2016-03-02 16:51 ` [GRUB2 PATCH v3 3/4] multiboot2: Do not pass memory maps to image if EFI boot services are enabled Daniel Kiper
2016-03-10 20:28   ` Vladimir 'phcoder' Serbinenko
2016-03-11 13:30     ` Daniel Kiper
2016-03-02 16:51 ` [GRUB2 PATCH v3 4/4] multiboot2: Add support for relocatable images Daniel Kiper
2016-03-04  6:51   ` Juergen Gross
2016-03-10 20:42     ` Vladimir 'phcoder' Serbinenko
2016-03-10 20:41   ` Vladimir 'phcoder' Serbinenko
2016-03-11 16:06     ` Daniel Kiper
2016-03-11 16:13       ` Vladimir 'phcoder' Serbinenko
2016-03-14 11:38         ` Daniel Kiper
2016-03-10 20:44   ` Vladimir 'phcoder' Serbinenko
2016-03-11 16:23     ` Daniel Kiper
2016-03-09 10:48 ` [GRUB2 PATCH v3 0/4] multiboot2: Add two extensions Daniel Kiper
2016-03-11 12:27   ` Vladimir 'phcoder' Serbinenko
2016-03-11 13:14     ` Daniel Kiper
2016-03-11 15:44       ` Vladimir 'phcoder' Serbinenko
2016-03-11 16:32         ` Daniel Kiper
2016-03-11 17:33           ` Vladimir 'phcoder' Serbinenko
2016-03-11 17:49             ` Daniel Kiper

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).