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

[Laszlo Ersek]

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()?

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