[PATCH v2] hw/ppc: Implement -dtb support for PowerNV

[PATCH v2] hw/ppc: Implement -dtb support for PowerNV

[Aditya Gupta]

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
===========
v2:
 + move reading dtb and warning to pnv_init

v1:
 + use 'g_file_get_contents' and add check for -append & -dtb as suggested by Daniel
---
---
 hw/ppc/pnv.c         | 29 ++++++++++++++++++++++++++---
 include/hw/ppc/pnv.h |  2 ++
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 3526852685b4..047725bd97fc 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -736,11 +736,14 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
         }
     }
 
-    fdt = pnv_dt_create(machine);
+    if (!pnv->fdt) {
+        pnv->fdt = pnv_dt_create(machine);
 
-    /* Pack resulting tree */
-    _FDT((fdt_pack(fdt)));
+        /* Pack resulting tree */
+        _FDT((fdt_pack(pnv->fdt)));
+    }
 
+    fdt = pnv->fdt;
     qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
     cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt));
 
@@ -952,6 +955,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);
 
     /*
@@ -1003,6 +1014,18 @@ static void pnv_init(MachineState *machine)
         }
     }
 
+    /* load dtb if passed */
+    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");
+
+        /* read the file 'machine->dtb', and load it into 'fdt' buffer */
+        if (!g_file_get_contents(machine->dtb, (gchar **)&pnv->fdt, NULL, NULL)) {
+            error_report("Could not load dtb '%s'", machine->dtb);
+            exit(1);
+        }
+    }
+
     /* MSIs are supported on this platform */
     msi_nonbroken = true;
 
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index fcb6699150c8..20b68fd9264e 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -91,6 +91,8 @@ struct PnvMachineState {
     uint32_t     initrd_base;
     long         initrd_size;
 
+    void         *fdt;
+
     uint32_t     num_chips;
     PnvChip      **chips;
 
-- 
2.45.2

Re: [PATCH v2] hw/ppc: Implement -dtb support for PowerNV

[Cédric Le Goater]

On 8/1/24 10:51, 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
> ===========
> v2:
>   + move reading dtb and warning to pnv_init
> 
> v1:
>   + use 'g_file_get_contents' and add check for -append & -dtb as suggested by Daniel
> ---
> ---
>   hw/ppc/pnv.c         | 29 ++++++++++++++++++++++++++---
>   include/hw/ppc/pnv.h |  2 ++
>   2 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 3526852685b4..047725bd97fc 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -736,11 +736,14 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
>           }
>       }
>   
> -    fdt = pnv_dt_create(machine);
> +    if (!pnv->fdt) {
> +        pnv->fdt = pnv_dt_create(machine);

At next reset, the dtb specified on the command line will be ignored.
Please reverse the logic :

     if (pnv->fdt) {
         fdt = pnv->fdt;
     } else {
         fdt = pnv_dt_create(machine);
         /* Pack resulting tree */
         _FDT((fdt_pack(fdt)));
     }

but, at the end of pnv_reset(), beware of :

     g_free(machine->fdt);
     machine->fdt = fdt;


> -    /* Pack resulting tree */
> -    _FDT((fdt_pack(fdt)));
> +        /* Pack resulting tree */
> +        _FDT((fdt_pack(pnv->fdt)));
> +    }
>   
> +    fdt = pnv->fdt;
>       qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
>       cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt));
>   
> @@ -952,6 +955,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);

I think this is redundant with the warn report below.

> +    }
> +
>       memory_region_add_subregion(get_system_memory(), 0, machine->ram);
>   
>       /*
> @@ -1003,6 +1014,18 @@ static void pnv_init(MachineState *machine)
>           }
>       }
>   
> +    /* load dtb if passed */
> +    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");
> +
> +        /* read the file 'machine->dtb', and load it into 'fdt' buffer */
> +        if (!g_file_get_contents(machine->dtb, (gchar **)&pnv->fdt, NULL, NULL)) {
> +            error_report("Could not load dtb '%s'", machine->dtb);
> +            exit(1);
> +        }

