Re: [PATCH 1/4] hw/ppc: Set machine->fdt in e500 machines

[Bernhard Beschow]

Am 26. Januar 2023 15:58:18 UTC schrieb Daniel Henrique Barboza 
<danielhb413@gmail.com>:
>
>
>On 1/25/23 10:00, Bernhard Beschow wrote:
>> This enables support for the 'dumpdtb' QMP/HMP command for all
>> e500 machines.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/ppc/e500.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 9fa1f8e6cf..7239993acc 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -659,9 +659,14 @@ done:
>>       if (!dry_run) {
>>           qemu_fdt_dumpdtb(fdt, fdt_size);
>>           cpu_physical_memory_write(addr, fdt, fdt_size);
>> +
>> +        /* Set machine->fdt for 'dumpdtb' QMP/HMP command */
>> +        g_free(machine->fdt);
>> +        machine->fdt = fdt;
>> +    } else {
>> +        g_free(fdt);
>>       }
>>       ret = fdt_size;
>> -    g_free(fdt);
>>   
>
>I tried to do this change last year when introducing 'dumpdtb' and Phil had 
>some
>comments in how the FDT was being handled by the e500 board:
>
>https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg03256.html
>
>
>================
>
>+
>+    /*
>+     * Update the machine->fdt pointer to enable support for the
>+     * 'dumpdtb' QMP/HMP command.
>+     *
>+     * The FDT is re-created during reset,
>
>Why are we doing that? Is it really necessary? This seems to be only required 
>at cold power-on.

The e500 boards have user-creatable eTSEC nics which are registered with the 
machine via the platform bus mechanism. IIUC this mechanism causes these nics 
to show up only after reset. The nics need to show up in the device tree, so 
the reset trigger was apparently chosen to create the fdt.

N.B.: The size of the fdt needs to be known during machine_init to check 
whether it fits into guest ram. That's what the dry_run parameter is for.

Does that explanation help?

Best regards,
Bernhard

>
>+ so free machine->fdt
>+     * to avoid leaking the old FDT.
>+     */
>+    g_free(machine->fdt);
>+    machine->fdt = fdt;
>================
>
>I ended up not going after Phil's concern. I don't think it's required to 
>accept
>this change, but it would simplify it a bit if the FDT isn't required to be
>re-generated on boot.
>
>
>I'm CCing Phil in case he wants to comment on it as well.
>
>
>
>
>Daniel
>
>
>>   out:
>>       g_free(pci_map);