| Message ID | 20240801085133.59781-1-adityag@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] hw/ppc: Implement -dtb support for PowerNV | expand |
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; >
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; >
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;
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(-)