There is a load_device_tree() routine you could use. Please check how other
machines handle -dtb for examples.


Thanks,

C.



> +    }
> +
>       /* MSIs are supported on this platform */
>       msi_nonbroken = true;
>   
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index fcb6699150c8..20b68fd9264e 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -91,6 +91,8 @@ struct PnvMachineState {
>       uint32_t     initrd_base;
>       long         initrd_size;
>   
> +    void         *fdt;
> +
>       uint32_t     num_chips;
>       PnvChip      **chips;
>   

Re: [PATCH v2] hw/ppc: Implement -dtb support for PowerNV

[Aditya Gupta]

Hi Cedric,

Sorry for the late reply.


On 01/08/24 15:22, Cédric Le Goater wrote:
> On 8/1/24 10:51, 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
>> ===========
>> v2:
>>   + move reading dtb and warning to pnv_init
>>
>> v1:
>>   + use 'g_file_get_contents' and add check for -append & -dtb as 
>> suggested by Daniel
>> ---
>> ---
>>   hw/ppc/pnv.c         | 29 ++++++++++++++++++++++++++---
>>   include/hw/ppc/pnv.h |  2 ++
>>   2 files changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 3526852685b4..047725bd97fc 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -736,11 +736,14 @@ static void pnv_reset(MachineState *machine, 
>> ShutdownCause reason)
>>           }
>>       }
>>   -    fdt = pnv_dt_create(machine);
>> +    if (!pnv->fdt) {
>> +        pnv->fdt = pnv_dt_create(machine);
>
> At next reset, the dtb specified on the command line will be ignored.
> Please reverse the logic :
>
>     if (pnv->fdt) {
>         fdt = pnv->fdt;
>     } else {
>         fdt = pnv_dt_create(machine);
>         /* Pack resulting tree */
>         _FDT((fdt_pack(fdt)));
>     }


Understood, thanks for pointing it out.

>
> but, at the end of pnv_reset(), beware of :
>
>     g_free(machine->fdt);
>     machine->fdt = fdt;
>
That should be okay right ? Even if machine->fdt is NULL, that's okay, 
and we assign the latest fdt to machine->fdt
>
>> -    /* Pack resulting tree */
>> -    _FDT((fdt_pack(fdt)));
>> +        /* Pack resulting tree */
>> +        _FDT((fdt_pack(pnv->fdt)));
>> +    }
>>   +    fdt = pnv->fdt;
>>       qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
>>       cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt));
>>   @@ -952,6 +955,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);
>
> I think this is redundant with the warn report below.

Can we keep it ?

The warning is meant to be generic, as other options, maybe even -smp 
might get ignored. There might be lot of such combinations.

While, the error above just covers one of the combinations, give an 
error if -append & -dtb are used together.

The error and warning seem repetitive, maybe I will add some more flags 
to the warning ?


Thanks,

Aditya Gupta

>
>> +    }
>> +
>>       memory_region_add_subregion(get_system_memory(), 0, machine->ram);
>>         /*
>> @@ -1003,6 +1014,18 @@ static void pnv_init(MachineState *machine)
>>           }
>>       }
>>   +    /* load dtb if passed */
>> +    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");
>> +
>> +        /* read the file 'machine->dtb', and load it into 'fdt' 
>> buffer */
>> +        if (!g_file_get_contents(machine->dtb, (gchar **)&pnv->fdt, 
>> NULL, NULL)) {
>> +            error_report("Could not load dtb '%s'", machine->dtb);
>> +            exit(1);
>> +        }
>
> There is a load_device_tree() routine you could use. Please check how 
> other
> machines handle -dtb for examples.
>
>
> Thanks,
>
> C.
>
>
>
>> +    }
>> +
>>       /* MSIs are supported on this platform */
>>       msi_nonbroken = true;
>>   diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index fcb6699150c8..20b68fd9264e 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -91,6 +91,8 @@ struct PnvMachineState {
>>       uint32_t     initrd_base;
>>       long         initrd_size;
>>   +    void         *fdt;
>> +
>>       uint32_t     num_chips;
>>       PnvChip      **chips;
>