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

[Daniel Henrique Barboza]

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