hw/riscv: boot: Reduce FDT address alignment constraints

[PATCH] hw/riscv: boot: Reduce FDT address alignment constraints

Posted by Alistair Francis 3 years, 8 months ago

From: Alistair Francis <alistair.francis@wdc.com>

We previously stored the device tree at a 16MB alignment from the end of
memory (or 3GB). This means we need at least 16MB of memory to be able
to do this. We don't actually need the FDT to be 16MB aligned, so let's
drop it down to 2MB so that we can support systems with less memory,
while also allowing FDT size expansion.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/992
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/boot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 57a41df8e9..e476d8f491 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -226,11 +226,11 @@ uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
     /*
      * We should put fdt as far as possible to avoid kernel/initrd overwriting
      * its content. But it should be addressable by 32 bit system as well.
-     * Thus, put it at an 16MB aligned address that less than fdt size from the
+     * Thus, put it at an 2MB aligned address that less than fdt size from the
      * end of dram or 3GB whichever is lesser.
      */
     temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
-    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 16 * MiB);
+    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
 
     ret = fdt_pack(fdt);
     /* Should only fail if we've built a corrupted tree */
-- 
2.36.1

Re: [PATCH] hw/riscv: boot: Reduce FDT address alignment constraints

Posted by Alistair Francis 3 years, 7 months ago

On Wed, Jun 8, 2022 at 4:20 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> We previously stored the device tree at a 16MB alignment from the end of
> memory (or 3GB). This means we need at least 16MB of memory to be able
> to do this. We don't actually need the FDT to be 16MB aligned, so let's
> drop it down to 2MB so that we can support systems with less memory,
> while also allowing FDT size expansion.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/992
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  hw/riscv/boot.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 57a41df8e9..e476d8f491 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -226,11 +226,11 @@ uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
>      /*
>       * We should put fdt as far as possible to avoid kernel/initrd overwriting
>       * its content. But it should be addressable by 32 bit system as well.
> -     * Thus, put it at an 16MB aligned address that less than fdt size from the
> +     * Thus, put it at an 2MB aligned address that less than fdt size from the
>       * end of dram or 3GB whichever is lesser.
>       */
>      temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
> -    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 16 * MiB);
> +    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
>
>      ret = fdt_pack(fdt);
>      /* Should only fail if we've built a corrupted tree */
> --
> 2.36.1
>

Re: [PATCH] hw/riscv: boot: Reduce FDT address alignment constraints

Posted by Bin Meng 3 years, 7 months ago

On Wed, Jun 8, 2022 at 2:20 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> We previously stored the device tree at a 16MB alignment from the end of
> memory (or 3GB). This means we need at least 16MB of memory to be able
> to do this. We don't actually need the FDT to be 16MB aligned, so let's
> drop it down to 2MB so that we can support systems with less memory,
> while also allowing FDT size expansion.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/992
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/boot.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-by: Bin Meng <bin.meng@windriver.com>

Tested booting Linux 64-bit/32-bit v5.18 kernel, VxWorks 7
64-bit/32-bit 22.06 release kernel
Tested-by: Bin Meng <bin.meng@windriver.com>

Re: [PATCH] hw/riscv: boot: Reduce FDT address alignment constraints

Posted by Atish Patra 3 years, 7 months ago

On Tue, Jun 7, 2022 at 11:21 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> We previously stored the device tree at a 16MB alignment from the end of
> memory (or 3GB). This means we need at least 16MB of memory to be able
> to do this. We don't actually need the FDT to be 16MB aligned, so let's
> drop it down to 2MB so that we can support systems with less memory,
> while also allowing FDT size expansion.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/992
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/boot.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 57a41df8e9..e476d8f491 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -226,11 +226,11 @@ uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
>      /*
>       * We should put fdt as far as possible to avoid kernel/initrd overwriting
>       * its content. But it should be addressable by 32 bit system as well.
> -     * Thus, put it at an 16MB aligned address that less than fdt size from the
> +     * Thus, put it at an 2MB aligned address that less than fdt size from the
>       * end of dram or 3GB whichever is lesser.
>       */
>      temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
> -    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 16 * MiB);
> +    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
>
>      ret = fdt_pack(fdt);
>      /* Should only fail if we've built a corrupted tree */
> --
> 2.36.1
>
>

Reviewed-by: Atish Patra <atishp@rivosinc.com>

-- 
Regards,
Atish

Re: [PATCH] hw/riscv: boot: Reduce FDT address alignment constraints

Posted by Bin Meng 3 years, 8 months ago

+Atish

On Wed, Jun 8, 2022 at 2:20 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> We previously stored the device tree at a 16MB alignment from the end of
> memory (or 3GB). This means we need at least 16MB of memory to be able
> to do this. We don't actually need the FDT to be 16MB aligned, so let's
> drop it down to 2MB so that we can support systems with less memory,
> while also allowing FDT size expansion.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/992
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/boot.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 57a41df8e9..e476d8f491 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -226,11 +226,11 @@ uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
>      /*
>       * We should put fdt as far as possible to avoid kernel/initrd overwriting
>       * its content. But it should be addressable by 32 bit system as well.
> -     * Thus, put it at an 16MB aligned address that less than fdt size from the
> +     * Thus, put it at an 2MB aligned address that less than fdt size from the
>       * end of dram or 3GB whichever is lesser.
>       */
>      temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
> -    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 16 * MiB);
> +    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
>

@Atish Patra  may have some pointers about the 16MiB alignment requirement.

Regards,
Bin

Re: [PATCH] hw/riscv: boot: Reduce FDT address alignment constraints

