[PATCH] hw/arm/boot: set initrd parameters to 64bit in fdt

Schspa Shi posted 1 patch 1 year, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20221108023542.17557-1-schspa@gmail.com
Maintainers: Peter Maydell <peter.maydell@linaro.org>
hw/arm/boot.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] hw/arm/boot: set initrd parameters to 64bit in fdt
Posted by Schspa Shi 1 year, 5 months ago
We use 32bit value for linux,initrd-[start/end], when we have
loader_start > 4GB, there will be a wrong initrd_start passed
to the kernel, and the kernel will report the following warning.

[    0.000000] ------------[ cut here ]------------
[    0.000000] initrd not fully accessible via the linear mapping -- please check your bootloader ...
[    0.000000] WARNING: CPU: 0 PID: 0 at arch/arm64/mm/init.c:355 arm64_memblock_init+0x158/0x244
[    0.000000] Modules linked in:
[    0.000000] CPU: 0 PID: 0 Comm: swapper Tainted: G        W          6.1.0-rc3-13250-g30a0b95b1335-dirty #28
[    0.000000] Hardware name: Horizon Sigi Virtual development board (DT)
[    0.000000] pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    0.000000] pc : arm64_memblock_init+0x158/0x244
[    0.000000] lr : arm64_memblock_init+0x158/0x244
[    0.000000] sp : ffff800009273df0
[    0.000000] x29: ffff800009273df0 x28: 0000001000cc0010 x27: 0000800000000000
[    0.000000] x26: 000000000050a3e2 x25: ffff800008b46000 x24: ffff800008b46000
[    0.000000] x23: ffff800008a53000 x22: ffff800009420000 x21: ffff800008a53000
[    0.000000] x20: 0000000004000000 x19: 0000000004000000 x18: 00000000ffff1020
[    0.000000] x17: 6568632065736165 x16: 6c70202d2d20676e x15: 697070616d207261
[    0.000000] x14: 656e696c20656874 x13: 0a2e2e2e20726564 x12: 0000000000000000
[    0.000000] x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000
[    0.000000] x8 : 0000000000000000 x7 : 796c6c756620746f x6 : 6e20647274696e69
[    0.000000] x5 : ffff8000093c7c47 x4 : ffff800008a2102f x3 : ffff800009273a88
[    0.000000] x2 : 80000000fffff038 x1 : 00000000000000c0 x0 : 0000000000000056
[    0.000000] Call trace:
[    0.000000]  arm64_memblock_init+0x158/0x244
[    0.000000]  setup_arch+0x164/0x1cc
[    0.000000]  start_kernel+0x94/0x4ac
[    0.000000]  __primary_switched+0xb4/0xbc
[    0.000000] ---[ end trace 0000000000000000 ]---
[    0.000000] Zone ranges:
[    0.000000]   DMA      [mem 0x0000001000000000-0x0000001007ffffff]

To fix it, we can change it to u64 type.

Signed-off-by: Schspa Shi <schspa@gmail.com>
---
 hw/arm/boot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 57efb61ee419..da719a4f8874 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -638,14 +638,14 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
     }
 
     if (binfo->initrd_size) {
-        rc = qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start",
+        rc = qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-start",
                                    binfo->initrd_start);
         if (rc < 0) {
             fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n");
             goto fail;
         }
 
-        rc = qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end",
+        rc = qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-end",
                                    binfo->initrd_start + binfo->initrd_size);
         if (rc < 0) {
             fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");
-- 
2.37.3
Re: [PATCH] hw/arm/boot: set initrd parameters to 64bit in fdt
Posted by Peter Maydell 1 year, 4 months ago
On Tue, 8 Nov 2022 at 02:35, Schspa Shi <schspa@gmail.com> wrote:
>
> We use 32bit value for linux,initrd-[start/end], when we have
> loader_start > 4GB, there will be a wrong initrd_start passed
> to the kernel, and the kernel will report the following warning

> To fix it, we can change it to u64 type.
>
> Signed-off-by: Schspa Shi <schspa@gmail.com>
> ---
>  hw/arm/boot.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 57efb61ee419..da719a4f8874 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -638,14 +638,14 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>      }
>
>      if (binfo->initrd_size) {
> -        rc = qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start",
> +        rc = qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-start",
>                                     binfo->initrd_start);
>          if (rc < 0) {
>              fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n");
>              goto fail;
>          }
>
> -        rc = qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end",
> +        rc = qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-end",
>                                     binfo->initrd_start + binfo->initrd_size);
>          if (rc < 0) {
>              fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");

Hi; the device-tree folks got back to me a bit late on this one,
but apparently the intention is that these fields should be
of a size that matches the #address-cells (and they'll fix the
schema docs to say that at some point). Some future kernel
or dtb-schema-check might warn about this, and also since it is
what u-boot does:
https://github.com/u-boot/u-boot/blob/218e2c45af83f2cb7b1374b9023b4ced6eb0bb77/common/fdt_support.c#L248
following that same approach is the safest thing in terms of not
breaking existing code.

