Re: [kvm-unit-tests PATCH v2 10/10] x86: Provide a common 64-bit AP entrypoint for EFI and non-EFI

[Sean Christopherson <seanjc@google.com>]

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.

> +}