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

[Aditya Gupta]

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;