Re: [Xen-devel] [PATCH v3 13/16 - RFC] x86: add multiboot2 protocol support for EFI platforms

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, May 25, 2016 at 03:32:37AM -0600, Jan Beulich wrote:
> >>> On 15.04.16 at 14:33, <daniel.kiper@xxxxxxxxxx> wrote:
> > @@ -100,19 +107,29 @@ multiboot2_header_end:
> >  gdt_boot_descr:
> >          .word   6*8-1
> >          .long   sym_phys(trampoline_gdt)
> > +        .long   0 /* Needed for 64-bit lgdt */
> > +
> > +cs32_switch_addr:
> > +        .long   sym_phys(cs32_switch)
> > +        .word   BOOT_CS32
> >
> >  .Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!"
> >  .Lbad_ldr_msg: .asciz "ERR: Not a Multiboot bootloader!"
> > +.Lbad_ldr_mb2: .asciz "ERR: On EFI platform use latest Multiboot2 
> > compatible bootloader!"
>
> What is "latest" going to mean 5 years from now?

:-))) I will try to fix it.

> >          .section .init.text, "ax", @progbits
> >
> >  bad_cpu:
> >          mov     $(sym_phys(.Lbad_cpu_msg)),%esi # Error message
> > -        jmp     print_err
> > +        mov     $0xB8000,%edi                   # VGA framebuffer
> > +        jmp     1f
> >  not_multiboot:
> >          mov     $(sym_phys(.Lbad_ldr_msg)),%esi # Error message
> > -print_err:
> > -        mov     $0xB8000,%edi  # VGA framebuffer
> > +        mov     $0xB8000,%edi                   # VGA framebuffer
> > +        jmp     1f
> > +mb2_too_old:
> > +        mov     $(sym_phys(.Lbad_ldr_mb2)),%esi # Error message
> > +        xor     %edi,%edi                       # No VGA framebuffer
>
> Leaving aside that "framebuffer" really is a bad term here (we're
> talking of text mode output after all), limiting the output to serial

Yep, but then we should change this in other places too. Maybe in separate 
patch.

> isn't going to be very helpful in the field, I'm afraid. Even more so
> that there's no guarantee for a UART to be at port 0x3f8. That's

