Re: [PATCH for-7.2 v2 10/20] hw/ppc: set machine->fdt in spapr machine

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 8/23/22 05:58, Alexey Kardashevskiy wrote:
> 
> 
> On 22/08/2022 20:30, Daniel Henrique Barboza wrote:
>>
>>
>> On 8/22/22 00:29, Alexey Kardashevskiy wrote:
>>>
>>>
>>> On 22/08/2022 13:05, David Gibson wrote:
>>>> On Fri, Aug 19, 2022 at 06:42:34AM -0300, Daniel Henrique Barboza wrote:
>>>>>
>>>>>
>>>>> On 8/18/22 23:11, Alexey Kardashevskiy wrote:
>>>>>>
>>>>>>
>>>>>> On 05/08/2022 19:39, Daniel Henrique Barboza wrote:
>>>>>>> The pSeries machine never bothered with the common machine->fdt
>>>>>>> attribute. We do all the FDT related work using spapr->fdt_blob.
>>>>>>>
>>>>>>> We're going to introduce HMP commands to read and save the FDT, which
>>>>>>> will rely on setting machine->fdt properly to work across all machine
>>>>>>> archs/types.
>>>>>>
>>>>>>
>>>>>> Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property enough?
>>>>>
>>>>> I tried to do the minimal changes needed for the commands to work. ms::fdt is
>>>>> one of the few MachineState fields that hasn't been QOMified by
>>>>> machine_class_init() yet. All pre-existing code that uses ms::fdt are using the
>>>>> pointer directly. To make a QOMified use of it would require extra patches
>>>>> in machine.c to QOMify the property first.
>>>>>
>>>>> There's also the issue with how each machine is creating the FDT. Most are using
>>>>> helpers from device_tree.c, some are creating it from scratch, others required
>>>>> a .dtb file, most of them are not doing a fdt_pack() and so on. To really QOMify
>>>>> the use of ms::fdt we would need some machine hooks that standardize all that.
>>>>> I believe it's worth the trouble, but it would be too much to do
>>>>> right now.
>>>>
>>>> Hmm.. I think this depends on what you mean by "QOM"ify exactly.  If
>>>> you're meaning make the full DT representation QOM objects, that you
>>>> can look into in detail, then, yes, that's pretty complicated.
>>>>
>>>> I suspect what Alexey was suggesting though, was merely to make
>>>> ms::fdt accessible as a single bytestring property on the machine QOM
>>>> object.  Effectively it's just "dumpdtb" but as a property get.
>>>
>>>
>>> Yes, I meant the bytestream, as DTC can easily decompile it onto a DTS.
>>>
>>>
>>>> I'm not 100% certain if QOM can safely represent arbitrary bytestrings
>>>> as QOM properties, which would need checking.
>>>
>>> I am not sure either but rather than adding another command to HMP, I'd explore this option first.
>>
>>
>> I'm not sure what you mean by that. The HMP version of 'dumpdtb' is more flexible
>> that the current "-machine dumpdtb", an extra machine option that would cause
>> the guest to exit after writing the dtb
> 
> True. Especially with CAS :)
> 
>> And 'info fdt' is a new command that
>> makes it easier to inspect specific nodes/props.
> 
> btw what is this new command going to do? decompile the tree or save dtb?

At this moment, 'info fdt <node> [prop]' is using the current ms->fdt bytestream
plus libfdt to output nodes and properties.

> 
>> I don't see how making ms::fdt being retrievable by object_property_get() internally
>> (remember that ms::fdt it's not fully QOMified, so there's no introspection of its
>> value from the QEMU monitor) would make any of these new HMP commands obsolete.
> 
> Well, there are QMP and HMP and my feeling was that HMP is slowly getting deprecated or something and QMP is the superior one. So I thought since this FDT is a property and there is no associated action with it, making it a property would do.

I don't have an option about HMP being deprecated and QMP being the preferable
choice. Perhaps someone else reading this thread can tell more about it.

> 
> For ages I've been using a python3 script to talk to QMP as HMP is really quite limited, the only thing in HMP which is not in QMP is dumping memory ("x", "xp"), in this case I wrap HMP into QMP and keep using QMP :)


Apparently we have a QMP enthusiast over here ;)



Daniel

> 
> 
>>
>>
>> Thanks,
>>
>>
>> Daniel
>>
>>>
>>>
>>>
>