I think that to do this in QEMU we just need to call

   qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-start",
                                acells, binfo->initrd_start);

and similarly for initrd-end.

Would you mind doing a respin and test of this patch that works
that way?

thanks
-- PMM
Re: [PATCH] hw/arm/boot: set initrd parameters to 64bit in fdt
Posted by Schspa Shi 1 year, 4 months ago
Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 8 Nov 2022 at 02:35, Schspa Shi <schspa@gmail.com> wrote:
>>
>> We use 32bit value for linux,initrd-[start/end], when we have
>> loader_start > 4GB, there will be a wrong initrd_start passed
>> to the kernel, and the kernel will report the following warning
>
>> To fix it, we can change it to u64 type.
>>
>> Signed-off-by: Schspa Shi <schspa@gmail.com>
>> ---
>>  hw/arm/boot.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 57efb61ee419..da719a4f8874 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -638,14 +638,14 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>>      }
>>
>>      if (binfo->initrd_size) {
>> -        rc = qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start",
>> +        rc = qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-start",
>>                                     binfo->initrd_start);
>>          if (rc < 0) {
>>              fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n");
>>              goto fail;
>>          }
>>
>> -        rc = qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end",
>> +        rc = qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-end",
>>                                     binfo->initrd_start + binfo->initrd_size);
>>          if (rc < 0) {
>>              fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");
>
> Hi; the device-tree folks got back to me a bit late on this one,
> but apparently the intention is that these fields should be
> of a size that matches the #address-cells (and they'll fix the
> schema docs to say that at some point). Some future kernel
> or dtb-schema-check might warn about this, and also since it is
> what u-boot does:
> https://github.com/u-boot/u-boot/blob/218e2c45af83f2cb7b1374b9023b4ced6eb0bb77/common/fdt_support.c#L248
> following that same approach is the safest thing in terms of not
> breaking existing code.
>
> I think that to do this in QEMU we just need to call
>
>    qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-start",
>                                 acells, binfo->initrd_start);
>
> and similarly for initrd-end.
>
> Would you mind doing a respin and test of this patch that works
> that way?
>

I have made a new patch & test it this way, it works perfectly.

> thanks
> -- PMM


-- 
BRs
Schspa Shi
Re: [PATCH] hw/arm/boot: set initrd parameters to 64bit in fdt
Posted by Alex Bennée 1 year, 5 months ago
Schspa Shi <schspa@gmail.com> writes:

> We use 32bit value for linux,initrd-[start/end], when we have
> loader_start > 4GB, there will be a wrong initrd_start passed
> to the kernel, and the kernel will report the following warning.
>
> [    0.000000] ------------[ cut here ]------------
> [    0.000000] initrd not fully accessible via the linear mapping -- please check your bootloader ...
> [    0.000000] WARNING: CPU: 0 PID: 0 at arch/arm64/mm/init.c:355 arm64_memblock_init+0x158/0x244
> [    0.000000] Modules linked in:
> [    0.000000] CPU: 0 PID: 0 Comm: swapper Tainted: G        W          6.1.0-rc3-13250-g30a0b95b1335-dirty #28
> [    0.000000] Hardware name: Horizon Sigi Virtual development board
> (DT)

Is this an out-of-tree board model?

