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

From: Doug Goldstein <cardoe@cardoe.com>
To: Daniel Kiper <daniel.kiper@oracle.com>, xen-devel@lists.xenproject.org
Cc: jgross@suse.com, sstabellini@kernel.org,
	andrew.cooper3@citrix.com, pgnet.dev@gmail.com,
	ning.sun@intel.com, julien.grall@arm.com, jbeulich@suse.com,
	qiaowei.ren@intel.com, gang.wei@intel.com, fu.wei@linaro.org
Subject: Re: [PATCH v11 07/13] x86: add multiboot2 protocol support for EFI platforms
Date: Tue, 10 Jan 2017 14:51:27 -0600	[thread overview]
Message-ID: <fbe45e67-45af-abf7-41eb-ecc5f68e08ae@cardoe.com> (raw)
In-Reply-To: <96e86b22-8940-dce2-d1b9-92d1c132ad0e@cardoe.com>

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

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.

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

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

>> -    }
>>  }
>>  
>>  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
relocation. Which after this call finishes then it does the trampoline
relocations in assembly. The code currently makes the assumption it can
use any conventional memory range below 1mb (and unfortunately does the
math incorrectly and instead uses the region following the conventional
memory region). You need to use AllocatePages() otherwise you are
trampling memory that might have been allocated by the bootloader or any
multiboot modules (e.g. tboot) prior to Xen.

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

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