On Tue, Apr 12, 2022, Varad Gautam wrote:
> ap_init() copies the SIPI vector to lowmem, sends INIT/SIPI to APs
> and waits on the APs to come up.
>
> Port this routine to C from asm and move it to smp.c to allow sharing
> this functionality between the EFI (-fPIC) and non-EFI builds.
>
> Call ap_init() from the EFI setup path to reset the APs to a known
> location.
>
> Signed-off-by: Varad Gautam <varad.gautam@xxxxxxxx>
> ---
> lib/x86/setup.c | 1 +
> lib/x86/smp.c | 28 ++++++++++++++++++++++++++--
> lib/x86/smp.h | 1 +
> x86/cstart64.S | 20 ++------------------
x86/cstart.S needs to join the party, without being kept in the loop KUT fails to
build on 32-bit targets.
x86/vmexit.o x86/cstart.o lib/libcflat.a
lib/libcflat.a(smp.o): In function `ap_init':
/home/sean/go/src/kernel.org/kvm-unit-tests/lib/x86/smp.c:150: undefined reference to `sipi_end'
/home/sean/go/src/kernel.org/kvm-unit-tests/lib/x86/smp.c:150: undefined reference to `sipi_entry'
/home/sean/go/src/kernel.org/kvm-unit-tests/lib/x86/smp.c:155: undefined reference to `sipi_entry'
collect2: error: ld returned 1 exit status
/home/sean/go/src/kernel.org/kvm-unit-tests/x86/Makefile.common:65: recipe for target 'x86/vmexit.elf' failed
> x86/efi/efistart64.S | 9 +++++++++
> 5 files changed, 39 insertions(+), 20 deletions(-)
>
> diff --git a/lib/x86/setup.c b/lib/x86/setup.c
> index 2d63a44..86ba6de 100644
> --- a/lib/x86/setup.c
> +++ b/lib/x86/setup.c
> @@ -323,6 +323,7 @@ efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo)
> load_idt();
> mask_pic_interrupts();
> enable_apic();
> + ap_init();
> enable_x2apic();
> smp_init();
> setup_page_table();
> diff --git a/lib/x86/smp.c b/lib/x86/smp.c
> index 683b25d..d7f5aba 100644
> --- a/lib/x86/smp.c
> +++ b/lib/x86/smp.c
> @@ -18,6 +18,9 @@ static volatile int ipi_done;
> static volatile bool ipi_wait;
> static int _cpu_count;
> static atomic_t active_cpus;
> +extern u8 sipi_entry;
> +extern u8 sipi_end;
> +volatile unsigned cpu_online_count = 1;
Please no bare "unsigned". But that's a moot point because it actually needs to
be a u16, the asm code does incw. That's also sort of a moot point because there's
zero chance any of this will work with 65536+ vCPUs, but it's still odd to see.
There's also a declaration in x86/svm_tests.c that needs to get deleted.
extern u16 cpu_online_count;
Ooh, actually, an even better idea. Make cpu_online_count a proper atomic_t,
same as active_cpus. Then the volatile goes away. Ugh, but atomic_t uses a
volatile. *sigh* At least that's contained in one spot that we can fix all at
once if someone gets motivated in the future.
And then rather than leave the
lock incw cpu_online_count
in asm code, move that to C code to, e.g. ap_call_in() or something (there's gotta
be a standard-ish name for this). The shortlog can be something like "Move AP
wakeup and rendezvous to smp.c.
And as a bonus, add a printf() in the C helper (assuming that doesn't cause
explosions) to state which AP came online. That would be super helpful for debug
when someone breaks SMP boot.
> static __attribute__((used)) void ipi(void)
> {
> @@ -114,8 +117,6 @@ void smp_init(void)
> int i;
> void ipi_entry(void);
>
> - _cpu_count = fwcfg_get_nb_cpus();
> -
> setup_idt();
> init_apic_map();
> set_idt_entry(IPI_VECTOR, ipi_entry, 0);
> @@ -142,3 +143,26 @@ void smp_reset_apic(void)
>
> atomic_inc(&active_cpus);
> }
> +
> +void ap_init(void)
> +{
> + u8 *dst_addr = 0;
> + size_t sipi_sz = (&sipi_end - &sipi_entry) + 1;
> +
> + asm volatile("cld");
> +
> + /* Relocate SIPI vector to dst_addr so it can run in 16-bit mode. */
> + memcpy(dst_addr, &sipi_entry, sipi_sz);
> +
> + /* INIT */
> + apic_icr_write(APIC_DEST_ALLBUT | APIC_DEST_PHYSICAL | APIC_DM_INIT | APIC_INT_ASSERT, 0);
> +
> + /* SIPI */
> + apic_icr_write(APIC_DEST_ALLBUT | APIC_DEST_PHYSICAL | APIC_DM_STARTUP, 0);
> +
> + _cpu_count = fwcfg_get_nb_cpus();
> +
Similar to above, I would be in favor of opportunitically adding a printf to state
that the BSP is about to wait for N number of APs to come online,
.
> + while (_cpu_count != cpu_online_count) {
> + ;
> + }
Curly braces technically aren't needed.
> +}
> diff --git a/lib/x86/smp.h b/lib/x86/smp.h
> index bd303c2..9c92853 100644
