Re: [RFC PATCH] hw/ppc: Implement -dtb support for PowerNV

[Aditya Gupta]

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));
>