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