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

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

Hi Cedric,


On 15/08/24 23:22, Cédric Le Goater wrote:
>
> 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.
>
Thanks for answering Nick's question, I did not check my mails.

Yes, it can be considered a debug feature.

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

Sure, will use it.


Thanks,

Aditya Gupta

>
>> 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.
>
>> 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.
>
> Thanks,
>
> C.
>
>
>
>
>
>> 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;
>>
>