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
===========
v3:
 + use 'load_device_tree' to read the device tree, instead of g_file_get_contents
 + tested that passed dtb does NOT get ignored on system_reset

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         | 34 ++++++++++++++++++++++++++++++----
 include/hw/ppc/pnv.h |  2 ++
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 3526852685b4..14225f7e48af 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -736,10 +736,13 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
         }
     }
 
-    fdt = pnv_dt_create(machine);
-
-    /* Pack resulting tree */
-    _FDT((fdt_pack(fdt)));
+    if (pnv->fdt) {
+        fdt = pnv->fdt;
+    } 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 +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,21 @@ static void pnv_init(MachineState *machine)
         }
     }
 
+    /* load dtb if passed */
+    if (machine->dtb) {
+        int fdt_size;
+
+        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 */
+        pnv->fdt = load_device_tree(machine->dtb, &fdt_size);
+        if (!pnv->fdt) {
+            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.46.0

On Tue Aug 13, 2024 at 11:45 PM AEST, 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>

This looks pretty good, I'm inclined to take it as a bug fix fo this
release.  One little nit is MachineState.fdt vs PnvMachineState.fdt
which is now confusing. I would call the new PnvMachineState member
something like fdt_from_dtb, or fdt_override?

The other question... Some machines rebuild fdt at init, others at
reset time. As far as I understood, spapr has to rebuild on reset
because C-A-S call can update the fdt so you have to undo that on
reset. Did powernv just copy that without really needing it, I wonder?
Maybe that rearranged to just do it at init time (e.g., see
hw/riscv/virt.c which is simpler).

Thanks,
Nick

>
> ---
> Changelog
> ===========
> v3:
>  + use 'load_device_tree' to read the device tree, instead of g_file_get_contents
>  + tested that passed dtb does NOT get ignored on system_reset
>
> 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         | 34 ++++++++++++++++++++++++++++++----
>  include/hw/ppc/pnv.h |  2 ++
>  2 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 3526852685b4..14225f7e48af 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -736,10 +736,13 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
>          }
>      }
>  
> -    fdt = pnv_dt_create(machine);
> -
> -    /* Pack resulting tree */
> -    _FDT((fdt_pack(fdt)));
> +    if (pnv->fdt) {
> +        fdt = pnv->fdt;
> +    } 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 +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,21 @@ static void pnv_init(MachineState *machine)
>          }
>      }
>  
> +    /* load dtb if passed */
> +    if (machine->dtb) {
> +        int fdt_size;
> +
> +        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 */
> +        pnv->fdt = load_device_tree(machine->dtb, &fdt_size);
> +        if (!pnv->fdt) {
> +            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;
>  

On 8/15/24 09:31, Nicholas Piggin wrote:
> On Tue Aug 13, 2024 at 11:45 PM AEST, 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>
> 
> This looks pretty good, I'm inclined to take it as a bug fix fo this
> release.  

I don't think this is a bug fix. is it ? AFAIUI, it is a debug
feature for skiboot. It's QEMU 9.2 material.

> One little nit is MachineState.fdt vs PnvMachineState.fdt
> which is now confusing. I would call the new PnvMachineState member
> something like fdt_from_dtb, or fdt_override?

I agree. this is confusing. machine->fdt could be used instead ?
  
> The other question... Some machines rebuild fdt at init, others at
> reset time. As far as I understood, spapr has to rebuild on reset
> because C-A-S call can update the fdt so you have to undo that on
> reset. 

C-A-S is a guest OS hcall. reset is called before the guest OS
is started.

> Did powernv just copy that without really needing it, I wonder?
> Maybe that rearranged to just do it at init time (e.g., see
> hw/riscv/virt.c which is simpler).

The machine is aware of user created devices (on the command line)
only at reset time.

Thanks,

C.



  

> Thanks,
> Nick
> 
>>
>> ---
>> Changelog
>> ===========
>> v3:
>>   + use 'load_device_tree' to read the device tree, instead of g_file_get_contents
>>   + tested that passed dtb does NOT get ignored on system_reset
>>
>> 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         | 34 ++++++++++++++++++++++++++++++----
>>   include/hw/ppc/pnv.h |  2 ++
>>   2 files changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 3526852685b4..14225f7e48af 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -736,10 +736,13 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
>>           }
>>       }
>>   
>> -    fdt = pnv_dt_create(machine);
>> -
>> -    /* Pack resulting tree */
>> -    _FDT((fdt_pack(fdt)));
>> +    if (pnv->fdt) {
>> +        fdt = pnv->fdt;
>> +    } 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 +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,21 @@ static void pnv_init(MachineState *machine)
>>           }
>>       }
>>   
>> +    /* load dtb if passed */
>> +    if (machine->dtb) {
>> +        int fdt_size;
>> +
>> +        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 */
>> +        pnv->fdt = load_device_tree(machine->dtb, &fdt_size);
>> +        if (!pnv->fdt) {
>> +            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;
>>   
>

Hi Cedric,


On 15/08/24 23:22, Cédric Le Goater wrote:
>
> I don't think this is a bug fix. is it ? AFAIUI, it is a debug
> feature for skiboot. It's QEMU 9.2 material.
>
Thanks for answering Nick's question, I did not check my mails.

Yes, it can be considered a debug feature.

>> One little nit is MachineState.fdt vs PnvMachineState.fdt
>> which is now confusing. I would call the new PnvMachineState member
>> something like fdt_from_dtb, or fdt_override?
>
> I agree. this is confusing. machine->fdt could be used instead ?

Sure, will use it.


Thanks,

Aditya Gupta

>
>> The other question... Some machines rebuild fdt at init, others at
>> reset time. As far as I understood, spapr has to rebuild on reset
>> because C-A-S call can update the fdt so you have to undo that on
>> reset. 
>
> C-A-S is a guest OS hcall. reset is called before the guest OS
> is started.
>
>> Did powernv just copy that without really needing it, I wonder?
>> Maybe that rearranged to just do it at init time (e.g., see
>> hw/riscv/virt.c which is simpler).
>
> The machine is aware of user created devices (on the command line)
> only at reset time.
>
> Thanks,
>
> C.
>
>
>
>
>
>> Thanks,
>> Nick
>>
>>>
>>> ---
>>> Changelog
>>> ===========
>>> v3:
>>>   + use 'load_device_tree' to read the device tree, instead of 
>>> g_file_get_contents
>>>   + tested that passed dtb does NOT get ignored on system_reset
>>>
>>> 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         | 34 ++++++++++++++++++++++++++++++----
>>>   include/hw/ppc/pnv.h |  2 ++
>>>   2 files changed, 32 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>>> index 3526852685b4..14225f7e48af 100644
>>> --- a/hw/ppc/pnv.c
>>> +++ b/hw/ppc/pnv.c
>>> @@ -736,10 +736,13 @@ static void pnv_reset(MachineState *machine, 
>>> ShutdownCause reason)
>>>           }
>>>       }
>>>   -    fdt = pnv_dt_create(machine);
>>> -
>>> -    /* Pack resulting tree */
>>> -    _FDT((fdt_pack(fdt)));
>>> +    if (pnv->fdt) {
>>> +        fdt = pnv->fdt;
>>> +    } 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 +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,21 @@ static void pnv_init(MachineState *machine)
>>>           }
>>>       }
>>>   +    /* load dtb if passed */
>>> +    if (machine->dtb) {
>>> +        int fdt_size;
>>> +
>>> +        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 */
>>> +        pnv->fdt = load_device_tree(machine->dtb, &fdt_size);
>>> +        if (!pnv->fdt) {
>>> +            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;
>>
>

On Fri Aug 16, 2024 at 3:52 AM AEST, Cédric Le Goater wrote:
> On 8/15/24 09:31, Nicholas Piggin wrote:
> > On Tue Aug 13, 2024 at 11:45 PM AEST, 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>
> > 
> > This looks pretty good, I'm inclined to take it as a bug fix fo this
> > release.  
>
> I don't think this is a bug fix. is it ? AFAIUI, it is a debug
> feature for skiboot. It's QEMU 9.2 material.

Okay.

> > One little nit is MachineState.fdt vs PnvMachineState.fdt
> > which is now confusing. I would call the new PnvMachineState member
> > something like fdt_from_dtb, or fdt_override?
>
> I agree. this is confusing. machine->fdt could be used instead ?

Yeah that could be another option. Test pnv.dtb or add a new bool
to pnv if you need to check whether the fdt has been provided by
cmdline.

> > The other question... Some machines rebuild fdt at init, others at
> > reset time. As far as I understood, spapr has to rebuild on reset
> > because C-A-S call can update the fdt so you have to undo that on
> > reset. 
>
> C-A-S is a guest OS hcall. reset is called before the guest OS
> is started.

Right, but when you reboot it needs to be reverted to initial
(pre-CAS) fdt.

> > Did powernv just copy that without really needing it, I wonder?
> > Maybe that rearranged to just do it at init time (e.g., see
> > hw/riscv/virt.c which is simpler).
>
> The machine is aware of user created devices (on the command line)
> only at reset time.

Ah, I should have followed a bit closer. riscv, arm use a
machine_done notifier for that (and x86, loongarch for ACPI / BIOS
tables). So that avoids fdt rebuild after the first reset I think.

Anyway I don't really mind then, following other archs would be okay,
but keeping similar with spapr and avoiding code change is also good.
Maybe add a small comment to we use reset rather than machine_done
notifier of other archs to be similar to spapr.

Thanks,
Nick

Hello Nick,

On 16/08/24 07:50, Nicholas Piggin wrote:
> <...snip...>
>>> One little nit is MachineState.fdt vs PnvMachineState.fdt
>>> which is now confusing. I would call the new PnvMachineState member
>>> something like fdt_from_dtb, or fdt_override?
>> I agree. this is confusing. machine->fdt could be used instead ?
> Yeah that could be another option. Test pnv.dtb or add a new bool
> to pnv if you need to check whether the fdt has been provided by
> cmdline.

Sure, I will use machine->fdt. Testing pnv.dtb should be good enough to 
check if -dtb was passed I think.


Regarding the conversation about CAS, I don't have idea on it, other 
than the minimum basics. But thanks to you and Cedric, got to know 
somethings.


Thanks,

Aditya Gupta

>>> The other question... Some machines rebuild fdt at init, others at
>>> reset time. As far as I understood, spapr has to rebuild on reset
>>> because C-A-S call can update the fdt so you have to undo that on
>>> reset.
>> C-A-S is a guest OS hcall. reset is called before the guest OS
>> is started.
> Right, but when you reboot it needs to be reverted to initial
> (pre-CAS) fdt.
>
>>> Did powernv just copy that without really needing it, I wonder?
>>> Maybe that rearranged to just do it at init time (e.g., see
>>> hw/riscv/virt.c which is simpler).
>> The machine is aware of user created devices (on the command line)
>> only at reset time.
> Ah, I should have followed a bit closer. riscv, arm use a
> machine_done notifier for that (and x86, loongarch for ACPI / BIOS
> tables). So that avoids fdt rebuild after the first reset I think.
>
> Anyway I don't really mind then, following other archs would be okay,
> but keeping similar with spapr and avoiding code change is also good.
> Maybe add a small comment to we use reset rather than machine_done
> notifier of other archs to be similar to spapr.
>
> Thanks,
> Nick