[PATCH] ppc/pnv: fix dumpdtb option

Shivang Upadhyay posted 1 patch 1 month, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260225081037.21990-1-shivangu@linux.ibm.com
Maintainers: Nicholas Piggin <npiggin@gmail.com>, Aditya Gupta <adityag@linux.ibm.com>, Glenn Miles <milesg@linux.ibm.com>
There is a newer version of this series
hw/ppc/pnv.c | 4 ++++
1 file changed, 4 insertions(+)
[PATCH] ppc/pnv: fix dumpdtb option
Posted by Shivang Upadhyay 1 month, 2 weeks ago
The '-machine dumpdtb' command line option stopped working on
PowerPC/pnv systems after recent design change [1].

Fixing this by generating fdt blob in `pnv_init`.

[1] https://lore.kernel.org/qemu-devel/20250206151214.2947842-1-peter.maydell@linaro.org/

Cc: Aditya Gupta <adityag@linux.ibm.com>
Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
---
 hw/ppc/pnv.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 1513575b8f..9861714ad5 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1261,6 +1261,10 @@ static void pnv_init(MachineState *machine)
     if (pmc->i2c_init) {
         pmc->i2c_init(pnv);
     }
+
+    if (!machine->fdt) {
+        machine->fdt = pnv_dt_create(machine);
+    }
 }
 
 /*
-- 
2.52.0
Re: [PATCH] ppc/pnv: fix dumpdtb option
Posted by Peter Maydell 1 month, 2 weeks ago
On Wed, 25 Feb 2026 at 08:11, Shivang Upadhyay <shivangu@linux.ibm.com> wrote:
>
> The '-machine dumpdtb' command line option stopped working on
> PowerPC/pnv systems after recent design change [1].
>
> Fixing this by generating fdt blob in `pnv_init`.
>
> [1] https://lore.kernel.org/qemu-devel/20250206151214.2947842-1-peter.maydell@linaro.org/
>
> Cc: Aditya Gupta <adityag@linux.ibm.com>
> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>

Thanks for catching this. I would suggest having

Cc: qemu-stable@nongnu.org
Fixes: 8fd2518ef2f8d34 ("hw: Centralize handling of -machine dumpdtb option")

> ---
>  hw/ppc/pnv.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 1513575b8f..9861714ad5 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1261,6 +1261,10 @@ static void pnv_init(MachineState *machine)
>      if (pmc->i2c_init) {
>          pmc->i2c_init(pnv);
>      }
> +
> +    if (!machine->fdt) {
> +        machine->fdt = pnv_dt_create(machine);
> +    }

So we now set the machine->fdt in pnv_init() (which is
the right place: the centralized dumpdtb handling assumes
the DTB is set after machine creation but before reset).

That means that this code in pnv_reset():

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

will never take the "else" path, and that code is dead.

It also suggests we should call fdt_pack() in pnv_init
before setting machine->fdt.

This bit code in pnv_reset():

    /* Update machine->fdt with latest fdt */
    if (machine->fdt != fdt) {
        /*
         * Set machine->fdt for 'dumpdtb' QMP/HMP command. Free
         * the existing machine->fdt to avoid leaking it during
         * a reset.
         */
        g_free(machine->fdt);
        machine->fdt = fdt;
    }

is also now dead and can be removed.

Q: can the generated DBT genuinely be different on a
subsequent reset than it was when QEMU first starts?
The comments in the pnv_reset() code seem to suggest it,
but on the other hand the code as written will only
call pnv_dt_create() and update machine->fdt once on first
reset, not on later resets. So perhaps the comment is
just confusingly worded ?

thanks
-- PMM
Re: [PATCH] ppc/pnv: fix dumpdtb option
Posted by shivang upadhyay 1 month, 1 week ago
Hi Peter,

Thanks for review.

