[GRUB2 PATCH v4 0/4] multiboot2: Add two extensions

[GRUB2 PATCH v4 0/4] multiboot2: Add two extensions

[Daniel Kiper]

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.

Daniel

 grub-core/Makefile.core.def           |    1 +
 grub-core/lib/i386/relocator64.S      |   11 ++++
 grub-core/lib/x86_64/efi/relocator.c  |   76 ++++++++++++++++++++++++++
 grub-core/loader/i386/multiboot_mbi.c |   13 ++++-
 grub-core/loader/multiboot.c          |   62 +++++++++++++++++----
 grub-core/loader/multiboot_elfxx.c    |   52 ++++++++++++------
 grub-core/loader/multiboot_mbi2.c     |  232 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------
 include/grub/i386/multiboot.h         |   11 ++++
 include/grub/i386/relocator.h         |   21 +++++++
 include/grub/multiboot.h              |   22 +++++++-
 include/multiboot2.h                  |   41 ++++++++++++++
 11 files changed, 441 insertions(+), 101 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

[GRUB2 PATCH v4 1/4] i386/relocator: Add grub_relocator64_efi relocator

[Daniel Kiper]

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.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
v4 - suggestions/fixes:
   - move EFI platform specific C code to separate file
     (suggested by Vladimir 'phcoder' Serbinenko),
   - add some comments
     (suggested by Vladimir 'phcoder' Serbinenko).

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/Makefile.core.def          |    1 +
 grub-core/lib/i386/relocator64.S     |   11 +++++
 grub-core/lib/x86_64/efi/relocator.c |   76 ++++++++++++++++++++++++++++++++++
 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 +
 8 files changed, 182 insertions(+), 9 deletions(-)
 create mode 100644 grub-core/lib/x86_64/efi/relocator.c

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 58b4208..21ad0dd 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1531,6 +1531,7 @@ module = {
   i386_xen = lib/i386/xen/relocator.S;
   x86_64_xen = lib/x86_64/xen/relocator.S;
   xen = lib/i386/relocator_common_c.c;
+  x86_64_efi = lib/x86_64/efi/relocator.c;
 
   extra_dist = lib/i386/relocator_common.S;
   extra_dist = kern/powerpc/cache_flush.S;
diff --git a/grub-core/lib/i386/relocator64.S b/grub-core/lib/i386/relocator64.S
index e4648d8..75725cf 100644
--- a/grub-core/lib/i386/relocator64.S
+++ b/grub-core/lib/i386/relocator64.S
@@ -73,6 +73,14 @@ VARIABLE(grub_relocator64_rsp)
 
 	movq	%rax, %rsp
 
+	/*
+	 * Here is grub_relocator64_efi_start() entry point.
+	 * Following code is shared between grub_relocator64_efi_start()
+	 * and grub_relocator64_start().
+	 *
+	 * Think twice before changing anything below!!!
+	 */
+VARIABLE(grub_relocator64_efi_start)
 	/* mov imm64, %rax */
 	.byte 	0x48
 	.byte	0xb8
@@ -120,6 +128,9 @@ LOCAL(jump_addr):
 VARIABLE(grub_relocator64_rip)
 	.quad	0
 
+	/* Here grub_relocator64_efi_start() ends. Ufff... */
+VARIABLE(grub_relocator64_efi_end)
+
 #ifndef __x86_64__
 	.p2align	4
 LOCAL(gdt):
diff --git a/grub-core/lib/x86_64/efi/relocator.c b/grub-core/lib/x86_64/efi/relocator.c
new file mode 100644
index 0000000..c93d061
--- /dev/null
+++ b/grub-core/lib/x86_64/efi/relocator.c
@@ -0,0 +1,76 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2009  Free Software Foundation, Inc.
+ *  Copyright (C) 2016  Oracle and/or its affiliates. All rights reserved.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/mm.h>
+#include <grub/misc.h>
+
+#include <grub/types.h>
+#include <grub/err.h>
+
+#include <grub/i386/relocator.h>
+#include <grub/relocator_private.h>
+
+extern grub_uint64_t grub_relocator64_rax;
+extern grub_uint64_t grub_relocator64_rbx;
+extern grub_uint64_t grub_relocator64_rcx;
+extern grub_uint64_t grub_relocator64_rdx;
+extern grub_uint64_t grub_relocator64_rip;
+extern grub_uint64_t grub_relocator64_rsi;
+
+extern grub_uint8_t grub_relocator64_efi_start;
+extern grub_uint8_t grub_relocator64_efi_end;
+
+#define RELOCATOR_SIZEOF(x)	(&grub_relocator##x##_end - &grub_relocator##x##_start)
+
+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;
+}
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 8b8c156..46e7b71 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

Re: [GRUB2 PATCH v4 1/4] i386/relocator: Add grub_relocator64_efi relocator

[Vladimir 'phcoder' Serbinenko]

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

Other than 2 typos (inline). Looks good. Let's give it a day if somebody
wants to object, then I'll commit it

>
>         movq    %rax, %rsp
>
> +       /*
> +        * Here is grub_relocator64_efi_start() entry point.
> +        * Following code is shared between grub_relocator64_efi_start()
> +        * and grub_relocator64_start().
> +        *
> +        * Think twice before changing anything below!!!
> +        */
>
above?

> +       /* Here grub_relocator64_efi_start() ends. Ufff... */
> +VARIABLE(grub_relocator64_efi_end)
> +
>
Probably without _start. Comment probably applies even more here than above.
 Are you ok with me moving ends to the same place when comitting?  This
would make the code less error-prone.


-- 
Regards
Vladimir 'phcoder' Serbinenko

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

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

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

Re: [GRUB2 PATCH v4 1/4] i386/relocator: Add grub_relocator64_efi relocator

[Daniel Kiper]

On Tue, Mar 15, 2016 at 05:00:33PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> Other than 2 typos (inline). Looks good. Let's give it a day if somebody
> wants to object, then I'll commit it
>
> >
> >         movq    %rax, %rsp
> >
> > +       /*
> > +        * Here is grub_relocator64_efi_start() entry point.
> > +        * Following code is shared between grub_relocator64_efi_start()
> > +        * and grub_relocator64_start().
> > +        *
> > +        * Think twice before changing anything below!!!
> > +        */
> >
> above?

Why? Everything outside of grub_relocator64_efi_start (above
grub_relocator64_efi_start) and grub_relocator64_efi_end (below
grub_relocator64_efi_end) is used by one function. So, we can
quite safely change anything there. However, everything between
grub_relocator64_efi_start (__below__ grub_relocator64_efi_start
label) and grub_relocator64_efi_end is owned by two functions. Hence,
every change should take into account that. Am I missing something?

> > +       /* Here grub_relocator64_efi_start() ends. Ufff... */
> > +VARIABLE(grub_relocator64_efi_end)
> > +
> >
> Probably without _start. Comment probably applies even more here than above.

Nope, grub_relocator64_efi_start is entry point, so, grub_relocator64_efi_end
label marks end of grub_relocator64_efi_start() func.

>  Are you ok with me moving ends to the same place when comitting?  This

If you wish. However, I think that grub_relocator64_efi_start() should
contain only code/data which is really used by it. Otherwise, it could
make some confusion. Why unused code/data is owned by grub_relocator64_efi_start()?
Is by design or by mistake?

> would make the code less error-prone.

I am not convinced that it will be less error-prone then. If we wish
that maybe we should not share code in that way... ;-)))

Daniel

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

[GRUB2 PATCH v4 2/4] multiboot2: Add tags used to pass ImageHandle to loaded image

[Daniel Kiper]

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>
---
v4 - suggestions/fixes:
   - reduce number of #ifdefs in grub_multiboot_get_mbi_size()
     (suggested by Vladimir 'phcoder' Serbinenko).

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 |   42 ++++++++++++++++++++++++++++++-------
 include/multiboot2.h              |   16 ++++++++++++++
 2 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
index a3dca90..6c04402 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,13 +409,15 @@ 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_efi32), MULTIBOOT_TAG_ALIGN)
-    + ALIGN_UP (sizeof (struct multiboot_tag_efi64), MULTIBOOT_TAG_ALIGN)
     + 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_efi32), MULTIBOOT_TAG_ALIGN)
+    + ALIGN_UP (sizeof (struct multiboot_tag_efi32_ih), MULTIBOOT_TAG_ALIGN)
+    + ALIGN_UP (sizeof (struct multiboot_tag_efi64), MULTIBOOT_TAG_ALIGN)
+    + ALIGN_UP (sizeof (struct multiboot_tag_efi64_ih), MULTIBOOT_TAG_ALIGN)
     + ALIGN_UP (sizeof (struct multiboot_tag_efi_mmap)
 		+ efi_mmap_size, MULTIBOOT_TAG_ALIGN)
 #endif
