Re: [External] Re: [PATCH v10 6/8] x86/smpboot: Support parallel startup of secondary CPUs

[Usama Arif]

On 24/02/2023 15:46, David Woodhouse wrote:
> On Fri, 2023-02-24 at 13:59 +0800, Yuan Yao wrote:
> > > +        * Make sure only one CPU fiddles with the realmode stack
> > > +        */
> > > +.Llock_rm:
> > > +       btl     $0, tr_lock
> > > +       jnc     2f
> > > +       pause
> > > +       jmp     .Llock_rm
> > > +2:
> > > +       lock
> > > +       btsl    $0, tr_lock
> > > +       jc      .Llock_rm
> > > +
> >
> > Looks these changes should be applied to trampoline_start64()
> > yet, which is used for boot up APs when apic->wakeup_secondary_cpu_64
> > is available, e.g when ACPI_MADT_TYPE_MULTIPROC_WAKEUP is available.
> >
> > One case I know is the INTEL TD guest, which using the MADT wakeup
> > for AP wake up now.
>
> Yeah.
>
> I think we could probably pull that all out into a separate asm
> "load_realmode_esp" function which takes the lock and actually does the
> 'movl $rm_stack_end, %esp'.
>
> Then we call it from all the places which currently use $rm_stack_end,
> including sev_es_trampoline_start (even though we currently disable
> parallel startup there because CPUID doesn't work that early).
>
> Oh... except of course it can't be a function because we haven't got a
> stack, have we? It's a macro.
>
> Usama, are you happy using .macro in gas?

Yes, makes sense. I guess something like below should be ok?
It makes sense to add it for sev as well.

diff --git a/arch/x86/realmode/rm/trampoline_64.S 
b/arch/x86/realmode/rm/trampoline_64.S
index e38d61d6562e..3a724d8d85b9 100644
--- a/arch/x86/realmode/rm/trampoline_64.S
+++ b/arch/x86/realmode/rm/trampoline_64.S
@@ -38,6 +38,25 @@
        .code16

        .balign PAGE_SIZE
+
+.macro LOAD_REALMODE_ESP
+       /*
+        * Make sure only one CPU fiddles with the realmode stack
+        */
+.Llock_rm\@:
+       btl     $0, tr_lock
+       jnc     2f
+       pause
+       jmp     .Llock_rm\@
+2:
+       lock
+       btsl    $0, tr_lock
+       jc      .Llock_rm\@
+
+       # Setup stack
+       movl    $rm_stack_end, %esp
+.endm
+
 SYM_CODE_START(trampoline_start)
        cli                     # We should be safe anyway
        wbinvd
@@ -49,8 +68,7 @@ SYM_CODE_START(trampoline_start)
        mov     %ax, %es
        mov     %ax, %ss

-       # Setup stack
-       movl    $rm_stack_end, %esp
+       LOAD_REALMODE_ESP

        call    verify_cpu              # Verify the cpu supports long mode
        testl   %eax, %eax              # Check for return code
@@ -93,8 +111,7 @@ SYM_CODE_START(sev_es_trampoline_start)
        mov     %ax, %es
        mov     %ax, %ss

-       # Setup stack
-       movl    $rm_stack_end, %esp
+       LOAD_REALMODE_ESP

        jmp     .Lswitch_to_protected
 SYM_CODE_END(sev_es_trampoline_start)
@@ -177,7 +194,7 @@ SYM_CODE_START(pa_trampoline_compat)
         * In compatibility mode.  Prep ESP and DX for startup_32, then 
disable
         * paging and complete the switch to legacy 32-bit mode.
         */
-       movl    $rm_stack_end, %esp
+       LOAD_REALMODE_ESP
        movw    $__KERNEL_DS, %dx

        movl    $(CR0_STATE & ~X86_CR0_PG), %eax