Right but we do not have big choice here at very early boot stage... :-(((

> not much of a problem for the other two messages as people are
> unlikely to try to boot Xen on an unsuitable system, but I view it
> as quite possible for Xen to be tried to get booted with an
> unsuitable grub2.
>
> IOW - this needs a better solution, presumably using EFI boot
> service output functions.

No way, here boot services are dead. GRUB2 (or other loader)
shutdown them... :-(((

> > @@ -130,6 +149,130 @@ print_err:
> >  .Lhalt: hlt
> >          jmp     .Lhalt
> >
> > +        .code64
> > +
> > +__efi64_start:
>
> As long as we have split files under boot/, I think I'd prefer 64-bit
> code to only go into x86_64.S.
>
> > +        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
>
> What I've said above would also eliminate the need to switch to
> 32-bit mode just for emitting an error message and halting the
> system.

It is not possible. We just know that we run on EFI platform here.
However, we are not able to get EFI SystemTable pointer.

> > +efi_multiboot2_proto:
>
> .Lefi_multiboot2_proto

OK if you insist. However, I think that we are loosing helpful
debug information this way.

> > +        /*
> > +         * Multiboot2 information address is 32-bit,
> > +         * so, zero higher half of %rbx.
> > +         */
> > +        mov     %ebx,%ebx
>
> Wait, no - that's a protocol bug then. We're being entered in 64-bit
> mode here, so registers should be in 64-bit clean state.

You mean higher half cleared. Right? This is not guaranteed.
Please check this: 
http://lists.gnu.org/archive/html/grub-devel/2016-03/msg00304.html

> > +        /* Skip Multiboot2 information fixed part. */
> > +        lea     (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%rcx
> > +        and     $~(MULTIBOOT2_TAG_ALIGN-1),%rcx
>
> Or if there really was a reason to do the above, and if there is a
> reason not to assume this data is located below 4Gb, then
> calculations like this could avoid the REX64 prefix by using %ecx.

Here you said something different:
  http://lists.xen.org/archives/html/xen-devel/2015-03/msg03557.html

So?

> > +0:
> > +        /* Get EFI SystemTable address from Multiboot2 information. */
> > +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%rcx)
> > +        jne     1f
> > +
> > +        mov     MB2_efi64_st(%rcx),%rsi
> > +
> > +        /* Do not clear BSS twice and do not go into real mode. */
> > +        movb    $1,skip_realmode(%rip)
>
> How is the setting of skip_realmode related to the clearing of BSS?
> Oh, I've found the connection below (albeit see there for its
> validity), but I think mentioning this here is more confusing than
> clarifying.

In general I have a problem skip_realmode. The name itself is confusing
here and there. Probably it should be changed. However, I do not have
idea for good compact name describing all skip_realmode usages. And I do
not think that we should add another variable which will be used in similar
way but with different name to clarify situation.

> > +        jmp     3f
> > +
> > +1:
> > +        /* Get EFI ImageHandle address from Multiboot2 information. */
> > +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI64_IH,MB2_tag_type(%rcx)
> > +        jne     2f
> > +
> > +        mov     MB2_efi64_ih(%rcx),%rdi
> > +        jmp     3f
> > +
> > +2:
> > +        /* Is it the end of Multiboot2 information? */
> > +        cmpl    $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx)
> > +        je      run_bs
> > +
> > +3:
> > +        /* Go to next Multiboot2 information tag. */
> > +        add     MB2_tag_size(%rcx),%ecx
> > +        add     $(MULTIBOOT2_TAG_ALIGN-1),%rcx
> > +        and     $~(MULTIBOOT2_TAG_ALIGN-1),%rcx
> > +        jmp     0b
>
> At least down to here it looks like this would better live in a C helper
> function. Did you consider this?

I will think about it.

> > +run_bs:
> > +        push    %rax
> > +        push    %rdi
> > +
> > +        /*
> > +         * Initialize BSS (no nasty surprises!).
> > +         * It must be done earlier than in BIOS case
> > +         * because efi_multiboot2() touches it.
> > +         */
> > +        lea     __bss_start(%rip),%rdi
> > +        lea     __bss_end(%rip),%rcx
> > +        sub     %rdi,%rcx
> > +        shr     $3,%rcx
> > +        xor     %eax,%eax
> > +        rep     stosq
>
> Please let's not repeat pre-existing mistakes: REP is not an
> instruction, and STOSB is not an operand. IOW there should be
> just a single space between the two.

OK.

[...]

> > @@ -170,12 +313,19 @@ multiboot2_proto:
> >          jne     1f
> >
> >          mov     MB2_mem_lower(%ecx),%edx
> > -        jmp     trampoline_setup
> > +        jmp     trampoline_bios_setup
> >
> >  1:
> > +        /* EFI mode is not supported via legacy BIOS path. */
> > +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx)
> > +        je      mb2_too_old
> > +
> > +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%ecx)
> > +        je      mb2_too_old
>
> According to the comment we're on the legacy BIOS boot path
> here, yet at mb2_too_old you assume no text mode VGA. I.e. I'm
> now even confused about the output handling there.

It is correct. Old GRUB2 (or other legacy multiboot2 compatible boot
loader) which runs on EFI platform and does not have my features will
jump into that path. And we do not support that, so, we should fail
in the best possible way here.

Your comment suggest that code comment should be improved and
phrased precisely. I will do that.

> > @@ -221,6 +372,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 what if skip_realmode is set on a legacy BIOS system because
> of the command line option or the use of TBOOT?
>
> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -228,6 +228,9 @@ static void *__init efi_arch_allocate_mmap_buffer(UINTN 
> > map_size)
> >
> >  static void __init efi_arch_pre_exit_boot(void)
> >  {
> > +    if ( !efi_enabled(EFI_LOADER) )
> > +        return;
> > +
> >      if ( !trampoline_phys )
>
> Please connect the two if()-s - afaiu you really only care about the
> trampoline handling to not be done. Otherwise the new conditional
> would probably rather belong on the (arch-independent) call site.
>
> > +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);
>
> __set_bit()
>
> > +    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() is not needed here. boot_cpu_data
> > +     * is set later in xen/arch/x86/boot/head.S.
> > +     */
>
> That's not a good explanation. Is there any harm in calling it? If not,
> I'd suggest calling it here just to avoid missing a dependency on what
> it does in any of the functions called subsequently. If there is, the
> precise details of that is what you should say here.

It looks that it should work. However, probably there was an reason for
which this call was disabled. So, I will reinvestigate the issue.

> > +    efi_tables();
> > +    setup_efi_pci();
> > +    efi_variables();
> > +
> > +    if ( gop )
> > +        efi_set_gop_mode(gop, gop_mode);
> > +
> > +    efi_exit_boot(ImageHandle, SystemTable);
> > +
> > +    /* Return highest available memory address below 1 MiB. */
> > +    return cfg.addr;
>
> Didn't you say on some systems all of the memory below 640k is in
> use by boot/loader code/data? If so, what meaning has the value
> you return here (I don't recall the consumer side special casing any
> error value)?

It is usually correct because boot services are dead here. However, if it
does not (e.g. somebody called Xen with mapbs argument) then we should fail
because there is no memory for trampoline.

> > --- 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! */
> > +}
>
> Why not just BUG()? What exactly you do here doesn't seem to
> matter, as the symbol is unreachable in this case anyway (you
> only need it to please the linker).

We should print meaningful message here using boot services.
To do that we need a few line of assembly probably. And BUG()
is not solution here.

> > @@ -728,7 +728,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";
> > +    }
> > +    else if ( efi_enabled(EFI_PLATFORM) )
> > +    {
> > +        memmap_type = "EFI";
> >      }
>
> Stray braces.
>
> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -192,7 +192,7 @@ SECTIONS
> >    } :text
> >
> >    /* Align BSS to speedup its initialization. */
> > -  . = ALIGN(4);
> > +  . = ALIGN(8);
> >    .bss : {                     /* BSS */
> >         . = ALIGN(STACK_SIZE);
> >         __bss_start = .;
> > @@ -207,7 +207,7 @@ SECTIONS
> >         *(.bss.percpu.read_mostly)
> >         . = ALIGN(SMP_CACHE_BYTES);
> >         __per_cpu_data_end = .;
> > -       . = ALIGN(4);
> > +       . = ALIGN(8);
> >         __bss_end = .;
>
> Is that really worth it? I.e. is going from STOSD to STOSQ really a
> meaningful win?

Probably yes but I do not think that anybody will be see boot time
difference. On the other hand why not do that? It does not cost a lot.

> > @@ -936,6 +947,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> > *SystemTable)
> >
> >  #ifndef CONFIG_ARM /* Disabled until runtime services implemented. */
> >      set_bit(EFI_PLATFORM, &efi.flags);
> > +    set_bit(EFI_LOADER, &efi.flags);
> >  #endif
>
> So _neither_ of the two bits get set for ARM? I'm even more puzzled
> now, and hence think even more that this can't be fine grained enough.

As I saw code around old efi_enabled, which was replaced by EFI_PLATFORM,
was changed recently. So, I must rethink this stuff too.

Daniel

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