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

[Eric Blake]

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).


>>> +# @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').

> 
>>> +
>>> +
>>> +##
>>> +# @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?

>>> +{ '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": [ { ... }, { ... } ] } }

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org