Re: [PATCH 2/4] hw/arm: use g_autofree for fdt in arm_load_dtb

["Alex Bennée" <alex.bennee@linaro.org>]

Alex Bennée <alex.bennee@linaro.org> writes:

> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On Mon, 1 Sept 2025 at 13:53, Alex Bennée <alex.bennee@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.bennee@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