On Mon, Jan 23, 2023 at 8:19 PM Daniel Henrique Barboza <[email protected]> wrote: > > > > On 1/22/23 19:53, Alistair Francis wrote: > > On Sun, Jan 22, 2023 at 5:16 AM Daniel Henrique Barboza > > <[email protected]> wrote: > >> > >> Conor, > >> > >> Thanks for the Icicle-kit walk-through! I'll not claim that I fully > >> understood it, > >> but I understood enough to handle the situation ATM. > >> > >> Without this change, this is where the FDT is being installed in the board > >> when > >> I start it with 8Gb of RAM (retrieved via 'info roms'): > >> > >> addr=00000000bfe00000 size=0x00a720 mem=ram name="fdt" > >> > >> Which surprised me at first because this is almost at the end of the LO > >> area which has > >> 1Gb and I figured it would be in the middle of another RAM area. I took > >> another read > >> at what we're doing in riscv_load_fdt(): > >> > >> ----------- > >> temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end; > >> fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB); > >> ----------- > >> > >> This code can be read as "if the starting address of the RAM is lower than > >> 3Gb, put > >> the FDT no further than 3Gb (0xc0000000). Otherwise, put it at the end of > >> dram", > >> where "dram_base" is the starting address of the RAM block that the > >> function > >> receives. > >> > >> For icicle-kit, this is being passed as > >> memmap[MICROCHIP_PFSOC_DRAM_LO].base, > >> 0x80000000, which is 2Gb. > >> > >> So, regardless of how much RAM we have (dram_end), the FDT will always be > >> capped at > >> 3Gb. At this moment, this fits exactly at the end of the LO area for the > >> Icicle Kit. > >> Which is funny because this 3Gb restriction was added by commit > >> 1a475d39ef54 to fix > >> 32 bit guest boot and it happened to also work for the Microchip SoC. > >> > >> So yeah, I thought that I was fixing a bug and in the end I caused one. > >> This patch > >> needs to go. > >> > >> > >> Alistair, I believe I should re-send v2, this time explaining why the > >> existing function > >> will not break the Microchip board because we'll never put the FDT out of > >> the LO area > >> of the board. Does this work for you? > > > > I think that's fine. My only worry is that we are losing some > > flexibility that some future board might want. > > What if we change riscv_load_fdt() parameters to pass a > MemoryRegion/MemMapEntry > instead of just dram_base? > > Instead of this: > > uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt) > > We would have this: > > uint64_t riscv_load_fdt(MemMapEntry mem, uint64_t mem_size, void *fdt) > > Or even this: > > uint64_t riscv_load_fdt(hwaddr dram_base, hwaddr dram_size, > uint64_t mem_size, void *fdt) > > > And then we can make assumptions based on the actual memory region that the > fdt > is going to fit into, instead of having a starting address and a total memory > size and have to deal with issues such as sparse memory. > > We can keep all the assumptions already made today (e.g. the 3Gb maximum addr) > while also having a guarantee that the fdt isn't going to be put in the wrong > memory region/spot if we decide to change the assumptions later on.
That seems like a good direction. We currently don't need this though, so don't feel like it needs to be done today. Alistair > > > Thanks, > > Daniel > > > > > > > Alistair
