On Mon, 15 Jun 2026 at 15:15, Alistair Francis <[email protected]> wrote:
>
> On Wed, Jun 3, 2026 at 5:04 PM Joel Stanley <[email protected]> wrote:
> > +static void load_fdt(TTAtlantisState *s)
> > +{
> > + MachineState *ms = MACHINE(s);
> > + char **node_path;
> > + Error *err = NULL;
> > +
> > + ms->fdt = load_device_tree(ms->dtb, &s->fdt_size);
> > + if (!ms->fdt) {
> > + error_report("load_device_tree() failed");
> > + exit(1);
> > + }
> > +
> > + qemu_fdt_add_path(ms->fdt, "/chosen");
> > +
> > + /* Clear memory nodes and update with the specified RAM size */
> > + node_path = qemu_fdt_node_unit_path(ms->fdt, "memory", &err);
> > + if (err) {
> > + warn_report_err(err);
> > + } else {
> > + for (int i = 0; node_path[i]; i++) {
> > + qemu_fdt_nop_node(ms->fdt, node_path[i]);
>
> Should we print a warning here to the user that we are doing this? I
> would be confused (and annoyed) if QEMU deleted my memory regions and
> I didn't know
The code is the same for the arm machines; they silently update the
device tree when provided one. I agree with you though, I will add a
warning.
I added this as in testing the device tree was larger than what the
machine had, which Linux happily tries to use and crashes during boot.
I can see the argument for not touching the device tree at all but on
balance I decided helping the user out is the right thing to do. If
users complain I'm happy to change it.
> > +static void tt_atlantis_machine_done(Notifier *notifier, void *data)
> > +{
> > + TTAtlantisState *s = container_of(notifier, TTAtlantisState,
> > machine_done);
> > + MachineState *machine = MACHINE(s);
> > + hwaddr start_addr = s->memmap[TT_ATL_DDR_LO].base;
> > + hwaddr mem_size;
> > + target_ulong firmware_end_addr, kernel_start_addr;
> > + const char *firmware_name = riscv_default_firmware_name(&s->soc);
> > + uint64_t fdt_load_addr;
> > + uint64_t kernel_entry;
> > + RISCVBootInfo boot_info;
> > +
> > + /*
> > + * A user provided dtb must include everything, including
> > + * dynamic sysbus devices. Our FDT needs to be finalized.
> > + */
> > + if (machine->dtb == NULL) {
> > + finalize_fdt(s);
>
> Was expecting a finalise here :)
Desensitized.
> > + /* DDR */
> > +
> > + /* The high address covers all of RAM, the low address just the first
> > 2GB */
> > + lo_ram_size = s->memmap[TT_ATL_DDR_LO].size;
> > + hi_ram_size = s->memmap[TT_ATL_DDR_HI].size;
> > + if (machine->ram_size > hi_ram_size) {
>
> What if machine->ram_size is less then s->memmap[TT_ATL_DDR_HI].size?
> You will still allocate s->memmap[TT_ATL_DDR_HI].size instead of what
> the user asked for. You then report what the user asked for in the DT,
> so there is a bunch of wasted memory.
>
> Memory is expensive these days, we can't waste it
>
> Also, if the user asks for 2GB or less, you shouldn't create ram.high at all
Good catch. There's an even better bug; ram.high and ram.low are
aliases into the machine ram region and they are always sized to their
default maximum, regardless of the machine ram size.
address-space: memory
0000000000000000-ffffffffffffffff (prio 0, container): system
0000000000000000-000000007fffffff (prio 0, ram): alias ram.low
@tt_atlantis.ram 0000000000000000-000000007fffffff
0000000100000000-00000010ffffffff (prio 0, ram): alias ram.high
@tt_atlantis.ram 0000000000000000-0000000fffffffff
memory-region: tt_atlantis.ram
0000000000000000-00000000ffffffff (prio 0, ram): tt_atlantis.ram
They should be sized to the configured size.
The device tree is okay because it sets the region based on the
configured ram size, and Linux politely doesn't write outside of that.
In hardware the high region is where the ram lives and low is an alias
of the first 2GB. I'll fix up the modelling to match, and set the
sizes appropriately.
> Otherwise looks good
Thanks for the review.
Cheers,
Joel
