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

[Sean Christopherson <seanjc@google.com>]

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.