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

[Laszlo Ersek]

On 03/08/19 19:07, Philippe Mathieu-Daudé wrote:
> 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?).

It's possible I referred to that a long time ago, but most recently, I
think it has been raised by Dave.

Thanks
Laszlo

> 
>>>>> +{ '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.
>