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

[Aditya Gupta]

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 :|
>