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

[Daniel Henrique Barboza <danielhb413@gmail.com>]

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.

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.

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.


Thanks,


Daniel


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