On Wed, Apr 13, 2022, Sean Christopherson wrote:
> 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?
Ooooh, and I finally realized in patch 7 that the "sipi" area encompasses the GDT
as well. I couldn't figure out how the GDT was getting relocated :-)
Definitely needs a different name. How about efi_rm_trampoline_start,
efi_rm_trampoline_end, and efi_rm_trampoline_size? The "real mode" part is
kinda misleading, but on the other hand it's also the main reason why this needs
to be relocated to super low memory. And add a comment
That will help make it a little more obvious that there's more than just the SIPI
code that is getting manually relocated.
It's probably still worth having an explicit sipi_entry label, with a comment to
document that it absolutely needs to be at the start of the trampoline so that
the SIPI vector sends APs to the right location.
