Re: [Xen-devel] [PATCH v2 5/5] fix: add multiboot2 protocol support for EFI platforms

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.01.17 at 15:15, <cardoe@xxxxxxxxxx> wrote:
> Doug v2 - new in this version to help show what's changed

Thanks for providing this.

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -519,6 +519,7 @@ trampoline_setup:
>  1:
>          /* Switch to low-memory stack.  */
>          mov     sym_phys(trampoline_phys),%edi
> +        /* The stack starts 64kb after the location of trampoline_phys */

"The stack ends ..." (as it grows downwards).

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -170,9 +170,10 @@ static void __init 
> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>              /* fall through */
>          case EfiConventionalMemory:
>              if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 &&
> -                 len >= cfg.size + extra_mem &&
> -                 desc->PhysicalStart + len > cfg.addr )
> +                 len >= cfg.size && desc->PhysicalStart + len > cfg.addr ) {
> +                ASSERT(cfg.size > 0);
>                  cfg.addr = (desc->PhysicalStart + len - cfg.size) & 
> PAGE_MASK;
> +            }

You leave the "extra_mem" variable unused afaict.

> @@ -686,6 +687,10 @@ paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, 
> EFI_SYSTEM_TABLE *SystemTa
>      setup_efi_pci();
>      efi_variables();
>  
> +    /* This is the maximum size of our trampoline + our low memory stack */
> +    cfg.size = 64 << 10;
> +    ASSERT(cfg.size >= ((trampoline_end - trampoline_start) + 4096));

If this assertion triggers, what would the user see? Namely -
anything other than a hang or instant reboot? Such checks
really need to be build time ones.

Jan


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