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

[Alistair Francis <alistair23@gmail.com>]

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