Re: QMP command dumpdtb crash bug

[Daniel Henrique Barboza]

On 3/23/23 10:38, Peter Maydell wrote:
> On Thu, 23 Mar 2023 at 13:29, Markus Armbruster <armbru@redhat.com> wrote:
> > Peter, Daniel offers two ways to fix this bug (see below).  Got a
> > preference?
>
> Not freeing seems the correct thing. As Daniel says, this
> should have been a prerequisite for implementing the
> command in the first place (you need to change the lifecycle
> of the fdt blob from "delete when done with in the arm boot code"
> to "delete on machine finalize"). It looks like somehow we added
> the command but missed out on getting all of the prerequisite
> patches in. (File under "need to be cautious about applying partial
> patchsets", I guess.)

Yeah, I'm at fault here. I should've been more insistent about acking
the ARM patch. All other patches that we left behind was optional, meaning
that the machine wouldn't implement the command but nothing bad would happen,
but the ARM patch was kind of mandatory because arm_load_dtb() is
freeing ms->fdt without assigning it to NULL.

> Did anything else from that initial patchset get omitted?

Searching the ML I see that I sent a message saying that I pushed patches 1,
6 and 8-15 via ppc-next. This means that these patches got left behind:

 2  hw/core: free ms->fdt in machine_finalize()
 3  hw/arm: do not free machine->fdt in arm_load_dtb()
 4  hw/mips: set machine->fdt in boston_mach_init()
 5  hw/microblaze: set machine->fdt in microblaze_load_dtb()
 7  hw/ppc: set machine->fdt in ppce500_load_device_tree()
15  hw/xtensa: set machine->fdt in xtfpga_init()


Patch 2 was suggested by Phil and changes the common code to free ms->fdt
during machine_finalize(). Can be re-sent I guess.

All other patches, aside from patch 3 from ARM, are optional because the
machine isn't freeing ms->fdt or anything like that.


I'll rebase and re-sent patch 3 as a bug fix. I'll re-sent the hw/core patch
as well for 8.1.


Daniel


> thanks
> -- PMM