Alex Bennée <alex.ben...@linaro.org> writes:
> Peter Maydell <peter.mayd...@linaro.org> writes:
>
>> On Mon, 1 Sept 2025 at 13:53, Alex Bennée <alex.ben...@linaro.org> 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 <alex.ben...@linaro.org>
>>> ---
>>> 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.
Ahh I see it now.
>
>
>>
>> thanks
>> -- PMM
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
