Re: [PATCH 16/18] efi: create efi_exit_boot()

2015-03-27 Thread Daniel Kiper 
On Mon, Mar 02, 2015 at 04:45:47PM +, Jan Beulich wrote:
  On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
  ..which gets memory map and calls ExitBootServices(). We need this
  to support multiboot2 protocol on EFI platforms.

 Patches from 9 up to here all make sense on the basis that patch 18
 does and assuming that you really need all this code moved out to
 separate functions. How much different is efi_multiboot2() introduced
 in #18 from what is left of efi_start() at this point? I.e. is splitting out

More or less efi_multiboot2() does not parse command line and do not
load modules itself as efi_start() does.

 all of this code really needed?

I think that it is worth doing. First of all efi_start() is huge and its
analysis is very difficult right now. So, splitting code into smaller chunks
will improve readability a lot (I am still thinking about extracting command
line parser and module loader from efi_start() even if both functions will be
used only in efi_start(); this way we will have very simple functions doing
one thing easy to understand). Additionally, we create pieces which are very
easy to reuse in efi_multiboot2() which is very simple and again easy
for analysis.

Potentially we can reuse efi_start() in multiboot2 case. However, I prefer
to have separate function because this way it is clear that multiboot2 case
is different thing then native EFI loader stuff. Additionally, efi_start()
is architecture independent and efi_multiboot2() is x86 only and it should
live in x86 files.

 If it is, please don't title all these patches create ... but split out
 ... or some such - you don't really create the code. Similarly the
 second sentence above is too imprecise for my taste - we want to
 re-use this code to support ... would seem more to the point.

OK.

Daniel

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

Re: [PATCH 16/18] efi: create efi_exit_boot()

2015-03-27 Thread Jan Beulich 
 On 27.03.15 at 13:00, daniel.ki...@oracle.com wrote:
 Additionally, efi_start()
 is architecture independent and efi_multiboot2() is x86 only and it should
 live in x86 files.

Is that really the case? Looking at the grub2 sources I see support
for other than x86...

Jan


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

Re: [PATCH 16/18] efi: create efi_exit_boot()

2015-03-27 Thread Daniel Kiper 
On Fri, Mar 27, 2015 at 12:10:56PM +, Jan Beulich wrote:
  On 27.03.15 at 13:00, daniel.ki...@oracle.com wrote:
  Additionally, efi_start()
  is architecture independent and efi_multiboot2() is x86 only and it should
  live in x86 files.

 Is that really the case? Looking at the grub2 sources I see support
 for other than x86...

Well... In theory multiboot protocol was designed as arch independet.
However, docs define more precisely stuff for i386 and MIPS (multiboot2
protocol only). As I know we do not care about MIPS. Additionally, so
called muliboot protocol for ARM is completely different thing then
multiboot protocols mentioned above (it is a mixture of EFI native
loader with DT). So, it looks that from our point of view efi_multiboot2()
is x86 only stuff (right now, I am not fortune teller, so, I am not
able to tell what happens in the future ;-))) ).

Daniel

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

Re: [PATCH 16/18] efi: create efi_exit_boot()

2015-03-27 Thread Ian Campbell 
On Fri, 2015-03-27 at 13:43 +0100, Daniel Kiper wrote:
 On Fri, Mar 27, 2015 at 12:10:56PM +, Jan Beulich wrote:
   On 27.03.15 at 13:00, daniel.ki...@oracle.com wrote:
   Additionally, efi_start()
   is architecture independent and efi_multiboot2() is x86 only and it should
   live in x86 files.
 
  Is that really the case? Looking at the grub2 sources I see support
  for other than x86...
 
 Well... In theory multiboot protocol was designed as arch independet.
 However, docs define more precisely stuff for i386 and MIPS (multiboot2
 protocol only). As I know we do not care about MIPS. Additionally, so
 called muliboot protocol for ARM is completely different thing then
 multiboot protocols mentioned above

The ARM multiboot DT thing fits into the multiboot1 namespace, since
it is unlikely there will ever be an ARM multiboot1.

It's possible that someone might spec and implement multiboot2 for ARM
as well, although whether than has any impact on the comments made in
this threadlet I've no idea..

Ian.


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

Re: [PATCH 16/18] efi: create efi_exit_boot()

2015-03-02 Thread Jan Beulich 
 On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
 ..which gets memory map and calls ExitBootServices(). We need this
 to support multiboot2 protocol on EFI platforms.

Patches from 9 up to here all make sense on the basis that patch 18
does and assuming that you really need all this code moved out to
separate functions. How much different is efi_multiboot2() introduced
in #18 from what is left of efi_start() at this point? I.e. is splitting out
all of this code really needed?

If it is, please don't title all these patches create ... but split out
... or some such - you don't really create the code. Similarly the
second sentence above is too imprecise for my taste - we want to
re-use this code to support ... would seem more to the point.