On 2/26/26 3:28 PM, Peter Maydell wrote:
> On Wed, 25 Feb 2026 at 08:11, Shivang Upadhyay <shivangu@linux.ibm.com> wrote:
>> The '-machine dumpdtb' command line option stopped working on
>> PowerPC/pnv systems after recent design change [1].
>>
>> Fixing this by generating fdt blob in `pnv_init`.
>>
>> [1] https://lore.kernel.org/qemu-devel/20250206151214.2947842-1-peter.maydell@linaro.org/
>>
>> Cc: Aditya Gupta <adityag@linux.ibm.com>
>> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
>> Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
> Thanks for catching this. I would suggest having
>
> Cc: qemu-stable@nongnu.org
> Fixes: 8fd2518ef2f8d34 ("hw: Centralize handling of -machine dumpdtb option")

Ack. Will add this.

>> ---
>>   hw/ppc/pnv.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 1513575b8f..9861714ad5 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1261,6 +1261,10 @@ static void pnv_init(MachineState *machine)
>>       if (pmc->i2c_init) {
>>           pmc->i2c_init(pnv);
>>       }
>> +
>> +    if (!machine->fdt) {
>> +        machine->fdt = pnv_dt_create(machine);
>> +    }
> So we now set the machine->fdt in pnv_init() (which is
> the right place: the centralized dumpdtb handling assumes
> the DTB is set after machine creation but before reset).
>
> That means that this code in pnv_reset():
>
>      if (machine->fdt) {
>          fdt = machine->fdt;
>      } else {
>          fdt = pnv_dt_create(machine);
>          /* Pack resulting tree */
>          _FDT((fdt_pack(fdt)));
>      }
>
> will never take the "else" path, and that code is dead.
>
> It also suggests we should call fdt_pack() in pnv_init
> before setting machine->fdt.
Correct. I missed this. Ill add this in v2.
> This bit code in pnv_reset():
>
>      /* Update machine->fdt with latest fdt */
>      if (machine->fdt != fdt) {
>          /*
>           * Set machine->fdt for 'dumpdtb' QMP/HMP command. Free
>           * the existing machine->fdt to avoid leaking it during
>           * a reset.
>           */
>          g_free(machine->fdt);
>          machine->fdt = fdt;
>      }
>
> is also now dead and can be removed.
>
> Q: can the generated DBT genuinely be different on a
> subsequent reset than it was when QEMU first starts?
> The comments in the pnv_reset() code seem to suggest it,
> but on the other hand the code as written will only
> call pnv_dt_create() and update machine->fdt once on first
> reset, not on later resets. So perhaps the comment is
> just confusingly worded ?

Yes, I am not much familiar with the design here. But, as per

my current understanding of this code, DTB content should not

not be changed.

> thanks
> -- PMM


~Shivang.
Re: [PATCH] ppc/pnv: fix dumpdtb option
Posted by Aditya Gupta 1 month, 1 week ago
On 26/02/27 03:43PM, shivang upadhyay wrote:
> > <...snip...>
> >
> > Q: can the generated DBT genuinely be different on a
> > subsequent reset than it was when QEMU first starts?
> > The comments in the pnv_reset() code seem to suggest it,
> > but on the other hand the code as written will only
> > call pnv_dt_create() and update machine->fdt once on first
> > reset, not on later resets. So perhaps the comment is
> > just confusingly worded ?
> 
> Yes, I am not much familiar with the design here. But, as per
> 
> my current understanding of this code, DTB content should not
> 
> not be changed.

To add to this, yes, normally reset path doesn't change the device tree,
since all reboots are the same as the first boot.

As of upstream this is true, I don't see dtb changing between resets.

One case where reset path does change the device tree compared to
pnv_init part is Memory-Preserving IPL (MPIPL), which is a
dump-collection feature in powernv for collecting dumps after kernel
crashes.
This is not merged yet in upstream, and is currently in discussion [1].

I am okay with the dead if condition in pnv_reset to be removed for now,
we can add it later for mpipl purpose if needed.

[1]: https://lore.kernel.org/qemu-devel/20260225124633.1841222-1-adityag@linux.ibm.com/

