diff mbox series

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

Message ID 20230125130024.158721-2-shentey@gmail.com (mailing list archive)
State New, archived
Headers show
Series E500 cleanups and enhancements | expand

Commit Message

Bernhard Beschow Jan. 25, 2023, 1 p.m. UTC
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(-)

Comments

Daniel Henrique Barboza Jan. 26, 2023, 3:58 p.m. UTC | #1
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.

+ 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);
Bernhard Beschow Jan. 26, 2023, 4:38 p.m. UTC | #2
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);
diff mbox series

Patch

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);
 
 out:
     g_free(pci_map);