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

From: Daniel Kiper <daniel.kiper@oracle.com>
To: Doug Goldstein <cardoe@cardoe.com>
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,
	xen-devel@lists.xenproject.org, 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: Wed, 11 Jan 2017 20:08:21 +0100	[thread overview]
Message-ID: <20170111190821.GY32675@olila.local.net-space.pl> (raw)
In-Reply-To: <96e86b22-8940-dce2-d1b9-92d1c132ad0e@cardoe.com>

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