Thanks,
- Aditya G
Re: [PATCH] ppc/pnv: fix dumpdtb option
Posted by BALATON Zoltan 1 month, 1 week ago
On Fri, 27 Feb 2026, Aditya Gupta wrote:
> On 26/02/27 03:43PM, shivang upadhyay wrote:
>>> <...snip...>
>>>
>>> Q: can the generated DBT genuinely be different on a
>>> subsequent reset than it was when QEMU first starts?
>>> The comments in the pnv_reset() code seem to suggest it,
>>> but on the other hand the code as written will only
>>> call pnv_dt_create() and update machine->fdt once on first
>>> reset, not on later resets. So perhaps the comment is
>>> just confusingly worded ?
>>
>> Yes, I am not much familiar with the design here. But, as per
>>
>> my current understanding of this code, DTB content should not
>>
>> not be changed.
>
> To add to this, yes, normally reset path doesn't change the device tree,
> since all reboots are the same as the first boot.
>
> As of upstream this is true, I don't see dtb changing between resets.

Isn't VOF allowing clients to do setprop that would be changing the device 
tree? Some settings are stored there so maybe it can be changed by the 
guest that should be preserved between reboots?

Regards,
BALATON Zoltan

> One case where reset path does change the device tree compared to
> pnv_init part is Memory-Preserving IPL (MPIPL), which is a
> dump-collection feature in powernv for collecting dumps after kernel
> crashes.
> This is not merged yet in upstream, and is currently in discussion [1].
>
> I am okay with the dead if condition in pnv_reset to be removed for now,
> we can add it later for mpipl purpose if needed.
>
> [1]: https://lore.kernel.org/qemu-devel/20260225124633.1841222-1-adityag@linux.ibm.com/
>
> Thanks,
> - Aditya G
>
>
>
Re: [PATCH] ppc/pnv: fix dumpdtb option
Posted by Aditya Gupta 1 month, 1 week ago
On 26/02/27 02:05PM, BALATON Zoltan wrote:
> On Fri, 27 Feb 2026, Aditya Gupta wrote:
> > On 26/02/27 03:43PM, shivang upadhyay wrote:
> > > > <...snip...>
> > > > 
> > > > Q: can the generated DBT genuinely be different on a
> > > > subsequent reset than it was when QEMU first starts?
> > > > The comments in the pnv_reset() code seem to suggest it,
> > > > but on the other hand the code as written will only
> > > > call pnv_dt_create() and update machine->fdt once on first
> > > > reset, not on later resets. So perhaps the comment is
> > > > just confusingly worded ?
> > > 
> > > Yes, I am not much familiar with the design here. But, as per
> > > 
> > > my current understanding of this code, DTB content should not
> > > 
> > > not be changed.
> > 
> > To add to this, yes, normally reset path doesn't change the device tree,
> > since all reboots are the same as the first boot.
> > 
> > As of upstream this is true, I don't see dtb changing between resets.
> 
> Isn't VOF allowing clients to do setprop that would be changing the device
> tree? Some settings are stored there so maybe it can be changed by the guest
> that should be preserved between reboots?

By VOF did you mean the pseries f/w ?

i don't see anything changing the dt in powernv as of now, but
for some edge case, where that is possible (will be possible atleast
after mpipl support is merged), we can always update the fdt in below 2
if cases in pnv_reset:

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

machine->fdt will never be NULL since it's being set in pnv_init
should we just do pnv_dt_create on every pnv_reset ?

    > /* Update machine->fdt with latest fdt */
    > if (machine->fdt != fdt) {
    >     /*
    >      * Set machine->fdt for 'dumpdtb' QMP/HMP command. Free
    >      * the existing machine->fdt to avoid leaking it during
    >      * a reset.
    >      */
    >     g_free(machine->fdt);
    >     machine->fdt = fdt;
    > }

we can unconditionally always update the fdt on a pnv reset. should be
harmless, for the default case it will generate the same dt.

this should handle any edge case where in future if dt should change
between pnv_init/pnv_reset

What do you all say ?

Thanks,
- Aditya G

