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