Re: [PATCH 3/5] i386/relocator: Remove unused avoid_efi_bootservices argument

2015-07-16 Thread Daniel Kiper 
On Wed, Jul 15, 2015 at 07:50:51PM +0200, Vladimir 'φ-coder/phcoder' Serbinenko 
wrote:
 On 30.05.2015 07:25, Andrei Borzenkov wrote:
  В Fri, 29 May 2015 22:58:43 +0200
  Daniel Kiper daniel.ki...@oracle.com пишет:
 
  Another questions is why grub_relocator_alloc_chunk_addr() does not 
  consult EFI
  memory map if grub_relocator_alloc_chunk_align() does. Should not we fix 
  it?
 
  My best guess is that grub_relocator_alloc_chunk_addr() gets target
  from elsewhere so there is nothing to consult (it is caller
  responsibility); while grub_relocator_alloc_chunk_align() needs to
  actually search for suitable memory region.
 
 My suggestion would be to pass avoid_boot_services = 1 in multiboot2 iff
 we don't terminate the services. Any problems with this approach?

I am just working on new GRUB2 patches. I thought to go that way. I hope
this is good idea. Probably I will post new version of patches next week.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 3/5] i386/relocator: Remove unused avoid_efi_bootservices argument

2015-07-15 Thread Vladimir 'φ-coder/phcoder' Serbinenko 
On 30.05.2015 07:25, Andrei Borzenkov wrote:
 В Fri, 29 May 2015 22:58:43 +0200
 Daniel Kiper daniel.ki...@oracle.com пишет:
 
 Another questions is why grub_relocator_alloc_chunk_addr() does not consult 
 EFI
 memory map if grub_relocator_alloc_chunk_align() does. Should not we fix it?
 
 My best guess is that grub_relocator_alloc_chunk_addr() gets target
 from elsewhere so there is nothing to consult (it is caller
 responsibility); while grub_relocator_alloc_chunk_align() needs to
 actually search for suitable memory region.
 
My suggestion would be to pass avoid_boot_services = 1 in multiboot2 iff
we don't terminate the services. Any problems with this approach?



signature.asc
Description: OpenPGP digital signature
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 3/5] i386/relocator: Remove unused avoid_efi_bootservices argument

2015-05-29 Thread Daniel Kiper 
Hey,

Sorry for late reply but I was busy with our internal EFI stuff. We did
more tests and some fixes and it seems that everything is going in right
direction. Well, I hope... :-))) Right now I am going to continue my work
on upstream EFI + GRUB2 + Xen stuff. I think that I will be able to release
new patch series for GRUB2 and Xen in second or third week of June (next
week we have 2 days holiday in Poland). Stay tuned...

On Thu, May 07, 2015 at 06:04:21PM +0200, Vladimir 'φ-coder/phcoder' Serbinenko 
wrote:
 Not unused:
 grub_efi_mmap_iterate (grub_relocator_alloc_chunk_align_iter, ctx,
  avoid_efi_boot_services);

In general I have a problem with avoid_efi_boot_services stuff in GRUB2.
By default it is disabled and GRUB2 assumes that boot services (BS) memory
regions (code and data) are free. This means that it may put there loaded image
and/or fills memory maps (MULTIBOOT_TAG_TYPE_BASIC_MEMINFO, 
MULTIBOOT_TAG_TYPE_MMAP)
with above mentioned regions marked as free. This is OK when BS are disabled
before loaded image exaction. However, we introduced new feature like
MULTIBOOT_TAG_TYPE_EFI_BS (I am going to improve/extend it) and this makes
this behavior more problematic. It means that now GRUB2 with 
MULTIBOOT_TAG_TYPE_EFI_BS
enabled may overwrite BS regions and/or memory maps are bogus. In turn, this 
means
that BS are potentially unusable by loaded image which asked for BS with above
mentioned tag. So, I think that we should find a solution for that issues.

The simplest fix for that problem seems total removal of avoid_efi_boot_services
and assumption that BS memory regions are not free. I did some tests with that
but not so many and not so deep. I discovered that at least linux loader (IIRC)
has some issues with this solution. Maybe it could be fixed easily but I did
not investigated this issue so long. Additionally, I realized that boot services
regions are quite big (dozens or even about 100 MiB) and maybe this is not very
nice idea to assume them used in all cases.

So, maybe we should just focus on multiboot2 loader only. In that case GRUB2 
should
at first assume that BS regions are used. Then it should check image header. If 
it
does not contain MULTIBOOT_TAG_TYPE_EFI_BS tag then starting from that point it 
should
assume that BS regions are free and behave as it behaves right now. However, if 
multiboot2
loader encounters MULTIBOOT_TAG_TYPE_EFI_BS tag then it should still assume 
that BS
regions are not free. Additionally, GRUB2 should not pass any memory map info 
(i.e,
MULTIBOOT_TAG_TYPE_BASIC_MEMINFO, MULTIBOOT_TAG_TYPE_MMAP, 
MULTIBOOT_TAG_TYPE_EFI_MMAP,
etc.) because they are bogus. In that case loaded image should take memory map 
itself
using relevant BS calls. Does it make sense?

Another questions is why grub_relocator_alloc_chunk_addr() does not consult EFI
memory map if grub_relocator_alloc_chunk_align() does. Should not we fix it?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 3/5] i386/relocator: Remove unused avoid_efi_bootservices argument

2015-01-30 Thread Daniel Kiper 
Signed-off-by: Daniel Kiper daniel.ki...@oracle.com
---
 grub-core/lib/i386/relocator.c   |5 ++---
 grub-core/loader/i386/bsd.c  |6 +++---
 grub-core/loader/i386/coreboot/chainloader.c |2 +-
 grub-core/loader/i386/linux.c|2 +-
 grub-core/loader/i386/pc/plan9.c |2 +-
 grub-core/loader/i386/xnu.c  |4 ++--
 grub-core/loader/multiboot.c |4 
 include/grub/i386/relocator.h|3 +--
 8 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/grub-core/lib/i386/relocator.c b/grub-core/lib/i386/relocator.c
