Re: [Xen-devel] [PATCH v2 6/6] x86/boot: Do not use trampoline for no-real-mode boot paths

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.08.2019 17:02, David Woodhouse wrote:
> From: David Woodhouse <dwmw@xxxxxxxxxxxx>
>
> Where booted from EFI or with no-real-mode, there is no need to stomp
> on low memory with the 16-boot code. Instead, just go straight to
> trampoline_protmode_entry() at its physical location within the Xen
> image.
>
> For now, the boot code (including the EFI loader path) still determines
> what the trampoline_phys address should be. The trampoline is actually
> relocated for that address and copied into low memory, from a
> relocate_trampoline() call made from __start_xen().
I assume this talks about the real mode part of the trampoline, as
opposed to the next paragraph? Would be nice if you made this
explicit.

> For subsequent AP startup and wakeup, the 32-bit trampoline can't
> trivially be used in-place as that region isn't mapped. So copy it
> down to low memory too, having relocated it (again) to work from
> there.
trampoline_protmode_entry gets entered with CR0.PG=0, i.e. at
that point there's not even the question yet of there being a
mapping. Subsequently idle_pg_table gets loaded into CR3. I wonder
if, rather than relocating the 32-bit part of the trampoline, it
wouldn't be better to install a 1:1 mapping into idle_pg_table.
Such a mapping would need to have the G bits clear in order to
not conflict with PV guest mappings of the same linear addresses.

> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -152,9 +152,9 @@ static void acpi_sleep_prepare(u32 state)
>           return;
>   
>       if ( acpi_sinfo.vector_width == 32 )
> -        *(uint32_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
> +        *(uint32_t *)wakeup_vector_va = trampsym_phys(wakeup_start);
>       else
> -        *(uint64_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
> +        *(uint64_t *)wakeup_vector_va = trampsym_phys(wakeup_start);
>   }
>   
>   static void acpi_sleep_post(u32 state) {}
> @@ -388,7 +388,7 @@ static void tboot_sleep(u8 sleep_state)
>       g_tboot_shared->acpi_sinfo.wakeup_vector = acpi_sinfo.wakeup_vector;
>       g_tboot_shared->acpi_sinfo.vector_width = acpi_sinfo.vector_width;
>       g_tboot_shared->acpi_sinfo.kernel_s3_resume_vector =
> -                                              bootsym_phys(wakeup_start);
> +                                              trampsym_phys(wakeup_start);
Shouldn't changes like these have happened earlier, when you
introduce the (logical only at that point) distinction between
trampoline pieces?

> @@ -97,7 +100,7 @@ GLOBAL(trampoline_realmode_entry)
>          cld
>          cli
>          lidt    trampsym(idt_48)
> -        lgdt    trampsym(gdt_48)
> +        lgdtl   trampsym(gdt_48)
Stray / unrelated change (and if needed, then also for lidt)?

> @@ -236,11 +239,23 @@ gdt_48: .word   7*8-1
>   
>   /* The first page of trampoline is permanent, the rest boot-time only. */
>   /* Reuse the boot trampoline on the 1st trampoline page as stack for wakeup. 
> */
> -        .equ    wakeup_stack, boot_trampoline_start + PAGE_SIZE
> +        .equ    wakeup_stack, perm_trampoline_start + PAGE_SIZE
>           .global wakeup_stack
>   
> +ENTRY(perm_trampoline_end)
> +
>   /* From here on early boot only. */
>   
> +ENTRY(boot_trampoline_start)
> +
> +        .word   0
> +boot16_idt:
> +        .word   0, 0, 0 # base = limit = 0
> +        .word   0
> +boot16_gdt:
> +        .word   7*8-1
> +        .long   tramp32sym_rel(trampoline_gdt,4)
Can we really not get away without a second copy of these?

> @@ -304,8 +319,8 @@ trampoline_boot_cpu_entry:
>           cli
>   
>           /* Reset GDT and IDT. Some BIOSes clobber GDTR. */
> -        lidt    bootsym(idt_48)
> -        lgdt    bootsym(gdt_48)
> +        lidt    bootsym(boot16_idt)
> +        lgdtl   bootsym(boot16_gdt)
As above - either both should gain a suffix, or neither of them.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -682,6 +682,42 @@ static unsigned int __init copy_bios_e820(struct e820entry 
> *map, unsigned int li
>       return n;
>   }
>   
> +extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
> +extern const s32 __trampoline32_rel_start[], __trampoline32_rel_stop[];
> +
> +static void __init relocate_trampoline(unsigned long phys)
> +{
> +    const s32 *trampoline_ptr;
> +    uint32_t tramp32_delta = 0;
> +
> +    /* Apply relocations to trampoline. */
> +    for ( trampoline_ptr = __trampoline_rel_start;
> +          trampoline_ptr < __trampoline_rel_stop;
> +          ++trampoline_ptr )
> +        *(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
> +
> +    tramp32_delta = phys;
Any reason this can't be the initializer of the variable, or the
zero initializer above can't be dropped?

> +    if (!efi_enabled(EFI_LOADER)) {
Style (missing blanks inside the parentheses, and brace to go on
its own line).

> --- a/xen/include/asm-x86/config.h
> +++ b/xen/include/asm-x86/config.h
> @@ -89,12 +89,12 @@
>  
>  #ifndef __ASSEMBLY__
>  extern unsigned long trampoline_phys;
> -#define bootsym_phys(sym)                                 \
> -    (((unsigned long)&(sym)-(unsigned 
> long)&boot_trampoline_start)+trampoline_phys)
> -#define bootsym(sym)                                      \
> +#define trampsym_phys(sym)                                 \
> +    (((unsigned long)&(sym)-(unsigned 
> long)&perm_trampoline_start)+trampoline_phys)
> +#define trampsym(sym)                                      \
>      (*RELOC_HIDE((typeof(&(sym)))__va(__pa(&(sym))),      \
> -                 trampoline_phys-__pa(boot_trampoline_start)))
> -extern char boot_trampoline_start[], boot_trampoline_end[];
> +                 trampoline_phys-__pa(perm_trampoline_start)))
As you're touching these, could you please also insert the missing
blanks around the binary + and - ?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel