Re: [PATCH] ppc/pnv: generate dtb after machine initialization is complete

[Aditya Gupta <adityag@linux.ibm.com>]

On 26/03/24 07:20PM, Shivang Upadhyay wrote:
> <...snip...>
>
> +static void pnv_machine_init_done(Notifier *notifier, void *data)
> +{
> +    PnvMachineState *pnv = container_of(notifier, PnvMachineState, machine_init_done);
> +    MachineState *machine = MACHINE(pnv);
> +
> +    if (!machine->fdt) {
> +        machine->fdt = pnv_dt_create(machine);
> +        _FDT((fdt_pack(machine->fdt)));
> +    }
> +}
> +
>  static void pnv_init(MachineState *machine)
>  {
>      const char *bios_name = machine->firmware ?: FW_FILE_NAME;
> @@ -1244,10 +1255,8 @@ static void pnv_init(MachineState *machine)
>          pmc->i2c_init(pnv);
>      }
>  
> -    if (!machine->fdt) {
> -        machine->fdt = pnv_dt_create(machine);
> -        _FDT((fdt_pack(machine->fdt)));
> -    }
> +    pnv->machine_init_done.notify = pnv_machine_init_done;
> +    qemu_add_machine_init_done_notifier(&pnv->machine_init_done);
>  }
>  
>  /*

Thanks for fixing it. Just adding few more info from what I see.
OPAL was stuck at cec-power-down, since it expects an ipmi-bt device for
shutdown to work in QEMU.

So, the issue was not directly due to missing BMC device, rather due to
missing 'isa-ipmi-bt' device, which the patch does fix, and hence the
issue is fixed.

But still the dts we have is different from what we should have at
pnv_reset. Adding a pnv_dt_create at the end of pnv_reset, as it used to
be before, gives me a 'bmc' node also in device tree.

	$ diff /tmp/dts.machinedone /tmp/dts.pnvreset 
	16a17,33
	>       bmc {
	> 
	>               sensors {
	>                       #size-cells = <0x00>;
	>                       #address-cells = <0x01>;
	> 
	>                       sensor@0 {
	>                               ipmi-sensor-type = <0x23>;
	>                               ipmi-entity-instance = <0x01>;
	>                               ipmi-entity-id = <0x23>;
	>                               ipmi-sensor-reading-type = <0x6f>;
	>                               compatible = "ibm,ipmi-sensor";
	>                               reg = <0x00>;
	>                       };
	>               };
	>       };
	> 

This isn't handled at machine_init_done as 'pnv->bmc' is not set at this
time

Two ways to fix it:
1. Have pnv_dt_create in pnv_reset as well, like previously
2. Move the pnv_bmc_find code in pnv_reset to pnv_init

What do you say ?

> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -111,6 +111,8 @@ struct PnvMachineState {
>  
>      bool         big_core;
>      bool         lpar_per_core;
> +
> +    Notifier     machine_init_done;

One minor note, all other machines in qemu use 'machine_done' as the
notifier name, but I am good with 'machine_init_done' also, as this
makes more sense to me.

Thanks,
- Aditya G