| Message ID | 759652afb52a3258f0da44de61ed28d0875774f8.camel@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/6] x86/boot: Remove gratuitous call back into low-memory code | expand |
On 09.08.2019 17:02, David Woodhouse wrote: > From: David Woodhouse <dwmw@amazon.co.uk> > > In preparation for splitting the boot and permanent trampolines from > each other. Some of these will change back, but most are boot so do the > plain search/replace that way first, then a subsequent patch will extract > the permanent trampoline code. To be honest I don't view it as helpful to do things in this order. If you first re-arranged the ordering of items within the trampoline, we'd then not end up with an intermediate state where the labels are misleading. Is there a reason things can't sensibly be done the other way around? Jan
On Mon, 2019-08-12 at 11:55 +0200, Jan Beulich wrote: > On 09.08.2019 17:02, David Woodhouse wrote: > > From: David Woodhouse <dwmw@amazon.co.uk> > > > > In preparation for splitting the boot and permanent trampolines from > > each other. Some of these will change back, but most are boot so do the > > plain search/replace that way first, then a subsequent patch will extract > > the permanent trampoline code. > > To be honest I don't view it as helpful to do things in this order. > If you first re-arranged the ordering of items within the trampoline, > we'd then not end up with an intermediate state where the labels are > misleading. Is there a reason things can't sensibly be done the other > way around? Obviously I did all this in a working tree first, swore at it a lot and finally got it working, then attempted to split it up into separate meaningful commits which individually made sense. There is plenty of room for subjectivity in the choices I made in that last step. I'm not sure I quite see why you say the labels are misleading. My intent was to apply labels based on what each object is *used* for, despite the fact that to start with they're all actually in the same place. And then to actually move each different type of symbol into its separate section/location to clean things up. Is it just the code comments at the start of trampoline.S that you find misleading in the interim stage? Because those *don't* purely talk about what bootsym/bootdatasym/trampsym/tramp32sym are used for; they do say how they are (eventually) relocated. I suppose I could rip that code comment out of patch #3 completely and add it again in a later commit... or just just add it again. I write code comments in an attempt to be helpful to those who come after me (especially when that's actually myself) but if they're going to cause problems, then maybe they're more hassle than they're worth?
On 19.08.2019 17:24, David Woodhouse wrote: > On Mon, 2019-08-12 at 11:55 +0200, Jan Beulich wrote: >> On 09.08.2019 17:02, David Woodhouse wrote: >>> From: David Woodhouse <dwmw@amazon.co.uk> >>> >>> In preparation for splitting the boot and permanent trampolines from >>> each other. Some of these will change back, but most are boot so do the >>> plain search/replace that way first, then a subsequent patch will extract >>> the permanent trampoline code. >> >> To be honest I don't view it as helpful to do things in this order. >> If you first re-arranged the ordering of items within the trampoline, >> we'd then not end up with an intermediate state where the labels are >> misleading. Is there a reason things can't sensibly be done the other >> way around? > > Obviously I did all this in a working tree first, swore at it a lot and > finally got it working, then attempted to split it up into separate > meaningful commits which individually made sense. There is plenty of > room for subjectivity in the choices I made in that last step. > > I'm not sure I quite see why you say the labels are misleading. My > intent was to apply labels based on what each object is *used* for, > despite the fact that to start with they're all actually in the same > place. And then to actually move each different type of symbol into its > separate section/location to clean things up. > > Is it just the code comments at the start of trampoline.S that you find > misleading in the interim stage? Because those *don't* purely talk > about what bootsym/bootdatasym/trampsym/tramp32sym are used for; they > do say how they are (eventually) relocated. I suppose I could rip that > code comment out of patch #3 completely and add it again in a later > commit... or just just add it again. I write code comments in an > attempt to be helpful to those who come after me (especially when > that's actually myself) but if they're going to cause problems, then > maybe they're more hassle than they're worth? No, it's actually the label names: The "boot" that this patch prefixes to them isn't correct until all post-boot (i.e. AP bringup) parts have been moved out of the framed block of code. Jan
> On 19.08.2019 17:24, David Woodhouse wrote: >> On Mon, 2019-08-12 at 11:55 +0200, Jan Beulich wrote: >>> On 09.08.2019 17:02, David Woodhouse wrote: >>>> From: David Woodhouse <dwmw@amazon.co.uk> >>>> >>>> In preparation for splitting the boot and permanent trampolines from >>>> each other. Some of these will change back, but most are boot so do >>>> the >>>> plain search/replace that way first, then a subsequent patch will >>>> extract >>>> the permanent trampoline code. >>> >>> To be honest I don't view it as helpful to do things in this order. >>> If you first re-arranged the ordering of items within the trampoline, >>> we'd then not end up with an intermediate state where the labels are >>> misleading. Is there a reason things can't sensibly be done the other >>> way around? >> >> Obviously I did all this in a working tree first, swore at it a lot and >> finally got it working, then attempted to split it up into separate >> meaningful commits which individually made sense. There is plenty of >> room for subjectivity in the choices I made in that last step. >> >> I'm not sure I quite see why you say the labels are misleading. My >> intent was to apply labels based on what each object is *used* for, >> despite the fact that to start with they're all actually in the same >> place. And then to actually move each different type of symbol into its >> separate section/location to clean things up. >> >> Is it just the code comments at the start of trampoline.S that you find >> misleading in the interim stage? Because those *don't* purely talk >> about what bootsym/bootdatasym/trampsym/tramp32sym are used for; they >> do say how they are (eventually) relocated. I suppose I could rip that >> code comment out of patch #3 completely and add it again in a later >> commit... or just just add it again. I write code comments in an >> attempt to be helpful to those who come after me (especially when >> that's actually myself) but if they're going to cause problems, then >> maybe they're more hassle than they're worth? > > No, it's actually the label names: The "boot" that this patch prefixes > to them isn't correct until all post-boot (i.e. AP bringup) parts > have been moved out of the framed block of code. Hm, OK. AFK at this moment but I'll take another look. Basically there wasn't a perfect way to label and move things; either way there were glitches during the transition and my recollection was that I preferred this one because it was purely cosmetic and only lasted for a commit or two. Will see if I can come up with something nicer within the amount of time it is reasonable to spend on such a transitional issue.
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 07621d1a30..556dab127f 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -754,20 +754,20 @@ trampoline_setup: cmpb $0, sym_fs(skip_realmode) jz 1f /* If no-real-mode, jump straight to trampoline_protmode_entry */ - lea trampoline_protmode_entry-trampoline_start(%edi),%eax + lea trampoline_protmode_entry-boot_trampoline_start(%edi),%eax /* EBX == 0 indicates we are the BP (Boot Processor). */ xor %ebx,%ebx jmp 2f 1: /* Go via 16-bit code in trampoline_boot_cpu_entry */ - lea trampoline_boot_cpu_entry-trampoline_start(%edi),%eax + lea trampoline_boot_cpu_entry-boot_trampoline_start(%edi),%eax 2: pushl $BOOT_CS32 push %eax /* Copy bootstrap trampoline to low memory, below 1MB. */ - mov $sym_offs(trampoline_start),%esi - mov $((trampoline_end - trampoline_start) / 4),%ecx + mov $sym_offs(boot_trampoline_start),%esi + mov $((boot_trampoline_end - boot_trampoline_start) / 4),%ecx rep movsl %fs:(%esi),%es:(%edi) /* Jump into the relocated trampoline. */ @@ -779,8 +779,8 @@ cmdline_parse_early: reloc: #include "reloc.S" -ENTRY(trampoline_start) +ENTRY(boot_trampoline_start) #include "trampoline.S" -ENTRY(trampoline_end) +ENTRY(boot_trampoline_end) #include "x86_64.S" diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S index 95a4bef553..26af9c6beb 100644 --- a/xen/arch/x86/boot/trampoline.S +++ b/xen/arch/x86/boot/trampoline.S @@ -31,7 +31,7 @@ * to be used for AP startup. */ #undef bootsym -#define bootsym(s) ((s)-trampoline_start) +#define bootsym(s) ((s)-boot_trampoline_start) #define bootsym_rel(sym, off, opnd...) \ bootsym(sym),##opnd; \ @@ -47,7 +47,7 @@ .long 111b - (off) - .; \ .popsection -#define bootdatasym(s) ((s)-trampoline_start) +#define bootdatasym(s) ((s)-boot_trampoline_start) #define bootdatasym_rel(sym, off, opnd...) \ bootdatasym(sym),##opnd; \ 111:; \ @@ -56,7 +56,7 @@ .popsection #undef trampsym -#define trampsym(s) ((s)-trampoline_start) +#define trampsym(s) ((s)-boot_trampoline_start) #define trampsym_rel(sym, off, opnd...) \ trampsym(sym),##opnd; \ @@ -66,7 +66,7 @@ .popsection #undef tramp32sym -#define tramp32sym(s) ((s)-trampoline_start) +#define tramp32sym(s) ((s)-boot_trampoline_start) #define tramp32sym_rel(sym, off, opnd...) \ tramp32sym(sym),##opnd; \ @@ -232,7 +232,7 @@ gdt_48: .word 6*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, trampoline_start + PAGE_SIZE + .equ wakeup_stack, boot_trampoline_start + PAGE_SIZE .global wakeup_stack /* From here on early boot only. */ diff --git a/xen/arch/x86/boot/video.S b/xen/arch/x86/boot/video.S index 03907e9e9a..5087c6a4d5 100644 --- a/xen/arch/x86/boot/video.S +++ b/xen/arch/x86/boot/video.S @@ -15,8 +15,8 @@ #include "video.h" -/* Scratch space layout: trampoline_end to trampoline_end+0x1000. */ -#define modelist bootsym(trampoline_end) /* 2kB (256 entries) */ +/* Scratch space layout: boot_trampoline_end to boot_trampoline_end+0x1000. */ +#define modelist bootsym(boot_trampoline_end) /* 2kB (256 entries) */ #define vesa_glob_info (modelist + 0x800) /* 1kB */ #define vesa_mode_info (vesa_glob_info + 0x400) /* 1kB */ diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 556942482e..fc2ea554b5 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -232,7 +232,7 @@ static void __init noreturn efi_arch_post_exit_boot(void) u64 cr4 = XEN_MINIMAL_CR4 & ~X86_CR4_PGE, efer; efi_arch_relocate_image(__XEN_VIRT_START - xen_phys_start); - memcpy((void *)trampoline_phys, trampoline_start, cfg.size); + memcpy((void *)trampoline_phys, boot_trampoline_start, cfg.size); /* Set system registers and transfer control. */ asm volatile("pushq $0\n\tpopfq"); @@ -566,7 +566,7 @@ static void __init efi_arch_memory_setup(void) cfg.addr = 0x100000; if ( efi_enabled(EFI_LOADER) ) - cfg.size = trampoline_end - trampoline_start; + cfg.size = boot_trampoline_end - boot_trampoline_start; else cfg.size = TRAMPOLINE_SPACE + TRAMPOLINE_STACK_SPACE; diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index decea2e77a..06e779368c 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1879,8 +1879,8 @@ int __hwdom_init xen_in_range(unsigned long mfn) if ( !xen_regions[0].s ) { /* S3 resume code (and other real mode trampoline code) */ - xen_regions[region_s3].s = bootsym_phys(trampoline_start); - xen_regions[region_s3].e = bootsym_phys(trampoline_end); + xen_regions[region_s3].s = bootsym_phys(boot_trampoline_start); + xen_regions[region_s3].e = bootsym_phys(boot_trampoline_end); /* * This needs to remain in sync with the uses of the same symbols in diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c index f3fdee4d39..325d94d23a 100644 --- a/xen/arch/x86/tboot.c +++ b/xen/arch/x86/tboot.c @@ -369,9 +369,9 @@ void tboot_shutdown(uint32_t shutdown_type) */ g_tboot_shared->num_mac_regions = 3; /* S3 resume code (and other real mode trampoline code) */ - g_tboot_shared->mac_regions[0].start = bootsym_phys(trampoline_start); - g_tboot_shared->mac_regions[0].size = bootsym_phys(trampoline_end) - - bootsym_phys(trampoline_start); + g_tboot_shared->mac_regions[0].start = bootsym_phys(boot_trampoline_start); + g_tboot_shared->mac_regions[0].size = bootsym_phys(boot_trampoline_end) - + bootsym_phys(boot_trampoline_start); /* hypervisor .text + .rodata */ g_tboot_shared->mac_regions[1].start = (uint64_t)__pa(&_stext); g_tboot_shared->mac_regions[1].size = __pa(&__2M_rodata_end) - diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c index 1919cae18b..149cc4f7b5 100644 --- 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(trampoline_end - trampoline_start), + PFN_UP(boot_trampoline_end - boot_trampoline_start), __PAGE_HYPERVISOR); } diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 400dffaf23..6968262a60 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -377,12 +377,12 @@ ASSERT(IS_ALIGNED(cpu0_stack, STACK_SIZE), "cpu0_stack misaligned") ASSERT(IS_ALIGNED(__init_begin, PAGE_SIZE), "__init_begin misaligned") ASSERT(IS_ALIGNED(__init_end, PAGE_SIZE), "__init_end misaligned") -ASSERT(IS_ALIGNED(trampoline_start, 4), "trampoline_start misaligned") -ASSERT(IS_ALIGNED(trampoline_end, 4), "trampoline_end misaligned") +ASSERT(IS_ALIGNED(boot_trampoline_start, 4), "boot_trampoline_start misaligned") +ASSERT(IS_ALIGNED(boot_trampoline_end, 4), "boot_trampoline_end misaligned") ASSERT(IS_ALIGNED(__bss_start, 8), "__bss_start misaligned") ASSERT(IS_ALIGNED(__bss_end, 8), "__bss_end misaligned") -ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN, +ASSERT((boot_trampoline_end - boot_trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN, "not enough room for trampoline and mbi data") ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN, "wakeup stack too small") diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h index 6e4f28d934..ada8c7b4f6 100644 --- a/xen/include/asm-x86/config.h +++ b/xen/include/asm-x86/config.h @@ -90,11 +90,11 @@ #ifndef __ASSEMBLY__ extern unsigned long trampoline_phys; #define bootsym_phys(sym) \ - (((unsigned long)&(sym)-(unsigned long)&trampoline_start)+trampoline_phys) + (((unsigned long)&(sym)-(unsigned long)&boot_trampoline_start)+trampoline_phys) #define bootsym(sym) \ (*RELOC_HIDE((typeof(&(sym)))__va(__pa(&(sym))), \ - trampoline_phys-__pa(trampoline_start))) -extern char trampoline_start[], trampoline_end[]; + trampoline_phys-__pa(boot_trampoline_start))) +extern char boot_trampoline_start[], boot_trampoline_end[]; extern char trampoline_realmode_entry[]; extern unsigned int trampoline_xen_phys_start; extern unsigned char trampoline_cpu_started;