Re: [PATCH v11 07/13] x86: add multiboot2 protocol support for EFI platforms

[Doug Goldstein <cardoe@cardoe.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 5964 bytes --]

On 1/12/17 2:28 PM, Daniel Kiper wrote:
> On Thu, Jan 12, 2017 at 09:52:15AM -0600, Doug Goldstein wrote:
>> On 1/12/17 6:50 AM, Daniel Kiper wrote:
>>> On Wed, Jan 11, 2017 at 02:20:15PM -0600, Doug Goldstein wrote:
>>>> On 1/11/17 1:47 PM, Daniel Kiper wrote:
>>>>> On Tue, Jan 10, 2017 at 02:51:27PM -0600, Doug Goldstein wrote:
>>>>>> On 1/9/17 7:37 PM, Doug Goldstein wrote:
>>>>>>> On 12/5/16 4:25 PM, Daniel Kiper wrote:
>>>>>>
>>>>>>>> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
>>>>>>>> index 62c010e..dc857d8 100644
>>>>>>>> --- a/xen/arch/x86/efi/efi-boot.h
>>>>>>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>>>>>>> @@ -146,6 +146,8 @@ 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);
>>>>>>>
>>>>>>> Just wondering where the constant came from? And if there should be a
>>>>>>> little bit of information about it. To me its just weird to shift 64.
>>>>>>
>>>>>> Its the size of the stack used in the assembly code.
>>>>>
>>>>> No, it is trampoline region size.
>>>>
>>>> trampoline + stack in head.S We take the address where we're going to
>>>> copy the trampoline and set the stack to 0x10000 past it.
>>>
>>> I suppose that you think about this:
>>>
>>>         /* Switch to low-memory stack.  */
>>>         mov     sym_fs(trampoline_phys),%edi
>>>         lea     0x10000(%edi),%esp
>>>
>>> However, trampoline region size is (should be) 64 KiB. No way. Please
>>> look below for more details.
>>
>> The trampoline + stack are 64kb together. The stack grows down and the
>> trampoline grows up. The stack starts at 64kb past the start of the
>> trampoline. %edi is the start of the trampoline.
> 
> Yep. I think that right now we are on the same boat.
> 
>>>>>>>>      /* Populate E820 table and check trampoline area availability. */
>>>>>>>>      e = e820map - 1;
>>>>>>>> @@ -168,7 +170,8 @@ 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 && desc->PhysicalStart + len > cfg.addr )
>>>>>>>> +                 len >= cfg.size + extra_mem &&
>>>>>>>> +                 desc->PhysicalStart + len > cfg.addr )
>>>>>>>>                  cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK;
>>>>>>>
>>>>>>> So this is where the current series blows up and fails on real hardware.
>>>>>>
>>>>>> Honestly this was my misunderstanding and this shouldn't ever be used to
>>>>>> get memory for the trampoline. This also has the bug in it that it needs
>>>>>> to be:
>>>>>>
>>>>>> ASSERT(cfg.size > 0);
>>>>>> cfg.addr = (desc->PhysicalStart + len - (cfg.size + extra_mem) & PAGE_MASK;
>>>>>
>>>>> As I said earlier. This extra_mem stuff is (maybe) wrong and should be fixed
>>>>> in one way or another. Hmmm... It looks OK. I will double check it because
>>>>> I do not looked at this code long time and maybe I am missing something.
>>>>
>>>> cfg.size needs to be the size of the trampolines + stack.
>>>
>>> It looks that during some code rearrangement I moved one instruction too
>>> much to trampoline_bios_setup. So, I can agree that right now cfg.size
>>> should be properly initialized. Though it should be cfg.size = 64 << 10.
>>> Then extra_mem should be dropped.
>>
>> That's fine as long as its clear that 64kb is for the trampoline + the
>> stack.
> 
> OK, but there are two stacks. We talk about "low-memory stack". I will improve
> the comment.
> 
> [...]
> 
>>>>>> memory region). You need to use AllocatePages() otherwise you are
>>>>>> trampling memory that might have been allocated by the bootloader or any
>>>>>
>>>>> Bootloader code/data should be dead here.
>>>>
>>>> Correct. Unfortunately on my Lenovo laptop and my Intel NUCs I can't
>>>> currently call ExitBootServices and a timer that iPXE has wired up has
>>>
>>> If you disable an important wheel in a machine you should not expect
>>> that the machine will work. Sorry! No way!
>>
>> Speak to your co-workers Konrad and Boris. We've had long email threads
>> about how certain hardware does not work with the way Xen calls
>> ExitBootServices.
> 
> Could you be more precise what is wrong? Or at least send links to
> relevant threads.

There have been several on the ML over the past 2 years. A quick Google
search turns these up.

http://xen.markmail.org/message/f6lx2ab4o2fch35r
https://lists.xenproject.org/archives/html/xen-devel/2015-01/msg03164.html


> 
>>>> some memory reserved down there and it was getting trampled. The real
>>>
>>> I still do not know why remnants of iPXE should run at this Xen boot stage.
>>> It looks like an iPXE bug and IMO it should be fixed first.
>>
>> Like I said above, its because on this machine I am unable to call Xen's
>> EBS.
> 
> I do not understand how ExitBootServices() call is related to iPXE timer remnants
> or so. Though if it is related somehow then I think that you should blame machine
> and/or iPXE designer/developer not Xen developer.

iPXE registers a callback for when EBS is called to clean up a timer.

> 
>>>> answer is that we need to fix up stock Xen to be able to always call EBS.
>>>
>>> It looks that ExitBootServices() is always called. So, I do not think that
>>> anything have to be fixed.
>>
>> It is commented out of this board using the patchset that Konrad
>> submitted to the ML years ago.
> 
> I do not know what patchset do you mean. Could you send it?
> 

See the above link.

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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