Jan


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

[PATCH 16/18] efi: create efi_exit_boot()

2015-01-30 Thread Daniel Kiper 
..which gets memory map and calls ExitBootServices(). We need this
to support multiboot2 protocol on EFI platforms.

Signed-off-by: Daniel Kiper daniel.ki...@oracle.com
---
 xen/common/efi/boot.c |   79 +++--
 1 file changed, 44 insertions(+), 35 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 63c930d..f8be3dd 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -864,6 +864,47 @@ static void __init 
efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
 efi_arch_video_init(gop, info_size, mode_info);
 }
 
+static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
*SystemTable)
+{
+EFI_STATUS status;
+UINTN map_key;
+bool_t retry;
+
+efi_bs-GetMemoryMap(efi_memmap_size, NULL, map_key,
+ efi_mdesc_size, mdesc_ver);
+efi_memmap = efi_arch_allocate_mmap_buffer(efi_memmap_size);
+if ( !efi_memmap )
+blexit(LUnable to allocate memory for EFI memory map);
+
+for ( retry = 0; ; retry = 1 )
+{
+status = efi_bs-GetMemoryMap(efi_memmap_size, efi_memmap, map_key,
+  efi_mdesc_size, mdesc_ver);
+if ( EFI_ERROR(status) )
+PrintErrMesg(LCannot obtain memory map, status);
+
+efi_arch_process_memory_map(SystemTable, efi_memmap, efi_memmap_size,
+efi_mdesc_size, mdesc_ver);
+
+efi_arch_pre_exit_boot();
+
+status = efi_bs-ExitBootServices(ImageHandle, map_key);
+if ( status != EFI_INVALID_PARAMETER || retry )
+break;
+}
+
+if ( EFI_ERROR(status) )
+PrintErrMesg(LCannot exit boot services, status);
+
+/* Adjust pointers into EFI. */
+efi_ct = (void *)efi_ct + DIRECTMAP_VIRT_START;
+#ifdef USE_SET_VIRTUAL_ADDRESS_MAP
+efi_rs = (void *)efi_rs + DIRECTMAP_VIRT_START;
+#endif
+efi_memmap = (void *)efi_memmap + DIRECTMAP_VIRT_START;
+efi_fw_vendor = (void *)efi_fw_vendor + DIRECTMAP_VIRT_START;
+}
+
 static int __init __maybe_unused set_color(u32 mask, int bpp, u8 *pos, u8 *sz)
 {
if ( bpp  0 )
@@ -888,11 +929,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
*SystemTable)
 EFI_STATUS status;
 unsigned int i, argc;
 CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL;
-UINTN map_key, gop_mode = ~0;
+UINTN gop_mode = ~0;
 EFI_SHIM_LOCK_PROTOCOL *shim_lock;
 EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
 union string section = { NULL }, name;
-bool_t base_video = 0, retry;
+bool_t base_video = 0;
 char *option_str;
 bool_t use_cfg_file;
 
@@ -1108,39 +1149,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
*SystemTable)
 
 efi_set_gop_mode(gop, gop_mode);
 
-efi_bs-GetMemoryMap(efi_memmap_size, NULL, map_key,
- efi_mdesc_size, mdesc_ver);
-efi_memmap = efi_arch_allocate_mmap_buffer(efi_memmap_size);
-if ( !efi_memmap )
-blexit(LUnable to allocate memory for EFI memory map);
-
-for ( retry = 0; ; retry = 1 )
-{
-status = efi_bs-GetMemoryMap(efi_memmap_size, efi_memmap, map_key,
-  efi_mdesc_size, mdesc_ver);
-if ( EFI_ERROR(status) )
-PrintErrMesg(LCannot obtain memory map, status);
-
-efi_arch_process_memory_map(SystemTable, efi_memmap, efi_memmap_size,
-efi_mdesc_size, mdesc_ver);
-
-efi_arch_pre_exit_boot();
-
-status = efi_bs-ExitBootServices(ImageHandle, map_key);
-if ( status != EFI_INVALID_PARAMETER || retry )
-break;
-}
-
-if ( EFI_ERROR(status) )
-PrintErrMesg(LCannot exit boot services, status);
-
-/* Adjust pointers into EFI. */
-efi_ct = (void *)efi_ct + DIRECTMAP_VIRT_START;
-#ifdef USE_SET_VIRTUAL_ADDRESS_MAP
-efi_rs = (void *)efi_rs + DIRECTMAP_VIRT_START;
-#endif
-efi_memmap = (void *)efi_memmap + DIRECTMAP_VIRT_START;
-efi_fw_vendor = (void *)efi_fw_vendor + DIRECTMAP_VIRT_START;
+efi_exit_boot(ImageHandle, SystemTable);
 
 efi_arch_post_exit_boot();
 for( ; ; ); /* not reached */
-- 
1.7.10.4


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