> 
> Regards,
> BALATON Zoltan
> 
> > One case where reset path does change the device tree compared to
> > pnv_init part is Memory-Preserving IPL (MPIPL), which is a
> > dump-collection feature in powernv for collecting dumps after kernel
> > crashes.
> > This is not merged yet in upstream, and is currently in discussion [1].
> > 
> > I am okay with the dead if condition in pnv_reset to be removed for now,
> > we can add it later for mpipl purpose if needed.
> > 
> > [1]: https://lore.kernel.org/qemu-devel/20260225124633.1841222-1-adityag@linux.ibm.com/
> > 
> > Thanks,
> > - Aditya G
> > 
> > 
> >
Re: [PATCH] ppc/pnv: fix dumpdtb option
Posted by BALATON Zoltan 1 month, 1 week ago
On Fri, 27 Feb 2026, Aditya Gupta wrote:
> On 26/02/27 02:05PM, BALATON Zoltan wrote:
>> On Fri, 27 Feb 2026, Aditya Gupta wrote:
>>> On 26/02/27 03:43PM, shivang upadhyay wrote:
>>>>> <...snip...>
>>>>>
>>>>> Q: can the generated DBT genuinely be different on a
>>>>> subsequent reset than it was when QEMU first starts?
>>>>> The comments in the pnv_reset() code seem to suggest it,
>>>>> but on the other hand the code as written will only
>>>>> call pnv_dt_create() and update machine->fdt once on first
>>>>> reset, not on later resets. So perhaps the comment is
>>>>> just confusingly worded ?
>>>>
>>>> Yes, I am not much familiar with the design here. But, as per
>>>>
>>>> my current understanding of this code, DTB content should not
>>>>
>>>> not be changed.
>>>
>>> To add to this, yes, normally reset path doesn't change the device tree,
>>> since all reboots are the same as the first boot.
>>>
>>> As of upstream this is true, I don't see dtb changing between resets.
>>
>> Isn't VOF allowing clients to do setprop that would be changing the device
>> tree? Some settings are stored there so maybe it can be changed by the guest
>> that should be preserved between reboots?
>
> By VOF did you mean the pseries f/w ?

I don't know much about these machines. I meant Virtual Open Firmware in 
QEMU or SLOF which it can replace. But maybe this is only used by spapr so 
does not affect pseries but in spapr there's spapr_vof_setprop() which 
looks like it can change the fdt. Maybe it's handled differently on 
pseries, I don't know.

> i don't see anything changing the dt in powernv as of now, but
> for some edge case, where that is possible (will be possible atleast
> after mpipl support is merged), we can always update the fdt in below 2
> if cases in pnv_reset:
>
>    > if (machine->fdt) {
>    >     fdt = machine->fdt;
>    > } else {
>    >     fdt = pnv_dt_create(machine);
>    >     /* Pack resulting tree */
>    >     _FDT((fdt_pack(fdt)));
>    > }
>
> machine->fdt will never be NULL since it's being set in pnv_init
> should we just do pnv_dt_create on every pnv_reset ?

I don't know either but wasn't original code meant to either create an fdt 
or take what the user specified with -dtb option or similiar? In that case 
maybe you don't want to override the user specified dtb but I could be 
wrong about what this is meant to do as I have no idea whatsoever about 
these machines.

Regards,
BALATON Zoltan
Re: [PATCH] ppc/pnv: fix dumpdtb option
Posted by Shivang Upadhyay 1 month, 1 week ago
Hi Bolaton,

Thanks for taking the time to review.

