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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

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.

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

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

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

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

> multiboot modules (e.g. tboot) prior to Xen.

How come?

> I'm just going to write a patch to fix the issues that I've found thus
> far. I believe at this point there is something wrong with the page
> tables because setting cr0 for the non-0 CPU in
> trampoline_protmode_entry causes the machine to reboot.

I have a feeling that problem lays somewhere else. Probably far before
trampoline_protmode_entry.

> I've also found where we have the possibility to call
> relocate_trampoline() twice in the EFI case. Its protected by a check to
> trampoline_phys but I'm not sure why we even have the code there to be
> able to do so.

Consider case when boot services use memory below 1 MiB.

Thank you for testing my patch series.

Daniel

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