Re: [PATCH v3 3/7] hw/riscv/microchip_pfsoc.c: add an Icicle Kit fdt address function

[Daniel Henrique Barboza]

On 1/22/23 19:53, Alistair Francis wrote:
> On Sun, Jan 22, 2023 at 5:16 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> 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.


Thanks,

Daniel



> Alistair