diff mbox series

hw/ppc/ppc405_boards: Change kernel load address

Message ID 20211202191446.1292125-1-clg@kaod.org (mailing list archive)
State New, archived
Headers show
Series hw/ppc/ppc405_boards: Change kernel load address | expand

Commit Message

Cédric Le Goater Dec. 2, 2021, 7:14 p.m. UTC
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(-)

Comments

Thomas Huth Dec. 2, 2021, 7:17 p.m. UTC | #1
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>
Christophe Leroy Dec. 2, 2021, 7:32 p.m. UTC | #2
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
Cédric Le Goater Dec. 3, 2021, 10:27 a.m. UTC | #3
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.
Thomas Huth Dec. 3, 2021, 10:31 a.m. UTC | #4
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
Peter Maydell Dec. 3, 2021, 10:40 a.m. UTC | #5
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
Cédric Le Goater Dec. 3, 2021, 12:25 p.m. UTC | #6
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.
Thomas Huth Dec. 8, 2021, 1:07 p.m. UTC | #7
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
Cédric Le Goater Dec. 8, 2021, 1:15 p.m. UTC | #8
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.
Thomas Huth Dec. 8, 2021, 1:19 p.m. UTC | #9
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
Cédric Le Goater Dec. 8, 2021, 4:54 p.m. UTC | #10
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.
diff mbox series

Patch

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