[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Daniel Henrique Barboza
Subject: Re: [PATCH for-7.2 v2 01/20] hw/arm: do not free machine->fdt in arm_load_dtb()
Date: Mon, 8 Aug 2022 20:00:49 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0

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

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 
      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.

I believe some of the other patches in the series have similar concerns.

Ok! I'll revisit those patches and be sure to return the updated value
of the fdt.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]