@@ -907,11 +911,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 46e7b71..f5bebe1 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

Re: [GRUB2 PATCH v4 2/4] multiboot2: Add tags used to pass ImageHandle to loaded image

[Vladimir 'phcoder' Serbinenko]

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

Looks good. Let's give others a day to comment before committing

On Tuesday, March 15, 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:;>>
> ---
> v4 - suggestions/fixes:
>    - reduce number of #ifdefs in grub_multiboot_get_mbi_size()
>      (suggested by Vladimir 'phcoder' Serbinenko).
>
> 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 |   42
> ++++++++++++++++++++++++++++++-------
>  include/multiboot2.h              |   16 ++++++++++++++
>  2 files changed, 51 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/loader/multiboot_mbi2.c
> b/grub-core/loader/multiboot_mbi2.c
> index a3dca90..6c04402 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,13 +409,15 @@ 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_efi32), MULTIBOOT_TAG_ALIGN)
> -    + ALIGN_UP (sizeof (struct multiboot_tag_efi64), MULTIBOOT_TAG_ALIGN)
>      + 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_efi32), MULTIBOOT_TAG_ALIGN)
> +    + ALIGN_UP (sizeof (struct multiboot_tag_efi32_ih),
> MULTIBOOT_TAG_ALIGN)
> +    + ALIGN_UP (sizeof (struct multiboot_tag_efi64), MULTIBOOT_TAG_ALIGN)
> +    + ALIGN_UP (sizeof (struct multiboot_tag_efi64_ih),
> MULTIBOOT_TAG_ALIGN)
>      + ALIGN_UP (sizeof (struct multiboot_tag_efi_mmap)
>                 + efi_mmap_size, MULTIBOOT_TAG_ALIGN)
>  #endif
> @@ -907,11 +911,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 46e7b71..f5bebe1 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
>
>

-- 
Regards
Vladimir 'phcoder' Serbinenko

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

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

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

Re: [GRUB2 PATCH v4 2/4] multiboot2: Add tags used to pass ImageHandle to loaded image

[Konrad Rzeszutek Wilk]

On Tue, Mar 15, 2016 at 04:25:59PM +0100, Daniel Kiper 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>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> v4 - suggestions/fixes:
>    - reduce number of #ifdefs in grub_multiboot_get_mbi_size()
>      (suggested by Vladimir 'phcoder' Serbinenko).
> 
> 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 |   42 ++++++++++++++++++++++++++++++-------
>  include/multiboot2.h              |   16 ++++++++++++++
>  2 files changed, 51 insertions(+), 7 deletions(-)
> 
> diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
> index a3dca90..6c04402 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,13 +409,15 @@ 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_efi32), MULTIBOOT_TAG_ALIGN)
> -    + ALIGN_UP (sizeof (struct multiboot_tag_efi64), MULTIBOOT_TAG_ALIGN)
>      + 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_efi32), MULTIBOOT_TAG_ALIGN)
> +    + ALIGN_UP (sizeof (struct multiboot_tag_efi32_ih), MULTIBOOT_TAG_ALIGN)
> +    + ALIGN_UP (sizeof (struct multiboot_tag_efi64), MULTIBOOT_TAG_ALIGN)
> +    + ALIGN_UP (sizeof (struct multiboot_tag_efi64_ih), MULTIBOOT_TAG_ALIGN)
>      + ALIGN_UP (sizeof (struct multiboot_tag_efi_mmap)
>  		+ efi_mmap_size, MULTIBOOT_TAG_ALIGN)
>  #endif
> @@ -907,11 +911,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 46e7b71..f5bebe1 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

[GRUB2 PATCH v4 3/4 - FOR REVIEW ONLY] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

[Daniel Kiper]

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 |   17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
index 6c04402..ad1553b 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)
@@ -755,6 +755,7 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
       }
   }
 
+  if (!keep_bs)
     {
       struct multiboot_tag_mmap *tag = (struct multiboot_tag_mmap *) ptrorig;
       grub_fill_multiboot_mmap (tag);
@@ -776,6 +777,7 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
       / sizeof (grub_properly_aligned_t);
   }
 
