| Message ID | 20200528173141.17495-3-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites | expand |
Gerd, Corey: there's a question for you near the end, please. On 05/28/20 19:31, Philippe Mathieu-Daudé wrote: > The 'gen_id' argument refers to a QOM object able to produce > data consumable by the fw_cfg device. The producer object must > implement the FW_CFG_DATA_GENERATOR interface. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > v7: > - renamed 'blob_id' -> 'gen_id' (danpb) > - update comment in code (lersek) > - fixed CODING_STYLE (lersek) > - use Laszlo's if (SUM(options)) != 1 { error } form > --- > softmmu/vl.c | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/softmmu/vl.c b/softmmu/vl.c > index ae5451bc23..cdb1d187ed 100644 > --- a/softmmu/vl.c > +++ b/softmmu/vl.c > @@ -489,6 +489,11 @@ static QemuOptsList qemu_fw_cfg_opts = { > .name = "string", > .type = QEMU_OPT_STRING, > .help = "Sets content of the blob to be inserted from a string", > + }, { > + .name = "gen_id", > + .type = QEMU_OPT_STRING, > + .help = "Sets id of the object generating the fw_cfg blob " > + "to be inserted", > }, > { /* end of list */ } > }, > @@ -2020,7 +2025,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) > { > gchar *buf; > size_t size; > - const char *name, *file, *str; > + const char *name, *file, *str, *gen_id; > FWCfgState *fw_cfg = (FWCfgState *) opaque; > > if (fw_cfg == NULL) { > @@ -2030,14 +2035,13 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) > name = qemu_opt_get(opts, "name"); > file = qemu_opt_get(opts, "file"); > str = qemu_opt_get(opts, "string"); > + gen_id = qemu_opt_get(opts, "gen_id"); > > - /* we need name and either a file or the content string */ > - if (!(nonempty_str(name) && (nonempty_str(file) || nonempty_str(str)))) { > - error_setg(errp, "invalid argument(s)"); > - return -1; > - } > - if (nonempty_str(file) && nonempty_str(str)) { > - error_setg(errp, "file and string are mutually exclusive"); > + /* we need the name, and exactly one of: file, content string, gen_id */ > + if (!nonempty_str(name) || > + nonempty_str(file) + nonempty_str(str) + nonempty_str(gen_id) != 1) { (1) I believe the indentation of this line is not correct. I think it should be out-dented by 2 spaces. > + error_setg(errp, "name, plus exactly one of file," > + " string and gen_id, are needed"); > return -1; > } > if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) { > @@ -2052,6 +2056,8 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) > if (nonempty_str(str)) { > size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */ > buf = g_memdup(str, size); > + } else if (nonempty_str(gen_id)) { > + return fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp); (2) This is no longer correct: fw_cfg_add_from_generator() now returns 0 on failure, but parse_fw_cfg() is supposed to return nonzer on failure. See the comment on qemu_opts_foreach() -- "parse_fw_cfg" is passed as the loop callback to qemu_opts_foreach(). Technically, we could simply write return !fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp); but that wouldn't be consistent with the -1 error codes returned elsewhere from parse_fw_cfg(). So how about: size_t fw_cfg_size; fw_cfg_size = fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp); return (fw_cfg_size > 0) ? 0 : -1; I think your testing may have missed this because the problem is only visible if you have *another* -fw_cfg option on the QEMU command line. Returning the wrong status code from here terminates the qemu_opts_foreach() loop, without attempting to set "error_fatal". Therefore the loop is silently terminated, thus the only symptom would be that -fw_cfg options beyond the "gen_id" one wouldn't take effect. (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!) *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)) { >
On 5/29/20 11:50 AM, 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: >> The 'gen_id' argument refers to a QOM object able to produce >> data consumable by the fw_cfg device. The producer object must >> implement the FW_CFG_DATA_GENERATOR interface. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> v7: >> - renamed 'blob_id' -> 'gen_id' (danpb) >> - update comment in code (lersek) >> - fixed CODING_STYLE (lersek) >> - use Laszlo's if (SUM(options)) != 1 { error } form >> --- >> softmmu/vl.c | 22 ++++++++++++++-------- >> 1 file changed, 14 insertions(+), 8 deletions(-) >> >> diff --git a/softmmu/vl.c b/softmmu/vl.c >> index ae5451bc23..cdb1d187ed 100644 >> --- a/softmmu/vl.c >> +++ b/softmmu/vl.c >> @@ -489,6 +489,11 @@ static QemuOptsList qemu_fw_cfg_opts = { >> .name = "string", >> .type = QEMU_OPT_STRING, >> .help = "Sets content of the blob to be inserted from a string", >> + }, { >> + .name = "gen_id", >> + .type = QEMU_OPT_STRING, >> + .help = "Sets id of the object generating the fw_cfg blob " >> + "to be inserted", >> }, >> { /* end of list */ } >> }, >> @@ -2020,7 +2025,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) >> { >> gchar *buf; >> size_t size; >> - const char *name, *file, *str; >> + const char *name, *file, *str, *gen_id; >> FWCfgState *fw_cfg = (FWCfgState *) opaque; >> >> if (fw_cfg == NULL) { >> @@ -2030,14 +2035,13 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) >> name = qemu_opt_get(opts, "name"); >> file = qemu_opt_get(opts, "file"); >> str = qemu_opt_get(opts, "string"); >> + gen_id = qemu_opt_get(opts, "gen_id"); >> >> - /* we need name and either a file or the content string */ >> - if (!(nonempty_str(name) && (nonempty_str(file) || nonempty_str(str)))) { >> - error_setg(errp, "invalid argument(s)"); >> - return -1; >> - } >> - if (nonempty_str(file) && nonempty_str(str)) { >> - error_setg(errp, "file and string are mutually exclusive"); >> + /* we need the name, and exactly one of: file, content string, gen_id */ >> + if (!nonempty_str(name) || >> + nonempty_str(file) + nonempty_str(str) + nonempty_str(gen_id) != 1) { > > (1) I believe the indentation of this line is not correct. I think it > should be out-dented by 2 spaces. > >> + error_setg(errp, "name, plus exactly one of file," >> + " string and gen_id, are needed"); >> return -1; >> } >> if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) { >> @@ -2052,6 +2056,8 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) >> if (nonempty_str(str)) { >> size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */ >> buf = g_memdup(str, size); >> + } else if (nonempty_str(gen_id)) { >> + return fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp); > > (2) This is no longer correct: fw_cfg_add_from_generator() now returns 0 > on failure, but parse_fw_cfg() is supposed to return nonzer on failure. > See the comment on qemu_opts_foreach() -- "parse_fw_cfg" is passed as > the loop callback to qemu_opts_foreach(). > > Technically, we could simply write > > return !fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp); > > but that wouldn't be consistent with the -1 error codes returned > elsewhere from parse_fw_cfg(). So how about: > > size_t fw_cfg_size; > > fw_cfg_size = fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp); > return (fw_cfg_size > 0) ? 0 : -1; > > I think your testing may have missed this because the problem is only > visible if you have *another* -fw_cfg option on the QEMU command line. > Returning the wrong status code from here terminates the > qemu_opts_foreach() loop, without attempting to set "error_fatal". > Therefore the loop is silently terminated, thus the only symptom would > be that -fw_cfg options beyond the "gen_id" one wouldn't take effect. > > > (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!) > > *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!) Since the filename is not listed in fw_cfg_order[], it falls to the "unknown stuff at the end": /* Stick unknown stuff at the end. */ error_report("warning: Unknown firmware file in legacy mode: %s\n", name); return FW_CFG_ORDER_OVERRIDE_LAST; Which seems safe (we do not mess with firmware specific DEVICE/USER entries). Gerd? > > With (1) optionally fixed, and (2) fixed, I'd be willing to R-b this > patch; but I'm really thrown off by (3). I addressed (1) and (2), thanks for your review :) > > Thanks, > Laszlo > > >> } else { >> GError *err = NULL; >> if (!g_file_get_contents(file, &buf, &size, &err)) { >> >
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)) { > > >
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)) { >>> >> >
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)) { >>>> >>> >> >
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)) { > >>>> > >>> > >> > > >
Hi, > > 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()? The whole point of the sorting is to make sure the fw_cfg directory listing entry (FW_CFG_FILE_DIR) is stable and doesn't change underneath the guest when it gets live-migrated. That sorting was added in qemu 2.6, to make sure things don't chance by accident in case the initialization order changes. Now you've got a problem when you migrate from qemu 2.5 (+older) to qemu 2.6 (+newer), because 2.5 has the entries in initialization order and 2.6 has the entries in alphabetical order. To deal with that machine types for 2.5 & older keep the old sort order. This is the reason why legacy_fw_cfg_order exists. For new features and files you can completely ignore the whole legacy sorting mess. cross-version live migration works only for features supported by both qemu versions, therefore the legacy sorting is only relevant for features & files already supported by qemu 2.5. HTH, Gerd
On 6/15/20 4:45 PM, Gerd Hoffmann wrote: > Hi, > >>> 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()? > > The whole point of the sorting is to make sure the fw_cfg directory > listing entry (FW_CFG_FILE_DIR) is stable and doesn't change underneath > the guest when it gets live-migrated. > > That sorting was added in qemu 2.6, to make sure things don't chance by > accident in case the initialization order changes. Now you've got a > problem when you migrate from qemu 2.5 (+older) to qemu 2.6 (+newer), > because 2.5 has the entries in initialization order and 2.6 has the > entries in alphabetical order. To deal with that machine types for 2.5 > & older keep the old sort order. This is the reason why > legacy_fw_cfg_order exists. > > For new features and files you can completely ignore the whole legacy > sorting mess. cross-version live migration works only for features > supported by both qemu versions, therefore the legacy sorting is only > relevant for features & files already supported by qemu 2.5. Thanks you Gerd for the whole explanation. I added an entry to my TODO list to document this, based on your comment (and Corey's). I'll address it later, as you confirmed it doesn't impact this series. Regards, Phil.
On 06/15/20 17:02, Philippe Mathieu-Daudé wrote: > On 6/15/20 4:45 PM, Gerd Hoffmann wrote: >> Hi, >> >>>> 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()? >> >> The whole point of the sorting is to make sure the fw_cfg directory >> listing entry (FW_CFG_FILE_DIR) is stable and doesn't change underneath >> the guest when it gets live-migrated. >> >> That sorting was added in qemu 2.6, to make sure things don't chance by >> accident in case the initialization order changes. Now you've got a >> problem when you migrate from qemu 2.5 (+older) to qemu 2.6 (+newer), >> because 2.5 has the entries in initialization order and 2.6 has the >> entries in alphabetical order. To deal with that machine types for 2.5 >> & older keep the old sort order. This is the reason why >> legacy_fw_cfg_order exists. >> >> For new features and files you can completely ignore the whole legacy >> sorting mess. cross-version live migration works only for features >> supported by both qemu versions, therefore the legacy sorting is only >> relevant for features & files already supported by qemu 2.5. > > Thanks you Gerd for the whole explanation. I added an entry to > my TODO list to document this, based on your comment (and Corey's). Yes, please! Apparently, I've been confused by commit bab47d9a75a3 ("Sort the fw_cfg file list", 2016-04-07) before (in January 2018): http://mid.mail-archive.com/5367c8a4-91bd-7712-525d-0a1ed6e6acab@redhat.com (See in particular my question which I believe remains relevant: "is the idea that the same machine type on a new QEMU release may only reorder the additions of the preexistent fw_cfg files across the source code, but must not expose *new* fw_cfg files?" And I think Gerd just answered that above (in the positive), namely, "cross-version live migration works only for features supported by both qemu versions". So indeed we must not have a new fw_cfg file appear in an old machine type on a new QEMU release, without the user explicitly asking for it on the command line.) > I'll address it later, as you confirmed it doesn't impact this > series. That's my understanding too. Thanks for explaining, Gerd! Laszlo
diff --git a/softmmu/vl.c b/softmmu/vl.c index ae5451bc23..cdb1d187ed 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -489,6 +489,11 @@ static QemuOptsList qemu_fw_cfg_opts = { .name = "string", .type = QEMU_OPT_STRING, .help = "Sets content of the blob to be inserted from a string", + }, { + .name = "gen_id", + .type = QEMU_OPT_STRING, + .help = "Sets id of the object generating the fw_cfg blob " + "to be inserted", }, { /* end of list */ } }, @@ -2020,7 +2025,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) { gchar *buf; size_t size; - const char *name, *file, *str; + const char *name, *file, *str, *gen_id; FWCfgState *fw_cfg = (FWCfgState *) opaque; if (fw_cfg == NULL) { @@ -2030,14 +2035,13 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) name = qemu_opt_get(opts, "name"); file = qemu_opt_get(opts, "file"); str = qemu_opt_get(opts, "string"); + gen_id = qemu_opt_get(opts, "gen_id"); - /* we need name and either a file or the content string */ - if (!(nonempty_str(name) && (nonempty_str(file) || nonempty_str(str)))) { - error_setg(errp, "invalid argument(s)"); - return -1; - } - if (nonempty_str(file) && nonempty_str(str)) { - error_setg(errp, "file and string are mutually exclusive"); + /* we need the name, and exactly one of: file, content string, gen_id */ + if (!nonempty_str(name) || + nonempty_str(file) + nonempty_str(str) + nonempty_str(gen_id) != 1) { + error_setg(errp, "name, plus exactly one of file," + " string and gen_id, are needed"); return -1; } if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) { @@ -2052,6 +2056,8 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) if (nonempty_str(str)) { size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */ buf = g_memdup(str, size); + } else if (nonempty_str(gen_id)) { + return fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp); } else { GError *err = NULL; if (!g_file_get_contents(file, &buf, &size, &err)) {
The 'gen_id' argument refers to a QOM object able to produce data consumable by the fw_cfg device. The producer object must implement the FW_CFG_DATA_GENERATOR interface. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- v7: - renamed 'blob_id' -> 'gen_id' (danpb) - update comment in code (lersek) - fixed CODING_STYLE (lersek) - use Laszlo's if (SUM(options)) != 1 { error } form --- softmmu/vl.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)