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

[Corey Minyard <minyard@acm.org>]

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.

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