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)