On Fri, Feb 27, 2026 at 04:53:56PM +0100, BALATON Zoltan wrote:
> On Fri, 27 Feb 2026, Aditya Gupta wrote:
> > On 26/02/27 02:05PM, BALATON Zoltan wrote:
> > > On Fri, 27 Feb 2026, Aditya Gupta wrote:
> > > > On 26/02/27 03:43PM, shivang upadhyay wrote:
> > > > > > <...snip...>
> > > > > > 
> > > > > > Q: can the generated DBT genuinely be different on a
> > > > > > subsequent reset than it was when QEMU first starts?
> > > > > > The comments in the pnv_reset() code seem to suggest it,
> > > > > > but on the other hand the code as written will only
> > > > > > call pnv_dt_create() and update machine->fdt once on first
> > > > > > reset, not on later resets. So perhaps the comment is
> > > > > > just confusingly worded ?
> > > > > 
> > > > > Yes, I am not much familiar with the design here. But, as per
> > > > > 
> > > > > my current understanding of this code, DTB content should not
> > > > > 
> > > > > not be changed.
> > > > 
> > > > To add to this, yes, normally reset path doesn't change the device tree,
> > > > since all reboots are the same as the first boot.
> > > > 
> > > > As of upstream this is true, I don't see dtb changing between resets.
> > > 
> > > Isn't VOF allowing clients to do setprop that would be changing the device
> > > tree? Some settings are stored there so maybe it can be changed by the guest
> > > that should be preserved between reboots?
> > 
> > By VOF did you mean the pseries f/w ?
> 
> I don't know much about these machines. I meant Virtual Open Firmware in
> QEMU or SLOF which it can replace. But maybe this is only used by spapr so
> does not affect pseries but in spapr there's spapr_vof_setprop() which looks
> like it can change the fdt. Maybe it's handled differently on pseries, I
> don't know.
Yes, Vof/Slof firmware are only used for pseries.
for pnv we use skiboot.lid firmware.
> 
> > i don't see anything changing the dt in powernv as of now, but
> > for some edge case, where that is possible (will be possible atleast
> > after mpipl support is merged), we can always update the fdt in below 2
> > if cases in pnv_reset:
> > 
> >    > if (machine->fdt) {
> >    >     fdt = machine->fdt;
> >    > } else {
> >    >     fdt = pnv_dt_create(machine);
> >    >     /* Pack resulting tree */
> >    >     _FDT((fdt_pack(fdt)));
> >    > }
> > 
> > machine->fdt will never be NULL since it's being set in pnv_init
> > should we just do pnv_dt_create on every pnv_reset ?
> 
> I don't know either but wasn't original code meant to either create an fdt
> or take what the user specified with -dtb option or similiar? In that case
> maybe you don't want to override the user specified dtb but I could be wrong
> about what this is meant to do as I have no idea whatsoever about these
> machines.
I think I should just remove all the fdt changing stuff from cpu_reset.
Becasue nothing is using it as of now. We can introduce it with mpipl patches.
That would make the code easier to understand there.
> 
> Regards,
> BALATON Zoltan


~Shivang.
Re: [PATCH] ppc/pnv: fix dumpdtb option
Posted by Aditya Gupta 1 month ago
On 26/03/02 02:54PM, Shivang Upadhyay wrote:
> On Fri, Feb 27, 2026 at 04:53:56PM +0100, BALATON Zoltan wrote:
> > On Fri, 27 Feb 2026, Aditya Gupta wrote:
> > > On 26/02/27 02:05PM, BALATON Zoltan wrote:
> > > > On Fri, 27 Feb 2026, Aditya Gupta wrote:
> > > > > On 26/02/27 03:43PM, shivang upadhyay wrote:
> > > > > > > <...snip...>
> > > > > > > 
> > > > > > > Q: can the generated DBT genuinely be different on a
> > > > > > > subsequent reset than it was when QEMU first starts?
> > > > > > > The comments in the pnv_reset() code seem to suggest it,
> > > > > > > but on the other hand the code as written will only
> > > > > > > call pnv_dt_create() and update machine->fdt once on first
> > > > > > > reset, not on later resets. So perhaps the comment is
> > > > > > > just confusingly worded ?
> > > > > > 
> > > > > > Yes, I am not much familiar with the design here. But, as per
> > > > > > 
> > > > > > my current understanding of this code, DTB content should not
> > > > > > 
> > > > > > not be changed.
> > > > > 
> > > > > To add to this, yes, normally reset path doesn't change the device tree,
> > > > > since all reboots are the same as the first boot.
> > > > > 
> > > > > As of upstream this is true, I don't see dtb changing between resets.
> > > > 
> > > > Isn't VOF allowing clients to do setprop that would be changing the device
> > > > tree? Some settings are stored there so maybe it can be changed by the guest
> > > > that should be preserved between reboots?
> > > 
> > > By VOF did you mean the pseries f/w ?
> > 
> > I don't know much about these machines. I meant Virtual Open Firmware in
> > QEMU or SLOF which it can replace. But maybe this is only used by spapr so
> > does not affect pseries but in spapr there's spapr_vof_setprop() which looks
> > like it can change the fdt. Maybe it's handled differently on pseries, I
> > don't know.
> Yes, Vof/Slof firmware are only used for pseries.
> for pnv we use skiboot.lid firmware.