+  if (!keep_bs)
     {
       struct multiboot_tag_basic_meminfo *tag
 	= (struct multiboot_tag_basic_meminfo *) ptrorig;
@@ -886,21 +888,17 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
     grub_efi_uintn_t efi_desc_size;
     grub_efi_uint32_t efi_desc_version;
 
+    if (!keep_bs)
+      {
 	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");
-      }
+
 	if (err)
 	  return err;
+
 	tag->descr_size = efi_desc_size;
 	tag->descr_vers = efi_desc_version;
 	tag->size = sizeof (*tag) + efi_mmap_size;
@@ -908,6 +906,7 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
 	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

Re: [GRUB2 PATCH v4 3/4 - FOR REVIEW ONLY] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

[Konrad Rzeszutek Wilk]

On Tue, Mar 15, 2016 at 04:26:00PM +0100, Daniel Kiper wrote:
> Do not pass memory maps to image if it asked for EFI boot services.

.. I would change this sentence a bit.

If image requested EFI boot services then skip multiboot2 memory maps.

> 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.

s/would already finish/would have finished/ ?

> If we keep boot services then it is easier to not provide maps. However,

s/easier/safer?/

> if image needs memory maps and they are not provided by bootloader then
> it should get them itself just before ExitBootServices() call.

s/them// ?

That is making an assumption that the user of Multiboot2 + EFI will
do this. Which is OK since only Xen is using it.. but is this
inline with the spec? Can you ignore not providing this information?


That aside - why not sync the multiboot memory map with what
the EFI one will be? Or is it too to complex to move the memory map
creation to a later phase of the bootup?

Wish there was some multboot memory map flag indicating 'STALE-CHECK-EFI'..


> 
> 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 |   17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
> index 6c04402..ad1553b 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)
> @@ -755,6 +755,7 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
>        }
>    }
>  
> +  if (!keep_bs)
>      {
>        struct multiboot_tag_mmap *tag = (struct multiboot_tag_mmap *) ptrorig;
>        grub_fill_multiboot_mmap (tag);
> @@ -776,6 +777,7 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
>        / sizeof (grub_properly_aligned_t);
>    }
>  
> +  if (!keep_bs)
>      {
>        struct multiboot_tag_basic_meminfo *tag
>  	= (struct multiboot_tag_basic_meminfo *) ptrorig;
> @@ -886,21 +888,17 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
>      grub_efi_uintn_t efi_desc_size;
>      grub_efi_uint32_t efi_desc_version;
>  
> +    if (!keep_bs)
> +      {
>  	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");
> -      }
> +
>  	if (err)
>  	  return err;
> +
>  	tag->descr_size = efi_desc_size;
>  	tag->descr_vers = efi_desc_version;
>  	tag->size = sizeof (*tag) + efi_mmap_size;
> @@ -908,6 +906,7 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
>  	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

Re: [GRUB2 PATCH v4 3/4 - FOR REVIEW ONLY] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

[Daniel Kiper]

On Tue, Mar 15, 2016 at 07:46:46PM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Mar 15, 2016 at 04:26:00PM +0100, Daniel Kiper wrote:
> > Do not pass memory maps to image if it asked for EFI boot services.
>
> .. I would change this sentence a bit.
>
> If image requested EFI boot services then skip multiboot2 memory maps.
>
> > 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.
>
> s/would already finish/would have finished/ ?
>
> > If we keep boot services then it is easier to not provide maps. However,
>
> s/easier/safer?/
>
> > if image needs memory maps and they are not provided by bootloader then
> > it should get them itself just before ExitBootServices() call.
>
> s/them// ?
>
> That is making an assumption that the user of Multiboot2 + EFI will
> do this. Which is OK since only Xen is using it.. but is this
> inline with the spec? Can you ignore not providing this information?

AIUI, spec does not require that anything must be provided. Of course
on every platform GRUB2 should provide minimal set of system information
to properly boot loaded image. However, docs does not say which set make
sense or not. This is role of image to know what it requires to properly
run on a given platform. And I think that it make sense.

> That aside - why not sync the multiboot memory map with what
> the EFI one will be? Or is it too to complex to move the memory map
> creation to a later phase of the bootup?

IIRC, GRUB2 does some allocations after getting memory map and it is
quite complicated to change that.

> Wish there was some multboot memory map flag indicating 'STALE-CHECK-EFI'..

Why provide map which is invalid and must be verified with something else?
Let's ignore (do not provide) invalid data and use correct one without
any additional (unneeded) checks.

Daniel

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

Re: [GRUB2 PATCH v4 3/4 - FOR REVIEW ONLY] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

[Toomas Soome]

> On 16. märts 2016, at 12:02, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
> On Tue, Mar 15, 2016 at 07:46:46PM -0400, Konrad Rzeszutek Wilk wrote:
>> On Tue, Mar 15, 2016 at 04:26:00PM +0100, Daniel Kiper wrote:
>>> Do not pass memory maps to image if it asked for EFI boot services.
>> 
>> .. I would change this sentence a bit.
>> 
>> If image requested EFI boot services then skip multiboot2 memory maps.
>> 
>>> 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.
>> 
>> s/would already finish/would have finished/ ?
>> 
>>> If we keep boot services then it is easier to not provide maps. However,
>> 
>> s/easier/safer?/
>> 
>>> if image needs memory maps and they are not provided by bootloader then
>>> it should get them itself just before ExitBootServices() call.
>> 
>> s/them// ?
>> 
>> That is making an assumption that the user of Multiboot2 + EFI will
>> do this. Which is OK since only Xen is using it.. but is this
>> inline with the spec? Can you ignore not providing this information?
> 
> AIUI, spec does not require that anything must be provided. Of course
> on every platform GRUB2 should provide minimal set of system information
> to properly boot loaded image. However, docs does not say which set make
> sense or not. This is role of image to know what it requires to properly
> run on a given platform. And I think that it make sense.
> 
>> That aside - why not sync the multiboot memory map with what
>> the EFI one will be? Or is it too to complex to move the memory map
>> creation to a later phase of the bootup?
> 
> IIRC, GRUB2 does some allocations after getting memory map and it is
> quite complicated to change that.
> 
>> Wish there was some multboot memory map flag indicating 'STALE-CHECK-EFI'..
> 
> Why provide map which is invalid and must be verified with something else?
> Let's ignore (do not provide) invalid data and use correct one without
> any additional (unneeded) checks.
> 

If you are *not* calling exit efi boot services from grub, there is no sense to provide EFI memory map at all, because to exit boot services [from OS], you *have to* fetch current memory map to get the map key to exit boot services.

basically it means, if BS is not disabled and grub is still providing EFI memory map, the OS has to assume this map is not valid - which is bad and better not to provide (potentially) invalid data in first place.

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

Re: [GRUB2 PATCH v4 3/4 - FOR REVIEW ONLY] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

[Vladimir 'phcoder' Serbinenko]

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

On Wednesday, March 16, 2016, Toomas Soome <tsoome@me.com> wrote:

>
> > On 16. märts 2016, at 12:02, Daniel Kiper <daniel.kiper@oracle.com
> <javascript:;>> wrote:
> >
> > On Tue, Mar 15, 2016 at 07:46:46PM -0400, Konrad Rzeszutek Wilk wrote:
> >> On Tue, Mar 15, 2016 at 04:26:00PM +0100, Daniel Kiper wrote:
> >>> Do not pass memory maps to image if it asked for EFI boot services.
> >>
> >> .. I would change this sentence a bit.
> >>
> >> If image requested EFI boot services then skip multiboot2 memory maps.
> >>
> >>> 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.
> >>
> >> s/would already finish/would have finished/ ?
> >>
> >>> If we keep boot services then it is easier to not provide maps.
> However,
> >>
> >> s/easier/safer?/
> >>
> >>> if image needs memory maps and they are not provided by bootloader then
> >>> it should get them itself just before ExitBootServices() call.
> >>
> >> s/them// ?
> >>
> >> That is making an assumption that the user of Multiboot2 + EFI will
> >> do this. Which is OK since only Xen is using it.. but is this
> >> inline with the spec? Can you ignore not providing this information?
> >
> > AIUI, spec does not require that anything must be provided. Of course
> > on every platform GRUB2 should provide minimal set of system information
> > to properly boot loaded image. However, docs does not say which set make
> > sense or not. This is role of image to know what it requires to properly
> > run on a given platform. And I think that it make sense.
> >
> >> That aside - why not sync the multiboot memory map with what
> >> the EFI one will be? Or is it too to complex to move the memory map
> >> creation to a later phase of the bootup?
> >
> > IIRC, GRUB2 does some allocations after getting memory map and it is
> > quite complicated to change that.
> >
> >> Wish there was some multboot memory map flag indicating
> 'STALE-CHECK-EFI'..
> >
> > Why provide map which is invalid and must be verified with something
> else?
> > Let's ignore (do not provide) invalid data and use correct one without
> > any additional (unneeded) checks.
> >
>
> If you are *not* calling exit efi boot services from grub, there is no
> sense to provide EFI memory map at all, because to exit boot services [from
> OS], you *have to* fetch current memory map to get the map key to exit boot
> services.
>
But this doesn't apply for e820-like and simple maps which are unaffected
by bootloader allocations

>
> basically it means, if BS is not disabled and grub is still providing EFI
> memory map, the OS has to assume this map is not valid - which is bad and
> better not to provide (potentially) invalid data in first place.
>
> rgds,
> toomas



-- 
Regards
Vladimir 'phcoder' Serbinenko

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

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

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

Re: [GRUB2 PATCH v4 3/4 - FOR REVIEW ONLY] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

[Konrad Rzeszutek Wilk]

On Wed, Mar 16, 2016 at 11:39:55AM +0100, Vladimir 'phcoder' Serbinenko wrote:
> On Wednesday, March 16, 2016, Toomas Soome <tsoome@me.com> wrote:
> 
> >
> > > On 16. märts 2016, at 12:02, Daniel Kiper <daniel.kiper@oracle.com
> > <javascript:;>> wrote:
> > >
> > > On Tue, Mar 15, 2016 at 07:46:46PM -0400, Konrad Rzeszutek Wilk wrote:
> > >> On Tue, Mar 15, 2016 at 04:26:00PM +0100, Daniel Kiper wrote:
> > >>> Do not pass memory maps to image if it asked for EFI boot services.
> > >>
> > >> .. I would change this sentence a bit.
> > >>
> > >> If image requested EFI boot services then skip multiboot2 memory maps.
> > >>
> > >>> 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.
> > >>
> > >> s/would already finish/would have finished/ ?
> > >>
> > >>> If we keep boot services then it is easier to not provide maps.
> > However,
> > >>
> > >> s/easier/safer?/
> > >>
> > >>> if image needs memory maps and they are not provided by bootloader then
> > >>> it should get them itself just before ExitBootServices() call.
> > >>
> > >> s/them// ?
> > >>
> > >> That is making an assumption that the user of Multiboot2 + EFI will
> > >> do this. Which is OK since only Xen is using it.. but is this
> > >> inline with the spec? Can you ignore not providing this information?
> > >
> > > AIUI, spec does not require that anything must be provided. Of course
> > > on every platform GRUB2 should provide minimal set of system information
> > > to properly boot loaded image. However, docs does not say which set make
> > > sense or not. This is role of image to know what it requires to properly
> > > run on a given platform. And I think that it make sense.
> > >
> > >> That aside - why not sync the multiboot memory map with what
> > >> the EFI one will be? Or is it too to complex to move the memory map
> > >> creation to a later phase of the bootup?
> > >
> > > IIRC, GRUB2 does some allocations after getting memory map and it is
> > > quite complicated to change that.
> > >
> > >> Wish there was some multboot memory map flag indicating
> > 'STALE-CHECK-EFI'..
> > >
> > > Why provide map which is invalid and must be verified with something
> > else?
> > > Let's ignore (do not provide) invalid data and use correct one without
> > > any additional (unneeded) checks.
> > >
> >
> > If you are *not* calling exit efi boot services from grub, there is no
> > sense to provide EFI memory map at all, because to exit boot services [from
> > OS], you *have to* fetch current memory map to get the map key to exit boot
> > services.
> >
> But this doesn't apply for e820-like and simple maps which are unaffected
> by bootloader allocations

Right. I think we are all on the same page then.

If EFI and payload (OS) has requested to be the one executing ExitBootServices
then don't provide the memory map?

But if the payload hasn't specified what to do with ExitBootServices, GRUB
is free to do so and provide the map.

> 
> >
> > basically it means, if BS is not disabled and grub is still providing EFI
> > memory map, the OS has to assume this map is not valid - which is bad and
> > better not to provide (potentially) invalid data in first place.

Right.

And the description of the patch (to my reading) was not exactly clear on this.

Perhaps just updating the description of the patch and repositing as reply
here would suffice?
> >
> > rgds,
> > toomas
> 
> 
> 
> -- 
> Regards
> Vladimir 'phcoder' Serbinenko

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

[GRUB2 PATCH v4 3/4 - FOR COMMIT] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

[Daniel Kiper]

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 6c04402..ad1553b 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)
@@ -755,12 +755,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
@@ -776,18 +777,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;
@@ -886,27 +888,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

Re: [GRUB2 PATCH v4 3/4 - FOR REVIEW ONLY] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

[Vladimir 'phcoder' Serbinenko]

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

Looks good. Let's give a day for others to comment. Is the second email the
version for commit?

