| Message ID | 20220412173407.13637-11-varad.gautam@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMP Support for x86 UEFI Tests | expand |
On Tue, Apr 12, 2022, Varad Gautam wrote: > ap_start64() currently serves as the 64-bit entrypoint for non-EFI > tests. > > Having ap_start64() and save_id() written in asm prevents sharing these > routines between EFI and non-EFI tests. > > Rewrite them in C and use ap_start64 as the 64-bit entrypoint in the EFI > boot flow. > > With this, EFI tests support -smp > 1. smptest.efi now passes. > > Signed-off-by: Varad Gautam <varad.gautam@suse.com> > --- > lib/x86/asm/setup.h | 3 +++ > lib/x86/setup.c | 54 +++++++++++++++++++++++++++++++++----------- > lib/x86/smp.c | 1 + > x86/cstart64.S | 24 -------------------- > x86/efi/efistart64.S | 5 ---- > 5 files changed, 45 insertions(+), 42 deletions(-) > > diff --git a/lib/x86/asm/setup.h b/lib/x86/asm/setup.h > index 24d4fa9..8502e7d 100644 > --- a/lib/x86/asm/setup.h > +++ b/lib/x86/asm/setup.h > @@ -16,4 +16,7 @@ efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo); > void setup_5level_page_table(void); > #endif /* CONFIG_EFI */ > > +void save_id(void); > +void ap_start64(void); > + > #endif /* _X86_ASM_SETUP_H_ */ > diff --git a/lib/x86/setup.c b/lib/x86/setup.c > index e2f7967..a0e0b0c 100644 > --- a/lib/x86/setup.c > +++ b/lib/x86/setup.c > @@ -14,8 +14,12 @@ > #include "apic.h" > #include "apic-defs.h" > #include "asm/setup.h" > +#include "processor.h" > +#include "atomic.h" > > extern char edata; > +extern unsigned char online_cpus[(MAX_TEST_CPUS + 7) / 8]; This is also in lib/x86/apic.c, I think it makes sense to move the declaration to smp.h. And opportunistically tweak the open coded size to: extern unsigned char online_cpus[DIV_ROUND_UP(MAX_TEST_CPUS), BITS_PER_BYTE)]; > +extern unsigned cpu_online_count; This should probably go into smp.h too, e.g. svm_tests.c also declares it. > @@ -328,6 +334,7 @@ efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo) > mask_pic_interrupts(); > setup_page_table(); > enable_apic(); > + save_id(); > ap_init(); > enable_x2apic(); > smp_init(); > @@ -350,3 +357,24 @@ void setup_libcflat(void) > add_setup_arg("bootloader"); > } > } > + > +void save_id(void) save_id() is a very odd name for what this is doing. Maybe mark_cpu_online()? Or am I overlooking something? > +{ > + u32 id = apic_id(); > + > + /* atomic_fetch_or() emits `lock or %dl, (%eax)` */ > + atomic_fetch_or(&online_cpus[id / 8], (1 << (id % 8))); Heh, this makes my brain go "what!?!". I strongly prefer we add^Wcopy the kernel's arch_set_bit() into lib/x86/atomic.h as atomic_set_bit(), then this becomes atomic_set_bit(&online_cpus, apic_id()); which is waaay easier to understand. > +} > + > +void ap_start64(void) > +{ > + setup_gdt_tss(); > + reset_apic(); > + load_idt(); > + save_id(); > + enable_apic(); > + enable_x2apic(); > + sti(); Hmm, so ap_start64 has a nop after the sti, presumably to consume the STI blocking shadow. _Why_ it needed to that, I have no idea, any pending IRQs should be serviced prior to actually executing HLT. Purely to be stupidly cautious, can you drop the "nop" from ap_start64 in a prep patch? Just so that if there's some magic we're missing, it shows up in a more obvious bisect. > + atomic_fetch_inc(&cpu_online_count); > + asm volatile("1: hlt; jmp 1b"); Unless I'm missing something, this should work and makes it more obvious that the vCPU is being put into a loop: for (;;) asm volatile("hlt"); or while(1) if that's your preference. > +}
diff --git a/lib/x86/asm/setup.h b/lib/x86/asm/setup.h index 24d4fa9..8502e7d 100644 --- a/lib/x86/asm/setup.h +++ b/lib/x86/asm/setup.h @@ -16,4 +16,7 @@ efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo); void setup_5level_page_table(void); #endif /* CONFIG_EFI */ +void save_id(void); +void ap_start64(void); + #endif /* _X86_ASM_SETUP_H_ */ diff --git a/lib/x86/setup.c b/lib/x86/setup.c index e2f7967..a0e0b0c 100644 --- a/lib/x86/setup.c +++ b/lib/x86/setup.c @@ -14,8 +14,12 @@ #include "apic.h" #include "apic-defs.h" #include "asm/setup.h" +#include "processor.h" +#include "atomic.h" extern char edata; +extern unsigned char online_cpus[(MAX_TEST_CPUS + 7) / 8]; +extern unsigned cpu_online_count; struct mbi_bootinfo { u32 flags; @@ -172,7 +176,22 @@ void setup_multiboot(struct mbi_bootinfo *bi) /* From x86/efi/efistart64.S */ extern void setup_segments64(u64 gs_base); extern u8 stacktop; +#endif + +static void setup_gdt_tss(void) +{ + size_t tss_offset; + /* 64-bit setup_tss does not use the stacktop argument. */ + tss_offset = setup_tss(NULL); + load_gdt_tss(tss_offset); +#ifdef CONFIG_EFI + u64 gs_base = (u64)(&stacktop) - (PAGE_SIZE * (pre_boot_apic_id() + 1)); + setup_segments64(gs_base); +#endif +} + +#ifdef CONFIG_EFI static efi_status_t setup_memory_allocator(efi_bootinfo_t *efi_bootinfo) { int i; @@ -269,19 +288,6 @@ static void setup_page_table(void) write_cr3((ulong)&ptl4); } -static void setup_gdt_tss(void) -{ - size_t tss_offset; - u64 gs_base; - - /* 64-bit setup_tss does not use the stacktop argument. */ - tss_offset = setup_tss(NULL); - load_gdt_tss(tss_offset); - - gs_base = (u64)(&stacktop) - (PAGE_SIZE * (pre_boot_apic_id() + 1)); - setup_segments64(gs_base); -} - efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo) { efi_status_t status; @@ -328,6 +334,7 @@ efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo) mask_pic_interrupts(); setup_page_table(); enable_apic(); + save_id(); ap_init(); enable_x2apic(); smp_init(); @@ -350,3 +357,24 @@ void setup_libcflat(void) add_setup_arg("bootloader"); } } + +void save_id(void) +{ + u32 id = apic_id(); + + /* atomic_fetch_or() emits `lock or %dl, (%eax)` */ + atomic_fetch_or(&online_cpus[id / 8], (1 << (id % 8))); +} + +void ap_start64(void) +{ + setup_gdt_tss(); + reset_apic(); + load_idt(); + save_id(); + enable_apic(); + enable_x2apic(); + sti(); + atomic_fetch_inc(&cpu_online_count); + asm volatile("1: hlt; jmp 1b"); +} diff --git a/lib/x86/smp.c b/lib/x86/smp.c index cee82ac..779d346 100644 --- a/lib/x86/smp.c +++ b/lib/x86/smp.c @@ -22,6 +22,7 @@ static atomic_t active_cpus; extern u8 sipi_entry; extern u8 sipi_end; volatile unsigned cpu_online_count = 1; +unsigned char online_cpus[(MAX_TEST_CPUS + 7) / 8]; static __attribute__((used)) void ipi(void) { diff --git a/x86/cstart64.S b/x86/cstart64.S index 6eb109d..6363293 100644 --- a/x86/cstart64.S +++ b/x86/cstart64.S @@ -123,27 +123,6 @@ gdt32_descr: sipi_end: .code64 -save_id: - movl $(APIC_DEFAULT_PHYS_BASE + APIC_ID), %eax - movl (%rax), %eax - shrl $24, %eax - lock btsl %eax, online_cpus - retq - -ap_start64: - call reset_apic - call load_idt - load_tss - call enable_apic - call save_id - call enable_x2apic - sti - nop - lock incw cpu_online_count - -1: hlt - jmp 1b - start64: call reset_apic call load_idt @@ -182,6 +161,3 @@ setup_5level_page_table: lretq lvl5: retq - -online_cpus: - .fill (max_cpus + 7) / 8, 1, 0 diff --git a/x86/efi/efistart64.S b/x86/efi/efistart64.S index 149e3f7..a5d7219 100644 --- a/x86/efi/efistart64.S +++ b/x86/efi/efistart64.S @@ -98,8 +98,3 @@ sipi_end: .code32 #include "../start32.S" - -.code64: - -ap_start64: - jmp ap_start64
ap_start64() currently serves as the 64-bit entrypoint for non-EFI tests. Having ap_start64() and save_id() written in asm prevents sharing these routines between EFI and non-EFI tests. Rewrite them in C and use ap_start64 as the 64-bit entrypoint in the EFI boot flow. With this, EFI tests support -smp > 1. smptest.efi now passes. Signed-off-by: Varad Gautam <varad.gautam@suse.com> --- lib/x86/asm/setup.h | 3 +++ lib/x86/setup.c | 54 +++++++++++++++++++++++++++++++++----------- lib/x86/smp.c | 1 + x86/cstart64.S | 24 -------------------- x86/efi/efistart64.S | 5 ---- 5 files changed, 45 insertions(+), 42 deletions(-)