On Wed, 3 Sept 2025 at 09:16, Alex Bennée <[email protected]> wrote: > > 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.
Yeah, I don't think it's really going to happen (at least at the moment: if somebody refactored hw/arm/sbsa-ref.c so that it used MachineState::fdt rather than having a sort-of-duplicate SBSAMachinestate::fdt then I think you can end up here). But mostly my point is that I don't want to have to think about this and audit all the arm boards with a get_dtb method when I'm reviewing a patch which is supposed to just be switching this function from explicit-free to g_autofree... -- PMM
