[PATCH] Fix the NULL dev resource usage

[Rudolf Marek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello,

During the suspend/resume programming I came to an issue that first 4KB of
memory must be clear with 0s because otherwise the resources of K8 will be
totally messed up.

It took long to figure it out and here it is:

res = probe_resource(dev, 0x100 + (reg | link));

This is called with dev = NULL and this is no good for probe_resource at all.
The attached patch fixes the potential problems and of course the problem
itself. On one particular place was missing test if the device really exists.
This was copied to fam10 and perhaps the same issue is in v3 (DID NOT check).
The rest of the patch is just very paranoid and do all checkings.

This was tested in SimNOW but I believe it will work on real hw too.

Signed-off-by: Rudolf Marek

Anyone knows if SimNOW can do breaks on memory ranges?

Because it was quite stupid to get to the bug. When the resources just worked
again I used data breakpoint to see what routine it is. Problem was that I
needed to find the address where it stop to work.

Maybe we really should allow writes to mem only in _RAMBASE - MEMTOPK range (of
course with some exception for table stuff).

- --- src/cpu/amd/car/clear_init_ram.c<-->(revision 4026)
+++ src/cpu/amd/car/clear_init_ram.c<-->(working copy)
@@ -6,7 +6,23 @@
 <----->// gcc 3.4.5 will inline the copy_and_run and clear_init_ram in
post_cache_as_ram
 <----->// will reuse %edi as 0 from clear_memory for copy_and_run part,
actually it is increased already
 <----->// so noline clear_init_ram
- -        clear_memory(0,  ((CONFIG_LB_MEM_TOPK<<10) - DCACHE_RAM_SIZE));
+        clear_memory(_RAMBASE,  ((CONFIG_LB_MEM_TOPK<<10) -_RAMBASE -
DCACHE_RAM_SIZE));
+<----->//512 NOT OK - I previously though this works so I need to test MORE :/
+<----->//256 NOT OK
+<----->//384 NOT OK
+<----->//448 NOT OK
+<----->//480 NOT OK
+<----->//496 NOT OK
+<----->//504 NOT OK
+<----->//508 NOT OK
+<----->// 768 NOT OK
+<----->//896 NOT OK
+<----->//960 OK
+<----->//928 OK
+<----->//912 NOT OK
+<----->//920 NOT OK
+<----->//924 OK
+        clear_memory(0, 924);
.

Oh btw the suspend/resume works, but the code needs to be fixed on some places.
There is a  copy_secondary_start_to_1m_below which just uses first 64KB of mem
for some reason - I dont understand why it cannot be realocable to something
different addr. Please check and tell.

Thanks,
Rudolf
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAknKdOAACgkQ3J9wPJqZRNXabQCdFRWA4pYWDuY+VDyFROATejRC
oDcAn02iYJM4fivIzcpJlQ/D9Qu4DFYE
=dtVh
-----END PGP SIGNATURE-----

[Myles Watson]

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]]
> On Behalf Of Rudolf Marek
> Sent: Wednesday, March 25, 2009 12:16 PM
> To: Coreboot
> Subject: [coreboot] [PATCH] Fix the NULL dev resource usage
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hello,
>
> During the suspend/resume programming I came to an issue that first 4KB of
> memory must be clear with 0s because otherwise the resources of K8 will be
> totally messed up.
>
> It took long to figure it out and here it is:
>
> res = probe_resource(dev, 0x100 + (reg | link));

Good find!
> This is called with dev = NULL and this is no good for probe_resource at
> all.
> The attached patch fixes the potential problems and of course the problem
> itself. On one particular place was missing test if the device really
> exists.
> This was copied to fam10 and perhaps the same issue is in v3 (DID NOT
> check).
> The rest of the patch is just very paranoid and do all checkings.
>
> This was tested in SimNOW but I believe it will work on real hw too.
>
> Signed-off-by: Rudolf Marek

Acked-by: Myles Watson

I don't have answers to your questions, but I didn't want that to hold up
the patch.

Thanks,
Myles

[Stefan Reinauer]

