Re: [PATCH v2 20/23] x86: add multiboot2 protocol support for EFI platforms

[Daniel Kiper]

On Thu, Aug 27, 2015 at 06:01:26AM -0600, Jan Beulich wrote:
> >>> On 20.07.15 at 16:29, <address@hidden> wrote:
> > Signed-off-by: Daniel Kiper <address@hidden>
>
> For a patch of this size, no description at all seems rather
> problematic.
>
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -89,6 +89,13 @@ multiboot1_header_end:
> >                     0, /* Number of the lines - no preference. */ \
> >                     0  /* Number of bits per pixel - no preference. */
> >
> > +        /* Do not disable EFI boot services. */
> > +        mb2ht_init MB2_HT(EFI_BS), MB2_HT(OPTIONAL)
> > +
> > +        /* EFI64 entry point. */
> > +        mb2ht_init MB2_HT(ENTRY_ADDRESS_EFI64), MB2_HT(OPTIONAL), \
> > +                   sym_phys(__efi64_start)
>
> Iirc at least one of those two is what upstream doesn't have yet,
> or not all earlier grub2 versions might have. If so - what happens
> if the resulting Xen gets booted on an incapable grub? Silent death
> (with black screen)? Or at least some note to the user that the
> grub2 version is not suitable?

There is the code below which sends relevant message to COM1 and...
...well, VGA which of course does not make sense and should be fixed.

> > @@ -130,6 +146,119 @@ print_err:
> >  .Lhalt: hlt
> >          jmp     .Lhalt
> >
> > +        .code64
> > +
> > +__efi64_start:
> > +        cld
> > +
> > +        /* Check for Multiboot2 bootloader. */
> > +        cmp     $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
> > +        je      efi_multiboot2_proto
> > +
> > +        /* Jump to not_multiboot after switching CPU to x86_32 mode. */
> > +        lea     not_multiboot(%rip),%rdi
> > +        jmp     x86_32_switch
> > +
> > +efi_multiboot2_proto:
>
> .L

Why do you want to hide labels which could be useful during debugging?

> > +run_bs:
>
> Again.

Ditto.

> > +x86_32_switch:
> > +        cli
> > +
> > +        /* Initialise GDT. */
> > +        lgdt    gdt_boot_descr(%rip)
> > +
> > +        /* Reload code selector. */
> > +        ljmpl   *cs32_switch_addr(%rip)
> > +
> > +        .code32
> > +
> > +cs32_switch:
> > +        /* Initialise basic data segments. */
> > +        mov     $BOOT_DS,%edx
> > +        mov     %edx,%ds
> > +        mov     %edx,%es
> > +        mov     %edx,%fs
> > +        mov     %edx,%gs
> > +        mov     %edx,%ss
>
> I see no point in loading %fs and %gs with other than nul selectors.
> I also think %ss should be loaded first. Which reminds me - what
> about %rsp? Is it guaranteed to have its upper 32 bits clear, so you
> can re-use the stack in 32-bit mode? (If it is, saying so in a comment
> would be very desirable.)

I am not reusing %rsp value. %esp is initialized later in 32-bit code.
Stack is not used until %esp is not initialized.

[...]

> > --- 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);
>
> This is ugly; I'm sure there is a way to avoid these declarations.

This probably requires play with '#include "efi-boot.h"' and move
somewhere before efi_start(). Maybe something else. If it is not
a problem for you I can do that.

Daniel