cvs commit: src/sys/amd64/amd64 local_apic.c src/sys/i386/acpica madt.c src/sys/i386/i386 local_apic.c src/sys/kern subr_smp.c src/sys/sun4v/mdesc mdesc_init.c

[John Baldwin]

On Tuesday 11 September 2007 06:54:09 pm Attilio Rao wrote:
> attilio     2007-09-11 22:54:09 UTC
> 
>   FreeBSD src repository
> 
>   Modified files:
>     sys/amd64/amd64      local_apic.c 
>     sys/i386/acpica      madt.c 
>     sys/i386/i386        local_apic.c 
>     sys/kern             subr_smp.c 
>     sys/sun4v/mdesc      mdesc_init.c 
>   Log:
>   This is a follow-up, cleaning-up commit about recent changes involving
>   topology foo functions.
>   Working at the patch for topology problems in ia32/amd64 evicted some
>   problems regarding functions ordering in the SI_SUB_CPU family of
>   SYSINIT'ed subsystems.
>   In order to avoid problems with new modified to involved functions, a
>   correct ordering is not semantically specified for SI_SUB_CPU functions
>   (for a larger view of the issue please visit:
>   http://lists.freebsd.org/pipermail/freebsd-current/2007-July/075409.html )

Could you clarify exactly what ordering this does?  AFAICT, nothing in 
cpu_startup() requires the APIC to be up and running.  If there were a 
dependency, then that would be something for the MD architecture code to fix.  
IIUC, the problem was that you had 3 sysinit functions like this:

A - SI_ORDER_FIRST, cpu_startup()
B - SI_ORDER_FIRST, apic_init()
C - SI_ORDER_SECOND, mp_start()

Now mp_start() requires both A and B to have run on x86.  What Peter did 
originally was to move B to SI_ORDER_SECOND because he found that B needed A 
to run.  That broke because C could end up running before B.  However, the 
actual patch that went into CVS left A and B as is and instead made them 
independent of each other so B no longer depends on A.  So, I don't think 
you've solved an actual problem.

Conceptually the way SI_SUB_CPU worked before was that the MD code had 
SI_ORDER_FIRST to get ready before it was asked to start SMP at 
SI_ORDER_SECOND.  Now the MD code gets SI_ORDER_FIRST and _SECOND even though 
it doesn't need it.  You should at least put the x86 apic routines back to 
SI_ORDER_FIRST becuase they do _not_ depend on cpu_startup().

The madt.c change in this commit is plain wrong however and should be 
reverted.  If it was correct it would have needed to be done in amd64's 
madt.c as well.

The sun4v change is bogus as well as mdesc_init() doesn't depend on 
cpu_startup() on sun4v at all, and it doesn't matter what order they run in.

Basically, I think at the least you should revert all the MD changes, and the 
change to subr_smp.c is basically a NOP.

-- 
John Baldwin