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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.07.15 at 16:29, <daniel.kiper@xxxxxxxxxx> wrote:
> Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>

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?

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

> +run_bs:

Again.

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

> @@ -182,7 +318,7 @@ multiboot2_proto:
>          and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
>          jmp     0b
>  
> -trampoline_setup:
> +trampoline_bios_setup:

Another .L candidate.

>          /* Set up trampoline segment 64k below EBDA */
>          movzwl  0x40e,%ecx          /* EBDA segment */
>          cmp     $0xa000,%ecx        /* sanity check (high) */
> @@ -198,12 +334,13 @@ trampoline_setup:
>           * multiboot structure (if available) and use the smallest.
>           */
>          cmp     $0x100,%edx         /* is the multiboot value too small? */
> -        jb      2f                  /* if so, do not use it */
> +        jb      trampoline_setup    /* if so, do not use it */
>          shl     $10-4,%edx
>          cmp     %ecx,%edx           /* compare with BDA value */
>          cmovb   %edx,%ecx           /* and use the smaller */
>  
> -2:      /* Reserve 64kb for the trampoline */
> +trampoline_setup:
> +        /* Reserve 64kb for the trampoline. */

And one more.

> @@ -220,6 +357,13 @@ trampoline_setup:
>          add     $12,%esp            /* Remove reloc() args from stack. */
>          mov     %eax,sym_phys(multiboot_ptr)
>  
> +        /*
> +         * Do not zero BSS on EFI platform here.
> +         * It was initialized earlier.
> +         */
> +        cmpb    $1,sym_phys(skip_realmode)
> +        je      1f

So why can't this be done uniformly here? I didn't see you write any
variable in .bss by now. And if there was any, moving it to .data
would avoid zeroing .bss in two different places. Or wait - I see, it's
the C code in efi_multiboot2() that you need to take care of. Well,
probably okay then this way, but please extend the comment at the
new zeroing site to say _why_ it needs to be done differently/earlier.

> +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_PLATFORM, &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();
> +    gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
> +    efi_arch_edd();
> +    efi_tables();
> +    setup_efi_pci();
> +    efi_variables();
> +    efi_set_gop_mode(gop, gop_mode);
> +    efi_exit_boot(ImageHandle, SystemTable);
> +
> +    return cfg.addr;

With all the refactoring in the earlier patches it is now almost impossible
to know what cfg.addr holds at this point - it looks to me as if this is
entirely unrelated to the mem_lower value the caller wants to derive
from this. Please at least add a brief comment.

> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -13,6 +13,11 @@ struct efi __read_mostly efi = {
>       .smbios3 = EFI_INVALID_TABLE_ADDR
>  };
>  
> +void __init efi_multiboot2(void)
> +{
> +    /* TODO: Fail if entered! */

No "TODO" or alike please in non-RFC patch submissions (unless there
are exceptional circumstances). Here - BUG() (or panic() if there was
a remote chance of this being reached).

> @@ -708,7 +708,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          l3_bootmap[l3_table_offset(BOOTSTRAP_MAP_BASE)] =
>              l3e_from_paddr(__pa(l2_bootmap), __PAGE_HYPERVISOR);
>  
> -        memmap_type = loader;
> +        memmap_type = "EFI";

What's wrong with keeping this line as is?

> +    }
> +    else if ( efi_enabled(EFI_PLATFORM) )
> +    {
> +        memmap_type = "EFI";
>      }

The braces are now pointless here.

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

Jan

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