| Message ID | 20240731132235.887918-1-adityag@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] hw/ppc: Implement -dtb support for PowerNV | expand |
On Wed, Jul 31, 2024 at 06:52:35PM +0530, 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 is an RFC patch, and hence might not be the final implementation, > though this current one is a solution which works > --- > --- > hw/ppc/pnv.c | 29 ++++++++++++++++++++++++++--- > 1 file changed, 26 insertions(+), 3 deletions(-) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 3526852685b4..12cc909b9e26 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -714,6 +714,8 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason) > PnvMachineState *pnv = PNV_MACHINE(machine); > IPMIBmc *bmc; > void *fdt; > + FILE *fdt_file; > + uint32_t fdt_size; > > qemu_devices_reset(reason); > > @@ -736,10 +738,31 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason) > } > } > > - fdt = pnv_dt_create(machine); > + if (machine->dtb) { > + warn_report("with manually passed dtb, some options like '-append'" > + " might ignored and the dtb passed will be used as-is"); Check whether append is actually set, and report an fatal error in that case. > > - /* Pack resulting tree */ > - _FDT((fdt_pack(fdt))); > + fdt = g_malloc0(FDT_MAX_SIZE); > + > + /* read the file 'machine->dtb', and load it into 'fdt' buffer */ > + fdt_file = fopen(machine->dtb, "r"); > + if (fdt_file != NULL) { > + fdt_size = fread(fdt, sizeof(char), FDT_MAX_SIZE, fdt_file); > + if (ferror(fdt_file) != 0) { > + error_report("Could not load dtb '%s'", > + machine->dtb); > + exit(1); > + } > + > + /* mark end of the fdt buffer with '\0' */ > + ((char *)fdt)[fdt_size] = '\0'; > + } Preferrable to use g_file_get_contents() > + } 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)); > -- > 2.45.2 > > With regards, Daniel
Hi Daniel, Thank you for the review. On 24/07/31 02:34PM, Daniel P. Berrangé wrote: > On Wed, Jul 31, 2024 at 06:52:35PM +0530, 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 is an RFC patch, and hence might not be the final implementation, > > though this current one is a solution which works > > --- > > --- > > hw/ppc/pnv.c | 29 ++++++++++++++++++++++++++--- > > 1 file changed, 26 insertions(+), 3 deletions(-) > > > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > > index 3526852685b4..12cc909b9e26 100644 > > --- a/hw/ppc/pnv.c > > +++ b/hw/ppc/pnv.c > > @@ -714,6 +714,8 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason) > > PnvMachineState *pnv = PNV_MACHINE(machine); > > IPMIBmc *bmc; > > void *fdt; > > + FILE *fdt_file; > > + uint32_t fdt_size; > > > > qemu_devices_reset(reason); > > > > @@ -736,10 +738,31 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason) > > } > > } > > > > - fdt = pnv_dt_create(machine); > > + if (machine->dtb) { > > + warn_report("with manually passed dtb, some options like '-append'" > > + " might ignored and the dtb passed will be used as-is"); > > Check whether append is actually set, and report an fatal error in > that case. Got it. Though there might be more options which might get ignored, such as maybe even -device options, as whatever QEMU adds to it's device tree, that will get ignored, and only the device nodes present in passed DTB will be used. > > > > > - /* Pack resulting tree */ > > - _FDT((fdt_pack(fdt))); > > + fdt = g_malloc0(FDT_MAX_SIZE); > > + > > + /* read the file 'machine->dtb', and load it into 'fdt' buffer */ > > + fdt_file = fopen(machine->dtb, "r"); > > + if (fdt_file != NULL) { > > + fdt_size = fread(fdt, sizeof(char), FDT_MAX_SIZE, fdt_file); > > + if (ferror(fdt_file) != 0) { > > + error_report("Could not load dtb '%s'", > > + machine->dtb); > > + exit(1); > > + } > > + > > + /* mark end of the fdt buffer with '\0' */ > > + ((char *)fdt)[fdt_size] = '\0'; > > + } > > Preferrable to use g_file_get_contents() Sure, thanks, didn't know that ! Thanks, Aditya Gupta > > > + } 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)); > > -- > > 2.45.2 > > > > > > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
Hello Aditya, On 7/31/24 15:22, 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. I thought we had enough controls with the QEMU command line options to generate a custom DTB. We should improve that first. Unless you want to mimic a bogus DTB as generated by hostboot and check skiboot behavior. Can you explain more the use case please ? Is it for skiboot testing ? Thanks, C. > 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 is an RFC patch, and hence might not be the final implementation, > though this current one is a solution which works > > --- > hw/ppc/pnv.c | 29 ++++++++++++++++++++++++++--- > 1 file changed, 26 insertions(+), 3 deletions(-) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 3526852685b4..12cc909b9e26 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -714,6 +714,8 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason) > PnvMachineState *pnv = PNV_MACHINE(machine); > IPMIBmc *bmc; > void *fdt; > + FILE *fdt_file; > + uint32_t fdt_size; > > qemu_devices_reset(reason); > > @@ -736,10 +738,31 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason) > } > } > > - fdt = pnv_dt_create(machine); > + if (machine->dtb) { > + warn_report("with manually passed dtb, some options like '-append'" > + " might ignored and the dtb passed will be used as-is"); > > - /* Pack resulting tree */ > - _FDT((fdt_pack(fdt))); > + fdt = g_malloc0(FDT_MAX_SIZE); > + > + /* read the file 'machine->dtb', and load it into 'fdt' buffer */ > + fdt_file = fopen(machine->dtb, "r"); > + if (fdt_file != NULL) { > + fdt_size = fread(fdt, sizeof(char), FDT_MAX_SIZE, fdt_file); > + if (ferror(fdt_file) != 0) { > + error_report("Could not load dtb '%s'", > + machine->dtb); > + exit(1); > + } > + > + /* mark end of the fdt buffer with '\0' */ > + ((char *)fdt)[fdt_size] = '\0'; > + } > + } 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));
On 7/31/24 15:51, Aditya Gupta wrote: > Hi Daniel, > > Thank you for the review. > > On 24/07/31 02:34PM, Daniel P. Berrangé wrote: >> On Wed, Jul 31, 2024 at 06:52:35PM +0530, 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 is an RFC patch, and hence might not be the final implementation, >>> though this current one is a solution which works >>> --- >>> --- >>> hw/ppc/pnv.c | 29 ++++++++++++++++++++++++++--- >>> 1 file changed, 26 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c >>> index 3526852685b4..12cc909b9e26 100644 >>> --- a/hw/ppc/pnv.c >>> +++ b/hw/ppc/pnv.c >>> @@ -714,6 +714,8 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason) >>> PnvMachineState *pnv = PNV_MACHINE(machine); >>> IPMIBmc *bmc; >>> void *fdt; >>> + FILE *fdt_file; >>> + uint32_t fdt_size; >>> >>> qemu_devices_reset(reason); >>> >>> @@ -736,10 +738,31 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason) >>> } >>> } >>> >>> - fdt = pnv_dt_create(machine); >>> + if (machine->dtb) { >>> + warn_report("with manually passed dtb, some options like '-append'" >>> + " might ignored and the dtb passed will be used as-is"); >> >> Check whether append is actually set, and report an fatal error in >> that case. > > Got it. and this check should be done preferably when the machine is initialized, not in the reset handler. Thanks, C.
Hi Cedric, On 24/07/31 04:43PM, Cédric Le Goater wrote: > Hello Aditya, > > On 7/31/24 15:22, 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. > > I thought we had enough controls with the QEMU command line options to > generate a custom DTB. We should improve that first. Unless you want > to mimic a bogus DTB as generated by hostboot and check skiboot behavior. > > Can you explain more the use case please ? Is it for skiboot testing ? My usecase is mostly experimental, where I am changing skiboot's relocation, trying to boot with almost no/minimal parts of skiboot, and thereby I am passing a custom DTB. Though the DTB I pass is basically the DTB QEMU passes to skiboot, and edited it to remove things, add an 'opal' node, and basically have more control on what device nodes QEMU passes to the firmware/kernel. The usecase you told seems interesting though, I have not encountered bogus DTBs by hostboot yet (still new), but might help test skiboot when that happens. '-dtb' option will not be for the usual usecase, but can help try these experimental things with a quick and dirty dtb. Thanks, Aditya Gupta > > Thanks, > > C. > > > > 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 is an RFC patch, and hence might not be the final implementation, > > though this current one is a solution which works > > > > --- > > hw/ppc/pnv.c | 29 ++++++++++++++++++++++++++--- > > 1 file changed, 26 insertions(+), 3 deletions(-) > > > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > > index 3526852685b4..12cc909b9e26 100644 > > --- a/hw/ppc/pnv.c > > +++ b/hw/ppc/pnv.c > > @@ -714,6 +714,8 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason) > > PnvMachineState *pnv = PNV_MACHINE(machine); > > IPMIBmc *bmc; > > void *fdt; > > + FILE *fdt_file; > > + uint32_t fdt_size; > > qemu_devices_reset(reason); > > @@ -736,10 +738,31 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason) > > } > > } > > - fdt = pnv_dt_create(machine); > > + if (machine->dtb) { > > + warn_report("with manually passed dtb, some options like '-append'" > > + " might ignored and the dtb passed will be used as-is"); > > - /* Pack resulting tree */ > > - _FDT((fdt_pack(fdt))); > > + fdt = g_malloc0(FDT_MAX_SIZE); > > + > > + /* read the file 'machine->dtb', and load it into 'fdt' buffer */ > > + fdt_file = fopen(machine->dtb, "r"); > > + if (fdt_file != NULL) { > > + fdt_size = fread(fdt, sizeof(char), FDT_MAX_SIZE, fdt_file); > > + if (ferror(fdt_file) != 0) { > > + error_report("Could not load dtb '%s'", > > + machine->dtb); > > + exit(1); > > + } > > + > > + /* mark end of the fdt buffer with '\0' */ > > + ((char *)fdt)[fdt_size] = '\0'; > > + } > > + } 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)); >
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index 3526852685b4..12cc909b9e26 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -714,6 +714,8 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason) PnvMachineState *pnv = PNV_MACHINE(machine); IPMIBmc *bmc; void *fdt; + FILE *fdt_file; + uint32_t fdt_size; qemu_devices_reset(reason); @@ -736,10 +738,31 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason) } } - fdt = pnv_dt_create(machine); + if (machine->dtb) { + warn_report("with manually passed dtb, some options like '-append'" + " might ignored and the dtb passed will be used as-is"); - /* Pack resulting tree */ - _FDT((fdt_pack(fdt))); + fdt = g_malloc0(FDT_MAX_SIZE); + + /* read the file 'machine->dtb', and load it into 'fdt' buffer */ + fdt_file = fopen(machine->dtb, "r"); + if (fdt_file != NULL) { + fdt_size = fread(fdt, sizeof(char), FDT_MAX_SIZE, fdt_file); + if (ferror(fdt_file) != 0) { + error_report("Could not load dtb '%s'", + machine->dtb); + exit(1); + } + + /* mark end of the fdt buffer with '\0' */ + ((char *)fdt)[fdt_size] = '\0'; + } + } 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));
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 is an RFC patch, and hence might not be the final implementation, though this current one is a solution which works --- --- hw/ppc/pnv.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-)