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:59, Peter Maydell wrote:
> On Thu, 23 Mar 2023 at 17:54, Daniel Henrique Barboza
> <danielhb413@gmail.com> wrote:
>>
>>
>>
>> On 3/23/23 14:38, Peter Maydell wrote:
>>> On Thu, 23 Mar 2023 at 16:11, Daniel Henrique Barboza
>>> <danielhb413@gmail.com> wrote:
>>>> -    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.
> 
> Mmm. I think that makes sense; we do make a few tweaks to the DTB
> even if it was user-provided and you might want to check those
> for debug purposes. So we should keep this assignment, and remove
> the now-unneeded setting of ms->fdt in create_fdt().


I don't think we can remove it. arm_load_dtb() does the following:

     if (binfo->dtb_filename) {
        (...)
     } else {
         fdt = binfo->get_dtb(binfo, &size);
         if (!fdt) {
             fprintf(stderr, "Board was unable to create a dtb blob\n");
             goto fail;
         }
     }

So if we don't have a '-dtb' option, fdt = binfo->get_dtb(). For the 'virt' machine,
machvirt_dtb(), will return ms->fdt. So we would SIGSEG right at the start.

And now that I think more about it, this patch is leaking the board FDT if we're
using the FDT from dtb_filename, isn't it? We're assigning a new ms->fdt on top
of the existing ms->fdt from the board. I'll send a new version.

Also, given that we're not using the board FDT at all if '-dtb' is present, I
think it would be good to move create_fdt() from machvirt_init() to machvirt_dtb().
Some code juggling would be required (some functions from init() are using ms->fdt)
but it think it would make the code clearer - create the fdt board only if we're
really going to use it. I'll see if I can pull this off and send a 8.1 patch
with it.


Thanks,


Daniel




> 
> thanks
> -- PMM