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

[Doug Goldstein <cardoe@xxxxxxxxxx>]

On 1/11/17 1:08 PM, Daniel Kiper wrote:
> On Mon, Jan 09, 2017 at 07:37:59PM -0600, 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.
> 
> 64 << 10 == 64 KiB which is the size of trampoline region reserved in
> xen/arch/x86/boot/head.S. Though I think that we should reserve real
> size needed for trampoline code. IIRC, it was discussed here earlier
> but there is no go for it in this patch series.

See my comments below but that's not entirely correct. But yes I would
avoid doing those changes in this series.

> 
>>>      /* 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.
>> 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.
> 
> Nice catch! Thanks a lot. I think that we should initialize cfg.size = 64 << 
> 10
> in efi_multiboot2(). It looks like real fix. extra_mem stuff is bogus.
> 

Except in head.S you start the stack 64kb after the end of the
trampoline size. So cfg.size = (64 << 10); won't work. It needs to be
the size of the trampoline + 64k.


>>> +    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.
> 
> This does not work in MB2 case.

You're looking at the oldest email. I've have further follow ups that
point that out and I've included a patch to fix the issues.

>>
>> So as an aside, IMHO this is where the series should end and the next
>> set of patches should be a follow on.
> 
> Hmmm... Why? If you do not apply rest of patches then MB2 does not
> work on all EFI platforms.
> 
> Daniel
> 

Q: How do you eat an elephant?
A: One bite at a time.

The point is we have 0 MB2 support presently. We can add it in
incremental hunks. Otherwise we get a patch series that's been floating
around for around 3 years and missed at least 2 releases where it should
have gotten in. We've only got several weeks before the 4.9 window
closes as well.

-- 
Doug Goldstein

Attachment: signature.asc
Description: OpenPGP digital signature

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