Yes, VOF/SLOF is not involved in PowerNV, so that shouldn't be a problem
here.

> > > i don't see anything changing the dt in powernv as of now, but
> > > for some edge case, where that is possible (will be possible atleast
> > > after mpipl support is merged), we can always update the fdt in below 2
> > > if cases in pnv_reset:
> > > 
> > >    > if (machine->fdt) {
> > >    >     fdt = machine->fdt;
> > >    > } else {
> > >    >     fdt = pnv_dt_create(machine);
> > >    >     /* Pack resulting tree */
> > >    >     _FDT((fdt_pack(fdt)));
> > >    > }
> > > 
> > > machine->fdt will never be NULL since it's being set in pnv_init
> > > should we just do pnv_dt_create on every pnv_reset ?
> > 
> > I don't know either but wasn't original code meant to either create an fdt
> > or take what the user specified with -dtb option or similiar? In that case
> > maybe you don't want to override the user specified dtb but I could be wrong
> > about what this is meant to do as I have no idea whatsoever about these
> > machines.
> I think I should just remove all the fdt changing stuff from cpu_reset.
> Becasue nothing is using it as of now. We can introduce it with mpipl patches.
> That would make the code easier to understand there.

I have below suggestion for this, acknowledging peter's dead code
suggestion and balaton's concern about things changing at runtime
(would be possible if let's say adding/removing a phb is allowed from
qemu console)

This generates the fdt everytime on a pnv_reset, which might be same as
previous fdt but takes care of the cases where any device changes
between init and reset.

What do you all think on this ?

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 1513575b8f37..7eb4e8ac2f06 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -773,26 +773,15 @@ static void pnv_reset(MachineState *machine, ResetType type)
         }
     }
 
-    if (machine->fdt) {
-        fdt = machine->fdt;
-    } else {
-        fdt = pnv_dt_create(machine);
-        /* Pack resulting tree */
-        _FDT((fdt_pack(fdt)));
-    }
+    fdt = pnv_dt_create(machine);
+    /* Pack resulting tree */
+    _FDT((fdt_pack(fdt)));
 
     cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt));
 
-    /* Update machine->fdt with latest fdt */
-    if (machine->fdt != fdt) {
-        /*
-         * Set machine->fdt for 'dumpdtb' QMP/HMP command. Free
-         * the existing machine->fdt to avoid leaking it during
-         * a reset.
-         */
-        g_free(machine->fdt);
-        machine->fdt = fdt;
-    }
+    /* Free the previously allocated fdt and update with latest */
+    g_free(machine->fdt);
+    machine->fdt = fdt;
 }
 
 static ISABus *pnv_chip_power8_isa_create(PnvChip *chip, Error **errp)
Re: [PATCH] ppc/pnv: fix dumpdtb option
Posted by Peter Maydell 1 month ago
On Fri, 6 Mar 2026 at 06:32, Aditya Gupta <adityag@linux.ibm.com> wrote:
>
> I have below suggestion for this, acknowledging peter's dead code
> suggestion and balaton's concern about things changing at runtime
> (would be possible if let's say adding/removing a phb is allowed from
> qemu console)
>
> This generates the fdt everytime on a pnv_reset, which might be same as
> previous fdt but takes care of the cases where any device changes
> between init and reset.

Do you intend this as a sketch of what to do later when the FDT
can change at runtime, or a plan for now?

I think that for now (i.e. fixing this bug) we should do the
simple thing, and generate the FDT once on startup.

We can design what to do for the "we really do need to
generate a new FDT on reset" case later, when we have the
requirement that motivates it.