On Tuesday, March 15, 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.
>
> 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 6c04402..ad1553b 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)
> @@ -755,12 +755,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
> @@ -776,18 +777,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;
> @@ -886,27 +888,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: 6412 bytes --]

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

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

Re: [GRUB2 PATCH v4 3/4 - FOR REVIEW ONLY] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

[Andrei Borzenkov]

15.03.2016 19:07, Vladimir 'phcoder' Serbinenko пишет:
> Looks good. Let's give a day for others to comment. Is the second email the
> version for commit?
> 
> On Tuesday, March 15, 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.
>>

Are there any existing users of it that rely on memory map provided by
bootloader? What if image explicitly requested non-optional memory map?
According to multiboot specification it is valid and bootloader must
either provide requested information or fail load.

>> 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 6c04402..ad1553b 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)
>> @@ -755,12 +755,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
>> @@ -776,18 +777,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;
>> @@ -886,27 +888,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

Re: [GRUB2 PATCH v4 3/4 - FOR REVIEW ONLY] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

[Vladimir 'phcoder' Serbinenko]

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

Le mar. 15 mars 2016 19:06, Andrei Borzenkov <arvidjaar@gmail.com> a écrit :

> 15.03.2016 19:07, Vladimir 'phcoder' Serbinenko пишет:
> > Looks good. Let's give a day for others to comment. Is the second email
> the
> > version for commit?
> >
> > On Tuesday, March 15, 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.
> >>
>
> Are there any existing users of it that rely on memory map provided by
> bootloader? What if image explicitly requested non-optional memory map?
> According to multiboot specification it is valid and bootloader must
> either provide requested information or fail load.
>
I think you have a point. @Daniel, is current behavior a problem for you?
Don't you just ignore the memory map in this case?

>
> >> 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 6c04402..ad1553b 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)
> >> @@ -755,12 +755,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
> >> @@ -776,18 +777,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;
> >> @@ -886,27 +888,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
> >>
> >>
> >
>
>

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

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

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

Re: [GRUB2 PATCH v4 3/4 - FOR REVIEW ONLY] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

[Daniel Kiper]

On Tue, Mar 15, 2016 at 06:10:48PM +0000, Vladimir 'phcoder' Serbinenko wrote:
> Le mar. 15 mars 2016 19:06, Andrei Borzenkov <arvidjaar@gmail.com> a écrit :
> > 15.03.2016 19:07, Vladimir 'phcoder' Serbinenko пишет:
> > > Looks good. Let's give a day for others to comment. Is the second email
> > the
> > > version for commit?
> > >
> > > On Tuesday, March 15, 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.
> > >>
> >
> > Are there any existing users of it that rely on memory map provided by
> > bootloader? What if image explicitly requested non-optional memory map?
> > According to multiboot specification it is valid and bootloader must
> > either provide requested information or fail load.
> >
> I think you have a point. @Daniel, is current behavior a problem for you?

Do you think about MULTIBOOT_HEADER_TAG_INFORMATION_REQUEST tag? If yes it
looks that multiboot2 doc is a bit imprecise and/or GRUB2 code is wrong.

multiboot2 doc says:

[...]

If this tag is present and ‘optional’ is set to ‘0’ information conveyed by
requested tag types must be present. If bootloader is unable to supply this
information it must fail with an error
Note: it doesn’t garantee that any tags of type ‘mbi_tag_types’ will actually
be present. E.g. on a videoless system even if you requested tag ‘8’ no tags
of type ‘8’ will be present in mbi.

[...]

However, grub_multiboot_load() just looks for MULTIBOOT_HEADER_TAG_INFORMATION_REQUEST
tag. If it exists and if ‘optional’ is set to ‘0’ then grub_multiboot_load() verify
list of requested tags. If unknown value is found then multiboot2 command fails
with "grub_error (GRUB_ERR_UNKNOWN_OS, "unsupported information tag: 0x%x", ...".
I cannot find additional checks anywhere...

So, AIUI, "If bootloader is unable to supply..." should be read as "If bootloader
does not support..." or even "If bootloader do not understand..." or something
like that. This does not mean that bootloader must have requested data in hand
and provide it to image. It means that bootloader must understand what image ask for.
And in this term my patch does not violate spec implied by implementation (well,
multiboot2 doc makes some confusion; see below). Image may ask/request for any
memory map. However, if a given map (or set of maps) is not available or it could
be provided but could make confusion in some cases then it can/should not be provided.

Anyway, I think that maybe multiboot2 doc should be fixed and more precise
MULTIBOOT_HEADER_TAG_INFORMATION_REQUEST tag description should be provided.
Or GRUB2 implementation is wrong then...

> Don't you just ignore the memory map in this case?

Yes to some extent. However, I think that loader should not provide
data which is known as (potentially) incorrect/faulty.

Daniel

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

Re: [GRUB2 PATCH v4 3/4 - FOR REVIEW ONLY] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

[Daniel Kiper]

On Tue, Mar 15, 2016 at 05:07:43PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> Looks good. Let's give a day for others to comment. Is the second email the
> version for commit?

Yep, as you asked for.

Daniel

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

[GRUB2 PATCH v4 4/4] multiboot2: Add support for relocatable images

[Daniel Kiper]

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 just 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 (at this stage load addresses from multiboot2
and/or ELF headers are ignored). 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_load_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.

So, this patch just provides support for self relocatable images. If ELF file
with relocs is loaded then GRUB2 complains loudly and ignores it. Support for
such files will be added later.

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>
---
v4 - suggestions/fixes:
   - pack grub_multiboot_load_elf() arguments into struct
     (suggested by Juergen Gross, Konrad Rzeszutek Wilk
     and Vladimir 'phcoder' Serbinenko),
   - fix entry point address calculation
     (suggested by Vladimir 'phcoder' Serbinenko),
   - fail if ELF file has relocs sections
     (suggested by Vladimir 'phcoder' Serbinenko).

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 |   13 ++++-
 grub-core/loader/multiboot.c          |   11 ++--
 grub-core/loader/multiboot_elfxx.c    |   52 +++++++++++------
 grub-core/loader/multiboot_mbi2.c     |   98 ++++++++++++++++++++++++++-------
 include/grub/multiboot.h              |   22 +++++++-
 include/multiboot2.h                  |   24 ++++++++
 6 files changed, 172 insertions(+), 48 deletions(-)

