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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.01.17 at 21:07, <cardoe@xxxxxxxxxx> wrote:
> --- 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 base is 64kb after the location of trampoline_phys */
>          lea     0x10000(%edi),%esp

I'm sorry for being picky, but the stack _base_ isn't where the stack
pointer should point initially, or else there would be no room on the
stack at all. Perhaps you had a reason for not using the wording I
did suggest, but whatever wording is chosen should not risk to cause
confusion (otherwise I think we'd be better off not adding any
comment here).

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -146,8 +146,6 @@ static void __init 
> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>  {
>      struct e820entry *e;
>      unsigned int i;
> -    /* Check for extra mem for mbi data if Xen is loaded via multiboot2 
> protocol. */
> -    UINTN extra_mem = efi_enabled(EFI_LOADER) ? 0 : (64 << 10);
>  
>      /* Populate E820 table and check trampoline area availability. */
>      e = e820map - 1;
> @@ -170,8 +168,7 @@ 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 )
>                  cfg.addr = (desc->PhysicalStart + len - cfg.size) & 
> PAGE_MASK;
>              /* fall through */
>          case EfiLoaderCode:
> @@ -686,6 +683,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 = max_t(UINTN, 64 << 10,
> +            (trampoline_end - trampoline_start) + 4096);

Considering the consuming code further up, I don't understand the
reason for the addition of 4096 here. And if there is a reason, I'm
pretty sure you actually mean PAGE_SIZE.

Jan


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