Re: [Qemu-ppc] [PATCH v2 13/18] hw/nvram/fw_cfg: Add QMP 'info fw_cfg' command

[Philippe Mathieu-Daudé]

On 3/8/19 6:31 PM, Eric Blake wrote:
> On 3/8/19 5:08 AM, Philippe Mathieu-Daudé wrote:
> 
>>>> {
>>>>     "return": [
>>>>         {
>>>>             "architecture_specific": false,
>>>>             "key": 0,
>>>>             "writeable": false,
>>>>             "size": 4,
>>>>             "keyname": "signature"
>>>
>>> You could return a base64-encoded representation of the field (we do
>>> that in other interfaces where raw binary can't be passed reliably over
>>> JSON).  For 4 bytes, that makes sense,
>>>
>>>
>>>>         {
>>>>             "architecture_specific": true,
>>>>             "key": 3,
>>>>             "writeable": false,
>>>>             "size": 324,
>>>>             "keyname": "e820_tables"
>>>
>>> for 324 bytes, it gets a bit long. Still, may be worth it for the added
>>> debug visibility.
>>
>> Until you see values in the next patch...:
>>
>> $ (echo info fw_cfg; echo q) | qemu-system-x86_64 -S -monitor stdio
>> Selector  Well-Known Key  Pathname ArchSpec Perm   Size Order Hex Data
>> [...]
>>  0x0019   file_dir                           RO    2052       0000000b..
>>  0x0022   file: etc/acpi/tables              RO  131072  130  46414353..
> 
> Yeah, that's a no-go.
>>
>> What about using a 512B limit on this QMP answer?
> 
> Seems reasonable. Either omit the field when its size exceeds the limit,
> or use the field to give a truncated version (where the size field still
> shows the full length, either way).

OK.

>>>> +# @path: If the key is a 'file', the named file path.
>>>> +# @order: If the key is a 'file', the named file order.
>>>> +#
>>>> +# Since 4.0
>>>> +##
>>>> +{ 'struct': 'FirmwareConfigurationItem',
>>>> +  'data': { 'key': 'uint16',
>>>> +            '*keyname': 'str',
>>>> +            'architecture_specific': 'bool',
>>>> +            'writeable': 'bool',
>>>> +            '*data': 'str',
>>>> +            'size': 'int',
>>>> +            '*path': 'str',
>>>> +            '*order': 'int' } }
>>>
>>> Is it worth making 'keyname' an enum type, and turning this into a flat
>>> union where 'path' and 'order' appear on the branch associated with
>>> 'file', and where all other well-known keynames have smaller branches?
>>
>> I have no idea about that, I will have a look at QMP flat unions.
> 
> Markus can help if you get stuck on what it might look like. But by now,
> there are several examples in .qapi files to copy from (look for
> 'discriminator').

OK.

>>
>>>> +
>>>> +
>>>> +##
>>>> +# @query-fw_cfg-items:
>>>
>>> That looks weird to mix - and _. Any reason we can't just go with
>>> query-firmware-config?
>>
>> Way better! I'll use query-firmware-config-items.
> 
> Do we need the -items suffix? Also, is there a chance that we might ever
> want to extend the command to return more information that is global to
> firmware-config rather than per-item?

Laszlo suggested it could be useful to ask for a specific item (with the
full data encoded?).

>>>> +{ 'command': 'query-fw_cfg-items', 'returns': 
>>>> ['FirmwareConfigurationItem']}
> 
> As written, this results in:
> 
> { "return": [ { ... }, { ... } ] }
> 
> but if you add a layer of nesting, you could have easier extensions:
> 
> { "return": { "global": {}, "items": [ { ... }, { ... } ] } }
> 

Oh I guess I got it, I'll try it.

Thanks!

Phil.