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

["Nicholas Piggin" <npiggin@gmail.com>]

On Tue Aug 13, 2024 at 11:45 PM AEST, 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 looks pretty good, I'm inclined to take it as a bug fix fo this
release.  One little nit is MachineState.fdt vs PnvMachineState.fdt
which is now confusing. I would call the new PnvMachineState member
something like fdt_from_dtb, or fdt_override?

The other question... Some machines rebuild fdt at init, others at
reset time. As far as I understood, spapr has to rebuild on reset
because C-A-S call can update the fdt so you have to undo that on
reset. Did powernv just copy that without really needing it, I wonder?
Maybe that rearranged to just do it at init time (e.g., see
hw/riscv/virt.c which is simpler).

Thanks,
Nick

>
> ---
> Changelog
> ===========
> v3:
>  + use 'load_device_tree' to read the device tree, instead of g_file_get_contents
>  + tested that passed dtb does NOT get ignored on system_reset
>
> 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         | 34 ++++++++++++++++++++++++++++++----
>  include/hw/ppc/pnv.h |  2 ++
>  2 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 3526852685b4..14225f7e48af 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -736,10 +736,13 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
>          }
>      }
>  
> -    fdt = pnv_dt_create(machine);
> -
> -    /* Pack resulting tree */
> -    _FDT((fdt_pack(fdt)));
> +    if (pnv->fdt) {
> +        fdt = pnv->fdt;
> +    } 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));
> @@ -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);
> +    }
> +
>      memory_region_add_subregion(get_system_memory(), 0, machine->ram);
>  
>      /*
> @@ -1003,6 +1014,21 @@ static void pnv_init(MachineState *machine)
>          }
>      }
>  
> +    /* load dtb if passed */
> +    if (machine->dtb) {
> +        int fdt_size;
> +
> +        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 */
> +        pnv->fdt = load_device_tree(machine->dtb, &fdt_size);
> +        if (!pnv->fdt) {
> +            error_report("Could not load dtb '%s'", machine->dtb);
> +            exit(1);
> +        }
> +    }
> +
>      /* 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;
>