diff --git a/grub-core/loader/i386/multiboot_mbi.c b/grub-core/loader/i386/multiboot_mbi.c
index f60b702..fd7b41b 100644
--- a/grub-core/loader/i386/multiboot_mbi.c
+++ b/grub-core/loader/i386/multiboot_mbi.c
@@ -70,9 +70,18 @@ load_kernel (grub_file_t file, const char *filename,
 	     char *buffer, struct multiboot_header *header)
 {
   grub_err_t err;
+  mbi_load_data_t mld;
+
+  mld.file = file;
+  mld.filename = filename;
+  mld.buffer = buffer;
+  mld.mbi_ver = 1;
+  mld.relocatable = 0;
+  mld.avoid_efi_boot_services = 0;
+
   if (grub_multiboot_quirks & GRUB_MULTIBOOT_QUIRK_BAD_KLUDGE)
     {
-      err = grub_multiboot_load_elf (file, filename, buffer);
+      err = grub_multiboot_load_elf (&mld);
       if (err == GRUB_ERR_NONE) {
 	return GRUB_ERR_NONE;
       }
@@ -121,7 +130,7 @@ 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 (&mld);
 }
 
 static struct multiboot_header *
diff --git a/grub-core/loader/multiboot.c b/grub-core/loader/multiboot.c
index 18038fd..bd9d5b3 100644
--- a/grub-core/loader/multiboot.c
+++ b/grub-core/loader/multiboot.c
@@ -207,13 +207,12 @@ 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)
+grub_multiboot_load_elf (mbi_load_data_t *mld)
 {
-  if (grub_multiboot_is_elf32 (buffer))
-    return grub_multiboot_load_elf32 (file, filename, buffer);
-  else if (grub_multiboot_is_elf64 (buffer))
-    return grub_multiboot_load_elf64 (file, filename, buffer);
+  if (grub_multiboot_is_elf32 (mld->buffer))
+    return grub_multiboot_load_elf32 (mld);
+  else if (grub_multiboot_is_elf64 (mld->buffer))
+    return grub_multiboot_load_elf64 (mld);
 
   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..13b418e 100644
--- a/grub-core/loader/multiboot_elfxx.c
+++ b/grub-core/loader/multiboot_elfxx.c
@@ -51,9 +51,9 @@ 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) (mbi_load_data_t *mld)
 {
-  Elf_Ehdr *ehdr = (Elf_Ehdr *) buffer;
+  Elf_Ehdr *ehdr = (Elf_Ehdr *) mld->buffer;
   char *phdr_base;
   int i;
 
@@ -75,9 +75,11 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char *filename, voi
   if (ehdr->e_phoff + (grub_uint32_t) ehdr->e_phnum * ehdr->e_phentsize > MULTIBOOT_SEARCH)
     return grub_error (GRUB_ERR_BAD_OS, "program header at a too high offset");
 
-  phdr_base = (char *) buffer + ehdr->e_phoff;
+  phdr_base = (char *) mld->buffer + ehdr->e_phoff;
 #define phdr(i)			((Elf_Phdr *) (phdr_base + (i) * ehdr->e_phentsize))
 
+  mld->link_base_addr = ~0;
+
   /* Load every loadable segment in memory.  */
   for (i = 0; i < ehdr->e_phnum; i++)
     {
@@ -89,34 +91,45 @@ 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) mld->align, mld->relocatable, mld->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 (mld->relocatable)
+	      err = grub_relocator_alloc_chunk_align (grub_multiboot_relocator, &ch,
+						      mld->min_addr, mld->max_addr - phdr(i)->p_memsz,
+						      phdr(i)->p_memsz, mld->align ? mld->align : 1,
+						      mld->preference, mld->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;
 	      }
+	    mld->link_base_addr = grub_min (mld->link_base_addr, phdr(i)->p_paddr);
+	    mld->load_base_addr = get_physical_target_address (ch);
 	    source = get_virtual_current_address (ch);
 	  }
 
 	  if (phdr(i)->p_filesz != 0)
 	    {
-	      if (grub_file_seek (file, (grub_off_t) phdr(i)->p_offset)
+	      if (grub_file_seek (mld->file, (grub_off_t) phdr(i)->p_offset)
 		  == (grub_off_t) -1)
 		return grub_errno;
 
-	      if (grub_file_read (file, source, phdr(i)->p_filesz)
+	      if (grub_file_read (mld->file, source, phdr(i)->p_filesz)
 		  != (grub_ssize_t) phdr(i)->p_filesz)
 		{
 		  if (!grub_errno)
 		    grub_error (GRUB_ERR_FILE_READ_ERROR, N_("premature end of file %s"),
-				filename);
+				mld->filename);
 		  return grub_errno;
 		}
 	    }
@@ -168,18 +181,18 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char *filename, voi
       if (!shdr)
 	return grub_errno;
       
-      if (grub_file_seek (file, ehdr->e_shoff) == (grub_off_t) -1)
+      if (grub_file_seek (mld->file, ehdr->e_shoff) == (grub_off_t) -1)
 	{
 	  grub_free (shdr);
 	  return grub_errno;
 	}
 
-      if (grub_file_read (file, shdr, (grub_uint32_t) ehdr->e_shnum * ehdr->e_shentsize)
+      if (grub_file_read (mld->file, shdr, (grub_uint32_t) ehdr->e_shnum * ehdr->e_shentsize)
               != (grub_ssize_t) ehdr->e_shnum * ehdr->e_shentsize)
 	{
 	  if (!grub_errno)
 	    grub_error (GRUB_ERR_FILE_READ_ERROR, N_("premature end of file %s"),
-			filename);
+			mld->filename);
 	  return grub_errno;
 	}
       
@@ -191,6 +204,9 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char *filename, voi
 	  grub_addr_t target;
 	  grub_err_t err;
 
+	  if (mld->mbi_ver >= 2 && (sh->sh_type == SHT_REL || sh->sh_type == SHT_RELA))
+	    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "ELF files with relocs are not supported yet");
+
 	  /* This section is a loaded section,
 	     so we don't care.  */
 	  if (sh->sh_addr != 0)
@@ -208,7 +224,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);
+						    mld->avoid_efi_boot_services);
 	    if (err)
 	      {
 		grub_dprintf ("multiboot_loader", "Error loading shdr %d\n", i);
@@ -218,15 +234,15 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, const char *filename, voi
 	    target = get_physical_target_address (ch);
 	  }
 
-	  if (grub_file_seek (file, sh->sh_offset) == (grub_off_t) -1)
+	  if (grub_file_seek (mld->file, sh->sh_offset) == (grub_off_t) -1)
 	    return grub_errno;
 
-          if (grub_file_read (file, src, sh->sh_size)
+          if (grub_file_read (mld->file, src, sh->sh_size)
               != (grub_ssize_t) sh->sh_size)
 	    {
 	      if (!grub_errno)
 		grub_error (GRUB_ERR_FILE_READ_ERROR, N_("premature end of file %s"),
-			    filename);
+			    mld->filename);
 	      return grub_errno;
 	    }
 	  sh->sh_addr = target;
diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
index ad1553b..ed7ca72 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 load_base_addr;
 
 void
 grub_multiboot_add_elfsyms (grub_size_t num, grub_size_t entsize,
@@ -101,36 +102,40 @@ find_header (grub_properly_aligned_t *buffer, grub_ssize_t len)
 grub_err_t
 grub_multiboot_load (grub_file_t file, const char *filename)
 {
-  grub_properly_aligned_t *buffer;
   grub_ssize_t len;
   struct multiboot_header *header;
   grub_err_t err;
   struct multiboot_header_tag *tag;
   struct multiboot_header_tag_address *addr_tag = NULL;
+  struct multiboot_header_tag_relocatable *rel_tag;
   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;
+  mbi_load_data_t mld;
 
-  buffer = grub_malloc (MULTIBOOT_SEARCH);
-  if (!buffer)
+  mld.mbi_ver = 2;
+  mld.relocatable = 0;
+
+  mld.buffer = grub_malloc (MULTIBOOT_SEARCH);
+  if (!mld.buffer)
     return grub_errno;
 
-  len = grub_file_read (file, buffer, MULTIBOOT_SEARCH);
+  len = grub_file_read (file, mld.buffer, MULTIBOOT_SEARCH);
   if (len < 32)
     {
-      grub_free (buffer);
+      grub_free (mld.buffer);
       return grub_error (GRUB_ERR_BAD_OS, N_("premature end of file %s"), filename);
     }
 
   COMPILE_TIME_ASSERT (MULTIBOOT_HEADER_ALIGN % 4 == 0);
 
-  header = find_header (buffer, len);
+  header = find_header (mld.buffer, len);
 
   if (header == 0)
     {
-      grub_free (buffer);
+      grub_free (mld.buffer);
       return grub_error (GRUB_ERR_BAD_ARGUMENT, "no multiboot header found");
     }
 
@@ -174,10 +179,11 @@ 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_LOAD_BASE_ADDR:
 		break;
 
 	      default:
-		grub_free (buffer);
+		grub_free (mld.buffer);
 		return grub_error (GRUB_ERR_UNKNOWN_OS,
 				   "unsupported information tag: 0x%x",
 				   request_tag->requests[i]);
@@ -215,6 +221,27 @@ grub_multiboot_load (grub_file_t file, const char *filename)
 	accepted_consoles |= GRUB_MULTIBOOT_CONSOLE_FRAMEBUFFER;
 	break;
 
+      case MULTIBOOT_HEADER_TAG_RELOCATABLE:
+	mld.relocatable = 1;
+	rel_tag = (struct multiboot_header_tag_relocatable *) tag;
+	mld.min_addr = rel_tag->min_addr;
+	mld.max_addr = rel_tag->max_addr;
+	mld.align = rel_tag->align;
+	switch (rel_tag->preference)
+	  {
+	  case MULTIBOOT_LOAD_PREFERENCE_LOW:
+	    mld.preference = GRUB_RELOCATOR_PREFERENCE_LOW;
+	    break;
+
+	  case MULTIBOOT_LOAD_PREFERENCE_HIGH:
+	    mld.preference = GRUB_RELOCATOR_PREFERENCE_HIGH;
+	    break;
+
+	  default:
+	    mld.preference = GRUB_RELOCATOR_PREFERENCE_NONE;
+	  }
+	break;
+
 	/* GRUB always page-aligns modules.  */
       case MULTIBOOT_HEADER_TAG_MODULE_ALIGN:
 	break;
@@ -228,7 +255,7 @@ grub_multiboot_load (grub_file_t file, const char *filename)
       default:
 	if (! (tag->flags & MULTIBOOT_HEADER_TAG_OPTIONAL))
 	  {
-	    grub_free (buffer);
+	    grub_free (mld.buffer);
 	    return grub_error (GRUB_ERR_UNKNOWN_OS,
 			       "unsupported tag: 0x%x", tag->type);
 	  }
@@ -237,7 +264,7 @@ grub_multiboot_load (grub_file_t file, const char *filename)
 
   if (addr_tag && !entry_specified && !(keep_bs && efi_entry_specified))
     {
-      grub_free (buffer);
+      grub_free (mld.buffer);
       return grub_error (GRUB_ERR_UNKNOWN_OS,
 			 "load address tag without entry address tag");
     }
@@ -246,8 +273,8 @@ grub_multiboot_load (grub_file_t file, const char *filename)
     {
       grub_uint64_t load_addr = (addr_tag->load_addr + 1)
 	? addr_tag->load_addr : (addr_tag->header_addr
-				 - ((char *) header - (char *) buffer));
-      int offset = ((char *) header - (char *) buffer -
+				 - ((char *) header - (char *) mld.buffer));
+      int offset = ((char *) header - (char *) mld.buffer -
 	   (addr_tag->header_addr - load_addr));
       int load_size = ((addr_tag->load_end_addr == 0) ? file->size - offset :
 		       addr_tag->load_end_addr - addr_tag->load_addr);
@@ -260,27 +287,35 @@ 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 (mld.relocatable)
+	err = grub_relocator_alloc_chunk_align (grub_multiboot_relocator, &ch,
+						mld.min_addr, mld.max_addr - code_size,
+						code_size, mld.align ? mld.align : 1,
+						mld.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);
+	  grub_free (mld.buffer);
 	  return err;
 	}
+      mld.link_base_addr = load_addr;
+      mld.load_base_addr = get_physical_target_address (ch);
       source = get_virtual_current_address (ch);
 
       if ((grub_file_seek (file, offset)) == (grub_off_t) -1)
 	{
-	  grub_free (buffer);
+	  grub_free (mld.buffer);
 	  return grub_errno;
 	}
 
       grub_file_read (file, source, load_size);
       if (grub_errno)
 	{
-	  grub_free (buffer);
+	  grub_free (mld.buffer);
 	  return grub_errno;
 	}
 
@@ -290,19 +325,32 @@ grub_multiboot_load (grub_file_t file, const char *filename)
     }
   else
     {
-      err = grub_multiboot_load_elf (file, filename, buffer);
+      mld.file = file;
+      mld.filename = filename;
+      mld.avoid_efi_boot_services = keep_bs;
+      err = grub_multiboot_load_elf (&mld);
       if (err)
 	{
-	  grub_free (buffer);
+	  grub_free (mld.buffer);
 	  return err;
 	}
     }
 
+  load_base_addr = mld.load_base_addr;
+
   if (keep_bs && efi_entry_specified)
     grub_multiboot_payload_eip = efi_entry;
   else if (entry_specified)
     grub_multiboot_payload_eip = entry;
 
+  if (mld.relocatable)
+    {
+      if (mld.load_base_addr >= mld.link_base_addr)
+	grub_multiboot_payload_eip += mld.load_base_addr - mld.link_base_addr;
+      else
+	grub_multiboot_payload_eip -= mld.link_base_addr - mld.load_base_addr;
+    }
+
   if (fbtag)
     err = grub_multiboot_set_console (GRUB_MULTIBOOT_CONSOLE_FRAMEBUFFER,
 				      accepted_consoles,
@@ -411,6 +459,7 @@ grub_multiboot_get_mbi_size (void)
     + ALIGN_UP (sizeof (struct multiboot_tag_framebuffer), MULTIBOOT_TAG_ALIGN)
     + ALIGN_UP (sizeof (struct multiboot_tag_old_acpi)
 		+ sizeof (struct grub_acpi_rsdp_v10), MULTIBOOT_TAG_ALIGN)
+    + ALIGN_UP (sizeof (struct multiboot_tag_load_base_addr), MULTIBOOT_TAG_ALIGN)
     + acpiv2_size ()
     + net_size ()
 #ifdef GRUB_MACHINE_EFI
@@ -694,6 +743,15 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
   ptrorig += (2 * sizeof (grub_uint32_t)) / sizeof (grub_properly_aligned_t);
 
   {
+    struct multiboot_tag_load_base_addr *tag = (struct multiboot_tag_load_base_addr *) ptrorig;
+    tag->type = MULTIBOOT_TAG_TYPE_LOAD_BASE_ADDR;
+    tag->size = sizeof (struct multiboot_tag_load_base_addr);
+    tag->load_base_addr = load_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..c96492b 100644
--- a/include/grub/multiboot.h
+++ b/include/grub/multiboot.h
@@ -91,10 +91,28 @@ grub_multiboot_set_console (int console_type, int accepted_consoles,
 			    int console_required);
 grub_err_t
 grub_multiboot_load (grub_file_t file, const char *filename);
+
+struct mbi_load_data
+{
+  grub_file_t file;
+  const char *filename;
+  void *buffer;
+  unsigned int mbi_ver;
+  int relocatable;
+  grub_uint32_t min_addr;
+  grub_uint32_t max_addr;
+  grub_size_t align;
+  grub_uint32_t preference;
+  grub_uint32_t link_base_addr;
+  grub_uint32_t load_base_addr;
+  int avoid_efi_boot_services;
+};
+typedef struct mbi_load_data mbi_load_data_t;
+
 /* Load ELF32 or ELF64.  */
 grub_err_t
-grub_multiboot_load_elf (grub_file_t file, const char *filename,
-			 void *buffer);
+grub_multiboot_load_elf (mbi_load_data_t *mld);
+
 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 f5bebe1..5a3db5a 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_LOAD_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_load_base_addr
+{
+  multiboot_uint32_t type;
+  multiboot_uint32_t size;
+  multiboot_uint32_t load_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

Re: [GRUB2 PATCH v4 4/4] multiboot2: Add support for relocatable images

[Vladimir 'phcoder' Serbinenko]

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

>
>
> +           if (mld->relocatable)
> +             err = grub_relocator_alloc_chunk_align
> (grub_multiboot_relocator, &ch,
> +                                                     mld->min_addr,
> mld->max_addr - phdr(i)->p_memsz,
> +                                                     phdr(i)->p_memsz,
> mld->align ? mld->align : 1,
> +                                                     mld->preference,
> mld->avoid_efi_boot_services);
> +           else
> +             err = grub_relocator_alloc_chunk_addr
> (grub_multiboot_relocator,
> +                                                    &ch, phdr(i)->p_paddr,
> +                                                    phdr(i)->p_memsz);
>
I believe this is faulty if you have more than one PHDR. You load every
PHDR individually to essentially random address. Pieces have no reasonable
way to find each other. Moreover entry point calculation is also faulty.
Imagine sth like this:
PHDR 1M-2M
PHDR 2M-5M
Entry point 2.5M (in second PHDR)
then if first PHDR is loaded to 1M and second to 10M then base and link
addr are both 1M, so entry point will be calculated as 2.5M, which points
to no segment. I see 2 solutions:
1) Look where entry falls in original layout, then adjust it in accordance
with where this phdr will be loaded. This requires least efforts. Finding
different PHDRs is still impossible but it will be possible in the future
with relocations.
2) Allocate a buffer of size highest - lowest and load everything into this
buffer keeping relative offsets. If we do this, then we need to document if
it's required for boorloader to behave this way or not. If it is, we can in
future provide a tag to say that image is fine with rearrangement of PHDR,
if it ever becomes relevant (I heavily doubt it).
I guess that xen is a single phdr image and so essentially any code will
work with it.
This problem appears in couple of other places, I'll skip commenting on
them explicitly.

> +  if (mld.relocatable)
> +    {
> +      if (mld.load_base_addr >= mld.link_base_addr)
> +       grub_multiboot_payload_eip += mld.load_base_addr -
> mld.link_base_addr;
> +      else
> +       grub_multiboot_payload_eip -= mld.link_base_addr -
> mld.load_base_addr;
> +    }
>
Both branches are mathematically equivalent. Any reason to have if at all?


-- 
Regards
Vladimir 'phcoder' Serbinenko

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

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

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

Re: [GRUB2 PATCH v4 4/4] multiboot2: Add support for relocatable images

[Vladimir 'phcoder' Serbinenko]

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

On Tuesday, March 15, 2016, Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>
wrote:

>
>> +           if (mld->relocatable)
>> +             err = grub_relocator_alloc_chunk_align
>> (grub_multiboot_relocator, &ch,
>> +                                                     mld->min_addr,
>> mld->max_addr - phdr(i)->p_memsz,
>> +                                                     phdr(i)->p_memsz,
>> mld->align ? mld->align : 1,
>> +                                                     mld->preference,
>> mld->avoid_efi_boot_services);
>> +           else
>> +             err = grub_relocator_alloc_chunk_addr
>> (grub_multiboot_relocator,
>> +                                                    &ch,
>> phdr(i)->p_paddr,
>> +                                                    phdr(i)->p_memsz);
>>
> I believe this is faulty if you have more than one PHDR. You load every
> PHDR individually to essentially random address. Pieces have no reasonable
> way to find each other. Moreover entry point calculation is also faulty.
> Imagine sth like this:
> PHDR 1M-2M
> PHDR 2M-5M
> Entry point 2.5M (in second PHDR)
> then if first PHDR is loaded to 1M and second to 10M then base and link
> addr are both 1M, so entry point will be calculated as 2.5M, which points
> to no segment. I see 2 solutions:
> 1) Look where entry falls in original layout, then adjust it in accordance
> with where this phdr will be loaded. This requires least efforts. Finding
> different PHDRs is still impossible but it will be possible in the future
> with relocations.
> 2) Allocate a buffer of size highest - lowest and load everything into
> this buffer keeping relative offsets. If we do this, then we need to
> document if it's required for boorloader to behave this way or not. If it
> is, we can in future provide a tag to say that image is fine with
> rearrangement of PHDR, if it ever becomes relevant (I heavily doubt it).
> I guess that xen is a single phdr image and so essentially any code will
> work with it.
> This problem appears in couple of other places, I'll skip commenting on
> them explicitly.
>
I take back the part "requires least effort" for solution 1. Solution 2 is
probably simpler and less error-prone as developper doesn't control if
binutils decode to put several phdrs.

