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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

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.

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

> >>> No where in the EFI + MB2 code path is cfg.size ever initialized. Its
> >>> only initialized in the straight EFI case. The result is that cfg.addr
> >>> is set to the section immediately following this. Took a bit to
> >>> trackdown because I checked for memory overlaps with where Xen was
> >>> loaded and where it was relocated to but forgot to check for overlaps
> >>> with the trampoline code. This is the address where the trampoline jumps
> >>> are copied.
> >>>
> >>> Personally I'd like to see an ASSERT added or the code swizzled around
> >>> in such a way that its not possible to get into a bad state. But this is
> >>> probably another patch series.
> >>>
> >>>>              /* fall through */
> >>>>          case EfiLoaderCode:
> >>>> @@ -210,12 +213,14 @@ static void *__init 
> >>>> efi_arch_allocate_mmap_buffer(UINTN map_size)
> >>>>
> >>>>  static void __init efi_arch_pre_exit_boot(void)
> >>>>  {
> >>>> -    if ( !trampoline_phys )
> >>>> -    {
> >>>> -        if ( !cfg.addr )
> >>>> -            blexit(L"No memory for trampoline");
> >>>> +    if ( trampoline_phys )
> >>>> +        return;
> >>>> +
> >>>> +    if ( !cfg.addr )
> >>>> +        blexit(L"No memory for trampoline");
> >>>> +
> >>>> +    if ( efi_enabled(EFI_LOADER) )
> >>>>          relocate_trampoline(cfg.addr);
> >>
> >> Why is this call even here anymore? Its called in
> >> efi_arch_memory_setup() already. If it was unable to allocate memory
> >> under the 1mb region its just going to trample over ANY conventional
> >> memory region that might be in use.
> >
> > Trampoline is relocated in xen/arch/x86/boot/head.S.
>
> For the MB2/MB case. But for the straight EFI case its called in
> efi_arch_memory_setup() and then you've added the wrapper of

That is true.

> efi_enabled(EFI_LOADER) which in theory would have it called again in
> the straight EFI case if trampoline_phys isn't set and cfg.addr is set.

Why "in theory"?

> >>>> -    }
> >>>>  }
> >>>>
> >>>>  static void __init noreturn efi_arch_post_exit_boot(void)
> >>>> @@ -653,6 +658,43 @@ static bool_t __init 
> >>>> efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
> >>>>
> >>>>  static void efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { 
> >>>> }
> >>>>
> >>>> +paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> >>>> *SystemTable)
> >>>> +{
> >>>> +    EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
> >>>> +    UINTN cols, gop_mode = ~0, rows;
> >>>> +
> >>>> +    __set_bit(EFI_BOOT, &efi_flags);
> >>>> +    __set_bit(EFI_RS, &efi_flags);
> >>>> +
> >>>> +    efi_init(ImageHandle, SystemTable);
> >>>> +
> >>>> +    efi_console_set_mode();
> >>>> +
> >>>> +    if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
> >>>> +                           &cols, &rows) == EFI_SUCCESS )
> >>>> +        efi_arch_console_init(cols, rows);
> >>>> +
> >>>> +    gop = efi_get_gop();
> >>>> +
> >>>> +    if ( gop )
> >>>> +        gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
> >>>> +
> >>>> +    efi_arch_edd();
> >>>> +    efi_arch_cpu();
> >>>> +
> >>>> +    efi_tables();
> >>>> +    setup_efi_pci();
> >>>> +    efi_variables();
> >>>
> >>> This is probably where you missed the call to "efi_arch_memory_setup();"
> >>> that gave me hell above.
> >>
> >> Well it turns out that calling "efi_arch_memory_setup()" isn't correct
> >> because it also messes with the page tables AND also does the trampoline
> >
> > Yep.
> >
> >> relocation. Which after this call finishes then it does the trampoline
> >> relocations in assembly. The code currently makes the assumption it can
> >
> > I am not sure what do you mean here.
> >
> >> use any conventional memory range below 1mb (and unfortunately does the
> >> math incorrectly and instead uses the region following the conventional
> >
> > Which code? Which math?
>
> The code where cfg.size = 0 and the extra_mem was missing.

This should be fixed as I stated above.

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

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

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

Daniel

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