Currently any device tree passed with -dtb option in QEMU, was ignored
by the PowerNV code.

Read and pass the passed -dtb to the kernel, thus enabling easier
debugging with custom DTBs.

The existing behaviour when -dtb is 'not' passed, is preserved as-is.

But when a '-dtb' is passed, it completely overrides any dtb nodes or
changes QEMU might have done, such as '-append' arguments to the kernel
(which are mentioned in /chosen/bootargs in the dtb), hence add warning
when -dtb is being used

Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>

---
Changelog
===========
v1:
+ use 'g_file_get_contents' and add check for -append & -dtb as suggested by Daniel
---
---
 hw/ppc/pnv.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 3526852685b4..03600fa62cbd 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -736,10 +736,21 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
         }
     }
 
-    fdt = pnv_dt_create(machine);
+    if (machine->dtb) {
+        warn_report("with manually passed dtb, some options like '-append'"
+                " will get ignored and the dtb passed will be used as-is");
 
-    /* Pack resulting tree */
-    _FDT((fdt_pack(fdt)));
+        /* read the file 'machine->dtb', and load it into 'fdt' buffer */
+        if (!g_file_get_contents(machine->dtb, (gchar **)&fdt, NULL, NULL)) {
+            error_report("Could not load dtb '%s'", machine->dtb);
+            exit(1);
+        }
+    } else {
+        fdt = pnv_dt_create(machine);
+
+        /* Pack resulting tree */
+        _FDT((fdt_pack(fdt)));
+    }
 
     qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
     cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt));
@@ -952,6 +963,14 @@ static void pnv_init(MachineState *machine)
         g_free(sz);
         exit(EXIT_FAILURE);
     }
+
+    /* checks for invalid option combinations */
+    if (machine->dtb && (strlen(machine->kernel_cmdline) != 0)) {
+        error_report("-append and -dtb cannot be used together, as passed"
+                " command line is ignored in case of custom dtb");
+        exit(EXIT_FAILURE);
+    }
+
     memory_region_add_subregion(get_system_memory(), 0, machine->ram);
 
     /*
-- 
2.45.2

Hello Aditya,

On 7/31/24 22:10, Aditya Gupta wrote:
> Currently any device tree passed with -dtb option in QEMU, was ignored
> by the PowerNV code.
> 
> Read and pass the passed -dtb to the kernel, thus enabling easier
> debugging with custom DTBs.
> 
> The existing behaviour when -dtb is 'not' passed, is preserved as-is.
> 
> But when a '-dtb' is passed, it completely overrides any dtb nodes or
> changes QEMU might have done, such as '-append' arguments to the kernel
> (which are mentioned in /chosen/bootargs in the dtb), hence add warning
> when -dtb is being used
> 
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> 
> ---
> Changelog
> ===========
> v1:
> + use 'g_file_get_contents' and add check for -append & -dtb as suggested by Daniel
> ---
> ---
>   hw/ppc/pnv.c | 25 ++++++++++++++++++++++---
>   1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 3526852685b4..03600fa62cbd 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -736,10 +736,21 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
>           }
>       }
>   
> -    fdt = pnv_dt_create(machine);
> +    if (machine->dtb) {
> +        warn_report("with manually passed dtb, some options like '-append'"
> +                " will get ignored and the dtb passed will be used as-is");

This warning should come after all other checks. See below.


> -    /* Pack resulting tree */
> -    _FDT((fdt_pack(fdt)));
> +        /* read the file 'machine->dtb', and load it into 'fdt' buffer */
> +        if (!g_file_get_contents(machine->dtb, (gchar **)&fdt, NULL, NULL)) {
> +            error_report("Could not load dtb '%s'", machine->dtb);
> +            exit(1);
> +        }

We should try to report such errors earlier than in reset.

Can you please introduce a PnvMachineState::dtb attribute and initialize it
in pnv_init() after ->initrd_filename.



Thanks,

C.



> +    } else {
> +        fdt = pnv_dt_create(machine);
> +
> +        /* Pack resulting tree */
> +        _FDT((fdt_pack(fdt)));
> +    }
>   
>       qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
>       cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt));
> @@ -952,6 +963,14 @@ static void pnv_init(MachineState *machine)
>           g_free(sz);
>           exit(EXIT_FAILURE);
>       }
> +
> +    /* checks for invalid option combinations */
> +    if (machine->dtb && (strlen(machine->kernel_cmdline) != 0)) {
> +        error_report("-append and -dtb cannot be used together, as passed"
> +                " command line is ignored in case of custom dtb");
> +        exit(EXIT_FAILURE);
> +    }
> +
>       memory_region_add_subregion(get_system_memory(), 0, machine->ram);
>   
>       /*

Hello Cedric,


>> <...snip...>
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 3526852685b4..03600fa62cbd 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -736,10 +736,21 @@ static void pnv_reset(MachineState *machine, 
>> ShutdownCause reason)
>>           }
>>       }
>>   -    fdt = pnv_dt_create(machine);
>> +    if (machine->dtb) {
>> +        warn_report("with manually passed dtb, some options like 
>> '-append'"
>> +                " will get ignored and the dtb passed will be used 
>> as-is");
>
> This warning should come after all other checks. See below.
>
>
>> -    /* Pack resulting tree */
>> -    _FDT((fdt_pack(fdt)));
>> +        /* read the file 'machine->dtb', and load it into 'fdt' 
>> buffer */
>> +        if (!g_file_get_contents(machine->dtb, (gchar **)&fdt, NULL, 
>> NULL)) {
>> +            error_report("Could not load dtb '%s'", machine->dtb);
>> +            exit(1);
>> +        }
>
> We should try to report such errors earlier than in reset.
>
Thanks, I will remember this from next time.

> Can you please introduce a PnvMachineState::dtb attribute and 
> initialize it
> in pnv_init() after ->initrd_filename.
>
Sure, I will move it to pnv_init.


Thanks,

Aditya Gupta

>
>
> Thanks,
>
> C.
>
>
>
>> +    } else {
>> +        fdt = pnv_dt_create(machine);
>> +
>> +        /* Pack resulting tree */
>> +        _FDT((fdt_pack(fdt)));
>> +    }
>>         qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
>>       cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt));
>> @@ -952,6 +963,14 @@ static void pnv_init(MachineState *machine)
>>           g_free(sz);
>>           exit(EXIT_FAILURE);
>>       }
>> +
>> +    /* checks for invalid option combinations */
>> +    if (machine->dtb && (strlen(machine->kernel_cmdline) != 0)) {
>> +        error_report("-append and -dtb cannot be used together, as 
>> passed"
>> +                " command line is ignored in case of custom dtb");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>>       memory_region_add_subregion(get_system_memory(), 0, machine->ram);
>>         /*
>

On 8/1/24 09:44, Cédric Le Goater wrote:
> 
>> -    /* Pack resulting tree */
>> -    _FDT((fdt_pack(fdt)));
>> +        /* read the file 'machine->dtb', and load it into 'fdt' buffer */
>> +        if (!g_file_get_contents(machine->dtb, (gchar **)&fdt, NULL, NULL)) {
>> +            error_report("Could not load dtb '%s'", machine->dtb);
>> +            exit(1);
>> +        }
> 
> We should try to report such errors earlier than in reset.
> 
> Can you please introduce a PnvMachineState::dtb attribute and initialize it
> in pnv_init() after ->initrd_filename.

PnvMachineState::fdt might be a more appropriate name.

Thanks,

C.