> +  if (mld.relocatable)
>> +    {
>> +      if (mld.load_base_addr >= mld.link_base_addr)
>> +       grub_multiboot_payload_eip += mld.load_base_addr -
>> mld.link_base_addr;
>> +      else
>> +       grub_multiboot_payload_eip -= mld.link_base_addr -
>> mld.load_base_addr;
>> +    }
>>
> Both branches are mathematically equivalent. Any reason to have if at all?
>
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
>
>

-- 
Regards
Vladimir 'phcoder' Serbinenko

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

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

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

Re: [GRUB2 PATCH v4 4/4] multiboot2: Add support for relocatable images

[Daniel Kiper]

On Tue, Mar 15, 2016 at 05:30:20PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> On Tuesday, March 15, 2016, Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>
> wrote:
>
> >
> >> +           if (mld->relocatable)
> >> +             err = grub_relocator_alloc_chunk_align
> >> (grub_multiboot_relocator, &ch,
> >> +                                                     mld->min_addr,
> >> mld->max_addr - phdr(i)->p_memsz,
> >> +                                                     phdr(i)->p_memsz,
> >> mld->align ? mld->align : 1,
> >> +                                                     mld->preference,
> >> mld->avoid_efi_boot_services);
> >> +           else
> >> +             err = grub_relocator_alloc_chunk_addr
> >> (grub_multiboot_relocator,
> >> +                                                    &ch,
> >> phdr(i)->p_paddr,
> >> +                                                    phdr(i)->p_memsz);
> >>
> > I believe this is faulty if you have more than one PHDR. You load every

Argh... You are right!

> > PHDR individually to essentially random address. Pieces have no reasonable
> > way to find each other. Moreover entry point calculation is also faulty.
> > Imagine sth like this:
> > PHDR 1M-2M
> > PHDR 2M-5M
> > Entry point 2.5M (in second PHDR)
> > then if first PHDR is loaded to 1M and second to 10M then base and link
> > addr are both 1M, so entry point will be calculated as 2.5M, which points
> > to no segment. I see 2 solutions:
> > 1) Look where entry falls in original layout, then adjust it in accordance
> > with where this phdr will be loaded. This requires least efforts. Finding
> > different PHDRs is still impossible but it will be possible in the future
> > with relocations.

It looks that we should store somewhere and export to image via relevant tags
link addresses and load addresses. Hmmm... Maybe we should just provide load
addresses to image. Image can have link addresses in its data. And this
probably does not require huge changes.

> > 2) Allocate a buffer of size highest - lowest and load everything into
> > this buffer keeping relative offsets. If we do this, then we need to
> > document if it's required for boorloader to behave this way or not. If it
> > is, we can in future provide a tag to say that image is fine with
> > rearrangement of PHDR, if it ever becomes relevant (I heavily doubt it).
> > I guess that xen is a single phdr image and so essentially any code will
> > work with it.
> > This problem appears in couple of other places, I'll skip commenting on
> > them explicitly.
> >
> I take back the part "requires least effort" for solution 1. Solution 2 is
> probably simpler and less error-prone as developper doesn't control if
> binutils decode to put several phdrs.

#2 looks promising but what if PHDR_1 is at 1 MiB - 2 MiB and PHDR_2 is at
808 MiB - 809 MiB? Then we will allocate more than 800 MiB just for an
unusable hole. So, I think that we should go that way if solution #1
is too complicated.

> > +  if (mld.relocatable)
> >> +    {
> >> +      if (mld.load_base_addr >= mld.link_base_addr)
> >> +       grub_multiboot_payload_eip += mld.load_base_addr -
> >> mld.link_base_addr;
> >> +      else
> >> +       grub_multiboot_payload_eip -= mld.link_base_addr -
> >> mld.load_base_addr;
> >> +    }
> >>
> > Both branches are mathematically equivalent. Any reason to have if at all?

Yep, you are right. However, it looks that real life (C?) is more complicated.
I am trying to avoid wrap around here if mld.load_base_addr < mld.link_base_addr.
If you look at C operator precedence then everything should work. However,
I am not 100% sure that a given compiler will not optimize/break my stuff.
So, maybe we should use signed 64-bit int here.

Daniel

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

Re: [GRUB2 PATCH v4 4/4] multiboot2: Add support for relocatable images

[Konrad Rzeszutek Wilk]

On Tue, Mar 15, 2016 at 10:42:21PM +0100, Daniel Kiper wrote:
> On Tue, Mar 15, 2016 at 05:30:20PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> > On Tuesday, March 15, 2016, Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>
> > wrote:
> >
> > >
> > >> +           if (mld->relocatable)
> > >> +             err = grub_relocator_alloc_chunk_align
> > >> (grub_multiboot_relocator, &ch,
> > >> +                                                     mld->min_addr,
> > >> mld->max_addr - phdr(i)->p_memsz,
> > >> +                                                     phdr(i)->p_memsz,
> > >> mld->align ? mld->align : 1,
> > >> +                                                     mld->preference,
> > >> mld->avoid_efi_boot_services);
> > >> +           else
> > >> +             err = grub_relocator_alloc_chunk_addr
> > >> (grub_multiboot_relocator,
> > >> +                                                    &ch,
> > >> phdr(i)->p_paddr,
> > >> +                                                    phdr(i)->p_memsz);
> > >>
> > > I believe this is faulty if you have more than one PHDR. You load every
> 
> Argh... You are right!
> 
> > > PHDR individually to essentially random address. Pieces have no reasonable
> > > way to find each other. Moreover entry point calculation is also faulty.
> > > Imagine sth like this:
> > > PHDR 1M-2M
> > > PHDR 2M-5M
> > > Entry point 2.5M (in second PHDR)
> > > then if first PHDR is loaded to 1M and second to 10M then base and link
> > > addr are both 1M, so entry point will be calculated as 2.5M, which points
> > > to no segment. I see 2 solutions:
> > > 1) Look where entry falls in original layout, then adjust it in accordance
> > > with where this phdr will be loaded. This requires least efforts. Finding
> > > different PHDRs is still impossible but it will be possible in the future
> > > with relocations.
> 
> It looks that we should store somewhere and export to image via relevant tags
> link addresses and load addresses. Hmmm... Maybe we should just provide load
> addresses to image. Image can have link addresses in its data. And this
> probably does not require huge changes.
> 
> > > 2) Allocate a buffer of size highest - lowest and load everything into
> > > this buffer keeping relative offsets. If we do this, then we need to
> > > document if it's required for boorloader to behave this way or not. If it
> > > is, we can in future provide a tag to say that image is fine with
> > > rearrangement of PHDR, if it ever becomes relevant (I heavily doubt it).
> > > I guess that xen is a single phdr image and so essentially any code will
> > > work with it.

Won't be in Xen 4.7.
> > > This problem appears in couple of other places, I'll skip commenting on
> > > them explicitly.
> > >
> > I take back the part "requires least effort" for solution 1. Solution 2 is
> > probably simpler and less error-prone as developper doesn't control if
> > binutils decode to put several phdrs.
> 
> #2 looks promising but what if PHDR_1 is at 1 MiB - 2 MiB and PHDR_2 is at
> 808 MiB - 809 MiB? Then we will allocate more than 800 MiB just for an
> unusable hole. So, I think that we should go that way if solution #1
> is too complicated.

Daniel, my xSplice patches make the Xen have two ELF PHDRS: 1)the PT_LOAD
and 2) PT_NOTE (which points to smack in the .text section) so you can try
that as an example payload.

