Re: [PULL 04/15] RISC-V: Copy the fdt in dram instead of ROM

[Alistair Francis]

On Tue, Jul 13, 2021 at 8:44 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 14 Jul 2020 at 01:44, Alistair Francis <alistair.francis@wdc.com> 
> wrote:
> >
> > From: Atish Patra <atish.patra@wdc.com>
> >
> > Currently, the fdt is copied to the ROM after the reset vector. The firmware
> > has to copy it to DRAM. Instead of this, directly copy the device tree to a
> > pre-computed dram address. The device tree load address should be as far as
> > possible from kernel and initrd images. That's why it is kept at the end of
> > the DRAM or 4GB whichever is lesser.
>
> Hi; Coverity reports an issue in this code (CID 1458136):
>
> > +uint32_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
> > +{
> > +    uint32_t temp, fdt_addr;
> > +    hwaddr dram_end = dram_base + mem_size;
> > +    int fdtsize = fdt_totalsize(fdt);
> > +
> > +    if (fdtsize <= 0) {
> > +        error_report("invalid device-tree");
> > +        exit(1);
> > +    }
> > +
> > +    /*
> > +     * 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 aligned address that less than fdt size from end 
> > of
> > +     * dram or 4GB whichever is lesser.
> > +     */
> > +    temp = MIN(dram_end, 4096 * MiB);
> > +    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
> > +
> > +    fdt_pack(fdt);
>
> fdt_pack() can return an error code, but we are not checking its
> return value here.
>
> (This is one of Coverity's heuristics where it only reports failure
> to check errors if it sees enough other callsites in the codebase
> which do check errors to make it decide this is an "always need a
> check" API, which is why the error has only popped up now a year on...)

Thanks Peter, sending a patch now.

Alistair

>
> > +    /* copy in the device tree */
> > +    qemu_fdt_dumpdtb(fdt, fdtsize);
> > +
> > +    rom_add_blob_fixed_as("fdt", fdt, fdtsize, fdt_addr,
> > +                          &address_space_memory);
> > +
> > +    return fdt_addr;
> > +}
>
> thanks
> -- PMM
>