Re: [SeaBIOS] [PATCH] smp: Replace QEMU SMP init assembler code with C; run only in 32bit mode.

[Paolo Bonzini]

Il 31/05/2014 03:20, Kevin O'Connor ha scritto:
> Change the multi-processor init code to trampoline into 32bit mode on
> each of the additional processors.  Implement an atomic lock so that
> each processor performs its initialization serially.

I don't see much benefit in this change, in fact the new code is more 
complex than the old one...  But anyway, if you prefer to go this way I 
have just a couple observations on the patch:

1) acquiring the lock can be done simply by a "1: lock btsl; jc 1b"; no 
need to use the cmpxchg.

2) There's no need to acquire the lock repeatedly in the BSP, I think 
writing the loop in assembly is acceptable:

asm volatile(
             "movl %%esp, %1\n"
        // Release lock, other processors use the stack now
        "mov $0, %0\n"
        "1: cmp %3, %2\n"
        "rep; nop\n"
        "jne 1b\n"
        // Reacquire lock; last processor is still using the stack!
        "1b: lock btsl $0, %0\n"
        "rep; nop\n"
        "jc 1b\n" :
    	"+m" (SMPLock), "=m" (SMPStack) : "r" (cmos_smp_count),
    	"m" (CountCPUs) : "cc", "memory");

Paolo
>        : "+m" (SMPLock), "+m" (SMPStack), "=&a" (eax)
>        : "r" (lockval)
>        : "cc", "memory");
>
> yield();
>
> // Restore memory.
> *(u64*)BUILD_AP_BOOT_ADDR = old;
>
> @@ -143,6 +142,6 @@ smp_setup(void)
>      if (!MaxCountCPUs || MaxCountCPUs < CountCPUs)
>          MaxCountCPUs = CountCPUs;
> dprintf(1, "Found %d cpu(s) max supported %d cpu(s)\n", readl(&CountCPUs),
>    MaxCountCPUs);
> dprintf(1, "Found %d cpu(s) max supported %d cpu(s)\n", CountCPUs,
>        MaxCountCPUs);
>
> }
> diff --git a/src/romlayout.S b/src/romlayout.S
> index 0d6af39..bee3943 100644
> --- a/src/romlayout.S
> +++ b/src/romlayout.S
> @@ -274,6 +274,29 @@ entry_smi:
>          rsm
>          .code16gcc
>
> +// Entry point for QEMU smp sipi interrupts.
>    DECLFUNC entry_smp
>
> +entry_smp:
>    // Transition to 32bit mode.
>    movl $1f + BUILD_BIOS_ADDR, %edx
>    jmp transition32
>    .code32
>    // Acquire lock
>
> +1:      movl $1, %ecx
> +2:      xorl %eax, %eax
>    lock cmpxchgl %ecx, SMPLock
>    jz 3f
>    rep nop
>    jmp 2b
>    // Setup stack and call handle_smp
>
> +3:      movl SMPStack, %esp
>    calll _cfunc32flat_handle_smp - BUILD_BIOS_ADDR
>    // Release lock and halt processor.
>    movl $0, SMPLock
>
> +4:      hlt
>    jmp 4b
>    .code16gcc
>
> // Resume (and reboot) entry point - called from entry_post
>          DECLFUNC entry_resume
>  entry_resume:
> diff --git a/src/util.h b/src/util.h
> index b54271b..8c794c4 100644
> --- a/src/util.h
> +++ b/src/util.h
> @@ -126,7 +126,6 @@ void smm_device_setup(void);
>  void smm_setup(void);
>
> // fw/smp.c
> -extern u32 CountCPUs;
>  extern u32 MaxCountCPUs;
>  void wrmsr_smp(u32 index, u64 val);
>  void smp_setup(void);

[PATCH] smp: Replace QEMU SMP init assembler code with C; run only in 32bit mode.

[Kevin O'Connor]

On Sat, May 31, 2014 at 12:12:13PM +0200, Paolo Bonzini wrote:
> Il 31/05/2014 03:20, Kevin O'Connor ha scritto:
> > Change the multi-processor init code to trampoline into 32bit mode on
> > each of the additional processors.  Implement an atomic lock so that
> > each processor performs its initialization serially.
>
> I don't see much benefit in this change, in fact the new code is more
> complex than the old one...

Thanks for reviewing!

The reason I implemented this is to reduce the amount of 16bit code.
The fw/smp.c file is currently being compiled in 16bit mode just to
pick up the ~30 lines of assembler.  I'd prefer to jump into 32bit
mode to avoid that.

As to converting the handler to C code - I agree with you on code
complexity, however I do think the existing assembler was already
sufficiently complex.  Also, being able to use dprintf() is quite
nice.
> But anyway, if you prefer to go this way I have
> just a couple observations on the patch:
> acquiring the lock can be done simply by a "1: lock btsl; jc 1b"; no need
>
> to use the cmpxchg.

Thanks - that is an improvement.
> There's no need to acquire the lock repeatedly in the BSP, I think
>
> writing the loop in assembly is acceptable:

I'll rework the patch and take a look at that.

-Kevin