(If you want to put your patches on top of mine:
git://xenbits.xen.org/people/konradwilk/xen.git #xsplice.v4)
> 
> > > +  if (mld.relocatable)
> > >> +    {
> > >> +      if (mld.load_base_addr >= mld.link_base_addr)
> > >> +       grub_multiboot_payload_eip += mld.load_base_addr -
> > >> mld.link_base_addr;
> > >> +      else
> > >> +       grub_multiboot_payload_eip -= mld.link_base_addr -
> > >> mld.load_base_addr;
> > >> +    }
> > >>
> > > Both branches are mathematically equivalent. Any reason to have if at all?
> 
> Yep, you are right. However, it looks that real life (C?) is more complicated.
> I am trying to avoid wrap around here if mld.load_base_addr < mld.link_base_addr.
> If you look at C operator precedence then everything should work. However,
> I am not 100% sure that a given compiler will not optimize/break my stuff.
> So, maybe we should use signed 64-bit int here.
> 
> Daniel

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

Re: [GRUB2 PATCH v4 4/4] multiboot2: Add support for relocatable images

[Daniel Kiper]

On Tue, Mar 15, 2016 at 07:54:08PM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Mar 15, 2016 at 10:42:21PM +0100, Daniel Kiper wrote:
> > On Tue, Mar 15, 2016 at 05:30:20PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> > > On Tuesday, March 15, 2016, Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>
> > > wrote:
> > >
> > > >
> > > >> +           if (mld->relocatable)
> > > >> +             err = grub_relocator_alloc_chunk_align
> > > >> (grub_multiboot_relocator, &ch,
> > > >> +                                                     mld->min_addr,
> > > >> mld->max_addr - phdr(i)->p_memsz,
> > > >> +                                                     phdr(i)->p_memsz,
> > > >> mld->align ? mld->align : 1,
> > > >> +                                                     mld->preference,
> > > >> mld->avoid_efi_boot_services);
> > > >> +           else
> > > >> +             err = grub_relocator_alloc_chunk_addr
> > > >> (grub_multiboot_relocator,
> > > >> +                                                    &ch,
> > > >> phdr(i)->p_paddr,
> > > >> +                                                    phdr(i)->p_memsz);
> > > >>
> > > > I believe this is faulty if you have more than one PHDR. You load every
> >
> > Argh... You are right!
> >
> > > > PHDR individually to essentially random address. Pieces have no reasonable
> > > > way to find each other. Moreover entry point calculation is also faulty.
> > > > Imagine sth like this:
> > > > PHDR 1M-2M
> > > > PHDR 2M-5M
> > > > Entry point 2.5M (in second PHDR)
> > > > then if first PHDR is loaded to 1M and second to 10M then base and link
> > > > addr are both 1M, so entry point will be calculated as 2.5M, which points
> > > > to no segment. I see 2 solutions:
> > > > 1) Look where entry falls in original layout, then adjust it in accordance
> > > > with where this phdr will be loaded. This requires least efforts. Finding
> > > > different PHDRs is still impossible but it will be possible in the future
> > > > with relocations.
> >
> > It looks that we should store somewhere and export to image via relevant tags
> > link addresses and load addresses. Hmmm... Maybe we should just provide load
> > addresses to image. Image can have link addresses in its data. And this
> > probably does not require huge changes.
> >
> > > > 2) Allocate a buffer of size highest - lowest and load everything into
> > > > this buffer keeping relative offsets. If we do this, then we need to
> > > > document if it's required for boorloader to behave this way or not. If it
> > > > is, we can in future provide a tag to say that image is fine with
> > > > rearrangement of PHDR, if it ever becomes relevant (I heavily doubt it).
> > > > I guess that xen is a single phdr image and so essentially any code will
> > > > work with it.
>
> Won't be in Xen 4.7.
> > > > This problem appears in couple of other places, I'll skip commenting on
> > > > them explicitly.
> > > >
> > > I take back the part "requires least effort" for solution 1. Solution 2 is
> > > probably simpler and less error-prone as developper doesn't control if
> > > binutils decode to put several phdrs.
> >
> > #2 looks promising but what if PHDR_1 is at 1 MiB - 2 MiB and PHDR_2 is at
> > 808 MiB - 809 MiB? Then we will allocate more than 800 MiB just for an
> > unusable hole. So, I think that we should go that way if solution #1
> > is too complicated.
>
> Daniel, my xSplice patches make the Xen have two ELF PHDRS: 1)the PT_LOAD
> and 2) PT_NOTE (which points to smack in the .text section) so you can try
> that as an example payload.
>
> (If you want to put your patches on top of mine:
> git://xenbits.xen.org/people/konradwilk/xen.git #xsplice.v4)

Thanks, but multiboot2 loads just PT_LOAD segments.

Daniel

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

Re: [GRUB2 PATCH v4 4/4] multiboot2: Add support for relocatable images

[Vladimir 'phcoder' Serbinenko]

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

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

> On Tue, Mar 15, 2016 at 05:30:20PM +0100, Vladimir 'phcoder' Serbinenko
> wrote:
> > On Tuesday, March 15, 2016, Vladimir 'phcoder' Serbinenko <
> phcoder@gmail.com <javascript:;>>
> > wrote:
> >
> > >
> > >> +           if (mld->relocatable)
> > >> +             err = grub_relocator_alloc_chunk_align
> > >> (grub_multiboot_relocator, &ch,
> > >> +                                                     mld->min_addr,
> > >> mld->max_addr - phdr(i)->p_memsz,
> > >> +
>  phdr(i)->p_memsz,
> > >> mld->align ? mld->align : 1,
> > >> +                                                     mld->preference,
> > >> mld->avoid_efi_boot_services);
> > >> +           else
> > >> +             err = grub_relocator_alloc_chunk_addr
> > >> (grub_multiboot_relocator,
> > >> +                                                    &ch,
> > >> phdr(i)->p_paddr,
> > >> +
> phdr(i)->p_memsz);
> > >>
> > > I believe this is faulty if you have more than one PHDR. You load every
>
> Argh... You are right!
>
> > > PHDR individually to essentially random address. Pieces have no
> reasonable
> > > way to find each other. Moreover entry point calculation is also
> faulty.
> > > Imagine sth like this:
> > > PHDR 1M-2M
> > > PHDR 2M-5M
> > > Entry point 2.5M (in second PHDR)
> > > then if first PHDR is loaded to 1M and second to 10M then base and link
> > > addr are both 1M, so entry point will be calculated as 2.5M, which
> points
> > > to no segment. I see 2 solutions:
> > > 1) Look where entry falls in original layout, then adjust it in
> accordance
> > > with where this phdr will be loaded. This requires least efforts.
> Finding
> > > different PHDRs is still impossible but it will be possible in the
> future
> > > with relocations.
>
> It looks that we should store somewhere and export to image via relevant
> tags
> link addresses and load addresses. Hmmm... Maybe we should just provide
> load
> addresses to image. Image can have link addresses in its data. And this
> probably does not require huge changes.
>
> > > 2) Allocate a buffer of size highest - lowest and load everything into
> > > this buffer keeping relative offsets. If we do this, then we need to
> > > document if it's required for boorloader to behave this way or not. If
> it
> > > is, we can in future provide a tag to say that image is fine with
> > > rearrangement of PHDR, if it ever becomes relevant (I heavily doubt
> it).
> > > I guess that xen is a single phdr image and so essentially any code
> will
> > > work with it.
> > > This problem appears in couple of other places, I'll skip commenting on
> > > them explicitly.
> > >
> > I take back the part "requires least effort" for solution 1. Solution 2
> is
> > probably simpler and less error-prone as developper doesn't control if
> > binutils decode to put several phdrs.
>
> #2 looks promising but what if PHDR_1 is at 1 MiB - 2 MiB and PHDR_2 is at
> 808 MiB - 809 MiB? Then we will allocate more than 800 MiB just for an
> unusable hole. So, I think that we should go that way if solution #1
> is too complicated.
>
If 'RELOCATABLE' means 'image is fine with any of its phdr going anywhere'
then why would it declare such a layout if it's fine with second phdr going
right after the first one? Why not describe the same thing as 1-2M and
2-3M?

>
> > > +  if (mld.relocatable)
> > >> +    {
> > >> +      if (mld.load_base_addr >= mld.link_base_addr)
> > >> +       grub_multiboot_payload_eip += mld.load_base_addr -
> > >> mld.link_base_addr;
> > >> +      else
> > >> +       grub_multiboot_payload_eip -= mld.link_base_addr -
> > >> mld.load_base_addr;
> > >> +    }
> > >>
> > > Both branches are mathematically equivalent. Any reason to have if at
> all?
>
> Yep, you are right. However, it looks that real life (C?) is more
> complicated.
> I am trying to avoid wrap around here if mld.load_base_addr <
> mld.link_base_addr.
> If you look at C operator precedence then everything should work. However,
> I am not 100% sure that a given compiler will not optimize/break my stuff.
> So, maybe we should use signed 64-bit int here.
>
> Daniel
>


-- 
Regards
Vladimir 'phcoder' Serbinenko

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

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

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