[coreboot] [PATCH] Fix the NULL dev resource usage

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Thu Mar 26 11:38:05 CET 2009


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 <r.marek at assembler.cz>
>
> 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

-- 
http://www.hailfinger.org/





More information about the coreboot mailing list