| Message ID | 20211130100445.31156-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Support for __ro_after_init | expand |
On 30.11.2021 11:04, Andrew Cooper wrote: > multiboot_ptr should be in __initdata - it is only used on the BSP path. > Furthermore, the .align 8 then .long means that stack_start is misaligned. > > Move both into setup.c, which lets the compiler handle the details correctly, > as well as providling proper debug information for them. > > Declare stack_start in setup.h and avoid extern-ing it locally in smpboot.c. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Nevertheless I'd like to state that defining a variable in C when all its uses are in assembly seems a little odd to me. Jan
On 02/12/2021 11:49, Jan Beulich wrote: > On 30.11.2021 11:04, Andrew Cooper wrote: >> multiboot_ptr should be in __initdata - it is only used on the BSP path. >> Furthermore, the .align 8 then .long means that stack_start is misaligned. >> >> Move both into setup.c, which lets the compiler handle the details correctly, >> as well as providling proper debug information for them. >> >> Declare stack_start in setup.h and avoid extern-ing it locally in smpboot.c. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. > Nevertheless I'd like to state that defining a variable in C when all > its uses are in assembly seems a little odd to me. I don't see it as odd, although I admit that I did try to see if I could remove multiboot_ptr entirely first. Xen is after all a single freestanding binary. Having the debug information (well - at a minimum, ELF size info) is important for livepatch binary diffing, and nothing in asm by default gets any of that. Letting the compiler do this all for us is physically shorter, and less prone to errors. ~Andrew
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S index d61048c583b3..27f52e7a7708 100644 --- a/xen/arch/x86/boot/x86_64.S +++ b/xen/arch/x86/boot/x86_64.S @@ -67,14 +67,6 @@ ENTRY(__high_start) call __start_xen BUG /* __start_xen() shouldn't return. */ - .data - .align 8 -multiboot_ptr: - .long 0 - -GLOBAL(stack_start) - .quad cpu0_stack + STACK_SIZE - CPUINFO_sizeof - .section .data.page_aligned, "aw", @progbits .align PAGE_SIZE, 0 /* diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 495b4b7d51fb..6613e56a2184 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -141,6 +141,12 @@ unsigned long __read_mostly xen_virt_end; char __section(".bss.stack_aligned") __aligned(STACK_SIZE) cpu0_stack[STACK_SIZE]; +/* Used by the BSP/AP paths to find the higher half stack mapping to use. */ +void *stack_start = cpu0_stack + STACK_SIZE - sizeof(struct cpu_info); + +/* Used by the boot asm to stash the relocated multiboot info pointer. */ +unsigned int __initdata multiboot_ptr; + struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 0, 0, -1 }; unsigned long __read_mostly mmu_cr4_features = XEN_MINIMAL_CR4; diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c index 329cfdb6c9f6..08c0f2d9df04 100644 --- a/xen/arch/x86/smpboot.c +++ b/xen/arch/x86/smpboot.c @@ -42,6 +42,7 @@ #include <asm/microcode.h> #include <asm/msr.h> #include <asm/mtrr.h> +#include <asm/setup.h> #include <asm/spec_ctrl.h> #include <asm/time.h> #include <asm/tboot.h> @@ -419,8 +420,6 @@ void start_secondary(void *unused) startup_cpu_idle_loop(); } -extern void *stack_start; - static int wakeup_secondary_cpu(int phys_apicid, unsigned long start_eip) { unsigned long send_status = 0, accept_status = 0; diff --git a/xen/include/asm-x86/setup.h b/xen/include/asm-x86/setup.h index 24be46115df2..eb9d7b433c13 100644 --- a/xen/include/asm-x86/setup.h +++ b/xen/include/asm-x86/setup.h @@ -12,6 +12,8 @@ extern char __2M_rwdata_start[], __2M_rwdata_end[]; extern unsigned long xenheap_initial_phys_start; extern uint64_t boot_tsc_stamp; +extern void *stack_start; + void early_cpu_init(void); void early_time_init(void);
multiboot_ptr should be in __initdata - it is only used on the BSP path. Furthermore, the .align 8 then .long means that stack_start is misaligned. Move both into setup.c, which lets the compiler handle the details correctly, as well as providling proper debug information for them. Declare stack_start in setup.h and avoid extern-ing it locally in smpboot.c. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> --- xen/arch/x86/boot/x86_64.S | 8 -------- xen/arch/x86/setup.c | 6 ++++++ xen/arch/x86/smpboot.c | 3 +-- xen/include/asm-x86/setup.h | 2 ++ 4 files changed, 9 insertions(+), 10 deletions(-)