Re: [kvm-unit-tests PATCH v2 01/10] x86: Move ap_init() to smp.c

[Sean Christopherson <seanjc@google.com>]

On Tue, Apr 12, 2022, Varad Gautam wrote:
> @@ -142,3 +143,26 @@ void smp_reset_apic(void)
>  
>  	atomic_inc(&active_cpus);
>  }
> +
> +void ap_init(void)
> +{
> +	u8 *dst_addr = 0;

Oof, this is subtle.  I didn't realize until patch 7 that this is actually using
va=pa=0 for the trampoline.

Does anything actually prevent KUT from allocating pa=0?  Ah, looks like __setup_vm()
excludes the lower 1mb.

'0' should be a #define somewhere, e.g. EFI_RM_TRAMPOLINE_PHYS_ADDR, probably in
lib/alloc_page.h next to AREA_ANY_NUMBER with a comment tying the two together.
And then patch 7 can (hopefully without too much pain) use the define instead of
open coding the reference to PA=0, which is really confusing and unnecessarily
fragile.

E.g. instead of

	/* Retrieve relocated gdt32_descr address at (PAGE_SIZE - 2). */
	mov (PAGE_SIZE - 2), %ebx

hopefully it can be

	mov (EFI_RM_TRAMPOLINE_PHYS_ADDR + PAGE_SIZE - 2), %ebx

> +	size_t sipi_sz = (&sipi_end - &sipi_entry) + 1;

Nit, maybe sipi_trampoline_size?

> +	asm volatile("cld");
> +
> +	/* Relocate SIPI vector to dst_addr so it can run in 16-bit mode. */
> +	memcpy(dst_addr, &sipi_entry, sipi_sz);

A more descriptive name than dst_addr would help, and I'm pretty sure it can be
a void *.  Maybe?

	void *rm_trampoline = EFI_RM_TRAMPOLINE_PHYS_ADDR?

And rather than add the assert+memset in patch 7, do that here.  Oh, and fill
the page with 0xcc, i.e. int3, instead of 0, so that if an AP wanders into the
weeds, it gets a fault and shutdown.  All zeros decodes to ADD [rax], rax (or maybe
the reverse?), i.e. isn't guaranteed to fail right away.

	assert(sipi_trampoline_size < PAGE_SIZE);

	/* Comment goes here about filling with 0xcc. */
	memset(rm_trampoline, 0xcc, PAGE_SIZE);
	memcpy(rm_trampoline, &sipi_entry, sipi_trampoline_size);