> [    0.000000] pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    0.000000] pc : arm64_memblock_init+0x158/0x244
> [    0.000000] lr : arm64_memblock_init+0x158/0x244
> [    0.000000] sp : ffff800009273df0
> [    0.000000] x29: ffff800009273df0 x28: 0000001000cc0010 x27: 0000800000000000
> [    0.000000] x26: 000000000050a3e2 x25: ffff800008b46000 x24: ffff800008b46000
> [    0.000000] x23: ffff800008a53000 x22: ffff800009420000 x21: ffff800008a53000
> [    0.000000] x20: 0000000004000000 x19: 0000000004000000 x18: 00000000ffff1020
> [    0.000000] x17: 6568632065736165 x16: 6c70202d2d20676e x15: 697070616d207261
> [    0.000000] x14: 656e696c20656874 x13: 0a2e2e2e20726564 x12: 0000000000000000
> [    0.000000] x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000
> [    0.000000] x8 : 0000000000000000 x7 : 796c6c756620746f x6 : 6e20647274696e69
> [    0.000000] x5 : ffff8000093c7c47 x4 : ffff800008a2102f x3 : ffff800009273a88
> [    0.000000] x2 : 80000000fffff038 x1 : 00000000000000c0 x0 : 0000000000000056
> [    0.000000] Call trace:
> [    0.000000]  arm64_memblock_init+0x158/0x244
> [    0.000000]  setup_arch+0x164/0x1cc
> [    0.000000]  start_kernel+0x94/0x4ac
> [    0.000000]  __primary_switched+0xb4/0xbc
> [    0.000000] ---[ end trace 0000000000000000 ]---
> [    0.000000] Zone ranges:
> [    0.000000]   DMA      [mem 0x0000001000000000-0x0000001007ffffff]
>
> To fix it, we can change it to u64 type.
>
> Signed-off-by: Schspa Shi <schspa@gmail.com>
> ---
>  hw/arm/boot.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 57efb61ee419..da719a4f8874 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -638,14 +638,14 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>      }
>  
>      if (binfo->initrd_size) {
> -        rc = qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start",
> +        rc = qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-start",
>                                     binfo->initrd_start);
>          if (rc < 0) {
>              fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n");
>              goto fail;
>          }
>  
> -        rc = qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end",
> +        rc = qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-end",
>                                     binfo->initrd_start + binfo->initrd_size);
>          if (rc < 0) {
>              fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");

On the face of things this seems fine because unlike the other linux
properties they are not specified to be "expressed in #address-cells and
#size-cells" but I do wonder how we got into the situation where the
kernel and initrd ended up so high in the physical address space.

There is a whole comment in boot.c talking about keeping initrd within
lowmem:

    /*
     * We want to put the initrd far enough into RAM that when the
     * kernel is uncompressed it will not clobber the initrd. However
     * on boards without much RAM we must ensure that we still leave
     * enough room for a decent sized initrd, and on boards with large
     * amounts of RAM we must avoid the initrd being so far up in RAM
     * that it is outside lowmem and inaccessible to the kernel.
     * So for boards with less  than 256MB of RAM we put the initrd
     * halfway into RAM, and for boards with 256MB of RAM or more we put
     * the initrd at 128MB.
     * We also refuse to put the initrd somewhere that will definitely
     * overlay the kernel we just loaded, though for kernel formats which
     * don't tell us their exact size (eg self-decompressing 32-bit kernels)
     * we might still make a bad choice here.
     */

Is this just because the base RAM address of the board is outside of the
32 bit address range?

-- 
Alex Bennée
Re: [PATCH] hw/arm/boot: set initrd parameters to 64bit in fdt
Posted by Schspa Shi 1 year, 5 months ago
Alex Bennée <alex.bennee@linaro.org> writes:

> Schspa Shi <schspa@gmail.com> writes:
>
>> We use 32bit value for linux,initrd-[start/end], when we have
>> loader_start > 4GB, there will be a wrong initrd_start passed
>> to the kernel, and the kernel will report the following warning.
>>
>> [    0.000000] ------------[ cut here ]------------
>> [    0.000000] initrd not fully accessible via the linear mapping -- please check your bootloader ...
>> [    0.000000] WARNING: CPU: 0 PID: 0 at arch/arm64/mm/init.c:355 arm64_memblock_init+0x158/0x244
>> [    0.000000] Modules linked in:
>> [    0.000000] CPU: 0 PID: 0 Comm: swapper Tainted: G        W          6.1.0-rc3-13250-g30a0b95b1335-dirty #28
>> [    0.000000] Hardware name: Horizon Sigi Virtual development board
>> (DT)
>
> Is this an out-of-tree board model?
>

Yes, this is a virtual board created by myself.

>> [    0.000000] pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> [    0.000000] pc : arm64_memblock_init+0x158/0x244
>> [    0.000000] lr : arm64_memblock_init+0x158/0x244
>> [    0.000000] sp : ffff800009273df0
>> [    0.000000] x29: ffff800009273df0 x28: 0000001000cc0010 x27: 0000800000000000
>> [    0.000000] x26: 000000000050a3e2 x25: ffff800008b46000 x24: ffff800008b46000
>> [    0.000000] x23: ffff800008a53000 x22: ffff800009420000 x21: ffff800008a53000
>> [    0.000000] x20: 0000000004000000 x19: 0000000004000000 x18: 00000000ffff1020
>> [    0.000000] x17: 6568632065736165 x16: 6c70202d2d20676e x15: 697070616d207261
>> [    0.000000] x14: 656e696c20656874 x13: 0a2e2e2e20726564 x12: 0000000000000000
>> [    0.000000] x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000
>> [    0.000000] x8 : 0000000000000000 x7 : 796c6c756620746f x6 : 6e20647274696e69
>> [    0.000000] x5 : ffff8000093c7c47 x4 : ffff800008a2102f x3 : ffff800009273a88
>> [    0.000000] x2 : 80000000fffff038 x1 : 00000000000000c0 x0 : 0000000000000056
>> [    0.000000] Call trace:
>> [    0.000000]  arm64_memblock_init+0x158/0x244
>> [    0.000000]  setup_arch+0x164/0x1cc
>> [    0.000000]  start_kernel+0x94/0x4ac
>> [    0.000000]  __primary_switched+0xb4/0xbc
>> [    0.000000] ---[ end trace 0000000000000000 ]---
>> [    0.000000] Zone ranges:
>> [    0.000000]   DMA      [mem 0x0000001000000000-0x0000001007ffffff]
>>
>> To fix it, we can change it to u64 type.
>>
>> Signed-off-by: Schspa Shi <schspa@gmail.com>
>> ---
>>  hw/arm/boot.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 57efb61ee419..da719a4f8874 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -638,14 +638,14 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>>      }
>>
>>      if (binfo->initrd_size) {
>> -        rc = qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start",
>> +        rc = qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-start",
>>                                     binfo->initrd_start);
>>          if (rc < 0) {
>>              fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n");
>>              goto fail;
>>          }
>>
>> -        rc = qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end",
>> +        rc = qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-end",
>>                                     binfo->initrd_start + binfo->initrd_size);
>>          if (rc < 0) {
>>              fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");
>
> On the face of things this seems fine because unlike the other linux
> properties they are not specified to be "expressed in #address-cells and
> #size-cells" but I do wonder how we got into the situation where the
> kernel and initrd ended up so high in the physical address space.
>

The reason why I faced this problem is there is no DRAM region below
4GB on the hardware. And I make this virtual board to have the same
memory layout as the hardware.

Although there is no something like #address-cells and #size-cells,
but Linux will handle the size correctly by calling the following code:

prop = of_get_flat_dt_prop(node, "linux,initrd-start", &len);

Please check the link at
Link: https://lore.kernel.org/all/20091105074724.10460.4083.stgit@angua/
Every Linux version support fdt on arm[64] platform should work without
problems.

> There is a whole comment in boot.c talking about keeping initrd within
> lowmem:
>
>     /*
>      * We want to put the initrd far enough into RAM that when the
>      * kernel is uncompressed it will not clobber the initrd. However
>      * on boards without much RAM we must ensure that we still leave
>      * enough room for a decent sized initrd, and on boards with large
>      * amounts of RAM we must avoid the initrd being so far up in RAM
>      * that it is outside lowmem and inaccessible to the kernel.
>      * So for boards with less  than 256MB of RAM we put the initrd
>      * halfway into RAM, and for boards with 256MB of RAM or more we put
>      * the initrd at 128MB.
>      * We also refuse to put the initrd somewhere that will definitely
>      * overlay the kernel we just loaded, though for kernel formats which
>      * don't tell us their exact size (eg self-decompressing 32-bit kernels)
>      * we might still make a bad choice here.
>      */
>

I think this lowmem does not mean below 4GB. and it is to make sure
the initrd_start > memblock_start_of_DRAM for Linux address range check.

> Is this just because the base RAM address of the board is outside of the
> 32 bit address range?

Yes, it is.


--
BRs
Schspa Shi
Re: [PATCH] hw/arm/boot: set initrd parameters to 64bit in fdt
Posted by Peter Maydell 1 year, 5 months ago
On Tue, 8 Nov 2022 at 12:52, Schspa Shi <schspa@gmail.com> wrote:
> Alex Bennée <alex.bennee@linaro.org> writes:
> > There is a whole comment in boot.c talking about keeping initrd within
> > lowmem:
> >
> >     /*
> >      * We want to put the initrd far enough into RAM that when the
> >      * kernel is uncompressed it will not clobber the initrd. However
> >      * on boards without much RAM we must ensure that we still leave
> >      * enough room for a decent sized initrd, and on boards with large
> >      * amounts of RAM we must avoid the initrd being so far up in RAM
> >      * that it is outside lowmem and inaccessible to the kernel.
> >      * So for boards with less  than 256MB of RAM we put the initrd
> >      * halfway into RAM, and for boards with 256MB of RAM or more we put
> >      * the initrd at 128MB.
> >      * We also refuse to put the initrd somewhere that will definitely
> >      * overlay the kernel we just loaded, though for kernel formats which
> >      * don't tell us their exact size (eg self-decompressing 32-bit kernels)
> >      * we might still make a bad choice here.
> >      */
> >
>
> I think this lowmem does not mean below 4GB. and it is to make sure
> the initrd_start > memblock_start_of_DRAM for Linux address range check.

The wording of this comment pre-dates 64-bit CPU support: it
is talking about the requirement in the 32-bit booting doc
https://www.kernel.org/doc/Documentation/arm/Booting
that says
"If an initramfs is in use then, as with the dtb, it must be placed in
a region of memory where the kernel decompressor will not overwrite it
while also with the region which will be covered by the kernel's
low-memory mapping."

So it does mean "below 4GB", because you can't boot a 32-bit kernel
if you don't put the kernel, initrd, etc below 4GB.

thanks
-- PMM
Re: [PATCH] hw/arm/boot: set initrd parameters to 64bit in fdt
Posted by Peter Maydell 1 year, 5 months ago
On Tue, 8 Nov 2022 at 13:54, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 8 Nov 2022 at 12:52, Schspa Shi <schspa@gmail.com> wrote:
> > Alex Bennée <alex.bennee@linaro.org> writes:
> > > There is a whole comment in boot.c talking about keeping initrd within
> > > lowmem:
> > >
> > >     /*
> > >      * We want to put the initrd far enough into RAM that when the
> > >      * kernel is uncompressed it will not clobber the initrd. However
> > >      * on boards without much RAM we must ensure that we still leave
> > >      * enough room for a decent sized initrd, and on boards with large
> > >      * amounts of RAM we must avoid the initrd being so far up in RAM
> > >      * that it is outside lowmem and inaccessible to the kernel.
> > >      * So for boards with less  than 256MB of RAM we put the initrd
> > >      * halfway into RAM, and for boards with 256MB of RAM or more we put
> > >      * the initrd at 128MB.
> > >      * We also refuse to put the initrd somewhere that will definitely
> > >      * overlay the kernel we just loaded, though for kernel formats which
> > >      * don't tell us their exact size (eg self-decompressing 32-bit kernels)
> > >      * we might still make a bad choice here.
> > >      */
> > >
> >
> > I think this lowmem does not mean below 4GB. and it is to make sure
> > the initrd_start > memblock_start_of_DRAM for Linux address range check.
>
> The wording of this comment pre-dates 64-bit CPU support: it
> is talking about the requirement in the 32-bit booting doc
> https://www.kernel.org/doc/Documentation/arm/Booting
> that says
> "If an initramfs is in use then, as with the dtb, it must be placed in
> a region of memory where the kernel decompressor will not overwrite it
> while also with the region which will be covered by the kernel's
> low-memory mapping."
>
> So it does mean "below 4GB", because you can't boot a 32-bit kernel
> if you don't put the kernel, initrd, etc below 4GB.

A kernel person corrects me on the meaning of "lowmem" here -- the
kernel means by it "within the first 768MB of RAM". There is also
an implicit requirement that everything be within the bottom 32-bits
of the physical address space.

-- PMM
Re: [PATCH] hw/arm/boot: set initrd parameters to 64bit in fdt
Posted by Schspa Shi 1 year, 5 months ago
Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 8 Nov 2022 at 13:54, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Tue, 8 Nov 2022 at 12:52, Schspa Shi <schspa@gmail.com> wrote:
>> > Alex Bennée <alex.bennee@linaro.org> writes:
>> > > There is a whole comment in boot.c talking about keeping initrd within
>> > > lowmem:
>> > >
>> > >     /*
>> > >      * We want to put the initrd far enough into RAM that when the
>> > >      * kernel is uncompressed it will not clobber the initrd. However
>> > >      * on boards without much RAM we must ensure that we still leave
>> > >      * enough room for a decent sized initrd, and on boards with large
>> > >      * amounts of RAM we must avoid the initrd being so far up in RAM
>> > >      * that it is outside lowmem and inaccessible to the kernel.
>> > >      * So for boards with less  than 256MB of RAM we put the initrd
>> > >      * halfway into RAM, and for boards with 256MB of RAM or more we put
>> > >      * the initrd at 128MB.
>> > >      * We also refuse to put the initrd somewhere that will definitely
>> > >      * overlay the kernel we just loaded, though for kernel formats which
>> > >      * don't tell us their exact size (eg self-decompressing 32-bit kernels)
>> > >      * we might still make a bad choice here.
>> > >      */
>> > >
>> >
>> > I think this lowmem does not mean below 4GB. and it is to make sure
>> > the initrd_start > memblock_start_of_DRAM for Linux address range check.
>>
>> The wording of this comment pre-dates 64-bit CPU support: it
>> is talking about the requirement in the 32-bit booting doc
>> https://www.kernel.org/doc/Documentation/arm/Booting
>> that says
>> "If an initramfs is in use then, as with the dtb, it must be placed in
>> a region of memory where the kernel decompressor will not overwrite it
>> while also with the region which will be covered by the kernel's
>> low-memory mapping."
>>
>> So it does mean "below 4GB", because you can't boot a 32-bit kernel
>> if you don't put the kernel, initrd, etc below 4GB.
>
> A kernel person corrects me on the meaning of "lowmem" here -- the
> kernel means by it "within the first 768MB of RAM". There is also
> an implicit requirement that everything be within the bottom 32-bits
> of the physical address space.
>

Thanks for your comment.

In this view, initrd shouldn't be placed higher than 4GB ? But it
seems the Linux kernel can boot when there is no memory below 4GB.

I know that lowmem is needed for SWIOTLB etc. It will be used to make
the 32bit IP work without IOMMU. But it seems it's not required to
boot.

> -- PMM

-- 
BRs
Schspa Shi
Re: [PATCH] hw/arm/boot: set initrd parameters to 64bit in fdt
Posted by Peter Maydell 1 year, 5 months ago
On Tue, 8 Nov 2022 at 15:50, Schspa Shi <schspa@gmail.com> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Tue, 8 Nov 2022 at 13:54, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>
> >> On Tue, 8 Nov 2022 at 12:52, Schspa Shi <schspa@gmail.com> wrote:
> >> > I think this lowmem does not mean below 4GB. and it is to make sure
> >> > the initrd_start > memblock_start_of_DRAM for Linux address range check.
> >>
> >> The wording of this comment pre-dates 64-bit CPU support: it
> >> is talking about the requirement in the 32-bit booting doc
> >> https://www.kernel.org/doc/Documentation/arm/Booting
> >> that says
> >> "If an initramfs is in use then, as with the dtb, it must be placed in
> >> a region of memory where the kernel decompressor will not overwrite it
> >> while also with the region which will be covered by the kernel's
> >> low-memory mapping."
> >>
> >> So it does mean "below 4GB", because you can't boot a 32-bit kernel
> >> if you don't put the kernel, initrd, etc below 4GB.
> >
> > A kernel person corrects me on the meaning of "lowmem" here -- the
> > kernel means by it "within the first 768MB of RAM". There is also
> > an implicit requirement that everything be within the bottom 32-bits
> > of the physical address space.
> >
>
> Thanks for your comment.
>
> In this view, initrd shouldn't be placed higher than 4GB ? But it
> seems the Linux kernel can boot when there is no memory below 4GB.

A *32 bit* kernel cannot -- it is completely unable to access
anything above the 4GB mark when the MMU is off, as it is on
initial boot. This QEMU code handles both 32 bit and 64 bit
kernel boot. These days of course there is 64-bit only hardware,
and that might choose to put its RAM above the 4GB mark,
because it isn't ever going to boot a 32-bit kernel anyway.

thanks
-- PMM
Re: [PATCH] hw/arm/boot: set initrd parameters to 64bit in fdt
Posted by Schspa Shi 1 year, 5 months ago
Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 8 Nov 2022 at 15:50, Schspa Shi <schspa@gmail.com> wrote:
>>
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> > On Tue, 8 Nov 2022 at 13:54, Peter Maydell <peter.maydell@linaro.org> wrote:
>> >>
>> >> On Tue, 8 Nov 2022 at 12:52, Schspa Shi <schspa@gmail.com> wrote:
>> >> > I think this lowmem does not mean below 4GB. and it is to make sure
>> >> > the initrd_start > memblock_start_of_DRAM for Linux address range check.
>> >>
>> >> The wording of this comment pre-dates 64-bit CPU support: it
>> >> is talking about the requirement in the 32-bit booting doc
>> >> https://www.kernel.org/doc/Documentation/arm/Booting
>> >> that says
>> >> "If an initramfs is in use then, as with the dtb, it must be placed in
>> >> a region of memory where the kernel decompressor will not overwrite it
>> >> while also with the region which will be covered by the kernel's
>> >> low-memory mapping."
>> >>
>> >> So it does mean "below 4GB", because you can't boot a 32-bit kernel
>> >> if you don't put the kernel, initrd, etc below 4GB.
>> >
>> > A kernel person corrects me on the meaning of "lowmem" here -- the
>> > kernel means by it "within the first 768MB of RAM". There is also
>> > an implicit requirement that everything be within the bottom 32-bits
>> > of the physical address space.
>> >
>>
>> Thanks for your comment.
>>
>> In this view, initrd shouldn't be placed higher than 4GB ? But it
>> seems the Linux kernel can boot when there is no memory below 4GB.
>
> A *32 bit* kernel cannot -- it is completely unable to access
> anything above the 4GB mark when the MMU is off, as it is on
> initial boot. This QEMU code handles both 32 bit and 64 bit
> kernel boot. These days of course there is 64-bit only hardware,
> and that might choose to put its RAM above the 4GB mark,
> because it isn't ever going to boot a 32-bit kernel anyway.
>

Yes, I think we should accept this patch, because it will not affect
32-bit devices, and provides support for 64-bit devices to put initrd
above 4GB.

> thanks
> -- PMM


-- 
BRs
Schspa Shi
Re: [PATCH] hw/arm/boot: set initrd parameters to 64bit in fdt
Posted by Peter Maydell 1 year, 5 months ago
On Wed, 16 Nov 2022 at 06:11, Schspa Shi <schspa@gmail.com> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Tue, 8 Nov 2022 at 15:50, Schspa Shi <schspa@gmail.com> wrote:
> >>
> >>
> >> Peter Maydell <peter.maydell@linaro.org> writes:
> >>
> >> > On Tue, 8 Nov 2022 at 13:54, Peter Maydell <peter.maydell@linaro.org> wrote:
> >> >>
> >> >> On Tue, 8 Nov 2022 at 12:52, Schspa Shi <schspa@gmail.com> wrote:
> >> >> > I think this lowmem does not mean below 4GB. and it is to make sure
> >> >> > the initrd_start > memblock_start_of_DRAM for Linux address range check.
> >> >>
> >> >> The wording of this comment pre-dates 64-bit CPU support: it
> >> >> is talking about the requirement in the 32-bit booting doc
> >> >> https://www.kernel.org/doc/Documentation/arm/Booting
> >> >> that says
> >> >> "If an initramfs is in use then, as with the dtb, it must be placed in
> >> >> a region of memory where the kernel decompressor will not overwrite it
> >> >> while also with the region which will be covered by the kernel's
> >> >> low-memory mapping."
> >> >>
> >> >> So it does mean "below 4GB", because you can't boot a 32-bit kernel
> >> >> if you don't put the kernel, initrd, etc below 4GB.
> >> >
> >> > A kernel person corrects me on the meaning of "lowmem" here -- the
> >> > kernel means by it "within the first 768MB of RAM". There is also
> >> > an implicit requirement that everything be within the bottom 32-bits
> >> > of the physical address space.
> >> >
> >>
> >> Thanks for your comment.
> >>
> >> In this view, initrd shouldn't be placed higher than 4GB ? But it
> >> seems the Linux kernel can boot when there is no memory below 4GB.
> >
> > A *32 bit* kernel cannot -- it is completely unable to access
> > anything above the 4GB mark when the MMU is off, as it is on
> > initial boot. This QEMU code handles both 32 bit and 64 bit
> > kernel boot. These days of course there is 64-bit only hardware,
> > and that might choose to put its RAM above the 4GB mark,
> > because it isn't ever going to boot a 32-bit kernel anyway.
> >
>
> Yes, I think we should accept this patch, because it will not affect
> 32-bit devices, and provides support for 64-bit devices to put initrd
> above 4GB.

Yes, I agree. However since it doesn't cause a problem for any
of the machine models in upstream QEMU, I think we should leave
it until after the in-progress 7.2 release, so that we have
plenty of time to investigate just in case it does cause an
unexpected issue on 32-bit boards.

This patch is on my list to review and deal with when 7.2
goes out and development reopens for 8.0 (should be in about
four weeks).

thanks
-- PMM
Re: [PATCH] hw/arm/boot: set initrd parameters to 64bit in fdt
Posted by Peter Maydell 1 year, 5 months ago
On Wed, 16 Nov 2022 at 13:14, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 16 Nov 2022 at 06:11, Schspa Shi <schspa@gmail.com> wrote:
> > Yes, I think we should accept this patch, because it will not affect
> > 32-bit devices, and provides support for 64-bit devices to put initrd
> > above 4GB.
>
> Yes, I agree. However since it doesn't cause a problem for any
> of the machine models in upstream QEMU, I think we should leave
> it until after the in-progress 7.2 release, so that we have
> plenty of time to investigate just in case it does cause an
> unexpected issue on 32-bit boards.
>
> This patch is on my list to review and deal with when 7.2
> goes out and development reopens for 8.0 (should be in about
> four weeks).

I've applied this patch to a target-arm-for-8.0 branch, which
will eventually turn into target-arm when 7.2 is released in a
few weeks' time.

thanks
-- PMM
Re: [PATCH] hw/arm/boot: set initrd parameters to 64bit in fdt
Posted by Schspa Shi 1 year, 5 months ago
Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 16 Nov 2022 at 06:11, Schspa Shi <schspa@gmail.com> wrote:
>>
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> > On Tue, 8 Nov 2022 at 15:50, Schspa Shi <schspa@gmail.com> wrote:
>> >>
>> >>
>> >> Peter Maydell <peter.maydell@linaro.org> writes:
>> >>
>> >> > On Tue, 8 Nov 2022 at 13:54, Peter Maydell <peter.maydell@linaro.org> wrote:
>> >> >>
>> >> >> On Tue, 8 Nov 2022 at 12:52, Schspa Shi <schspa@gmail.com> wrote:
>> >> >> > I think this lowmem does not mean below 4GB. and it is to make sure
>> >> >> > the initrd_start > memblock_start_of_DRAM for Linux address range check.
>> >> >>
>> >> >> The wording of this comment pre-dates 64-bit CPU support: it
>> >> >> is talking about the requirement in the 32-bit booting doc
>> >> >> https://www.kernel.org/doc/Documentation/arm/Booting
>> >> >> that says
>> >> >> "If an initramfs is in use then, as with the dtb, it must be placed in
>> >> >> a region of memory where the kernel decompressor will not overwrite it
>> >> >> while also with the region which will be covered by the kernel's
>> >> >> low-memory mapping."
>> >> >>
>> >> >> So it does mean "below 4GB", because you can't boot a 32-bit kernel
>> >> >> if you don't put the kernel, initrd, etc below 4GB.
>> >> >
>> >> > A kernel person corrects me on the meaning of "lowmem" here -- the
>> >> > kernel means by it "within the first 768MB of RAM". There is also
>> >> > an implicit requirement that everything be within the bottom 32-bits
>> >> > of the physical address space.
>> >> >
>> >>
>> >> Thanks for your comment.
>> >>
>> >> In this view, initrd shouldn't be placed higher than 4GB ? But it
>> >> seems the Linux kernel can boot when there is no memory below 4GB.
>> >
>> > A *32 bit* kernel cannot -- it is completely unable to access
>> > anything above the 4GB mark when the MMU is off, as it is on
>> > initial boot. This QEMU code handles both 32 bit and 64 bit
>> > kernel boot. These days of course there is 64-bit only hardware,
>> > and that might choose to put its RAM above the 4GB mark,
>> > because it isn't ever going to boot a 32-bit kernel anyway.
>> >
>>
>> Yes, I think we should accept this patch, because it will not affect
>> 32-bit devices, and provides support for 64-bit devices to put initrd
>> above 4GB.
>
> Yes, I agree. However since it doesn't cause a problem for any
> of the machine models in upstream QEMU, I think we should leave
> it until after the in-progress 7.2 release, so that we have
> plenty of time to investigate just in case it does cause an
> unexpected issue on 32-bit boards.
>
> This patch is on my list to review and deal with when 7.2
> goes out and development reopens for 8.0 (should be in about
> four weeks).
>
> thanks
> -- PMM

OK, thank you very much.

-- 
BRs
Schspa Shi