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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

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.

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

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

Daniel

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