|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v11 07/13] x86: add multiboot2 protocol support for EFI platforms
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:
signature.asc _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |