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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.01.17 at 14:33, <cardoe@xxxxxxxxxx> wrote:
> 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.

Well, to me "start" and "base" are the lowest address and "end" the
highest (or one byte beyond).

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

Well, I don't mind the max() (albeit it's not very useful, especially
if trampoline size would ever get pretty close to 64k). And as said
in reply to the earlier version - I think this would better be checked
at build time (see the various ASSERT()s at the end of xen.lds.S).

Jan


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