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

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 3/23/23 14:38, Peter Maydell wrote:
> 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.

My original intent with this command was to dump the current state of the FDT,
regardless of whether the FDT was loaded via -dtb or at runtime.

Ideally it would also reflect hotplug changes that affects the FDT as well, although
I'm aware of only one board that does that (ppc64 pseries) and we would need some
work done that to update ms->fdt after the hotplug/hotunplug path.

Perhaps a doc path would also be good.


Daniel

> 
> thanks
> -- PMM