On Fri, Jan 30, 2015 at 07:06:53PM +0000, Andrew Cooper wrote:
> On 30/01/15 17:54, Daniel Kiper wrote:
> > Signed-off-by: Daniel Kiper <daniel.ki...@oracle.com>
> > ---
> > xen/arch/x86/boot/head.S | 174
> > +++++++++++++++++++++++++++++++++++--
> > xen/arch/x86/efi/efi-boot.h | 29 +++++++
> > xen/arch/x86/setup.c | 23 ++---
> > xen/arch/x86/x86_64/asm-offsets.c | 2 +
> > xen/common/efi/boot.c | 11 +++
> > 5 files changed, 222 insertions(+), 17 deletions(-)
> >
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> > index 7861057..89f5aa7 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -8,6 +8,7 @@
> > #include <asm/page.h>
> > #include <asm/msr.h>
> > #include <asm/cpufeature.h>
> > +#include <asm/processor.h>
> >
> > .text
> > .code32
> > @@ -57,6 +58,9 @@ ENTRY(start)
> > .long .Lmultiboot2_info_req_end - .Lmultiboot2_info_req
> > .long MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO
> > .long MULTIBOOT2_TAG_TYPE_MMAP
> > + .long MULTIBOOT2_TAG_TYPE_EFI_BS
> > + .long MULTIBOOT2_TAG_TYPE_EFI64
> > + .long MULTIBOOT2_TAG_TYPE_EFI64_IH
> > .Lmultiboot2_info_req_end:
> >
> > .align MULTIBOOT2_TAG_ALIGN
> > @@ -80,6 +84,19 @@ ENTRY(start)
> > .long 0 /* Number of the lines - no preference. */
> > .long 0 /* Number of bits per pixel - no preference. */
> >
> > + /* Do not disable EFI boot services. */
> > + .align MULTIBOOT2_TAG_ALIGN
> > + .short MULTIBOOT2_HEADER_TAG_EFI_BS
> > + .short MULTIBOOT2_HEADER_TAG_OPTIONAL
> > + .long 8 /* Tag size. */
> > +
> > + /* EFI64 entry point. */
> > + .align MULTIBOOT2_TAG_ALIGN
> > + .short MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS_EFI64
> > + .short MULTIBOOT2_HEADER_TAG_OPTIONAL
> > + .long 12 /* Tag size. */
> > + .long sym_phys(__efi64_start)
> > +
> > /* Multiboot2 header end tag. */
> > .align MULTIBOOT2_TAG_ALIGN
> > .short MULTIBOOT2_HEADER_TAG_END
> > @@ -94,6 +111,17 @@ ENTRY(start)
> > 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)
> > + .long BOOT_CS32
>
> .word
>
> ljmpl refers to an m32:16 not an m32:32
>
> > +
> > +efi_st:
> > + .quad 0
> > +
> > +efi_ih:
> > + .quad 0
> >
> > .Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!"
> > .Lbad_ldr_msg: .asciz "ERR: Not a Multiboot bootloader!"
> > @@ -124,6 +152,133 @@ print_err:
> > .Lhalt: hlt
> > jmp .Lhalt
> >
> > + .code64
> > +
> > +__efi64_start:
> > + cld
> > +
> > + /* Bootloaders may set multiboot[12].mem_lower to a nonzero value
> > */
> > + xor %edx,%edx
> > +
> > + /* Check for Multiboot2 bootloader */
> > + cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
> > + je efi_multiboot2_proto
> > +
> > + jmp not_multiboot
>
> not_multiboot is 32bit code. You must drop out of 64bit before using
> it, or make a 64bit variant.
>
> > +
> > +efi_multiboot2_proto:
> > + /* Skip Multiboot2 information fixed part */
> > + lea MB2_fixed_sizeof(%ebx),%ecx
> > +
> > +0:
> > + /* Get mem_lower from Multiboot2 information */
> > + cmpl $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx)
> > + jne 1f
> > +
> > + mov MB2_mem_lower(%ecx),%edx
> > + jmp 4f
> > +
> > +1:
> > + /* Get EFI SystemTable address from Multiboot2 information */
> > + cmpl $MULTIBOOT2_TAG_TYPE_EFI64,(%ecx)
> > + jne 2f
> > +
> > + lea MB2_efi64_st(%ecx),%esi
> > + lea efi_st(%rip),%edi
> > + movsq
>
> This is complete overkill for copying a 64bit variable out of the tag
> and into a local variable. Just use a plain 64bit load and store.
I am not sure what do you mean by "64bit load and store" but I have
just realized that we do not need these variables. They are remnants
from early developments when I thought that we need ImageHandle
and SystemTable here and later somewhere else.
> > + /* Do not go into real mode on EFI platform */
> > + movb $1,skip_realmode(%rip)
> > +
> > + jmp 4f
> > +
> > +2:
> > + /* Get EFI ImageHandle address from Multiboot2 information */
> > + cmpl $MULTIBOOT2_TAG_TYPE_EFI64_IH,(%ecx)
> > + jne 3f
> > +
> > + lea MB2_efi64_ih(%ecx),%esi
> > + lea efi_ih(%rip),%edi
> > + movsq
>
> And here.
Ditto.
> > + jmp 4f
> > +
> > +3:
> > + /* Is it the end of Multiboot2 information? */
> > + cmpl $MULTIBOOT2_TAG_TYPE_END,(%ecx)
> > + je run_bs
> > +
> > +4:
> > + /* Go to next Multiboot2 information tag */
> > + add MB2_tag_size(%ecx),%ecx
> > + add $(MULTIBOOT2_TAG_ALIGN-1),%ecx
> > + and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
> > + jmp 0b
> > +
> > +run_bs:
> > + push %rax
> > + push %rdx
>
> Does the EFI spec guarantee that we have a good stack to use at this point?
Unified Extensible Firmware Interface Specification, Version 2.4 Errata B,
section 2.3.4, x64 Platforms says: During boot services time the processor
is in the following execution mode: ..., 128 KiB, or more, of available
stack space. GRUB2 uses this stack too and do not move it to different
memory region. So, I think that here we are on safe side.
> > + /* Initialize BSS (no nasty surprises!) */
> > + lea __bss_start(%rip),%rdi
> > + lea _end(%rip),%rcx
> > + sub %rdi,%rcx
> > + xor %rax,%rax
>
> xor %eax,%eax is shorter.
>
> > + rep stosb
>
> It would be more efficient to make sure that the linker aligns
> __bss_start and _end on 8 byte boundaries, and use stosq instead.
Right but just for this. Is it pays? We do this only once.
However, if you wish...
> > + mov efi_ih(%rip),%rdi /* EFI ImageHandle */
> > + mov efi_st(%rip),%rsi /* EFI SystemTable */
> > + call efi_multiboot2
> > +
> > + pop %rcx
> > + pop %rax
> > +
> > + shl $10-4,%rcx /* Convert multiboot2.mem_lower to
> > bytes/16 */
> > +
> > + cli
>
> This looks suspiciously out of place. Surely the EFI spec doesn't
> permit entry with interrupts enabled?
Unified Extensible Firmware Interface Specification, Version 2.4 Errata B,
section 2.3.4, x64 Platforms says: During boot services time the processor
is in the following execution mode: ..., Interrupts are enabled–though no
interrupt services are supported other than the UEFI boot services timer
functions (All loaded device drivers are serviced synchronously by “polling.”).
So, I think that we should use BS with interrupts enabled and disable
them after ExitBootServices(). Hmmm... Now I think that we should use
cli immediately after efi_multiboot2() call.
[...]
> > diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> > index 6e98bc8..f50c10a 100644
> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -223,6 +223,9 @@ static void *__init efi_arch_allocate_mmap_buffer(UINTN
> > *map_size)
> >
> > static void __init efi_arch_pre_exit_boot(void)
> > {
> > + if ( !efi_loader )
> > + return;
> > +
> > if ( !trampoline_phys )
> > {
> > if ( !cfg.addr )
> > @@ -650,6 +653,32 @@ static bool_t __init
> > efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
> > return 1; /* x86 always uses a config file */
> > }
> >
> > +void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
> > *SystemTable)
> > +{
> > + EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
> > + UINTN cols, gop_mode = ~0, rows;
> > +
> > + efi_platform = 1;
> > + efi_loader = 0;
> > +
> > + 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);
> > +}
> > +
> > /*
> > * Local variables:
> > * mode: C
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index aebd010..8991b12 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -663,20 +663,23 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> > if ( ((unsigned long)cpu0_stack & (STACK_SIZE-1)) != 0 )
> > panic("Misaligned CPU0 stack.");
> >
> > - if ( efi_loader )
> > + if ( efi_platform )
> > {
> > - set_pdx_range(xen_phys_start >> PAGE_SHIFT,
> > - (xen_phys_start + BOOTSTRAP_MAP_BASE) >> PAGE_SHIFT);
> > + if ( efi_loader )
> > + {
> > + set_pdx_range(xen_phys_start >> PAGE_SHIFT,
> > + (xen_phys_start + BOOTSTRAP_MAP_BASE) >>
> > PAGE_SHIFT);
> >
> > - /* Clean up boot loader identity mappings. */
> > - destroy_xen_mappings(xen_phys_start,
> > - xen_phys_start + BOOTSTRAP_MAP_BASE);
> > + /* Clean up boot loader identity mappings. */
> > + destroy_xen_mappings(xen_phys_start,
> > + xen_phys_start + BOOTSTRAP_MAP_BASE);
> >
> > - /* Make boot page tables match non-EFI boot. */
> > - l3_bootmap[l3_table_offset(BOOTSTRAP_MAP_BASE)] =
> > - l3e_from_paddr(__pa(l2_bootmap), __PAGE_HYPERVISOR);
> > + /* Make boot page tables match non-EFI boot. */
> > + l3_bootmap[l3_table_offset(BOOTSTRAP_MAP_BASE)] =
> > + l3e_from_paddr(__pa(l2_bootmap), __PAGE_HYPERVISOR);
> > + }
> >
> > - memmap_type = loader;
> > + memmap_type = "EFI";
> > }
> > else if ( e820_raw_nr != 0 )
> > {
> > diff --git a/xen/arch/x86/x86_64/asm-offsets.c
> > b/xen/arch/x86/x86_64/asm-offsets.c
> > index ca3712f..d27596b 100644
> > --- a/xen/arch/x86/x86_64/asm-offsets.c
> > +++ b/xen/arch/x86/x86_64/asm-offsets.c
> > @@ -172,4 +172,6 @@ void __dummy__(void)
> > DEFINE(MB2_fixed_sizeof, sizeof(multiboot2_fixed_t));
> > OFFSET(MB2_tag_size, multiboot2_tag_t, size);
> > OFFSET(MB2_mem_lower, multiboot2_tag_basic_meminfo_t, mem_lower);
> > + OFFSET(MB2_efi64_st, multiboot2_tag_efi64_t, pointer);
> > + OFFSET(MB2_efi64_ih, multiboot2_tag_efi64_ih_t, pointer);
> > }
> > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> > index f8be3dd..c5725ca 100644
> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -75,6 +75,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);
> > +
>
> If any of these forward declarations are needed, they should be
They are needed because efi-boot.h is included before above
mentioned functions definitions.
> introduced in the appropriate create efi_$FOO patch. However, I can't
I thought about that during development. However, I stated that if I do what
you suggest then it is not clear who needs/uses these forward declarations.
> spot a need for any of them.
efi-boot.h:efi_multiboot2() uses them.
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
