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;
@@ -538,13 +538,13 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
fdt = load_device_tree(filename, &size);
if (!fdt) {
fprintf(stderr, "Couldn't open dtb file %s\n", filename);
- goto fail;
+ return -1;
}
} else {
fdt = binfo->get_dtb(binfo, &size);
if (!fdt) {
fprintf(stderr, "Board was unable to create a dtb blob\n");
- goto fail;
+ return -1;
}
}
@@ -553,7 +553,6 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
* Whether this constitutes failure is up to the caller to decide,
* so just return 0 as size, i.e., no error.
*/
- g_free(fdt);
return 0;
}
@@ -563,7 +562,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
NULL, &error_fatal);
if (acells == 0 || scells == 0) {
fprintf(stderr, "dtb file invalid (#address-cells or #size-cells 0)\n");
- goto fail;
+ return -1;
}
if (scells < 2 && binfo->ram_size >= 4 * GiB) {
@@ -572,14 +571,14 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
*/
fprintf(stderr, "qemu: dtb file not compatible with "
"RAM size > 4GB\n");
- goto fail;
+ return -1;
}
/* nop all root nodes matching /memory or /memory@unit-address */
node_path = qemu_fdt_node_unit_path(fdt, "memory", &err);
if (err) {
error_report_err(err);
- goto fail;
+ return -1;
}
while (node_path[n]) {
if (g_str_has_prefix(node_path[n], "/memory")) {
@@ -611,7 +610,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
if (rc < 0) {
fprintf(stderr, "couldn't add /memory@%"PRIx64" node\n",
mem_base);
- goto fail;
+ return -1;
}
mem_base += mem_len;
@@ -622,7 +621,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
if (rc < 0) {
fprintf(stderr, "couldn't add /memory@%"PRIx64" node\n",
binfo->loader_start);
- goto fail;
+ return -1;
}
}
@@ -636,7 +635,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
ms->kernel_cmdline);
if (rc < 0) {
fprintf(stderr, "couldn't set /chosen/bootargs\n");
- goto fail;
+ return -1;
}
}
@@ -645,7 +644,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
acells, binfo->initrd_start);
if (rc < 0) {
fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n");
- goto fail;
+ return -1;
}
rc = qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-end",
@@ -654,7 +653,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
binfo->initrd_size);
if (rc < 0) {
fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");
- goto fail;
+ return -1;
}
}
@@ -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;
}
static void do_cpu_reset(void *opaque)
--
2.47.2
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.
thanks
-- PMM
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.
>
> thanks
> -- PMM
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
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
On Wed, 3 Sept 2025 at 09:16, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> 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.
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
On Mon, Sep 1, 2025 at 3:53 PM 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>
> ---
Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@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;
> @@ -538,13 +538,13 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> fdt = load_device_tree(filename, &size);
> if (!fdt) {
> fprintf(stderr, "Couldn't open dtb file %s\n", filename);
> - goto fail;
> + return -1;
> }
> } else {
> fdt = binfo->get_dtb(binfo, &size);
> if (!fdt) {
> fprintf(stderr, "Board was unable to create a dtb blob\n");
> - goto fail;
> + return -1;
> }
> }
>
> @@ -553,7 +553,6 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> * Whether this constitutes failure is up to the caller to decide,
> * so just return 0 as size, i.e., no error.
> */
> - g_free(fdt);
> return 0;
> }
>
> @@ -563,7 +562,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> NULL, &error_fatal);
> if (acells == 0 || scells == 0) {
> fprintf(stderr, "dtb file invalid (#address-cells or #size-cells 0)\n");
> - goto fail;
> + return -1;
> }
>
> if (scells < 2 && binfo->ram_size >= 4 * GiB) {
> @@ -572,14 +571,14 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> */
> fprintf(stderr, "qemu: dtb file not compatible with "
> "RAM size > 4GB\n");
> - goto fail;
> + return -1;
> }
>
> /* nop all root nodes matching /memory or /memory@unit-address */
> node_path = qemu_fdt_node_unit_path(fdt, "memory", &err);
> if (err) {
> error_report_err(err);
> - goto fail;
> + return -1;
> }
> while (node_path[n]) {
> if (g_str_has_prefix(node_path[n], "/memory")) {
> @@ -611,7 +610,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> if (rc < 0) {
> fprintf(stderr, "couldn't add /memory@%"PRIx64" node\n",
> mem_base);
> - goto fail;
> + return -1;
> }
>
> mem_base += mem_len;
> @@ -622,7 +621,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> if (rc < 0) {
> fprintf(stderr, "couldn't add /memory@%"PRIx64" node\n",
> binfo->loader_start);
> - goto fail;
> + return -1;
> }
> }
>
> @@ -636,7 +635,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> ms->kernel_cmdline);
> if (rc < 0) {
> fprintf(stderr, "couldn't set /chosen/bootargs\n");
> - goto fail;
> + return -1;
> }
> }
>
> @@ -645,7 +644,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> acells, binfo->initrd_start);
> if (rc < 0) {
> fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n");
> - goto fail;
> + return -1;
> }
>
> rc = qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-end",
> @@ -654,7 +653,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> binfo->initrd_size);
> if (rc < 0) {
> fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");
> - goto fail;
> + return -1;
> }
> }
>
> @@ -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;
> }
>
> static void do_cpu_reset(void *opaque)
> --
> 2.47.2
>
>
© 2016 - 2025 Red Hat, Inc.