Re: [PATCH v7 2/5] softmmu/vl: Let -fw_cfg option take a 'gen_id' argument

[Philippe Mathieu-Daudé]

On 6/11/20 1:31 PM, Laszlo Ersek wrote:
> On 06/09/20 17:50, Corey Minyard wrote:
>> On Fri, May 29, 2020 at 11:50:24AM +0200, Laszlo Ersek wrote:
>>> Gerd, Corey: there's a question for you near the end, please.
>>>
>>> On 05/28/20 19:31, Philippe Mathieu-Daudé wrote:
>>
>> snip...
>>
>>>
>>>
>>> (3) I've noticed another *potential* issue, from looking at the larger
>>> context. I apologize for missing it in v6.
>>>
>>> See commit bab47d9a75a3 ("Sort the fw_cfg file list", 2016-04-07). (I'm
>>> copying Corey; Gerd is already copied.) From that commit, we have, at
>>> the end of this function:
>>>
>>>     /* For legacy, keep user files in a specific global order. */
>>>     fw_cfg_set_order_override(fw_cfg, FW_CFG_ORDER_OVERRIDE_USER);
>>>     fw_cfg_add_file(fw_cfg, name, buf, size);
>>>     fw_cfg_reset_order_override(fw_cfg);
>>>
>>> This takes effect for "file" and "string", but not for "gen_id". Should
>>> we apply it to "gen_id" as well? (Sorry, I really don't understand what
>>> commit bab47d9a75a3 is about!)
>>
>> I can explain the rationale for that change, but I'm not sure of the
>> answer to your question.  That changes makes sure that the fw_cfg data
>> remains exactly the same even on newer versions of qemu if the machine
>> is set the same.  This way you can do migrations to newer qemu versions
>> and anything using fw_cfg won't get confused because the data changes.
>>
>> The reason that change was so complex was preserving the order for
>> migrating from older versions.
>>
>> This is only about migration.  I'm not sure what gen_id is, but if it's
>> migrated, it better be future proof.
> 
> Whenever introducing a new fw_cfg file (*any* new named file), how do we
> decide whether we need fw_cfg_set_order_override()?

Good idea to ask, so we can document the answer in the fw_cfg API doc.

> 
> Thanks
> Laszlo
> 
> 
>>
>> -corey
>>
>>>
>>> *IF* we want to apply the same logic to "gen_id", then we should
>>> *perhaps* do, on the "nonempty_str(gen_id)" branch:
>>>
>>>         size_t fw_cfg_size;
>>>
>>>         fw_cfg_set_order_override(fw_cfg, FW_CFG_ORDER_OVERRIDE_USER);
>>>         fw_cfg_size = fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
>>>         fw_cfg_reset_order_override(fw_cfg);
>>>         return (fw_cfg_size > 0) ? 0 : -1;
>>>
>>> I think???
>>>
>>> Or maybe even use FW_CFG_ORDER_OVERRIDE_DEVICE rather than
>>> FW_CFG_ORDER_OVERRIDE_USER? I don't have the slightest clue.
>>>
>>> (I guess if I understood what commit bab47d9a75a3 was about, I'd be less
>>> in doubt now. But that commit only hints at "avoid[ing] any future
>>> issues of moving the file creation" -- I don't know what those issues
>>> were in the first place!)
>>>
>>> With (1) optionally fixed, and (2) fixed, I'd be willing to R-b this
>>> patch; but I'm really thrown off by (3).
>>>
>>> Thanks,
>>> Laszlo
>>>
>>>
>>>>      } else {
>>>>          GError *err = NULL;
>>>>          if (!g_file_get_contents(file, &buf, &size, &err)) {
>>>>
>>>
>>
>