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

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

On Fri Aug 16, 2024 at 3:52 AM AEST, Cédric Le Goater wrote:
> On 8/15/24 09:31, Nicholas Piggin wrote:
> > 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.  
>
> I don't think this is a bug fix. is it ? AFAIUI, it is a debug
> feature for skiboot. It's QEMU 9.2 material.

Okay.

> > 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?
>
> I agree. this is confusing. machine->fdt could be used instead ?

Yeah that could be another option. Test pnv.dtb or add a new bool
to pnv if you need to check whether the fdt has been provided by
cmdline.

> > 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. 
>
> C-A-S is a guest OS hcall. reset is called before the guest OS
> is started.

Right, but when you reboot it needs to be reverted to initial
(pre-CAS) fdt.

> > 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).
>
> The machine is aware of user created devices (on the command line)
> only at reset time.

Ah, I should have followed a bit closer. riscv, arm use a
machine_done notifier for that (and x86, loongarch for ACPI / BIOS
tables). So that avoids fdt rebuild after the first reset I think.

Anyway I don't really mind then, following other archs would be okay,
but keeping similar with spapr and avoiding code change is also good.
Maybe add a small comment to we use reset rather than machine_done
notifier of other archs to be similar to spapr.

Thanks,
Nick