> Index: src/cpu/amd/car/clear_1m_ram.c
> ===================================================================
> --- src/cpu/amd/car/clear_1m_ram.c    (revision 0)
> +++ src/cpu/amd/car/clear_1m_ram.c    (revision 0)

I'm not real comfortable with C include files that are simply
one big assembly statement.  Something just seems wrong.

I can understand the need for control when you are turning cache
as ram on and off though.

Testing for CONFIG_USE_INIT in any file that depends on cache_as_ram
seems the unnecessary.  Am I missing something here?

> ===================================================================
> --- src/cpu/amd/car/cache_as_ram.inc  (revision 2079)
> +++ src/cpu/amd/car/cache_as_ram.inc  (working copy)
> @@ -8,10 +8,17 @@
>
>       /* Save the BIST result */
>       movl    %eax, %ebp
> +
> +     // for normal part %ebx already contain cpu_init_detected from fallback call
>
>  CacheAsRam:
>       /* hope we can skip the double set for normal part */
>  #if USE_FALLBACK_IMAGE == 1

This looks like a testing/validation problem only enabling cache_as_ram
in the fallback image.  I would rather continue to compile fallback.c
with romcc than to have weird special cases like this.

> ===================================================================
> --- src/cpu/amd/dualcore/dualcore.c   (revision 2079)
> +++ src/cpu/amd/dualcore/dualcore.c   (working copy)
> @@ -2,6 +2,31 @@
>
>  #include "cpu/amd/dualcore/dualcore_id.c"
>
> +static inline unsigned get_core_num_in_bsp(unsigned nodeid)
> +{
> +        return ((pci_read_config32(PCI_DEV(0, 0x18+nodeid, 3), 0xe8)>>12) & 3);
> +}
> +
> +static inline uint8_t set_apicid_cpuid_lo(void)
> +{
> +        if(is_cpu_pre_e0()) return 0; // pre_e0 can not be set
> +
> +
> +        if(read_option(CMOS_VSTART_dual_core, CMOS_VLEN_dual_core, 0) != 0)  { // disable dual_core
> +                return 0;
> +        }
> +
> +                // set the NB_CFG[54]=1; why the OS will be happy with that ???
> +        msr_t msr;
> +        msr = rdmsr(NB_CFG_MSR);
> +        msr.hi |= (1<<(54-32)); // InitApicIdCpuIdLo
> +        wrmsr(NB_CFG_MSR, msr);
> +
> +        return 1;
> +
> +}

Our code is battling here.  The goal of do_k8_init_and_stop_secondaries
was to have code that could be shared between the two cases.
Even if it isn't so and we need to duplicate the logic it should
be as close as possible between the two cases.

Right now we seem to have to totally different ways at looking at
the world.

>  }

I'm not yet comfortable with the concept of having the equivalent
of failover.c in the primary file.  Mostly because using cache_as_ram
this early means we can't test changes to it in the normal image.

> -                        stop_this_cpu(); // it will stop all cores except core0 of cpu0
> -                }
> +              init_cpus(cpu_init_detectedx, sizeof(cpu)/sizeof(cpu[0]), cpu);

This looks like a good start, there is a lot less duplicate code here.

Why do we need to pass in the cpus?
Where is init_cpus defined?

I think we need a little harmonization between this and k8_init_and_stop_secondaries.

>          }
>

Eric