Re: [kvm-unit-tests PATCH v2 5/6] cstart64.S: x86_64 bootstrapping after exiting EFI

Krish Sadhukhan
2021-08-24T22:11:48Z

On 8/19/21 4:33 AM, Varad Gautam wrote:
> EFI sets up long mode with arbitrary state before calling the
> image entrypoint. To run the testcases at hand, it is necessary
> to redo some of the bootstrapping to not rely on what EFI
> provided.
>
> Adapt start64() for EFI testcases to fixup %rsp/GDT/IDT/TSS and
> friends, and jump here after relocation from efi_main. Switch to
> RIP-relative addressing where necessary.
>
> Initially leave out:
> - AP init - leave EFI to single CPU
> - Testcase arg passing
>
> Signed-off-by: Varad Gautam<varad.gautam@suse.com>
> ---
> v2: Fix TSS setup in cstart64 on CONFIG_EFI.
>
> x86/cstart64.S | 70 +++++++++++++++++++++++++++++++++++++++++++++-----
> x86/efi_main.c | 1 +
> 2 files changed, 65 insertions(+), 6 deletions(-)
>
> diff --git a/x86/cstart64.S b/x86/cstart64.S
> index 98e7848..547f3fb 100644
> --- a/x86/cstart64.S
> +++ b/x86/cstart64.S

I am wondering if it's cleaner to create a separate .S file for EFI
because so many #ifdefs are reducing readability of the code.

> @@ -242,16 +242,17 @@ ap_start32:
>
> .code64
> save_id:
> -#ifndef CONFIG_EFI
> movl $(APIC_DEFAULT_PHYS_BASE + APIC_ID), %eax
> movl (%rax), %eax
> shrl $24, %eax
> +#ifdef CONFIG_EFI
> + lock btsl %eax, online_cpus(%rip)
> +#else
> lock btsl %eax, online_cpus
> #endif
> retq
>
> ap_start64:
> -#ifndef CONFIG_EFI
> call reset_apic
> call load_tss
> call enable_apic
> @@ -259,12 +260,38 @@ ap_start64:
> call enable_x2apic
> sti
> nop
> +#ifdef CONFIG_EFI
> + lock incw cpu_online_count(%rip)
> +#else
> lock incw cpu_online_count
> #endif
> +
> 1: hlt
> jmp 1b
>
> #ifdef CONFIG_EFI
> +setup_gdt64:
> + lgdt gdt64_desc(%rip)
> + call load_tss
> +
> + setup_segments
> +
> + movabsq $flush_cs, %rax
> + pushq $0x8
> + pushq %rax
> + retfq
> +flush_cs:
> + ret
> +
> +setup_idt64:
> + lidtq idt_descr(%rip)
> + ret
> +
> +setup_cr3:
> + movabsq $ptl4, %rax
> + mov %rax, %cr3
> + ret
> +
> .globl _efi_pe_entry
> _efi_pe_entry:
> # EFI image loader calls this with rcx=efi_handle,
> @@ -276,15 +303,27 @@ _efi_pe_entry:
> pushq %rsi
>
> call efi_main
> -#endif
>
> +.globl start64
> start64:
> -#ifndef CONFIG_EFI
> + cli
> + lea stacktop(%rip), %rsp
> +
> + setup_percpu_area
> + call setup_gdt64
> + call setup_idt64
> + call setup_cr3
> +#else
> +start64:
> +#endif
> call reset_apic
> +#ifndef CONFIG_EFI
> call load_tss
> +#endif
> call mask_pic_interrupts
> call enable_apic
> call save_id
> +#ifndef CONFIG_EFI
> mov mb_boot_info(%rip), %rbx
> mov %rbx, %rdi
> call setup_multiboot
> @@ -292,18 +331,24 @@ start64:
> mov mb_cmdline(%rbx), %eax
> mov %rax, __args(%rip)
> call __setup_args
> +#endif
>
> call ap_init
> call enable_x2apic
> call smp_init
>
> +#ifdef CONFIG_EFI
> + mov $0, %edi
> + mov $0, %rsi
> + mov $0, %rdx
> +#else
> mov __argc(%rip), %edi
> lea __argv(%rip), %rsi
> lea __environ(%rip), %rdx
> +#endif
> call main
> mov %eax, %edi
> call exit
> -#endif
>
> .globl setup_5level_page_table
> setup_5level_page_table:
> @@ -330,6 +375,7 @@ online_cpus:
> load_tss:
> #ifndef CONFIG_EFI
> lidtq idt_descr
> +#endif
> mov $(APIC_DEFAULT_PHYS_BASE + APIC_ID), %eax
> mov (%rax), %eax
> shr $24, %eax
> @@ -337,6 +383,18 @@ load_tss:
> shl $4, %ebx
> mov $((tss_end - tss) / max_cpus), %edx
> imul %edx
> +#ifdef CONFIG_EFI
> + lea tss(%rip), %rax
> + lea tss_descr(%rip), %rcx
> + add %rbx, %rcx
> + mov %ax, 2(%rcx)
> + shr $16, %rax
> + mov %al, 4(%rcx)
> + shr $8, %rax
> + mov %al, 7(%rcx)
> + shr $8, %rax
> + mov %eax, 8(%rcx)
> +#else
> add $tss, %rax
> mov %ax, tss_descr+2(%rbx)
> shr $16, %rax
> @@ -345,9 +403,9 @@ load_tss:
> mov %al, tss_descr+7(%rbx)
> shr $8, %rax
> mov %eax, tss_descr+8(%rbx)
> +#endif
> lea tss_descr-gdt64(%rbx), %rax
> ltr %ax
> -#endif
> ret
>
> ap_init:
> diff --git a/x86/efi_main.c b/x86/efi_main.c
> index be3f9ab..c542fb9 100644
> --- a/x86/efi_main.c
> +++ b/x86/efi_main.c
> @@ -7,6 +7,7 @@ efi_system_table_t *efi_system_table = NULL;
>
> extern char ImageBase;
> extern char _DYNAMIC;
> +extern void start64(void);
>
> static void efi_free_pool(void *ptr)
> {