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);