index 71dd4f0..b879e5b 100644
--- a/grub-core/lib/i386/relocator.c
+++ b/grub-core/lib/i386/relocator.c
@@ -73,8 +73,7 @@ extern struct grub_i386_idt grub_relocator16_idt;
 
 grub_err_t
 grub_relocator32_boot (struct grub_relocator *rel,
-  struct grub_relocator32_state state,
-  int avoid_efi_bootservices)
+  struct grub_relocator32_state state)
 {
   grub_err_t err;
   void *relst;
@@ -87,7 +86,7 @@ grub_relocator32_boot (struct grub_relocator *rel,
  0x9a000 - RELOCATOR_SIZEOF (32),
  RELOCATOR_SIZEOF (32), 16,
  GRUB_RELOCATOR_PREFERENCE_LOW,
- avoid_efi_bootservices);
+ 0);
   if (err)
 return err;
 
diff --git a/grub-core/loader/i386/bsd.c b/grub-core/loader/i386/bsd.c
index bc377b3..c2bf09f 100644
--- a/grub-core/loader/i386/bsd.c
+++ b/grub-core/loader/i386/bsd.c
@@ -797,7 +797,7 @@ grub_freebsd_boot (void)
   stack[6] = stack_target + 9 * sizeof (grub_uint32_t);
   stack[7] = bi.tags;
   stack[8] = kern_end;
-  return grub_relocator32_boot (relocator, state, 0);
+  return grub_relocator32_boot (relocator, state);
 }
 
   /* Not reached.  */
@@ -913,7 +913,7 @@ grub_openbsd_boot (void)
   stack[7] = (grub_uint8_t *) curarg - (grub_uint8_t *) arg0;
   stack[8] = ((grub_uint8_t *) arg0 - (grub_uint8_t *) buf0) + buf_target;
 
-  return grub_relocator32_boot (relocator, state, 0);
+  return grub_relocator32_boot (relocator, state);
 }
 
 static grub_err_t
@@ -1228,7 +1228,7 @@ grub_netbsd_boot (void)
   stack[5] = grub_mmap_get_upper ()  10;
   stack[6] = grub_mmap_get_lower ()  10;
 
-  return grub_relocator32_boot (relocator, state, 0);
+  return grub_relocator32_boot (relocator, state);
 }
 
 static grub_err_t
diff --git a/grub-core/loader/i386/coreboot/chainloader.c 
b/grub-core/loader/i386/coreboot/chainloader.c
index d4cc40b..4bf6dff 100644
--- a/grub-core/loader/i386/coreboot/chainloader.c
+++ b/grub-core/loader/i386/coreboot/chainloader.c
@@ -47,7 +47,7 @@ grub_chain_boot (void)
   grub_video_set_mode (text, 0, 0);
 
   state.eip = entry;
-  return grub_relocator32_boot (relocator, state, 0);
+  return grub_relocator32_boot (relocator, state);
 }
 
 static grub_err_t
diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
index 291f728..a554da8 100644
--- a/grub-core/loader/i386/linux.c
+++ b/grub-core/loader/i386/linux.c
@@ -661,7 +661,7 @@ grub_linux_boot (void)
   state.esi = ctx.real_mode_target;
   state.esp = ctx.real_mode_target;
   state.eip = ctx.params-code32_start;
-  return grub_relocator32_boot (relocator, state, 0);
+  return grub_relocator32_boot (relocator, state);
 }
 
 static grub_err_t
diff --git a/grub-core/loader/i386/pc/plan9.c b/grub-core/loader/i386/pc/plan9.c
index 814a49d..bfff497 100644
--- a/grub-core/loader/i386/pc/plan9.c
+++ b/grub-core/loader/i386/pc/plan9.c
@@ -90,7 +90,7 @@ grub_plan9_boot (void)
   };
   grub_video_set_mode (text, 0, 0);
 
-  return grub_relocator32_boot (rel, state, 0);
+  return grub_relocator32_boot (rel, state);
 }
 
 static grub_err_t
diff --git a/grub-core/loader/i386/xnu.c b/grub-core/loader/i386/xnu.c
index e0506a6..a2bc876 100644
--- a/grub-core/loader/i386/xnu.c
+++ b/grub-core/loader/i386/xnu.c
@@ -791,7 +791,7 @@ grub_xnu_boot_resume (void)
   state.eip = grub_xnu_entry_point;
   state.eax = grub_xnu_arg1;
 
-  return grub_relocator32_boot (grub_xnu_relocator, state, 0); 
+  return grub_relocator32_boot (grub_xnu_relocator, state); 
 }
 
 /* Setup video for xnu. */
@@ -1099,7 +1099,7 @@ grub_xnu_boot (void)
   grub_outb (0xff, 0x21);
   grub_outb (0xff, 0xa1);
 
-  return grub_relocator32_boot (grub_xnu_relocator, state, 0);
+  return grub_relocator32_boot (grub_xnu_relocator, state);
 }
 
 static grub_command_t cmd_devprop_load;
diff --git a/grub-core/loader/multiboot.c b/grub-core/loader/multiboot.c
index 4b71f33..307da32 100644
--- a/grub-core/loader/multiboot.c
+++ b/grub-core/loader/multiboot.c
@@ -131,11 +131,7 @@ grub_multiboot_boot (void)
   if (err)
 return err;
 
-#if defined (__i386__) || defined (__x86_64__)
-  grub_relocator32_boot (grub_multiboot_relocator, state, 0);
-#else