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 <adit...@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;
