Re: [PATCH v4 3/3] hw/riscv: change riscv_compute_fdt_addr() semantics

[Bin Meng]

On Tue, Jan 31, 2023 at 5:57 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 1/30/23 22:00, Bin Meng wrote:
> > On Tue, Jan 31, 2023 at 1:16 AM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >>
> >>
> >> On 1/29/23 02:45, Bin Meng wrote:
> >>> On Thu, Jan 26, 2023 at 9:54 PM Daniel Henrique Barboza
> >>> <dbarboza@ventanamicro.com> wrote:
> >>>>
> >>>> As it is now, riscv_compute_fdt_addr() is receiving a dram_base, a
> >>>> mem_size (which is defaulted to MachineState::ram_size in all boards)
> >>>> and the FDT pointer. And it makes a very important assumption: the DRAM
> >>>> interval dram_base + mem_size is contiguous. This is indeed the case for
> >>>> most boards that uses a FDT.
> >>>
> >>> s/uses/use
> >>>
> >>>>
> >>>> The Icicle Kit board works with 2 distinct RAM banks that are separated
> >>>> by a gap. We have a lower bank with 1GiB size, a gap follows, then at
> >>>> 64GiB the high memory starts. MachineClass::default_ram_size for this
> >>>> board is set to 1.5Gb, and machine_init() is enforcing it as minimal RAM
> >>>> size, meaning that there we'll always have at least 512 MiB in the Hi
> >>>> RAM area.
> >>>>
> >>>> Using riscv_compute_fdt_addr() in this board is weird because not only
> >>>> the board has sparse RAM, and it's calling it using the base address of
> >>>> the Lo RAM area, but it's also using a mem_size that we have guarantees
> >>>> that it will go up to the Hi RAM. All the function assumptions doesn't
> >>>> work for this board.
> >>>>
> >>>> In fact, what makes the function works at all in this case is a
> >>>> coincidence.  Commit 1a475d39ef54 introduced a 3GB boundary for the FDT,
> >>>> down from 4Gb, that is enforced if dram_base is lower than 3072 MiB. For
> >>>> the Icicle Kit board, memmap[MICROCHIP_PFSOC_DRAM_LO].base is 0x80000000
> >>>> (2 Gb) and it has a 1Gb size, so it will fall in the conditions to put
> >>>> the FDT under a 3Gb address, which happens to be exactly at the end of
> >>>> DRAM_LO. If the base address of the Lo area started later than 3Gb this
> >>>> function would be unusable by the board. Changing any assumptions inside
> >>>> riscv_compute_fdt_addr() can also break it by accident as well.
> >>>>
> >>>> Let's change riscv_compute_fdt_addr() semantics to be appropriate to the
> >>>> Icicle Kit board and for future boards that might have sparse RAM
> >>>> topologies to worry about:
> >>>>
> >>>> - relieve the condition that the dram_base + mem_size area is contiguous,
> >>>> since this is already not the case today;
> >>>>
> >>>> - receive an extra 'dram_size' size attribute that refers to a contiguous
> >>>> RAM block that the board wants the FDT to reside on.
> >>>>
> >>>> Together with 'mem_size' and 'fdt', which are now now being consumed by a
> >>>> MachineState pointer, we're able to make clear assumptions based on the
> >>>> DRAM block and total mem_size available to ensure that the FDT will be 
> >>>> put
> >>>> in a valid RAM address.
> >>>>
> >>>
> >>> Well written commit message. Thanks!
> >>>
> >>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >>>> ---
> >>>>    hw/riscv/boot.c            | 38 ++++++++++++++++++++++++++------------
> >>>>    hw/riscv/microchip_pfsoc.c |  3 ++-
> >>>>    hw/riscv/sifive_u.c        |  3 ++-
> >>>>    hw/riscv/spike.c           |  3 ++-
> >>>>    hw/riscv/virt.c            |  3 ++-
> >>>>    include/hw/riscv/boot.h    |  4 ++--
> >>>>    6 files changed, 36 insertions(+), 18 deletions(-)
> >>>>
> >>>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> >>>> index a6f7b8ae8e..8f4991480b 100644
> >>>> --- a/hw/riscv/boot.c
> >>>> +++ b/hw/riscv/boot.c
> >>>> @@ -284,33 +284,47 @@ out:
> >>>>    }
> >>>>
> >>>>    /*
> >>>> - * The FDT should be put at the farthest point possible to
> >>>> - * avoid overwriting it with the kernel/initrd.
> >>>> + * This function makes an assumption that the DRAM interval
> >>>> + * 'dram_base' + 'dram_size' is contiguous.
> >>>>     *
> >>>> - * This function makes an assumption that the DRAM is
> >>>> - * contiguous. It also cares about 32-bit systems and
> >>>> - * will limit fdt_addr to be addressable by them even for
> >>>> - * 64-bit CPUs.
> >>>> + * Considering that 'dram_end' is the lowest value between
> >>>> + * the end of the DRAM block and MachineState->ram_size, the
> >>>> + * FDT location will vary according to 'dram_base':
> >>>> + *
> >>>> + * - if 'dram_base' is less that 3072 MiB, the FDT will be
> >>>> + * put at the lowest value between 3072 MiB and 'dram_end';
> >>>> + *
> >>>> + * - if 'dram_base' is higher than 3072 MiB, the FDT will be
> >>>> + * put at 'dram_end'.
> >>>>     *
> >>>>     * The FDT is fdt_packed() during the calculation.
> >>>>     */
> >>>> -uint32_t riscv_compute_fdt_addr(hwaddr dram_base, uint64_t mem_size,
> >>>> -                                void *fdt)
> >>>> +hwaddr riscv_compute_fdt_addr(hwaddr dram_base, hwaddr dram_size,
> >>>
> >>> Using hwaddr to represent a size looks weird. Although technically
> >>> they are the same ... I would leave this as it is.
> >>
> >> I'll leave it as it was back in patch 2 (uint64_t).
> >>
> >>>
> >>>> +                              MachineState *ms)
> >>>>    {
> >>>> -    uint64_t temp;
> >>>> -    hwaddr dram_end = dram_base + mem_size;
> >>>> -    int ret = fdt_pack(fdt);
> >>>> +    int ret = fdt_pack(ms->fdt);
> >>>> +    hwaddr dram_end, temp;
> >>>>        int fdtsize;
> >>>>
> >>>>        /* Should only fail if we've built a corrupted tree */
> >>>>        g_assert(ret == 0);
> >>>>
> >>>> -    fdtsize = fdt_totalsize(fdt);
> >>>> +    fdtsize = fdt_totalsize(ms->fdt);
> >>>>        if (fdtsize <= 0) {
> >>>>            error_report("invalid device-tree");
> >>>>            exit(1);
> >>>>        }
> >>>>
> >>>> +    /*
> >>>> +     * A dram_size == 0, usually from a MemMapEntry[].size element,
> >>>> +     * means that the DRAM block goes all the way to ms->ram_size.
> >>>> +     */
> >>>> +    if (dram_size == 0x0) {
> >>>> +        dram_end = dram_base + ms->ram_size;
> >>>> +    } else {
> >>>> +        dram_end = dram_base + MIN(ms->ram_size, dram_size);
> >>>> +    }
> >>>
> >>> How about:
> >>>
> >>> g_assert(dram_size < ms->ram_size);
> >>
> >> I don't believe that dram_size > ms->ram_size should be an error. A board 
> >> can
> >> have a declared MemMapEntry.size that is larger than its current setting of
> >> ms->ram_size.
> >
> > What use case is that? This updated function now has the assumption that:
> >
> > 1. dram_size being 0 meaning contiguous system RAM region from dram_base
> > 2. otherwise dram_size being the *first* contiguous system RAM region
> > from dram_base
>
> Yes, but that doesn't mean that dram_size is necessarily smaller than 
> ms->ram_size.
>
> We don't have any board where this happens today but let's pretend that the 
> Icicle
> Kit board didn't have the 1.5Gb minimal RAM restriction. Its first region has
> dram_size 1Gb:
>
> [MICROCHIP_PFSOC_DRAM_LO] =         { 0x80000000,   0x40000000 },
>
> So, if I start the board with 512M, ms->ram_size = 512M and this assert will 
> trigger
> because dram_size = 1Gb.
>

Ah, I see. So for a machine whose memory size can be dynamically
configured, yes that makes sense.

Regards,
Bin