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

[Cédric Le Goater]

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.