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@xxxxxxxx>
> ---
> 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.
> +}
