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.
> > /* 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.
[...]
> > +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.
This does not work in MB2 case.
[...]
> > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> > index 0a93e61..70ed836 100644
> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -79,6 +79,17 @@ static size_t wstrlen(const CHAR16 * s);
> > static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz);
> > static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
> >
> > +static void efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
> > *SystemTable);
> > +static void efi_console_set_mode(void);
> > +static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(void);
> > +static UINTN efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
> > + UINTN cols, UINTN rows, UINTN depth);
> > +static void efi_tables(void);
> > +static void setup_efi_pci(void);
> > +static void efi_variables(void);
> > +static void efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN
> > gop_mode);
> > +static void efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
> > *SystemTable);
> > +
> > static const EFI_BOOT_SERVICES *__initdata efi_bs;
> > static UINT32 __initdata efi_bs_revision;
> > static EFI_HANDLE __initdata efi_ih;
> >
>
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
