Re: [PATCH for-7.2 v2 01/20] hw/arm: do not free machine->fdt in arm_load_dtb()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4568 bytes --]

On Fri, Aug 12, 2022 at 07:03:26PM -0300, Daniel Henrique Barboza wrote:
> David,
> 
> On 8/8/22 00:23, David Gibson wrote:
> > On Fri, Aug 05, 2022 at 06:39:29AM -0300, Daniel Henrique Barboza 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.
> > > 
> > > This is not an issue right now because there's no code that access
> > > machine->fdt after arm_load_dtb(), but we're going to add a couple do
> > > FDT HMP commands that will rely on machine->fdt being valid.
> > > 
> > > Instead of freeing 'fdt' at the end of arm_load_dtb(), assign it to
> > > machine->fdt. This will allow the FDT of ARM machines that relies on
> > > arm_load_dtb() to be accessed later on.
> > > 
> > > Since all ARM machines allocates the FDT only once, we don't need to
> > > worry about leaking the existing FDT during a machine reset (which is
> > > something that other machines have to look after, e.g. the ppc64 pSeries
> > > machine).
> > > 
> > > Cc: Peter Maydell <peter.maydell@linaro.org>
> > > Cc: qemu-arm@nongnu.org
> > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > ---
> > >   hw/arm/boot.c | 8 +++++++-
> > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > > index ada2717f76..9f5ceb62d2 100644
> > > --- a/hw/arm/boot.c
> > > +++ b/hw/arm/boot.c
> > > @@ -684,7 +684,13 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> > >        */
> > >       rom_add_blob_fixed_as("dtb", fdt, size, addr, as);
> > > -    g_free(fdt);
> > > +    /*
> > > +     * Update the ms->fdt pointer to enable support for 'dumpdtb'
> > > +     * and 'info fdt' commands. Use fdt_pack() to shrink the blob
> > > +     * size we're going to store.
> > > +     */
> > > +    fdt_pack(fdt);
> > > +    ms->fdt = fdt;
> > >       return size;
> > 
> > fdt_pack() could change (reduce) the effective size of the dtb blob,
> > so returning a 'size' value from above rather than the new value of
> > fdt_totalsize(fdt) doesn't see right.
> 
> After some thought I think executing fdt_pack() like I'm doing here is not
> a good idea. The first problem is that I'm not returning the updated size,
> as you've said.
> 
> But I can't just amend a 'return fdt_totalsize(fdt);' either. I'm packing the
> FDT **after** the machine store it in the guest physical memory. If I return
> the packed size, but the machine isn't packing the FDT before a
> cpu_physical_memory_write(), I'll be under-reporting the FDT size written.

Ah... good point.

> Machines such as e500 (patch 4) uses this returned value to put more stuff in
> the guest memory. In that case, returning a smaller size that what was actually
> written can cause the machine to overwrite the FDT by accident. In fact, only
> a handful of machines (ppc/pseries, ppc/pvn, riscv, oepenrisc) is doing a
> fdt_pack() before a cpu_physical_memory_write(). So this change would be
> potentially harmful to a lot of people.
> 
> One alternative would be to do a fdt_pack() before the machine writes the
> FDT in the guest memory, but that is too intrusive to do because I can't say
> if each of these machines will be OK with that. I have a hunch that it would
> be OK, but a hunch isn't going to cut it.

Hmm.. I'd be fairly confident that would be ok.  It certainly should
be ok for the fdt content itself, fdt_pack() doesn't change that
semantically.  If the machine is using that size to put stuff after, I
can't really see how that could break, either.  Unless the machine
were using the fdtsize in one place and a fixed size somewhere else to
address the same data, which would be a bug.

> I'll just drop the fdt_pack() for each of these patches. If the machine code
> is already packing it, fine. If not, that's fine too. Each maintainer can
> assess whether packing the FDT is worth it or not.

That's probably reasonable for the time being.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]