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

[Doug Goldstein <cardoe@xxxxxxxxxx>]

On 1/18/17 4:52 AM, Jan Beulich wrote:
>>>> 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).

So this was my perception. Maybe I'm totally wrong here.

|----------|---------|------|
a          b         c      d

example values
a = 0x0 (0)
b = 0x8c000 (trampoline_phys)
c = 0x9c000 (base of stack as it grows towards 0x8c000)
d = 0x100000 (1mb)

mov    sym_phys(trampoline_phys),%edi
lea    0x10000(%edi),%esp

I would think that 0x10000 + %edi would be the base or start of the
stack since its growing downwards towards b.

I'll use "ends" however and resubmit.

> 
>> --- 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
> 

You are correct. Given that the assembly is hardcoded at 64k there is no
reason for this. I had kicked around doing a similar max() call in
assembly instead of hardcoding the value but didn't do it. So I should
just remove this.

-- 
Doug Goldstein

Attachment: signature.asc
Description: OpenPGP digital signature

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