Re: [Xen-devel] [PATCH 05/18] efi: split efi_enabled to efi_platform and efi_loader

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.01.15 at 18:54, <daniel.kiper@xxxxxxxxxx> wrote:
> We need more fine grained knowledge about EFI environment and check
> for EFI platform and EFI loader separately to properly support
> multiboot2 protocol.

... because of ... (i.e. I can't see from the description what the
separation is good for). Looking at the comments you placed
aside the variables doesn't help me either.

> In general Xen loaded by this protocol uses
> memory mappings and loaded modules in simliar way to Xen loaded
> by multiboot (v1) protocol. Hence, split efi_enabled to efi_platform
> and efi_loader.

And if I'm guessing things right, then introducing efi_loader but
leaving efi_enabled alone (only converting where needed) would
seem the right approach.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -431,7 +431,7 @@ static void __init parse_video_info(void)
>      struct boot_video_info *bvi = &bootsym(boot_vid_info);
>  
>      /* The EFI loader fills vga_console_info directly. */
> -    if ( efi_enabled )
> +    if ( efi_platform )

This looks wrong considering the comment.

> @@ -663,7 +663,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      if ( ((unsigned long)cpu0_stack & (STACK_SIZE-1)) != 0 )
>          panic("Misaligned CPU0 stack.");
>  
> -    if ( efi_enabled )
> +    if ( efi_loader )
>      {
>          set_pdx_range(xen_phys_start >> PAGE_SHIFT,
>                        (xen_phys_start + BOOTSTRAP_MAP_BASE) >> PAGE_SHIFT);
> @@ -774,7 +774,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>       * we can relocate the dom0 kernel and other multiboot modules. Also, on
>       * x86/64, we relocate Xen to higher memory.
>       */
> -    for ( i = 0; !efi_enabled && i < mbi->mods_count; i++ )
> +    for ( i = 0; !efi_loader && i < mbi->mods_count; i++ )
>      {
>          if ( mod[i].mod_start & (PAGE_SIZE - 1) )
>              panic("Bootloader didn't honor module alignment request.");
> @@ -962,7 +962,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      if ( !xen_phys_start )
>          panic("Not enough memory to relocate Xen.");
> -    reserve_e820_ram(&boot_e820, efi_enabled ? mbi->mem_upper : 
> __pa(&_start),
> +    reserve_e820_ram(&boot_e820, efi_loader ? mbi->mem_upper : __pa(&_start),

For all of these it's really hard to tell whether they're right without
knowing which parts of the current EFI loading code you intend to
re-use. Perhaps switching these would better be done along with
adding the re-using code (or splitting out the not re-used parts)?

> --- a/xen/include/xen/efi.h
> +++ b/xen/include/xen/efi.h
> @@ -5,7 +5,11 @@
>  #include <xen/types.h>
>  #endif
>  
> -extern const bool_t efi_enabled;
> +/* If true then Xen runs on EFI platform. */
> +extern bool_t efi_platform;
> +
> +/* If true then Xen was loaded by native EFI loader as PE application. */
> +extern bool_t efi_loader;

I don't see why you're losing the constness.

Jan


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