thanks
-- PMM
Re: [PATCH] ppc/pnv: fix dumpdtb option
Posted by Aditya Gupta 1 month ago
On 06/03/26 14:27, Peter Maydell wrote:
> On Fri, 6 Mar 2026 at 06:32, Aditya Gupta <adityag@linux.ibm.com> wrote:
>> I have below suggestion for this, acknowledging peter's dead code
>> suggestion and balaton's concern about things changing at runtime
>> (would be possible if let's say adding/removing a phb is allowed from
>> qemu console)
>>
>> This generates the fdt everytime on a pnv_reset, which might be same as
>> previous fdt but takes care of the cases where any device changes
>> between init and reset.
> Do you intend this as a sketch of what to do later when the FDT
> can change at runtime, or a plan for now?
Yes that is my plan for the case atleast when FDT can change at runtime,
eg. when MPIPL is introduced.
>
> I think that for now (i.e. fixing this bug) we should do the
> simple thing, and generate the FDT once on startup.
>
> We can design what to do for the "we really do need to
> generate a new FDT on reset" case later, when we have the
> requirement that motivates it.
Makes sense. I am okay with either way since I don't see fdt changing at 
runtime
for PowerNV as of now

Thanks,

- Aditya G
Re: [PATCH] ppc/pnv: fix dumpdtb option
Posted by Aditya Gupta 1 month, 2 weeks ago
On 25/02/26 13:40, Shivang Upadhyay wrote:

> The '-machine dumpdtb' command line option stopped working on
> PowerPC/pnv systems after recent design change [1].
>
> Fixing this by generating fdt blob in `pnv_init`.

Thanks for fixing this Shivang.

The generated dtb is same as the dtb qemu passes to OPAL, which is good:

     # diff original.dts dumpdtb.dts
     Files original.dts and dumpdtb.dts are identical

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


Thanks,

- Aditya G

>
> [1] https://lore.kernel.org/qemu-devel/20250206151214.2947842-1-peter.maydell@linaro.org/
>
> Cc: Aditya Gupta <adityag@linux.ibm.com>
> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
> ---
>   hw/ppc/pnv.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 1513575b8f..9861714ad5 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1261,6 +1261,10 @@ static void pnv_init(MachineState *machine)
>       if (pmc->i2c_init) {
>           pmc->i2c_init(pnv);
>       }
> +
> +    if (!machine->fdt) {
> +        machine->fdt = pnv_dt_create(machine);
> +    }
>   }
>   
>   /*

Re: [PATCH] ppc/pnv: fix dumpdtb option
Posted by shivang upadhyay 1 month, 1 week ago
Hi Aditya,

Thanks for your review.

On 2/25/26 6:37 PM, Aditya Gupta wrote:
> On 25/02/26 13:40, Shivang Upadhyay wrote:
>
>> The '-machine dumpdtb' command line option stopped working on
>> PowerPC/pnv systems after recent design change [1].
>>
>> Fixing this by generating fdt blob in `pnv_init`.
>
> Thanks for fixing this Shivang.
>
> The generated dtb is same as the dtb qemu passes to OPAL, which is good:
>
>     # diff original.dts dumpdtb.dts
>     Files original.dts and dumpdtb.dts are identical
>
> Reviewed-by: Aditya Gupta <adityag@linux.ibm.com>
>
>
> Thanks,
>
> - Aditya G
>
>>
>> [1] 
>> https://lore.kernel.org/qemu-devel/20250206151214.2947842-1-peter.maydell@linaro.org/
>>
>> Cc: Aditya Gupta <adityag@linux.ibm.com>
>> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
>> Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
>> ---
>>   hw/ppc/pnv.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 1513575b8f..9861714ad5 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1261,6 +1261,10 @@ static void pnv_init(MachineState *machine)
>>       if (pmc->i2c_init) {
>>           pmc->i2c_init(pnv);
>>       }
>> +
>> +    if (!machine->fdt) {
>> +        machine->fdt = pnv_dt_create(machine);
>> +    }
>>   }
>>     /*