Re: [PATCH 1/1] hw/arm: do not free machine->fdt in arm_load_dtb()

[Peter Maydell]

On Thu, 23 Mar 2023 at 16:11, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
> At this moment, arm_load_dtb() can free machine->fdt when
> binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be
> retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is
> the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to
> machine->fdt. And, in that case, the existing g_free(fdt) at the end of
> arm_load_dtb() will make machine->fdt point to an invalid memory region.
>
> After the command 'dumpdtb' were introduced a couple of releases ago,
> running it with any ARM machine that uses arm_load_dtb() will crash
> QEMU.
>
> One alternative would be to mark machine->fdt = NULL when exiting
> arm_load_dtb() when freeing the fdt. Another is to not free the fdt and,
> instead, update machine->fdt with the new fdt generated. This will
> enable dumpdtb for all ARM machines that uses arm_load_dtb(), regardless
> of having 'dtb_filename' or not.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Fixes: bf353ad55590f ("qmp/hmp, device_tree.c: introduce dumpdtb")
> Reported-by: Markus Armbruster <armbru@redhat.com>i
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hw/arm/boot.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 50e5141116..9418cc3373 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -689,7 +689,8 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info 
> *binfo,
>      qemu_register_reset_nosnapshotload(qemu_fdt_randomize_seeds,
>                                         rom_ptr_for_as(as, addr, size));
>
> -    g_free(fdt);
> +    /* Set ms->fdt for 'dumpdtb' QMP/HMP command */
> +    ms->fdt = fdt;

With this we're now setting ms->fdt twice for the virt board: we set
it in create_fdt() in hw/arm/virt.c, and then we set it again here.
Which is the right place to set it?

Is the QMP 'dumpdtb' command intended to dump the DTB only for
board types where the DTB is created at runtime by QEMU? Or
is it supposed to also work for DTBs that were originally
provided by the user using the '-dtb' command line? The docs
don't say. If we want the former, then we should be setting
ms->fdt in the board code; if the latter, then here is right.

thanks
-- PMM