Peter Maydell <[email protected]> writes: > On Mon, 1 Sept 2025 at 13:53, Alex Bennée <[email protected]> wrote: >> >> With the fdt being protected by g_autofree we can skip the goto fail >> and bail out straight away. The only thing we must take care of is >> stealing the pointer in the one case when we do need it to survive. >> >> Signed-off-by: Alex Bennée <[email protected]> >> --- >> hw/arm/boot.c | 29 ++++++++++++----------------- >> 1 file changed, 12 insertions(+), 17 deletions(-) >> >> diff --git a/hw/arm/boot.c b/hw/arm/boot.c >> index 56fd13b9f7c..749f2d08341 100644 >> --- a/hw/arm/boot.c >> +++ b/hw/arm/boot.c >> @@ -519,7 +519,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info >> *binfo, >> hwaddr addr_limit, AddressSpace *as, MachineState *ms, >> ARMCPU *cpu) >> { >> - void *fdt = NULL; >> + g_autofree void *fdt = NULL; >> int size, rc, n = 0; >> uint32_t acells, scells; >> unsigned int i; > > >> @@ -673,14 +672,10 @@ int arm_load_dtb(hwaddr addr, const struct >> arm_boot_info *binfo, >> >> if (fdt != ms->fdt) { >> g_free(ms->fdt); >> - ms->fdt = fdt; >> + ms->fdt = g_steal_pointer(&fdt); >> } >> >> return size; >> -> -fail: >> - g_free(fdt); >> - return -1; >> } > > Previously, if we get to the end of the function and fdt == ms->fdt > then we continue to use that DTB, and we don't free it. > After this change, if fdt == ms->fdt then we will skip the > g_steal_pointer() and the g_autofree will free the memory, > but leave ms->fdt still pointing to it. > > Since arm_load_dtb() is only called once it's a bit unclear > to me whether this can happen -- I think you would need to have > a board-specific arm_boot_info::get_dtb function which returned > the MachineState::fdt pointer. But as this is supposed to > just be a refactoring patch and the previous code clearly was > written to account for the possibility of fdt == ms->fdt, > I think we should continue to handle that case.
Hmm I was thinking we could assert if ms->fdt is set because we clearly shouldn't be loading one. For arm the only thing that sets ms->fdt is create_fdt which also implies: vms->bootinfo.skip_dtb_autoload = true; so on start-up we should either create or load - not both. but then I am confused about why we do another arm_load_dtb in the machine_done notifier. Either way I can't see how fdt = g_malloc0(dt_size) could ever match what might already be in ms->fdt. > > thanks > -- PMM -- Alex Bennée Virtualisation Tech Lead @ Linaro
