[PATCH] hw/ppc/ppc405_boards: Change kernel load address

[PATCH] hw/ppc/ppc405_boards: Change kernel load address

[Cédric Le Goater]

The default addresses to load the kernel, fdt, initrd of AMCC boards
in U-Boot v2015.10 are :

	"kernel_addr_r=1000000\0"
	"fdt_addr_r=1800000\0"
	"ramdisk_addr_r=1900000\0"

The taihu is one of these boards, the ref405ep is not but we don't
have much information on it and both boards have a very similar
address space layout. Change the kernel load address to match U-Boot
expectations and fix loading.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/ppc405_boards.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
index 972a7a4a3e5d..b4249f4626e6 100644
--- a/hw/ppc/ppc405_boards.c
+++ b/hw/ppc/ppc405_boards.c
@@ -45,7 +45,7 @@
 #define BIOS_FILENAME "ppc405_rom.bin"
 #define BIOS_SIZE (2 * MiB)
 
-#define KERNEL_LOAD_ADDR 0x00000000
+#define KERNEL_LOAD_ADDR 0x01000000
 #define INITRD_LOAD_ADDR 0x01800000
 
 #define USE_FLASH_BIOS
-- 
2.31.1

Re: [PATCH] hw/ppc/ppc405_boards: Change kernel load address

[Thomas Huth]

On 02/12/2021 20.14, Cédric Le Goater wrote:
> The default addresses to load the kernel, fdt, initrd of AMCC boards
> in U-Boot v2015.10 are :
> 
> 	"kernel_addr_r=1000000\0"
> 	"fdt_addr_r=1800000\0"
> 	"ramdisk_addr_r=1900000\0"
> 
> The taihu is one of these boards, the ref405ep is not but we don't
> have much information on it and both boards have a very similar
> address space layout. Change the kernel load address to match U-Boot
> expectations and fix loading.

You could additionally mention that U-Boot corrupts the kernel if it gets 
loaded to address 0.

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/ppc/ppc405_boards.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> index 972a7a4a3e5d..b4249f4626e6 100644
> --- a/hw/ppc/ppc405_boards.c
> +++ b/hw/ppc/ppc405_boards.c
> @@ -45,7 +45,7 @@
>   #define BIOS_FILENAME "ppc405_rom.bin"
>   #define BIOS_SIZE (2 * MiB)
>   
> -#define KERNEL_LOAD_ADDR 0x00000000
> +#define KERNEL_LOAD_ADDR 0x01000000
>   #define INITRD_LOAD_ADDR 0x01800000
>   
>   #define USE_FLASH_BIOS

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH] hw/ppc/ppc405_boards: Change kernel load address

[Cédric Le Goater]

On 12/2/21 20:17, Thomas Huth wrote:
> On 02/12/2021 20.14, Cédric Le Goater wrote:
>> The default addresses to load the kernel, fdt, initrd of AMCC boards
>> in U-Boot v2015.10 are :
>>
>>     "kernel_addr_r=1000000\0"
>>     "fdt_addr_r=1800000\0"
>>     "ramdisk_addr_r=1900000\0"
>>
>> The taihu is one of these boards, the ref405ep is not but we don't
>> have much information on it and both boards have a very similar
>> address space layout. Change the kernel load address to match U-Boot
>> expectations and fix loading.
> 
> You could additionally mention that U-Boot corrupts the kernel if it gets loaded to address 0.

True. The first word is overwritten with zeros. I looked for it
in the code but could not find the reason.

Thanks,

C.

> 
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/ppc/ppc405_boards.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
>> index 972a7a4a3e5d..b4249f4626e6 100644
>> --- a/hw/ppc/ppc405_boards.c
>> +++ b/hw/ppc/ppc405_boards.c
>> @@ -45,7 +45,7 @@
>>   #define BIOS_FILENAME "ppc405_rom.bin"
>>   #define BIOS_SIZE (2 * MiB)
>> -#define KERNEL_LOAD_ADDR 0x00000000
>> +#define KERNEL_LOAD_ADDR 0x01000000
>>   #define INITRD_LOAD_ADDR 0x01800000
>>   #define USE_FLASH_BIOS
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Re: [PATCH] hw/ppc/ppc405_boards: Change kernel load address

