K8 RAMinit problematic code

[Carl-Daniel Hailfinger]

Hi,

this patch is not to be applied, but an annotation from my tree.

Index: northbridge/amd/k8/dqs.c
===================================================================
--- northbridge/amd/k8/dqs.c	(Revision 853)
+++ northbridge/amd/k8/dqs.c	(Arbeitskopie)
@@ -2001,6 +2001,7 @@
 static inline void train_ram_on_node(unsigned nodeid, unsigned coreid, struct sys_info *sysinfo, unsigned retcall)
 {
    if(coreid) return; // only do it on core0
+#error This is broken beyond repair. We need to use the generic global variable infrastructure, especially if we relocate the global variables without telling anybody.
    struct sys_info *sysinfox = ((CONFIG_LB_MEM_TOPK<<10) - DCACHE_RAM_GLOBAL_VAR_SIZE);
    wait_till_sysinfo_in_ram(); // use pci to get it

@@ -2012,6 +2013,7 @@
    	sysinfo->mem_trained[nodeid] = sysinfox->mem_trained[nodeid];
    	memcpy(&sysinfo->ctrl[nodeid], &sysinfox->ctrl[nodeid], sizeof(struct mem_controller));
    #else
+#error Broken here as well.
    	memcpy(sysinfo, sysinfox, DCACHE_RAM_GLOBAL_VAR_SIZE);
    #endif
    	set_top_mem_ap(sysinfo->tom_k, sysinfo->tom2_k); // keep the ap's tom consistent with bsp's

Even if the stuff above is fixed, we absolutely have to finish the
concept of accessing global variables by adding locking and/or spelling
out the access rules, especially for the CAR stage.

Oh, and can we please mark struct sys_info as const where possible and
also not pass it around as parameter. Especially if we perform stack
relocation, this can lead to really nasty problems.
Thanks.

Regards,
Carl-Daniel

[ron minnich]

On Mon, Sep 1, 2008 at 8:12 AM, Carl-Daniel Hailfinger
 wrote:
> Hi,
>
> this patch is not to be applied, but an annotation from my tree.
> Index: northbridge/amd/k8/dqs.c
>
> --- northbridge/amd/k8/dqs.c    (Revision 853)
> +++ northbridge/amd/k8/dqs.c    (Arbeitskopie)
> @@ -2001,6 +2001,7 @@
>  static inline void train_ram_on_node(unsigned nodeid, unsigned coreid, struct sys_info *sysinfo, unsigned retcall)
>  {
>        if(coreid) return; // only do it on core0
> +#error This is broken beyond repair. We need to use the generic global variable infrastructure, especially if we relocate the global variables without telling anybody.

These fixes come in to play once single core is done.

We're going to write this code as always SMP safe, and I want to
remove the CONFIG_*_SMP conditionals. They make no sense for a bios.

ron

[Carl-Daniel Hailfinger]

On 01.09.2008 17:39, ron minnich wrote:
> On Mon, Sep 1, 2008 at 8:12 AM, Carl-Daniel Hailfinger
>  wrote:
> > Hi,
> >
> > this patch is not to be applied, but an annotation from my tree.
> > Index: northbridge/amd/k8/dqs.c
> >
> > --- northbridge/amd/k8/dqs.c    (Revision 853)
> > +++ northbridge/amd/k8/dqs.c    (Arbeitskopie)
> > @@ -2001,6 +2001,7 @@
> >  static inline void train_ram_on_node(unsigned nodeid, unsigned coreid, struct sys_info *sysinfo, unsigned retcall)
> >  {
> >        if(coreid) return; // only do it on core0
> > +#error This is broken beyond repair. We need to use the generic global variable infrastructure, especially if we relocate the global variables without telling anybody.
>
> These fixes come in to play once single core is done.

Sorry, I meant that

struct sys_info *sysinfox = ((CONFIG_LB_MEM_TOPK<<10) - DCACHE_RAM_GLOBAL_VAR_SIZE);

is broken beyond repair. Basically, that code tries to establish a new
location for global variables without telling anyone.
> We're going to write this code as always SMP safe, and I want to
> remove the CONFIG_*_SMP conditionals. They make no sense for a bios.

Interesting point of view. As long as we're allowed to make locking on
pure uniprocessor architectures a no-op, I'm totally for it.

Regards,
Carl-Daniel

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> Interesting point of view. As long as we're allowed to make locking on
> pure uniprocessor architectures a no-op, I'm totally for it.

The thing is, without those defines, figuring out whether we're SMP or
not is more complex than doing the actual locking.
So we might as well do the locking unconditionally, too.

[ron minnich]

On Mon, Sep 1, 2008 at 10:10 AM, Carl-Daniel Hailfinger
 wrote:
> Sorry, I meant that
>
> struct sys_info *sysinfox = ((CONFIG_LB_MEM_TOPK<<10) - DCACHE_RAM_GLOBAL_VAR_SIZE);
>
> is broken beyond repair. Basically, that code tries to establish a new
> location for global variables without telling anyone.

yow! that's supposed to have been gone already! I wonder if a patch got lost.
> > We're going to write this code as always SMP safe, and I want to
> > remove the CONFIG_*_SMP conditionals. They make no sense for a bios.
>
> Interesting point of view. As long as we're allowed to make locking on
> pure uniprocessor architectures a no-op, I'm totally for it.

yeah, locks on up always succeed and there's no contention :-)

ron

[Carl-Daniel Hailfinger]

On 01.09.2008 20:19, ron minnich wrote:
> On Mon, Sep 1, 2008 at 10:10 AM, Carl-Daniel Hailfinger
>  wrote:
> > Sorry, I meant that
> >
> > struct sys_info *sysinfox = ((CONFIG_LB_MEM_TOPK<<10) - DCACHE_RAM_GLOBAL_VAR_SIZE);
> >
> > is broken beyond repair. Basically, that code tries to establish a new
> > location for global variables without telling anyone.
>
> yow! that's supposed to have been gone already! I wonder if a patch got lost.

Can you repost your remaining diff? I'll try to review ASAP.
> > > We're going to write this code as always SMP safe, and I want to
> > > remove the CONFIG_*_SMP conditionals. They make no sense for a bios.
> >
> > Interesting point of view. As long as we're allowed to make locking on
> > pure uniprocessor architectures a no-op, I'm totally for it.
>
> yeah, locks on up always succeed and there's no contention :-)

I had a redefinition of the lock functions in mind because some
architectures may not have any fast/easy locking instructions.

Regards,
Carl-Daniel

[Stefan Reinauer]

ron minnich wrote:
> These fixes come in to play once single core is done.
>
> We're going to write this code as always SMP safe, and I want to
> remove the CONFIG_*_SMP conditionals. They make no sense for a bios.

So you say a Geode LX bios should know how to do LAPIC, IOAPIC, INIT
IPI, SIPI, and all that stuff?

These conditionals _might_ make sense to save quite some space. Small
machines often have small flash.

Then there's this other thing... Intel Core (2) Duo/Solo/Quad CPUs
expect the BIOS to send the APs back to sleep. K8 does not do this.
Should we assume we (have to) do this anyways? Might make sense, power
consumption wise, if you're running a non-SMP OS on an SMP K8 system.

Stefan