|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 5/5] x86/boot: Do not use trampoline for no-real-mode boot paths
On 21.08.2019 18:35, 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, having applied suitable relocations.
>
> This means that the GDT has to be loaded with lgdtl because the 16-bit
> lgdt instruction would drop the high 8 bits of the gdt_trampoline
> address, causing failures if the Xen image was loaded above 16MiB.
There's a 2nd case where we assume an address not exceeding 16MiB:
The BOOT_PSEUDORM_{C,D}S entries on trampoline_gdt. While they'll
be unused (afaict) when not going through real mode, them getting
corrupted by applying relocations still seems somewhat risky to
me. I therefore wonder whether we shouldn't re-arrange this GDT's
layout in a prereq patch, such that theses two entries would go
last, and the GDT limit be reduced by two entries when skipping
real mode.
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -689,16 +689,23 @@ trampoline_setup:
> lea __PAGE_HYPERVISOR+sym_esi(l1_identmap),%edi
> mov %edi,sym_fs(l2_bootmap)
>
> - /* Apply relocations to bootstrap trampoline. */
> - mov sym_fs(trampoline_phys),%edx
> - mov $sym_offs(__trampoline_rel_start),%edi
> -1:
> - mov %fs:(%edi),%eax
> - add %edx,%fs:(%edi,%eax)
> - add $4,%edi
> - cmp $sym_offs(__trampoline_rel_stop),%edi
> - jb 1b
> + /* Do not parse command line on EFI platform here. */
> + cmpb $0,sym_fs(efi_platform)
> + jnz 1f
>
> + /* Bail if there is no command line to parse. */
> + mov sym_fs(multiboot_ptr),%ebx
> + testl $MBI_CMDLINE,MB_flags(%ebx)
> + jz 1f
As a really minor nit - I think it is generally better to have CMP
followed by JE/JNE, while (as you already have it) TEST/AND/OR are
better followed by JZ/JNZ. (Obviously this is a remark applicable to
the series as a whole.)
> @@ -90,6 +92,7 @@ GLOBAL(bootdata_start)
> * It is entered in Real Mode, with %cs = trampoline_realmode_entry >> 4 and
> * %ip = 0.
> */
> +
> GLOBAL(trampoline_realmode_entry)
> mov %cs,%ax
> mov %ax,%ds
I'd prefer if you didn't insert a blank line here, as the comment is
specifically associated with this label.
> @@ -97,7 +100,7 @@ GLOBAL(trampoline_realmode_entry)
> cld
> cli
> lidt trampsym(idt_48)
> - lgdt trampsym(gdt_48)
> + lgdtl trampsym(gdt_48)
> mov $1,%bl # EBX != 0 indicates we are an AP
> xor %ax, %ax
> inc %ax
As per the remark further up, trampoline_gdt's two entries (below
here) having a relocation associate with them should imo at least get
a comment added to state why (other than for the LGDTL here) there's
no issue with a relocation to an address above 16MiB.
> @@ -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
Doesn't this, at the very least, render the comment stale?
> .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)
As there's no change in this patch to how/where the boot trampoline
gets copied, am I understanding right that both end up at
trampoline_phys, just at different points in time? Otherwise I wonder
what the alignment of the relocated boot_trampoline_start end up
being, and hence whether the initial .word here is actually useful at
all.
> @@ -343,7 +358,8 @@ trampoline_boot_cpu_entry:
> xor %ebx,%ebx
>
> /* Jump to the common bootstrap entry point. */
> - jmp trampoline_protmode_entry
> + mov $tramp32sym_rel(trampoline_protmode_entry,4,%eax)
> + jmp *%eax
Do you really need to switch to an indirect branch here? I.e. can't
there be a relocation associated with the direct JMP's displacement?
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -679,6 +679,45 @@ 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;
> +
> + /* 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;
> + if ( !efi_enabled(EFI_LOADER) )
> + {
> + /*
> + * The non-EFI boot code uses the 32-bit trampoline in place
> + * so will have relocated it to the physical address of
> + * perm_trampoline_start already. Undo that as it needs to
> + * run from low memory for AP startup, because the Xen
> + * physical address range won't be mapped.
> + */
> + tramp32_delta -= trampoline_xen_phys_start;
> + tramp32_delta -= (unsigned long)(perm_trampoline_start -
> __XEN_VIRT_START);
> + }
> + for ( trampoline_ptr = __trampoline32_rel_start;
> + trampoline_ptr < __trampoline32_rel_stop;
> + ++trampoline_ptr )
> + {
> + *(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += tramp32_delta;
> + }
Along the lines of the loop further up, lease omit the braces here.
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -47,7 +47,7 @@
> #include <asm/tboot.h>
> #include <mach_apic.h>
>
> -#define setup_trampoline() (bootsym_phys(trampoline_realmode_entry))
> +#define setup_trampoline() (trampsym_phys(trampoline_realmode_entry))
Would you mind taking the opportunity and strip the unnecessary pair
of parentheses here?
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -697,7 +697,7 @@ void __init zap_low_mappings(void)
>
> /* Replace with mapping of the boot trampoline only. */
> map_pages_to_xen(trampoline_phys, maddr_to_mfn(trampoline_phys),
> - PFN_UP(boot_trampoline_end - boot_trampoline_start),
> + PFN_UP(perm_trampoline_end - perm_trampoline_start),
> __PAGE_HYPERVISOR);
You also want to adjust the comment then.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |