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

[Corey Minyard]

On Thu, Jun 11, 2020 at 01:49:31PM +0200, Philippe Mathieu-Daudé wrote:
> 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.

fw_cfg_set_order_override() is only needed in cases where you are loading
data for devices (VGA, NICs, and other devices) and when loading a
user-specified file.  So basically, anything that is not a named entry
in fw_config_order[].  If it has a name in fw_config_order[], then you
shouldn't use an override.  Otherwise you should.

I'm not aware of anything that wouldn't fall into the existing cases,
so I don't see a reason to add a new call.  Assuming that device
initialization order and such all stays the same, order should be
preserved, and I don't know of another category you would add.  Am I
missing something?

-corey

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