> Index: src/northbridge/amd/amdk8/northbridge.c
>
> --- src/northbridge/amd/amdk8/northbridge.c	(revision 4026)
> +++ src/northbridge/amd/amdk8/northbridge.c	(working copy)
> @@ -291,11 +291,12 @@
>    unsigned nodeid, link;
>    int result;
>    res = 0;
> for(nodeid = 0; !res && (nodeid < 8); nodeid++) {
> for(nodeid = 0; !res && (nodeid < FX_DEVS); nodeid++) {
> device_t dev;
> dev = __f0_dev[nodeid];
> for(link = 0; !res && (link < 3); link++) {
> 	res = probe_resource(dev, 0x100 + (reg | link));
> 	if (dev)
> 		res = probe_resource(dev, 0x100 + (reg | link));

Going through the loop and checking dev every time is a bit nasty. What
about

for(nodeid = 0; !res && (nodeid < FX_DEVS); nodeid++) {
    device_t dev = __f0_dev[nodeid];
    if (!dev) continue;
    for(link = 0; !res && (link < 3); link++) {
    	res = probe_resource(dev, 0x100 + (reg | link));

...
> }
>
> }
>    result = 2;
> @@ -760,19 +761,20 @@
>    	mem_hole.hole_startk = HW_MEM_HOLE_SIZEK;
>    	mem_hole.node_id = -1;
> for (i = 0; i < 8; i++) {
> for (i = 0; i < FX_DEVS; i++) {
> uint32_t base;
> uint32_t hole;
> base  = f1_read_config32(0x40 + (i << 3));
> if ((base & ((1<<1)|(1<<0))) != ((1<<1)|(1<<0))) {
> 	continue;
> }
> 	hole = pci_read_config32(__f1_dev[i], 0xf0);
> 	if(hole & 1) { // we find the hole
> 		mem_hole.hole_startk = (hole & (0xff<<24)) >> 10;
> 		mem_hole.node_id = i; // record the node No with hole
> 		break; // only one hole
> 	if (__f1_dev[i]) {

Same as above. Just add another continue under the base check above.
> 		hole = pci_read_config32(__f1_dev[i], 0xf0);
> 		if(hole & 1) { // we find the hole
> 			mem_hole.hole_startk = (hole & (0xff<<24)) >> 10;
> 			mem_hole.node_id = i; // record the node No with hole
> 			break; // only one hole
> 		}
> }
> }
>
> @@ -834,15 +836,15 @@
>    limit = f1_read_config32(0x44 + (i << 3));
>    f1_write_config32(0x44 + (i << 3),limit - (hole_sizek << 2));
>    dev = __f1_dev[i];
> hoist = pci_read_config32(dev, 0xf0);
> if(hoist & 1) {
> pci_write_config32(dev, 0xf0, 0);
> if (dev) {

And the same as above.
> hoist = pci_read_config32(dev, 0xf0);
> if(hoist & 1) {
> 	pci_write_config32(dev, 0xf0, 0);
> } else {
> 	base = pci_read_config32(dev, 0x40 + (i << 3));
> 	f1_write_config32(0x40 + (i << 3),base - (hole_sizek << 2));
> }
> }
> else {
> base = pci_read_config32(dev, 0x40 + (i << 3));
> f1_write_config32(0x40 + (i << 3),base - (hole_sizek << 2));
> }
>
> }

...
> Index: src/northbridge/amd/amdfam10/northbridge.c
>
> --- src/northbridge/amd/amdfam10/northbridge.c	(revision 4026)
> +++ src/northbridge/amd/amdfam10/northbridge.c	(working copy)
> @@ -339,7 +339,8 @@
>    	device_t dev;
>    	dev = __f0_dev[nodeid];
>    	for(link = 0; !res && (link < 8); link++) {
> 	res = probe_resource(dev, 0x1000 + reg + (link<<16)); // 8 links, 0x1000 man f1,
> 	if (dev)
> 		res = probe_resource(dev, 0x1000 + reg + (link<<16)); // 8 links, 0x1000 man f1,

And the same.
> }
>
> }
>    result = 2;

[Rudolf Marek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Stefan,

OK good idea. Might work actually.

Rudolf
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAknKuh4ACgkQ3J9wPJqZRNUk6wCfSwuscF6Ljf1m4UMginKh3oKY
GU4AnA8A+u0iroiOHGOS1taA/58GCVuC
=AeS/
-----END PGP SIGNATURE-----

[Carl-Daniel Hailfinger]

On 25.03.2009 23:17, Stefan Reinauer wrote:
> > Index: src/northbridge/amd/amdk8/northbridge.c
> >
> > --- src/northbridge/amd/amdk8/northbridge.c	(revision 4026)
> > +++ src/northbridge/amd/amdk8/northbridge.c	(working copy)
> > @@ -291,11 +291,12 @@
> >    unsigned nodeid, link;
> >    int result;
> >    res = 0;
> > for(nodeid = 0; !res && (nodeid < 8); nodeid++) {
> > for(nodeid = 0; !res && (nodeid < FX_DEVS); nodeid++) {
> > device_t dev;
> > dev = __f0_dev[nodeid];
> > for(link = 0; !res && (link < 3); link++) {
> > 	res = probe_resource(dev, 0x100 + (reg | link));
> > 	if (dev)
> > 		res = probe_resource(dev, 0x100 + (reg | link));
>
> Going through the loop and checking dev every time is a bit nasty. What
> about

I was about writing the same comment, but then I stumbled upon a
condition which should be watched. See below.
> for(nodeid = 0; !res && (nodeid < FX_DEVS); nodeid++) {
>    device_t dev = __f0_dev[nodeid];
>    if (!dev) continue;
>    for(link = 0; !res && (link < 3); link++) {
>    	res = probe_resource(dev, 0x100 + (reg | link));
>     ...

Is link accessed in read mode after the loop? If yes, this changes the
semantics.
I don't have access to the full source code right now, so I can't check.
> > }
> >
> > }
> >    result = 2;
> > @@ -760,19 +761,20 @@
> >    	mem_hole.hole_startk = HW_MEM_HOLE_SIZEK;
> >    	mem_hole.node_id = -1;
> > for (i = 0; i < 8; i++) {
> > for (i = 0; i < FX_DEVS; i++) {
> > uint32_t base;
> > uint32_t hole;
> > base  = f1_read_config32(0x40 + (i << 3));
> > if ((base & ((1<<1)|(1<<0))) != ((1<<1)|(1<<0))) {
> > 	continue;
> > }
> > 	hole = pci_read_config32(__f1_dev[i], 0xf0);
> > 	if(hole & 1) { // we find the hole
> > 		mem_hole.hole_startk = (hole & (0xff<<24)) >> 10;
> > 		mem_hole.node_id = i; // record the node No with hole
> > 		break; // only one hole
> > 	if (__f1_dev[i]) {
>
> Same as above. Just add another continue under the base check above.

Same here.
> > 		hole = pci_read_config32(__f1_dev[i], 0xf0);
> > 		if(hole & 1) { // we find the hole
> > 			mem_hole.hole_startk = (hole & (0xff<<24)) >> 10;
> > 			mem_hole.node_id = i; // record the node No with hole
> > 			break; // only one hole
> > 		}
> > }
> > }
> >
> > @@ -834,15 +836,15 @@
> >    limit = f1_read_config32(0x44 + (i << 3));
> >    f1_write_config32(0x44 + (i << 3),limit - (hole_sizek << 2));
> >    dev = __f1_dev[i];
> > hoist = pci_read_config32(dev, 0xf0);
> > if(hoist & 1) {
> > pci_write_config32(dev, 0xf0, 0);
> > if (dev) {
>
> And the same as above.

Same here.
> > hoist = pci_read_config32(dev, 0xf0);
> > if(hoist & 1) {
> > 	pci_write_config32(dev, 0xf0, 0);
> > } else {
> > 	base = pci_read_config32(dev, 0x40 + (i << 3));
> > 	f1_write_config32(0x40 + (i << 3),base - (hole_sizek << 2));
> > }
> > }
> > else {
> > base = pci_read_config32(dev, 0x40 + (i << 3));
> > f1_write_config32(0x40 + (i << 3),base - (hole_sizek << 2));
> > }
> >
> > }
>
> ...
> > Index: src/northbridge/amd/amdfam10/northbridge.c
> >
> > --- src/northbridge/amd/amdfam10/northbridge.c	(revision 4026)
> > +++ src/northbridge/amd/amdfam10/northbridge.c	(working copy)
> > @@ -339,7 +339,8 @@
> >    	device_t dev;
> >    	dev = __f0_dev[nodeid];
> >    	for(link = 0; !res && (link < 8); link++) {
> > 	res = probe_resource(dev, 0x1000 + reg + (link<<16)); // 8 links, 0x1000 man f1,
> > 	if (dev)
> > 		res = probe_resource(dev, 0x1000 + reg + (link<<16)); // 8 links, 0x1000 man f1,
>
> And the same.

Same here.
> > }
> >
> > }
> >    result = 2;

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 25.03.2009 19:16, Rudolf Marek wrote:
> Hello,
>
> During the suspend/resume programming I came to an issue that first 4KB of
> memory must be clear with 0s because otherwise the resources of K8 will be
> totally messed up.
>
> It took long to figure it out and here it is:
>
> res = probe_resource(dev, 0x100 + (reg | link));
>
> This is called with dev = NULL and this is no good for probe_resource
> at all.
> The attached patch fixes the potential problems and of course the problem
> itself. On one particular place was missing test if the device really
> exists.
> This was copied to fam10 and perhaps the same issue is in v3 (DID NOT
> check).
> The rest of the patch is just very paranoid and do all checkings.
>
> This was tested in SimNOW but I believe it will work on real hw too.
>
> Signed-off-by: Rudolf Marek
>
> Anyone knows if SimNOW can do breaks on memory ranges?
>
> Because it was quite stupid to get to the bug. When the resources just
> worked
> again I used data breakpoint to see what routine it is. Problem was that I
> needed to find the address where it stop to work.
>
> Maybe we really should allow writes to mem only in _RAMBASE - MEMTOPK
> range (of
> course with some exception for table stuff).
>
> --- src/cpu/amd/car/clear_init_ram.c<-->(revision 4026)
> +++ src/cpu/amd/car/clear_init_ram.c<-->(working copy)
> @@ -6,7 +6,23 @@
>  <----->// gcc 3.4.5 will inline the copy_and_run and clear_init_ram in
> post_cache_as_ram
>  <----->// will reuse %edi as 0 from clear_memory for copy_and_run part,
> actually it is increased already
>  <----->// so noline clear_init_ram
>    clear_memory(0,  ((CONFIG_LB_MEM_TOPK<<10) - DCACHE_RAM_SIZE));
>    clear_memory(_RAMBASE,  ((CONFIG_LB_MEM_TOPK<<10) -_RAMBASE -
>
> DCACHE_RAM_SIZE));
> [...]
>    clear_memory(0, 924);
>
> .
>
> Oh btw the suspend/resume works, but the code needs to be fixed on
> some places.
> There is a  copy_secondary_start_to_1m_below which just uses first
> 64KB of mem
> for some reason - I dont understand why it cannot be realocable to
> something
> different addr. Please check and tell.

Awesome! Can you add a check to pci_{read,write}_config{8,16,32} for
dev!= NULL with a big LOUD warning printk? That would be great and maybe
find more bugs.

Regards,
Carl-Daniel

[Stefan Reinauer]

On 26.03.2009 11:38 Uhr, Carl-Daniel Hailfinger wrote:
> Awesome! Can you add a check to pci_{read,write}_config{8,16,32} for
> dev!= NULL with a big LOUD warning printk? That would be great and maybe
> find more bugs.

Make sure the code is not inlined, then

[Rudolf Marek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ok its in as Committed revision 4030. Please check if it really did not break
anything. /me goes to play with suspend/resume to have it before rev 4096 ;)

Rudolf
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAknL98QACgkQ3J9wPJqZRNUvlgCeK5+lOpClr/mD/RNPOzT4NRT2
4JoAoLIEugdVcozu3cYKIDNtZcQVTTEc
=+qAs
-----END PGP SIGNATURE-----