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.mayd...@linaro.org>
> > > Cc: qemu-...@nongnu.org
> > > Signed-off-by: Daniel Henrique Barboza <danielhb...@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
signature.asc
Description: PGP signature