Posted by Alistair Francis 3 years, 7 months ago

On Wed, Jun 8, 2022 at 4:41 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> +Atish
>
> On Wed, Jun 8, 2022 at 2:20 PM Alistair Francis
> <alistair.francis@opensource.wdc.com> wrote:
> >
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > We previously stored the device tree at a 16MB alignment from the end of
> > memory (or 3GB). This means we need at least 16MB of memory to be able
> > to do this. We don't actually need the FDT to be 16MB aligned, so let's
> > drop it down to 2MB so that we can support systems with less memory,
> > while also allowing FDT size expansion.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/992
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  hw/riscv/boot.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index 57a41df8e9..e476d8f491 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -226,11 +226,11 @@ uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
> >      /*
> >       * We should put fdt as far as possible to avoid kernel/initrd overwriting
> >       * its content. But it should be addressable by 32 bit system as well.
> > -     * Thus, put it at an 16MB aligned address that less than fdt size from the
> > +     * Thus, put it at an 2MB aligned address that less than fdt size from the
> >       * end of dram or 3GB whichever is lesser.
> >       */
> >      temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
> > -    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 16 * MiB);
> > +    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
> >
>
> @Atish Patra  may have some pointers about the 16MiB alignment requirement.

Any thoughts?

Alistair

>
> Regards,
> Bin

Re: [PATCH] hw/riscv: boot: Reduce FDT address alignment constraints

Posted by Atish Kumar Patra 3 years, 7 months ago

On Wed, Jun 22, 2022 at 9:15 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Jun 8, 2022 at 4:41 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > +Atish
> >
> > On Wed, Jun 8, 2022 at 2:20 PM Alistair Francis
> > <alistair.francis@opensource.wdc.com> wrote:
> > >
> > > From: Alistair Francis <alistair.francis@wdc.com>
> > >
> > > We previously stored the device tree at a 16MB alignment from the end of
> > > memory (or 3GB). This means we need at least 16MB of memory to be able
> > > to do this. We don't actually need the FDT to be 16MB aligned, so let's
> > > drop it down to 2MB so that we can support systems with less memory,
> > > while also allowing FDT size expansion.
> > >
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/992
> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > ---
> > >  hw/riscv/boot.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > > index 57a41df8e9..e476d8f491 100644
> > > --- a/hw/riscv/boot.c
> > > +++ b/hw/riscv/boot.c
> > > @@ -226,11 +226,11 @@ uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
> > >      /*
> > >       * We should put fdt as far as possible to avoid kernel/initrd overwriting
> > >       * its content. But it should be addressable by 32 bit system as well.
> > > -     * Thus, put it at an 16MB aligned address that less than fdt size from the
> > > +     * Thus, put it at an 2MB aligned address that less than fdt size from the
> > >       * end of dram or 3GB whichever is lesser.
> > >       */
> > >      temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
> > > -    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 16 * MiB);
> > > +    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
> > >
> >
> > @Atish Patra  may have some pointers about the 16MiB alignment requirement.
>

Sorry. I missed this patch for some reason. 16MiB alignment was just
chosen as a safe option.
We couldn't put it at the end of DRAM and I just wanted to place it at
 a reasonable distance.

2MB should be totally okay.

> Any thoughts?
>
> Alistair
>
> >
> > Regards,
> > Bin

Re: [PATCH] hw/riscv: boot: Reduce FDT address alignment constraints

Posted by Alistair Francis 3 years, 7 months ago

On Thu, Jun 23, 2022 at 3:45 PM Atish Kumar Patra <atishp@rivosinc.com> wrote:
>
> On Wed, Jun 22, 2022 at 9:15 PM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Jun 8, 2022 at 4:41 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > +Atish
> > >
> > > On Wed, Jun 8, 2022 at 2:20 PM Alistair Francis
> > > <alistair.francis@opensource.wdc.com> wrote:
> > > >
> > > > From: Alistair Francis <alistair.francis@wdc.com>
> > > >
> > > > We previously stored the device tree at a 16MB alignment from the end of
> > > > memory (or 3GB). This means we need at least 16MB of memory to be able
> > > > to do this. We don't actually need the FDT to be 16MB aligned, so let's
> > > > drop it down to 2MB so that we can support systems with less memory,
> > > > while also allowing FDT size expansion.
> > > >
> > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/992
> > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > ---
> > > >  hw/riscv/boot.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > > > index 57a41df8e9..e476d8f491 100644
> > > > --- a/hw/riscv/boot.c
> > > > +++ b/hw/riscv/boot.c
> > > > @@ -226,11 +226,11 @@ uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
> > > >      /*
> > > >       * We should put fdt as far as possible to avoid kernel/initrd overwriting
> > > >       * its content. But it should be addressable by 32 bit system as well.
> > > > -     * Thus, put it at an 16MB aligned address that less than fdt size from the
> > > > +     * Thus, put it at an 2MB aligned address that less than fdt size from the
> > > >       * end of dram or 3GB whichever is lesser.
> > > >       */
> > > >      temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
> > > > -    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 16 * MiB);
> > > > +    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
> > > >
> > >
> > > @Atish Patra  may have some pointers about the 16MiB alignment requirement.
> >
>
> Sorry. I missed this patch for some reason. 16MiB alignment was just
> chosen as a safe option.
> We couldn't put it at the end of DRAM and I just wanted to place it at
>  a reasonable distance.
>
> 2MB should be totally okay.

Can I get a review or Ack :)

Alistair

>
> > Any thoughts?
> >
> > Alistair
> >
> > >
> > > Regards,
> > > Bin