[LEROY Christophe]

Le 02/12/2021 à 20:19, Cédric Le Goater a écrit :
> On 12/2/21 20:17, Thomas Huth wrote:
>> On 02/12/2021 20.14, Cédric Le Goater wrote:
>>> The default addresses to load the kernel, fdt, initrd of AMCC boards
>>> in U-Boot v2015.10 are :
>>>
>>>     "kernel_addr_r=1000000\0"
>>>     "fdt_addr_r=1800000\0"
>>>     "ramdisk_addr_r=1900000\0"
>>>
>>> The taihu is one of these boards, the ref405ep is not but we don't
>>> have much information on it and both boards have a very similar
>>> address space layout. Change the kernel load address to match U-Boot
>>> expectations and fix loading.
>>
>> You could additionally mention that U-Boot corrupts the kernel if it 
>> gets loaded to address 0.
> 
> True. The first word is overwritten with zeros. I looked for it
> in the code but could not find the reason.
> 

Isn't it because U-boot unzips the kernel at address 0 ?

Christophe

Re: [PATCH] hw/ppc/ppc405_boards: Change kernel load address

[Cédric Le Goater]

On 12/2/21 20:32, LEROY Christophe wrote:
> 
> 
> Le 02/12/2021 à 20:19, Cédric Le Goater a écrit :
>> On 12/2/21 20:17, Thomas Huth wrote:
>>> On 02/12/2021 20.14, Cédric Le Goater wrote:
>>>> The default addresses to load the kernel, fdt, initrd of AMCC boards
>>>> in U-Boot v2015.10 are :
>>>>
>>>>      "kernel_addr_r=1000000\0"
>>>>      "fdt_addr_r=1800000\0"
>>>>      "ramdisk_addr_r=1900000\0"
>>>>
>>>> The taihu is one of these boards, the ref405ep is not but we don't
>>>> have much information on it and both boards have a very similar
>>>> address space layout. Change the kernel load address to match U-Boot
>>>> expectations and fix loading.
>>>
>>> You could additionally mention that U-Boot corrupts the kernel if it
>>> gets loaded to address 0.
>>
>> True. The first word is overwritten with zeros. I looked for it
>> in the code but could not find the reason.
>>
> 
> Isn't it because U-boot unzips the kernel at address 0 ?

Address 0 is trashed before U-boot loads the kernel.

C.

Re: [PATCH] hw/ppc/ppc405_boards: Change kernel load address

[Thomas Huth]

On 03/12/2021 11.27, Cédric Le Goater wrote:
> On 12/2/21 20:32, LEROY Christophe wrote:
>>
>>
>> Le 02/12/2021 à 20:19, Cédric Le Goater a écrit :
>>> On 12/2/21 20:17, Thomas Huth wrote:
>>>> On 02/12/2021 20.14, Cédric Le Goater wrote:
>>>>> The default addresses to load the kernel, fdt, initrd of AMCC boards
>>>>> in U-Boot v2015.10 are :
>>>>>
>>>>>      "kernel_addr_r=1000000\0"
>>>>>      "fdt_addr_r=1800000\0"
>>>>>      "ramdisk_addr_r=1900000\0"
>>>>>
>>>>> The taihu is one of these boards, the ref405ep is not but we don't
>>>>> have much information on it and both boards have a very similar
>>>>> address space layout. Change the kernel load address to match U-Boot
>>>>> expectations and fix loading.
>>>>
>>>> You could additionally mention that U-Boot corrupts the kernel if it
>>>> gets loaded to address 0.
>>>
>>> True. The first word is overwritten with zeros. I looked for it
>>> in the code but could not find the reason.
>>>
>>
>> Isn't it because U-boot unzips the kernel at address 0 ?
> 
> Address 0 is trashed before U-boot loads the kernel.

I guess it's an accidential NULL pointer dereference somewhere in the u-boot 
code ... which will be quite hard to track down when the first page of 
memory is marked as writable... :-/

  Thomas

Re: [PATCH] hw/ppc/ppc405_boards: Change kernel load address

[Peter Maydell]

On Fri, 3 Dec 2021 at 10:32, Thomas Huth <thuth@redhat.com> wrote:
> I guess it's an accidential NULL pointer dereference somewhere in the u-boot
> code ... which will be quite hard to track down when the first page of
> memory is marked as writable... :-/

Attach a target-arch gdb to the QEMU gdbstub and put a watchpoint on
address zero ? (Or if you suspect something inside QEMU is doing it
then run QEMU under gdb and watchpoint the host memory location
corresponding to guest address 0, but that's more painful.) Nothing
in the pre-kernel part of the boot process will have set up paging,
so the watchpointing should be pretty reliable.

-- PMM

Re: [PATCH] hw/ppc/ppc405_boards: Change kernel load address

[Cédric Le Goater]

On 12/3/21 11:40, Peter Maydell wrote:
> On Fri, 3 Dec 2021 at 10:32, Thomas Huth <thuth@redhat.com> wrote:
>> I guess it's an accidential NULL pointer dereference somewhere in the u-boot
>> code ... which will be quite hard to track down when the first page of
>> memory is marked as writable... :-/
> 
> Attach a target-arch gdb to the QEMU gdbstub and put a watchpoint on
> address zero ? (Or if you suspect something inside QEMU is doing it
> then run QEMU under gdb and watchpoint the host memory location
> corresponding to guest address 0, but that's more painful.) Nothing
> in the pre-kernel part of the boot process will have set up paging,
> so the watchpointing should be pretty reliable.

That's the guy:

   https://gitlab.com/huth/u-boot/-/blob/taihu/arch/powerpc/cpu/ppc4xx/sdram.c#L199

There must be an error in how get_ram_size() restores the RAM values :

   https://gitlab.com/huth/u-boot/-/blob/taihu/common/memsize.c

C.

Re: [PATCH] hw/ppc/ppc405_boards: Change kernel load address

[Thomas Huth]

On 03/12/2021 13.25, Cédric Le Goater wrote:
> On 12/3/21 11:40, Peter Maydell wrote:
>> On Fri, 3 Dec 2021 at 10:32, Thomas Huth <thuth@redhat.com> wrote:
>>> I guess it's an accidential NULL pointer dereference somewhere in the u-boot
>>> code ... which will be quite hard to track down when the first page of
>>> memory is marked as writable... :-/
>>
>> Attach a target-arch gdb to the QEMU gdbstub and put a watchpoint on
>> address zero ? (Or if you suspect something inside QEMU is doing it
>> then run QEMU under gdb and watchpoint the host memory location
>> corresponding to guest address 0, but that's more painful.) Nothing
>> in the pre-kernel part of the boot process will have set up paging,
>> so the watchpointing should be pretty reliable.
> 
> That's the guy:
> 
>    
> https://gitlab.com/huth/u-boot/-/blob/taihu/arch/powerpc/cpu/ppc4xx/sdram.c#L199 
> 
> 
> There must be an error in how get_ram_size() restores the RAM values :
> 
>    https://gitlab.com/huth/u-boot/-/blob/taihu/common/memsize.c

There is definitely something wrong in that function. Seems like they tried 
to fix it once here:

  https://source.denx.de/u-boot/u-boot/-/commit/b8496cced856ff411f

but that patch got later reverted without a replacement later...

  Thomas

Re: [PATCH] hw/ppc/ppc405_boards: Change kernel load address

[Cédric Le Goater]

On 12/8/21 14:07, Thomas Huth wrote:
> On 03/12/2021 13.25, Cédric Le Goater wrote:
>> On 12/3/21 11:40, Peter Maydell wrote:
>>> On Fri, 3 Dec 2021 at 10:32, Thomas Huth <thuth@redhat.com> wrote:
>>>> I guess it's an accidential NULL pointer dereference somewhere in the u-boot
>>>> code ... which will be quite hard to track down when the first page of
>>>> memory is marked as writable... :-/
>>>
>>> Attach a target-arch gdb to the QEMU gdbstub and put a watchpoint on
>>> address zero ? (Or if you suspect something inside QEMU is doing it
>>> then run QEMU under gdb and watchpoint the host memory location
>>> corresponding to guest address 0, but that's more painful.) Nothing
>>> in the pre-kernel part of the boot process will have set up paging,
>>> so the watchpointing should be pretty reliable.
>>
>> That's the guy:
>>
>> https://gitlab.com/huth/u-boot/-/blob/taihu/arch/powerpc/cpu/ppc4xx/sdram.c#L199
>>
>> There must be an error in how get_ram_size() restores the RAM values :
>>
>>    https://gitlab.com/huth/u-boot/-/blob/taihu/common/memsize.c
> 
> There is definitely something wrong in that function. Seems like they tried to fix it once here:
> 
>   https://source.denx.de/u-boot/u-boot/-/commit/b8496cced856ff411f
> 
> but that patch got later reverted without a replacement later...


a fix restoring address 0, something like :

@@ -60,6 +60,9 @@ long get_ram_size(long *base, long maxsi
  		return (0);
  	}
  
+	addr = base;
+	*addr = save[i];
+
  	for (cnt = 1; cnt < maxsize / sizeof(long); cnt <<= 1) {
  		addr = base + cnt;	/* pointer arith! */
  		val = *addr;

is not enough. trap_init() will also overwrite the kernel image.
And u-boot will complain about a wrong CRC.

The 405 series I sent improves support and latest kernel 5.16-rc4
can be loaded without uboot. It's a start to debug user space.

Thanks,

C.

Re: [PATCH] hw/ppc/ppc405_boards: Change kernel load address

[Thomas Huth]

On 08/12/2021 14.15, Cédric Le Goater wrote:
> On 12/8/21 14:07, Thomas Huth wrote:
>> On 03/12/2021 13.25, Cédric Le Goater wrote:
>>> On 12/3/21 11:40, Peter Maydell wrote:
>>>> On Fri, 3 Dec 2021 at 10:32, Thomas Huth <thuth@redhat.com> wrote:
>>>>> I guess it's an accidential NULL pointer dereference somewhere in the 
>>>>> u-boot
>>>>> code ... which will be quite hard to track down when the first page of
>>>>> memory is marked as writable... :-/
>>>>
>>>> Attach a target-arch gdb to the QEMU gdbstub and put a watchpoint on
>>>> address zero ? (Or if you suspect something inside QEMU is doing it
>>>> then run QEMU under gdb and watchpoint the host memory location
>>>> corresponding to guest address 0, but that's more painful.) Nothing
>>>> in the pre-kernel part of the boot process will have set up paging,
>>>> so the watchpointing should be pretty reliable.
>>>
>>> That's the guy:
>>>
>>> https://gitlab.com/huth/u-boot/-/blob/taihu/arch/powerpc/cpu/ppc4xx/sdram.c#L199 
>>>
>>>
>>> There must be an error in how get_ram_size() restores the RAM values :
>>>
>>>    https://gitlab.com/huth/u-boot/-/blob/taihu/common/memsize.c
>>
>> There is definitely something wrong in that function. Seems like they 
>> tried to fix it once here:
>>
>>   https://source.denx.de/u-boot/u-boot/-/commit/b8496cced856ff411f
>>
>> but that patch got later reverted without a replacement later...
> 
> 
> a fix restoring address 0, something like :
> 
> @@ -60,6 +60,9 @@ long get_ram_size(long *base, long maxsi
>           return (0);
>       }
> 
> +    addr = base;
> +    *addr = save[i];
> +
>       for (cnt = 1; cnt < maxsize / sizeof(long); cnt <<= 1) {
>           addr = base + cnt;    /* pointer arith! */
>           val = *addr;
> 
> is not enough. trap_init() will also overwrite the kernel image.
> And u-boot will complain about a wrong CRC.
> 
> The 405 series I sent improves support and latest kernel 5.16-rc4
> can be loaded without uboot. It's a start to debug user space.

Yes, your series is certainly the better way forward. I'll stop messing with 
u-boot now ... since upstream u-boot already removed the 405 support a long 
time ago, my hack was a dead end anyway (but it helped to get at least a 
kernel running again, so it was not in vain)

  Thomas

Re: [PATCH] hw/ppc/ppc405_boards: Change kernel load address

[Cédric Le Goater]

On 12/8/21 14:19, Thomas Huth wrote:
> On 08/12/2021 14.15, Cédric Le Goater wrote:
>> On 12/8/21 14:07, Thomas Huth wrote:
>>> On 03/12/2021 13.25, Cédric Le Goater wrote:
>>>> On 12/3/21 11:40, Peter Maydell wrote:
>>>>> On Fri, 3 Dec 2021 at 10:32, Thomas Huth <thuth@redhat.com> wrote:
>>>>>> I guess it's an accidential NULL pointer dereference somewhere in the u-boot
>>>>>> code ... which will be quite hard to track down when the first page of
>>>>>> memory is marked as writable... :-/
>>>>>
>>>>> Attach a target-arch gdb to the QEMU gdbstub and put a watchpoint on
>>>>> address zero ? (Or if you suspect something inside QEMU is doing it
>>>>> then run QEMU under gdb and watchpoint the host memory location
>>>>> corresponding to guest address 0, but that's more painful.) Nothing
>>>>> in the pre-kernel part of the boot process will have set up paging,
>>>>> so the watchpointing should be pretty reliable.
>>>>
>>>> That's the guy:
>>>>
>>>> https://gitlab.com/huth/u-boot/-/blob/taihu/arch/powerpc/cpu/ppc4xx/sdram.c#L199
>>>>
>>>> There must be an error in how get_ram_size() restores the RAM values :
>>>>
>>>>    https://gitlab.com/huth/u-boot/-/blob/taihu/common/memsize.c
>>>
>>> There is definitely something wrong in that function. Seems like they tried to fix it once here:
>>>
>>>   https://source.denx.de/u-boot/u-boot/-/commit/b8496cced856ff411f
>>>
>>> but that patch got later reverted without a replacement later...
>>
>>
>> a fix restoring address 0, something like :
>>
>> @@ -60,6 +60,9 @@ long get_ram_size(long *base, long maxsi
>>           return (0);
>>       }
>>
>> +    addr = base;
>> +    *addr = save[i];
>> +
>>       for (cnt = 1; cnt < maxsize / sizeof(long); cnt <<= 1) {
>>           addr = base + cnt;    /* pointer arith! */
>>           val = *addr;
>>
>> is not enough. trap_init() will also overwrite the kernel image.
>> And u-boot will complain about a wrong CRC.
>>
>> The 405 series I sent improves support and latest kernel 5.16-rc4
>> can be loaded without uboot. It's a start to debug user space.
> 
> Yes, your series is certainly the better way forward. I'll stop messing 
> with u-boot now ... since upstream u-boot already removed the 405 support 
> a long time ago, my hack was a dead end anyway (but it helped to get at 
> least a kernel running again, so it was not in vain)

This is not what I meant ! sorry ! What I wanted to express is that
now nothing is blocking us from fixing user space :)

The patchset I sent does not break booting from U-Boot. It still
very much works and we should keep it because it exercises more
our models. Even if the SDRAM still needs a U-Boot hack. I believe
that can be fixed in QEMU with some time. When done, I hope we
can merge the U-Boot binary in QEMU.

Regarding Linux loading in QEMU or U-Boot, the problem is the PPC
board information which is out of sync in all components ...

Today, the easiest is to boot directly from a Linux 405 hotfoot
kernel under QEMU because we are a bit lucky. The proc frequency
is set to some non zero value so the kernel boots. I pushed
experiments a bit further and network seems to be OK also.

My idea was to merge the first round of changes for direct linux
boot. See how that goes (user space) and then merge a custom
ppc405-hotfoot machine to work around the board information
problem.

Thanks,

C.