[Qemu-devel] [PATCH V4 0/4] fw-cfg: cleanup and user-provided command line blobs

[Qemu-devel] [PATCH V4 0/4] fw-cfg: cleanup and user-provided command line blobs

[Gabriel L. Somlo]

These patches are what's left over from before the pre-2.3 code freeze,
after the documentation patch was applied independently:

    1/4:       (clean-up) disallow guest-side data writes to fw_cfg
    2/4 & 3/4: (clean-up) prevent selector and file name conflicts on insert
    4/4        (feature)  support insertion of fw_cfg files via qemu cmdline

Thanks,
  Gabriel

Gabriel L. Somlo (4):
  fw_cfg: remove support for guest-side data writes
  fw_cfg: prevent selector key conflict
  fw_cfg: prohibit insertion of duplicate fw_cfg file names
  fw_cfg: insert fw_cfg file blobs via qemu cmdline

 docs/specs/fw_cfg.txt     | 21 ++++++++++++++++
 hw/nvram/fw_cfg.c         | 45 ++++++---------------------------
 include/hw/nvram/fw_cfg.h |  2 --
 qemu-options.hx           | 11 +++++++++
 trace-events              |  2 --
 vl.c                      | 63 +++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 103 insertions(+), 41 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH V4 1/4] fw_cfg: remove support for guest-side data writes

[Gabriel L. Somlo]

>From this point forward, any guest-side writes to the fw_cfg
data register will be treated as no-ops. This patch also removes
the unused host-side API function fw_cfg_add_callback(), which
allowed the registration of a callback to be executed each time
the guest completed a full overwrite of a given fw_cfg data item.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/nvram/fw_cfg.c         | 33 +--------------------------------
 include/hw/nvram/fw_cfg.h |  2 --
 trace-events              |  1 -
 3 files changed, 1 insertion(+), 35 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 68eff77..ed70798 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -46,7 +46,6 @@ typedef struct FWCfgEntry {
     uint32_t len;
     uint8_t *data;
     void *callback_opaque;
-    FWCfgCallback callback;
     FWCfgReadCallback read_callback;
 } FWCfgEntry;
 
@@ -232,19 +231,7 @@ static void fw_cfg_reboot(FWCfgState *s)
 
 static void fw_cfg_write(FWCfgState *s, uint8_t value)
 {
-    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
-    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
-
-    trace_fw_cfg_write(s, value);
-
-    if (s->cur_entry & FW_CFG_WRITE_CHANNEL && e->callback &&
-        s->cur_offset < e->len) {
-        e->data[s->cur_offset++] = value;
-        if (s->cur_offset == e->len) {
-            e->callback(e->callback_opaque, e->data);
-            s->cur_offset = 0;
-        }
-    }
+    /* nothing, write support removed in QEMU v2.4+ */
 }
 
 static int fw_cfg_select(FWCfgState *s, uint16_t key)
@@ -458,7 +445,6 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
     s->entries[arch][key].data = data;
     s->entries[arch][key].len = len;
     s->entries[arch][key].callback_opaque = NULL;
-    s->entries[arch][key].callback = NULL;
 
     return ptr;
 }
@@ -502,23 +488,6 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
     fw_cfg_add_bytes(s, key, copy, sizeof(value));
 }
 
-void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
-                         void *callback_opaque, void *data, size_t len)
-{
-    int arch = !!(key & FW_CFG_ARCH_LOCAL);
-
-    assert(key & FW_CFG_WRITE_CHANNEL);
-
-    key &= FW_CFG_ENTRY_MASK;
-
-    assert(key < FW_CFG_MAX_ENTRY && len <= UINT32_MAX);
-
-    s->entries[arch][key].data = data;
-    s->entries[arch][key].len = (uint32_t)len;
-    s->entries[arch][key].callback_opaque = callback_opaque;
-    s->entries[arch][key].callback = callback;
-}
-
 void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
                               FWCfgReadCallback callback, void *callback_opaque,
                               void *data, size_t len)
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 6d8a8ac..b2e10c2 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -69,8 +69,6 @@ void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value);
 void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value);
 void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value);
 void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
-void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
-                         void *callback_opaque, void *data, size_t len);
 void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
                      size_t len);
 void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
diff --git a/trace-events b/trace-events
index 30eba92..1275b70 100644
--- a/trace-events
+++ b/trace-events
@@ -193,7 +193,6 @@ ecc_diag_mem_writeb(uint64_t addr, uint32_t val) "Write diagnostic %"PRId64" = %
 ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read diagnostic %"PRId64"= %02x"
 
 # hw/nvram/fw_cfg.c
-fw_cfg_write(void *s, uint8_t value) "%p %d"
 fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d"
 fw_cfg_read(void *s, uint8_t ret) "%p = %d"
 fw_cfg_add_file_dupe(void *s, char *name) "%p %s"
-- 
2.1.0

[Qemu-devel] [PATCH V4 2/4] fw_cfg: prevent selector key conflict

[Gabriel L. Somlo]

Enforce a single assignment of data for each distinct selector key.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/nvram/fw_cfg.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index ed70798..227beaf 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -423,6 +423,7 @@ static void fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
     key &= FW_CFG_ENTRY_MASK;
 
     assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
+    assert(s->entries[arch][key].data == NULL); /* avoid key conflict */
 
     s->entries[arch][key].data = data;
     s->entries[arch][key].len = (uint32_t)len;
-- 
2.1.0

[Qemu-devel] [PATCH V4 3/4] fw_cfg: prohibit insertion of duplicate fw_cfg file names

[Gabriel L. Somlo]

Exit with an error (instead of simply logging a trace event)
whenever the same fw_cfg file name is added multiple times via
one of the fw_cfg_add_file[_callback]() host-side API calls.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/nvram/fw_cfg.c | 11 ++++++-----
 trace-events      |  1 -
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 227beaf..8d4ea25 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -505,18 +505,19 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
     index = be32_to_cpu(s->files->count);
     assert(index < FW_CFG_FILE_SLOTS);
 
-    fw_cfg_add_bytes_read_callback(s, FW_CFG_FILE_FIRST + index,
-                                   callback, callback_opaque, data, len);
-
     pstrcpy(s->files->f[index].name, sizeof(s->files->f[index].name),
             filename);
     for (i = 0; i < index; i++) {
         if (strcmp(s->files->f[index].name, s->files->f[i].name) == 0) {
-            trace_fw_cfg_add_file_dupe(s, s->files->f[index].name);
-            return;
+            error_report("duplicate fw_cfg file name: %s",
+                         s->files->f[index].name);
+            exit(1);
         }
     }
 
+    fw_cfg_add_bytes_read_callback(s, FW_CFG_FILE_FIRST + index,
+                                   callback, callback_opaque, data, len);
+
     s->files->f[index].size   = cpu_to_be32(len);
     s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index);
     trace_fw_cfg_add_file(s, index, s->files->f[index].name, len);
diff --git a/trace-events b/trace-events
index 1275b70..a340c5a 100644
--- a/trace-events
+++ b/trace-events
@@ -195,7 +195,6 @@ ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read diagnostic %"PRId64"= %02x
 # hw/nvram/fw_cfg.c
 fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d"
 fw_cfg_read(void *s, uint8_t ret) "%p = %d"
-fw_cfg_add_file_dupe(void *s, char *name) "%p %s"
 fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)"
 
 # hw/block/hd-geometry.c
-- 
2.1.0

[Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Gabriel L. Somlo]

Allow user supplied files to be inserted into the fw_cfg
device before starting the guest. Since fw_cfg_add_file()
already disallows duplicate fw_cfg file names, qemu will
exit with an error message if the user supplies multiple
blobs with the same fw_cfg file name, or if a blob name
collides with a fw_cfg name programmatically added from
within the QEMU source code. A warning message will be
printed if the fw_cfg item name does not begin with the
prefix "opt/", which is recommended for external, user
provided blobs.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 docs/specs/fw_cfg.txt | 21 +++++++++++++++++
 qemu-options.hx       | 11 +++++++++
 vl.c                  | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 95 insertions(+)

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index 6accd92..74351dd 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -203,3 +203,24 @@ completes fully overwriting the item's data.
 
 NOTE: This function is deprecated, and will be completely removed
 starting with QEMU v2.4.
+
+== Externally Provided Items ==
+
+As of v2.4, "file" fw_cfg items (i.e., items with selector keys above
+FW_CFG_FILE_FIRST, and with a corresponding entry in the fw_cfg file
+directory structure) may be inserted via the QEMU command line, using
+the following syntax:
+
+    -fw_cfg [name=]<item_name>,file=<path>
+
+where <item_name> is the fw_cfg item name, and <path> is the location
+on the host file system of a file containing the data to be inserted.
+
+NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/"
+when using the "-fw_cfg" command line option, to avoid conflicting with
+item names used internally by QEMU. For instance:
+
+    -fw_cfg name=opt/my_item_name,file=./my_blob.bin
+
+Similarly, QEMU developers *SHOULD NOT* use item names prefixed with
+"opt/" when inserting items programmatically, e.g. via fw_cfg_add_file().
diff --git a/qemu-options.hx b/qemu-options.hx
index 319d971..aa386b4 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2668,6 +2668,17 @@ STEXI
 @table @option
 ETEXI
 
+DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg,
+    "-fw_cfg [name=]<name>,file=<file>\n"
+    "                add named fw_cfg entry from file\n",
+    QEMU_ARCH_ALL)
+STEXI
+@item -fw_cfg [name=]@var{name},file=@var{file}
+@findex -fw_cfg
+Add named fw_cfg entry from file. @var{name} determines the name of
+the entry in the fw_cfg file directory exposed to the guest.
+ETEXI
+
 DEF("serial", HAS_ARG, QEMU_OPTION_serial, \
     "-serial dev     redirect the serial port to char device 'dev'\n",
     QEMU_ARCH_ALL)
diff --git a/vl.c b/vl.c
index 74c2681..b02b2d4 100644
--- a/vl.c
+++ b/vl.c
@@ -490,6 +490,25 @@ static QemuOptsList qemu_semihosting_config_opts = {
     },
 };
 
+static QemuOptsList qemu_fw_cfg_opts = {
+    .name = "fw_cfg",
+    .implied_opt_name = "name",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_fw_cfg_opts.head),
+    .desc = {
+        {
+            .name = "name",
+            .type = QEMU_OPT_STRING,
+            .help = "Sets the fw_cfg name of the blob to be inserted",
+        }, {
+            .name = "file",
+            .type = QEMU_OPT_STRING,
+            .help = "Sets the name of the file from which\n"
+                    "the fw_cfg blob will be loaded",
+        },
+        { /* end of list */ }
+    },
+};
+
 /**
  * Get machine options
  *
@@ -2118,6 +2137,38 @@ char *qemu_find_file(int type, const char *name)
     return NULL;
 }
 
+static int parse_fw_cfg(QemuOpts *opts, void *opaque)
+{
+    gchar *buf;
+    size_t size;
+    const char *name, *file;
+
+    if (opaque == NULL) {
+        error_report("fw_cfg device not available");
+        return -1;
+    }
+    name = qemu_opt_get(opts, "name");
+    file = qemu_opt_get(opts, "file");
+    if (name == NULL || *name == '\0' || file == NULL || *file == '\0') {
+        error_report("invalid argument value");
+        return -1;
+    }
+    if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
+        error_report("name too long (max. %d char)", FW_CFG_MAX_FILE_PATH - 1);
+        return -1;
+    }
+    if (strncmp(name, "opt/", 4) != 0) {
+        error_report("WARNING: externally provided fw_cfg item names "
+                     "should be prefixed with \"opt/\"!");
+    }
+    if (!g_file_get_contents(file, &buf, &size, NULL)) {
+        error_report("can't load %s", file);
+        return -1;
+    }
+    fw_cfg_add_file((FWCfgState *)opaque, name, buf, size);
+    return 0;
+}
+
 static int device_help_func(QemuOpts *opts, void *opaque)
 {
     return qdev_device_help(opts);
@@ -2806,6 +2857,7 @@ int main(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_numa_opts);
     qemu_add_opts(&qemu_icount_opts);
     qemu_add_opts(&qemu_semihosting_config_opts);
+    qemu_add_opts(&qemu_fw_cfg_opts);
 
     runstate_init();
 
@@ -3422,6 +3474,12 @@ int main(int argc, char **argv, char **envp)
                 }
                 do_smbios_option(opts);
                 break;
+            case QEMU_OPTION_fwcfg:
+                opts = qemu_opts_parse(qemu_find_opts("fw_cfg"), optarg, 1);
+                if (opts == NULL) {
+                    exit(1);
+                }
+                break;
             case QEMU_OPTION_enable_kvm:
                 olist = qemu_find_opts("machine");
                 qemu_opts_parse(olist, "accel=kvm", 0);
@@ -4233,6 +4291,11 @@ int main(int argc, char **argv, char **envp)
 
     numa_post_machine_init();
 
+    if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
+                          parse_fw_cfg, fw_cfg_find(), 1) != 0) {
+        exit(1);
+    }
+
     /* init USB devices */
     if (usb_enabled()) {
         if (foreach_device_config(DEV_USB, usb_parse) < 0)
-- 
2.1.0

Re: [Qemu-devel] [PATCH V4 0/4] fw-cfg: cleanup and user-provided command line blobs

[Gabriel L. Somlo]

Ping?

On Wed, Apr 29, 2015 at 11:21:49AM -0400, Gabriel L. Somlo wrote:
> These patches are what's left over from before the pre-2.3 code freeze,
> after the documentation patch was applied independently:
> 
>     1/4:       (clean-up) disallow guest-side data writes to fw_cfg
>     2/4 & 3/4: (clean-up) prevent selector and file name conflicts on insert
>     4/4        (feature)  support insertion of fw_cfg files via qemu cmdline
> 
> Thanks,
>   Gabriel
> 
> Gabriel L. Somlo (4):
>   fw_cfg: remove support for guest-side data writes
>   fw_cfg: prevent selector key conflict
>   fw_cfg: prohibit insertion of duplicate fw_cfg file names
>   fw_cfg: insert fw_cfg file blobs via qemu cmdline
> 
>  docs/specs/fw_cfg.txt     | 21 ++++++++++++++++
>  hw/nvram/fw_cfg.c         | 45 ++++++---------------------------
>  include/hw/nvram/fw_cfg.h |  2 --
>  qemu-options.hx           | 11 +++++++++
>  trace-events              |  2 --
>  vl.c                      | 63 +++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 103 insertions(+), 41 deletions(-)
> 
> -- 
> 2.1.0
> 

Re: [Qemu-devel] [PATCH V4 0/4] fw-cfg: cleanup and user-provided command line blobs

[Gerd Hoffmann]

On Mo, 2015-05-18 at 09:05 -0400, Gabriel L. Somlo wrote:
> Ping?

Added to fw_cfg queue.

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Michael S. Tsirkin]

On Wed, Apr 29, 2015 at 11:21:53AM -0400, Gabriel L. Somlo wrote:
> Allow user supplied files to be inserted into the fw_cfg
> device before starting the guest. Since fw_cfg_add_file()
> already disallows duplicate fw_cfg file names, qemu will
> exit with an error message if the user supplies multiple
> blobs with the same fw_cfg file name, or if a blob name
> collides with a fw_cfg name programmatically added from
> within the QEMU source code. A warning message will be
> printed if the fw_cfg item name does not begin with the
> prefix "opt/", which is recommended for external, user
> provided blobs.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Would it make sense to make an illegal prefix a hard failure
instead?
If we don't, we'll be unable to add new file names without
fear of conflicts in the future.

> ---
>  docs/specs/fw_cfg.txt | 21 +++++++++++++++++
>  qemu-options.hx       | 11 +++++++++
>  vl.c                  | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 95 insertions(+)
> 
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index 6accd92..74351dd 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -203,3 +203,24 @@ completes fully overwriting the item's data.
>  
>  NOTE: This function is deprecated, and will be completely removed
>  starting with QEMU v2.4.
> +
> +== Externally Provided Items ==
> +
> +As of v2.4, "file" fw_cfg items (i.e., items with selector keys above
> +FW_CFG_FILE_FIRST, and with a corresponding entry in the fw_cfg file
> +directory structure) may be inserted via the QEMU command line, using
> +the following syntax:
> +
> +    -fw_cfg [name=]<item_name>,file=<path>
> +
> +where <item_name> is the fw_cfg item name, and <path> is the location
> +on the host file system of a file containing the data to be inserted.
> +
> +NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/"
> +when using the "-fw_cfg" command line option, to avoid conflicting with
> +item names used internally by QEMU. For instance:
> +
> +    -fw_cfg name=opt/my_item_name,file=./my_blob.bin
> +
> +Similarly, QEMU developers *SHOULD NOT* use item names prefixed with
> +"opt/" when inserting items programmatically, e.g. via fw_cfg_add_file().
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 319d971..aa386b4 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2668,6 +2668,17 @@ STEXI
>  @table @option
>  ETEXI
>  
> +DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg,
> +    "-fw_cfg [name=]<name>,file=<file>\n"
> +    "                add named fw_cfg entry from file\n",
> +    QEMU_ARCH_ALL)
> +STEXI
> +@item -fw_cfg [name=]@var{name},file=@var{file}
> +@findex -fw_cfg
> +Add named fw_cfg entry from file. @var{name} determines the name of
> +the entry in the fw_cfg file directory exposed to the guest.
> +ETEXI
> +
>  DEF("serial", HAS_ARG, QEMU_OPTION_serial, \
>      "-serial dev     redirect the serial port to char device 'dev'\n",
>      QEMU_ARCH_ALL)
> diff --git a/vl.c b/vl.c
> index 74c2681..b02b2d4 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -490,6 +490,25 @@ static QemuOptsList qemu_semihosting_config_opts = {
>      },
>  };
>  
> +static QemuOptsList qemu_fw_cfg_opts = {
> +    .name = "fw_cfg",
> +    .implied_opt_name = "name",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_fw_cfg_opts.head),
> +    .desc = {
> +        {
> +            .name = "name",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Sets the fw_cfg name of the blob to be inserted",
> +        }, {
> +            .name = "file",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Sets the name of the file from which\n"
> +                    "the fw_cfg blob will be loaded",
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>  /**
>   * Get machine options
>   *
> @@ -2118,6 +2137,38 @@ char *qemu_find_file(int type, const char *name)
>      return NULL;
>  }
>  
> +static int parse_fw_cfg(QemuOpts *opts, void *opaque)
> +{
> +    gchar *buf;
> +    size_t size;
> +    const char *name, *file;
> +
> +    if (opaque == NULL) {
> +        error_report("fw_cfg device not available");
> +        return -1;
> +    }
> +    name = qemu_opt_get(opts, "name");
> +    file = qemu_opt_get(opts, "file");
> +    if (name == NULL || *name == '\0' || file == NULL || *file == '\0') {
> +        error_report("invalid argument value");
> +        return -1;
> +    }
> +    if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
> +        error_report("name too long (max. %d char)", FW_CFG_MAX_FILE_PATH - 1);
> +        return -1;
> +    }
> +    if (strncmp(name, "opt/", 4) != 0) {
> +        error_report("WARNING: externally provided fw_cfg item names "
> +                     "should be prefixed with \"opt/\"!");
> +    }
> +    if (!g_file_get_contents(file, &buf, &size, NULL)) {
> +        error_report("can't load %s", file);
> +        return -1;
> +    }
> +    fw_cfg_add_file((FWCfgState *)opaque, name, buf, size);
> +    return 0;
> +}
> +
>  static int device_help_func(QemuOpts *opts, void *opaque)
>  {
>      return qdev_device_help(opts);
> @@ -2806,6 +2857,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_add_opts(&qemu_numa_opts);
>      qemu_add_opts(&qemu_icount_opts);
>      qemu_add_opts(&qemu_semihosting_config_opts);
> +    qemu_add_opts(&qemu_fw_cfg_opts);
>  
>      runstate_init();
>  
> @@ -3422,6 +3474,12 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  do_smbios_option(opts);
>                  break;
> +            case QEMU_OPTION_fwcfg:
> +                opts = qemu_opts_parse(qemu_find_opts("fw_cfg"), optarg, 1);
> +                if (opts == NULL) {
> +                    exit(1);
> +                }
> +                break;
>              case QEMU_OPTION_enable_kvm:
>                  olist = qemu_find_opts("machine");
>                  qemu_opts_parse(olist, "accel=kvm", 0);
> @@ -4233,6 +4291,11 @@ int main(int argc, char **argv, char **envp)
>  
>      numa_post_machine_init();
>  
> +    if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
> +                          parse_fw_cfg, fw_cfg_find(), 1) != 0) {
> +        exit(1);
> +    }
> +
>      /* init USB devices */
>      if (usb_enabled()) {
>          if (foreach_device_config(DEV_USB, usb_parse) < 0)
> -- 
> 2.1.0
> 

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Laszlo Ersek]

On 05/31/15 20:10, Michael S. Tsirkin wrote:
> On Wed, Apr 29, 2015 at 11:21:53AM -0400, Gabriel L. Somlo wrote:
>> Allow user supplied files to be inserted into the fw_cfg
>> device before starting the guest. Since fw_cfg_add_file()
>> already disallows duplicate fw_cfg file names, qemu will
>> exit with an error message if the user supplies multiple
>> blobs with the same fw_cfg file name, or if a blob name
>> collides with a fw_cfg name programmatically added from
>> within the QEMU source code. A warning message will be
>> printed if the fw_cfg item name does not begin with the
>> prefix "opt/", which is recommended for external, user
>> provided blobs.
>>
>> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Would it make sense to make an illegal prefix a hard failure
> instead?
> If we don't, we'll be unable to add new file names without
> fear of conflicts in the future.

We discussed this earlier. The idea was "mechanism, not policy", and
that if a user violates the convention (not rule) / ignores the warning
at some point, he's on his own when it breaks in the next version.

I don't feel overly strongly about it; just "mechanism, not policy"
looks like a good tradition (well, good excuse anyway).

Thanks
Laszlo

>> ---
>>  docs/specs/fw_cfg.txt | 21 +++++++++++++++++
>>  qemu-options.hx       | 11 +++++++++
>>  vl.c                  | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 95 insertions(+)
>>
>> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
>> index 6accd92..74351dd 100644
>> --- a/docs/specs/fw_cfg.txt
>> +++ b/docs/specs/fw_cfg.txt
>> @@ -203,3 +203,24 @@ completes fully overwriting the item's data.
>>  
>>  NOTE: This function is deprecated, and will be completely removed
>>  starting with QEMU v2.4.
>> +
>> +== Externally Provided Items ==
>> +
>> +As of v2.4, "file" fw_cfg items (i.e., items with selector keys above
>> +FW_CFG_FILE_FIRST, and with a corresponding entry in the fw_cfg file
>> +directory structure) may be inserted via the QEMU command line, using
>> +the following syntax:
>> +
>> +    -fw_cfg [name=]<item_name>,file=<path>
>> +
>> +where <item_name> is the fw_cfg item name, and <path> is the location
>> +on the host file system of a file containing the data to be inserted.
>> +
>> +NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/"
>> +when using the "-fw_cfg" command line option, to avoid conflicting with
>> +item names used internally by QEMU. For instance:
>> +
>> +    -fw_cfg name=opt/my_item_name,file=./my_blob.bin
>> +
>> +Similarly, QEMU developers *SHOULD NOT* use item names prefixed with
>> +"opt/" when inserting items programmatically, e.g. via fw_cfg_add_file().
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 319d971..aa386b4 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -2668,6 +2668,17 @@ STEXI
>>  @table @option
>>  ETEXI
>>  
>> +DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg,
>> +    "-fw_cfg [name=]<name>,file=<file>\n"
>> +    "                add named fw_cfg entry from file\n",
>> +    QEMU_ARCH_ALL)
>> +STEXI
>> +@item -fw_cfg [name=]@var{name},file=@var{file}
>> +@findex -fw_cfg
>> +Add named fw_cfg entry from file. @var{name} determines the name of
>> +the entry in the fw_cfg file directory exposed to the guest.
>> +ETEXI
>> +
>>  DEF("serial", HAS_ARG, QEMU_OPTION_serial, \
>>      "-serial dev     redirect the serial port to char device 'dev'\n",
>>      QEMU_ARCH_ALL)
>> diff --git a/vl.c b/vl.c
>> index 74c2681..b02b2d4 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -490,6 +490,25 @@ static QemuOptsList qemu_semihosting_config_opts = {
>>      },
>>  };
>>  
>> +static QemuOptsList qemu_fw_cfg_opts = {
>> +    .name = "fw_cfg",
>> +    .implied_opt_name = "name",
>> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_fw_cfg_opts.head),
>> +    .desc = {
>> +        {
>> +            .name = "name",
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "Sets the fw_cfg name of the blob to be inserted",
>> +        }, {
>> +            .name = "file",
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "Sets the name of the file from which\n"
>> +                    "the fw_cfg blob will be loaded",
>> +        },
>> +        { /* end of list */ }
>> +    },
>> +};
>> +
>>  /**
>>   * Get machine options
>>   *
>> @@ -2118,6 +2137,38 @@ char *qemu_find_file(int type, const char *name)
>>      return NULL;
>>  }
>>  
>> +static int parse_fw_cfg(QemuOpts *opts, void *opaque)
>> +{
>> +    gchar *buf;
>> +    size_t size;
>> +    const char *name, *file;
>> +
>> +    if (opaque == NULL) {
>> +        error_report("fw_cfg device not available");
>> +        return -1;
>> +    }
>> +    name = qemu_opt_get(opts, "name");
>> +    file = qemu_opt_get(opts, "file");
>> +    if (name == NULL || *name == '\0' || file == NULL || *file == '\0') {
>> +        error_report("invalid argument value");
>> +        return -1;
>> +    }
>> +    if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
>> +        error_report("name too long (max. %d char)", FW_CFG_MAX_FILE_PATH - 1);
>> +        return -1;
>> +    }
>> +    if (strncmp(name, "opt/", 4) != 0) {
>> +        error_report("WARNING: externally provided fw_cfg item names "
>> +                     "should be prefixed with \"opt/\"!");
>> +    }
>> +    if (!g_file_get_contents(file, &buf, &size, NULL)) {
>> +        error_report("can't load %s", file);
>> +        return -1;
>> +    }
>> +    fw_cfg_add_file((FWCfgState *)opaque, name, buf, size);
>> +    return 0;
>> +}
>> +
>>  static int device_help_func(QemuOpts *opts, void *opaque)
>>  {
>>      return qdev_device_help(opts);
>> @@ -2806,6 +2857,7 @@ int main(int argc, char **argv, char **envp)
>>      qemu_add_opts(&qemu_numa_opts);
>>      qemu_add_opts(&qemu_icount_opts);
>>      qemu_add_opts(&qemu_semihosting_config_opts);
>> +    qemu_add_opts(&qemu_fw_cfg_opts);
>>  
>>      runstate_init();
>>  
>> @@ -3422,6 +3474,12 @@ int main(int argc, char **argv, char **envp)
>>                  }
>>                  do_smbios_option(opts);
>>                  break;
>> +            case QEMU_OPTION_fwcfg:
>> +                opts = qemu_opts_parse(qemu_find_opts("fw_cfg"), optarg, 1);
>> +                if (opts == NULL) {
>> +                    exit(1);
>> +                }
>> +                break;
>>              case QEMU_OPTION_enable_kvm:
>>                  olist = qemu_find_opts("machine");
>>                  qemu_opts_parse(olist, "accel=kvm", 0);
>> @@ -4233,6 +4291,11 @@ int main(int argc, char **argv, char **envp)
>>  
>>      numa_post_machine_init();
>>  
>> +    if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
>> +                          parse_fw_cfg, fw_cfg_find(), 1) != 0) {
>> +        exit(1);
>> +    }
>> +
>>      /* init USB devices */
>>      if (usb_enabled()) {
>>          if (foreach_device_config(DEV_USB, usb_parse) < 0)
>> -- 
>> 2.1.0
>>

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Michael S. Tsirkin]

On Mon, Jun 01, 2015 at 09:06:19AM +0200, Laszlo Ersek wrote:
> On 05/31/15 20:10, Michael S. Tsirkin wrote:
> > On Wed, Apr 29, 2015 at 11:21:53AM -0400, Gabriel L. Somlo wrote:
> >> Allow user supplied files to be inserted into the fw_cfg
> >> device before starting the guest. Since fw_cfg_add_file()
> >> already disallows duplicate fw_cfg file names, qemu will
> >> exit with an error message if the user supplies multiple
> >> blobs with the same fw_cfg file name, or if a blob name
> >> collides with a fw_cfg name programmatically added from
> >> within the QEMU source code. A warning message will be
> >> printed if the fw_cfg item name does not begin with the
> >> prefix "opt/", which is recommended for external, user
> >> provided blobs.
> >>
> >> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > 
> > Would it make sense to make an illegal prefix a hard failure
> > instead?
> > If we don't, we'll be unable to add new file names without
> > fear of conflicts in the future.
> 
> We discussed this earlier. The idea was "mechanism, not policy", and
> that if a user violates the convention (not rule) / ignores the warning
> at some point, he's on his own when it breaks in the next version.
> 
> I don't feel overly strongly about it; just "mechanism, not policy"
> looks like a good tradition (well, good excuse anyway).
> 
> Thanks
> Laszlo

Most users never see warnings. We ship it, we support it.
If we don't want to support it, let's not ship it.

> >> ---
> >>  docs/specs/fw_cfg.txt | 21 +++++++++++++++++
> >>  qemu-options.hx       | 11 +++++++++
> >>  vl.c                  | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 95 insertions(+)
> >>
> >> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> >> index 6accd92..74351dd 100644
> >> --- a/docs/specs/fw_cfg.txt
> >> +++ b/docs/specs/fw_cfg.txt
> >> @@ -203,3 +203,24 @@ completes fully overwriting the item's data.
> >>  
> >>  NOTE: This function is deprecated, and will be completely removed
> >>  starting with QEMU v2.4.
> >> +
> >> +== Externally Provided Items ==
> >> +
> >> +As of v2.4, "file" fw_cfg items (i.e., items with selector keys above
> >> +FW_CFG_FILE_FIRST, and with a corresponding entry in the fw_cfg file
> >> +directory structure) may be inserted via the QEMU command line, using
> >> +the following syntax:
> >> +
> >> +    -fw_cfg [name=]<item_name>,file=<path>
> >> +
> >> +where <item_name> is the fw_cfg item name, and <path> is the location
> >> +on the host file system of a file containing the data to be inserted.
> >> +
> >> +NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/"
> >> +when using the "-fw_cfg" command line option, to avoid conflicting with
> >> +item names used internally by QEMU. For instance:
> >> +
> >> +    -fw_cfg name=opt/my_item_name,file=./my_blob.bin
> >> +
> >> +Similarly, QEMU developers *SHOULD NOT* use item names prefixed with
> >> +"opt/" when inserting items programmatically, e.g. via fw_cfg_add_file().
> >> diff --git a/qemu-options.hx b/qemu-options.hx
> >> index 319d971..aa386b4 100644
> >> --- a/qemu-options.hx
> >> +++ b/qemu-options.hx
> >> @@ -2668,6 +2668,17 @@ STEXI
> >>  @table @option
> >>  ETEXI
> >>  
> >> +DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg,
> >> +    "-fw_cfg [name=]<name>,file=<file>\n"
> >> +    "                add named fw_cfg entry from file\n",
> >> +    QEMU_ARCH_ALL)
> >> +STEXI
> >> +@item -fw_cfg [name=]@var{name},file=@var{file}
> >> +@findex -fw_cfg
> >> +Add named fw_cfg entry from file. @var{name} determines the name of
> >> +the entry in the fw_cfg file directory exposed to the guest.
> >> +ETEXI
> >> +
> >>  DEF("serial", HAS_ARG, QEMU_OPTION_serial, \
> >>      "-serial dev     redirect the serial port to char device 'dev'\n",
> >>      QEMU_ARCH_ALL)
> >> diff --git a/vl.c b/vl.c
> >> index 74c2681..b02b2d4 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> >> @@ -490,6 +490,25 @@ static QemuOptsList qemu_semihosting_config_opts = {
> >>      },
> >>  };
> >>  
> >> +static QemuOptsList qemu_fw_cfg_opts = {
> >> +    .name = "fw_cfg",
> >> +    .implied_opt_name = "name",
> >> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_fw_cfg_opts.head),
> >> +    .desc = {
> >> +        {
> >> +            .name = "name",
> >> +            .type = QEMU_OPT_STRING,
> >> +            .help = "Sets the fw_cfg name of the blob to be inserted",
> >> +        }, {
> >> +            .name = "file",
> >> +            .type = QEMU_OPT_STRING,
> >> +            .help = "Sets the name of the file from which\n"
> >> +                    "the fw_cfg blob will be loaded",
> >> +        },
> >> +        { /* end of list */ }
> >> +    },
> >> +};
> >> +
> >>  /**
> >>   * Get machine options
> >>   *
> >> @@ -2118,6 +2137,38 @@ char *qemu_find_file(int type, const char *name)
> >>      return NULL;
> >>  }
> >>  
> >> +static int parse_fw_cfg(QemuOpts *opts, void *opaque)
> >> +{
> >> +    gchar *buf;
> >> +    size_t size;
> >> +    const char *name, *file;
> >> +
> >> +    if (opaque == NULL) {
> >> +        error_report("fw_cfg device not available");
> >> +        return -1;
> >> +    }
> >> +    name = qemu_opt_get(opts, "name");
> >> +    file = qemu_opt_get(opts, "file");
> >> +    if (name == NULL || *name == '\0' || file == NULL || *file == '\0') {
> >> +        error_report("invalid argument value");
> >> +        return -1;
> >> +    }
> >> +    if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
> >> +        error_report("name too long (max. %d char)", FW_CFG_MAX_FILE_PATH - 1);
> >> +        return -1;
> >> +    }
> >> +    if (strncmp(name, "opt/", 4) != 0) {
> >> +        error_report("WARNING: externally provided fw_cfg item names "
> >> +                     "should be prefixed with \"opt/\"!");
> >> +    }
> >> +    if (!g_file_get_contents(file, &buf, &size, NULL)) {
> >> +        error_report("can't load %s", file);
> >> +        return -1;
> >> +    }
> >> +    fw_cfg_add_file((FWCfgState *)opaque, name, buf, size);
> >> +    return 0;
> >> +}
> >> +
> >>  static int device_help_func(QemuOpts *opts, void *opaque)
> >>  {
> >>      return qdev_device_help(opts);
> >> @@ -2806,6 +2857,7 @@ int main(int argc, char **argv, char **envp)
> >>      qemu_add_opts(&qemu_numa_opts);
> >>      qemu_add_opts(&qemu_icount_opts);
> >>      qemu_add_opts(&qemu_semihosting_config_opts);
> >> +    qemu_add_opts(&qemu_fw_cfg_opts);
> >>  
> >>      runstate_init();
> >>  
> >> @@ -3422,6 +3474,12 @@ int main(int argc, char **argv, char **envp)
> >>                  }
> >>                  do_smbios_option(opts);
> >>                  break;
> >> +            case QEMU_OPTION_fwcfg:
> >> +                opts = qemu_opts_parse(qemu_find_opts("fw_cfg"), optarg, 1);
> >> +                if (opts == NULL) {
> >> +                    exit(1);
> >> +                }
> >> +                break;
> >>              case QEMU_OPTION_enable_kvm:
> >>                  olist = qemu_find_opts("machine");
> >>                  qemu_opts_parse(olist, "accel=kvm", 0);
> >> @@ -4233,6 +4291,11 @@ int main(int argc, char **argv, char **envp)
> >>  
> >>      numa_post_machine_init();
> >>  
> >> +    if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
> >> +                          parse_fw_cfg, fw_cfg_find(), 1) != 0) {
> >> +        exit(1);
> >> +    }
> >> +
> >>      /* init USB devices */
> >>      if (usb_enabled()) {
> >>          if (foreach_device_config(DEV_USB, usb_parse) < 0)
> >> -- 
> >> 2.1.0
> >>

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Paolo Bonzini]

On 01/06/2015 09:28, Michael S. Tsirkin wrote:
> > I don't feel overly strongly about it; just "mechanism, not policy"
> > looks like a good tradition (well, good excuse anyway).
> 
> Most users never see warnings. We ship it, we support it.
> If we don't want to support it, let's not ship it.

Then we should rm -rf half of QEMU. :)

Seriously, I agree wholeheartedly with not baking policy into QEMU.  A
lot of QEMU command-line hacking really is just a shortcut to avoid
continuous recompilation.  I don't think it's reasonable to expect that
it constitutes a stable API.

Paolo

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Michael S. Tsirkin]

On Mon, Jun 01, 2015 at 11:01:23AM +0200, Paolo Bonzini wrote:
> 
> 
> On 01/06/2015 09:28, Michael S. Tsirkin wrote:
> > > I don't feel overly strongly about it; just "mechanism, not policy"
> > > looks like a good tradition (well, good excuse anyway).
> > 
> > Most users never see warnings. We ship it, we support it.
> > If we don't want to support it, let's not ship it.
> 
> Then we should rm -rf half of QEMU. :)
> 
> Seriously, I agree wholeheartedly with not baking policy into QEMU.  A
> lot of QEMU command-line hacking really is just a shortcut to avoid
> continuous recompilation.  I don't think it's reasonable to expect that
> it constitutes a stable API.
> 
> Paolo

Still, reserving part of the namespace for QEMU internal use
is *not* policy, it's just good engineering.

How about we forbid adding files under "etc/" ?

That would be enough to avoid conflicts.


-- 
MST

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Laszlo Ersek]

On 06/01/15 12:23, Michael S. Tsirkin wrote:
> On Mon, Jun 01, 2015 at 11:01:23AM +0200, Paolo Bonzini wrote:
>>
>>
>> On 01/06/2015 09:28, Michael S. Tsirkin wrote:
>>>> I don't feel overly strongly about it; just "mechanism, not policy"
>>>> looks like a good tradition (well, good excuse anyway).
>>>
>>> Most users never see warnings. We ship it, we support it.
>>> If we don't want to support it, let's not ship it.
>>
>> Then we should rm -rf half of QEMU. :)
>>
>> Seriously, I agree wholeheartedly with not baking policy into QEMU.  A
>> lot of QEMU command-line hacking really is just a shortcut to avoid
>> continuous recompilation.  I don't think it's reasonable to expect that
>> it constitutes a stable API.
>>
>> Paolo
> 
> Still, reserving part of the namespace for QEMU internal use
> is *not* policy, it's just good engineering.
> 
> How about we forbid adding files under "etc/" ?
> 
> That would be enough to avoid conflicts.

Some of the current fw_cfg files, like "bootorder", are not under
"etc/". Hence the earlier proposal to restrict the user (to under opt/,
IIRC), rather than ourselves.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Michael S. Tsirkin]

On Mon, Jun 01, 2015 at 12:35:34PM +0200, Laszlo Ersek wrote:
> On 06/01/15 12:23, Michael S. Tsirkin wrote:
> > On Mon, Jun 01, 2015 at 11:01:23AM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 01/06/2015 09:28, Michael S. Tsirkin wrote:
> >>>> I don't feel overly strongly about it; just "mechanism, not policy"
> >>>> looks like a good tradition (well, good excuse anyway).
> >>>
> >>> Most users never see warnings. We ship it, we support it.
> >>> If we don't want to support it, let's not ship it.
> >>
> >> Then we should rm -rf half of QEMU. :)
> >>
> >> Seriously, I agree wholeheartedly with not baking policy into QEMU.  A
> >> lot of QEMU command-line hacking really is just a shortcut to avoid
> >> continuous recompilation.  I don't think it's reasonable to expect that
> >> it constitutes a stable API.
> >>
> >> Paolo
> > 
> > Still, reserving part of the namespace for QEMU internal use
> > is *not* policy, it's just good engineering.
> > 
> > How about we forbid adding files under "etc/" ?
> > 
> > That would be enough to avoid conflicts.
> 
> Some of the current fw_cfg files, like "bootorder", are not under
> "etc/".

Well bootorder is there so at least it will always fail.
We do have stuff under /rom.

> Hence the earlier proposal to restrict the user (to under opt/,
> IIRC), rather than ourselves.
> 
> Thanks
> Laszlo

How about we pre-pend opt/ to user-supplied names?
Will fix this without limiting user in any way.

-- 
MST

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Paolo Bonzini]

On 01/06/2015 12:23, Michael S. Tsirkin wrote:
> Still, reserving part of the namespace for QEMU internal use
> is *not* policy, it's just good engineering.
> 
> How about we forbid adding files under "etc/" ?
> 
> That would be enough to avoid conflicts.

I do not understand.  What we're doing is free-beer.  We can always say
no.  What's your worry?

One usecase of this feature is to avoid recompiling QEMU while playing
with firmware.  If you cannot mimic QEMU's behavior (which is to add
"etc/" files), the feature is pointless, or at least I totally cannot
understand its purpose and I'm against merging it.

Paolo

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Michael S. Tsirkin]

On Mon, Jun 01, 2015 at 12:43:35PM +0200, Paolo Bonzini wrote:
> 
> 
> On 01/06/2015 12:23, Michael S. Tsirkin wrote:
> > Still, reserving part of the namespace for QEMU internal use
> > is *not* policy, it's just good engineering.
> > 
> > How about we forbid adding files under "etc/" ?
> > 
> > That would be enough to avoid conflicts.
> 
> I do not understand.  What we're doing is free-beer.  We can always say
> no.  What's your worry?

Someone writes a tool using a specific path.
We then add same path upstream, script breaks.

> One usecase of this feature is to avoid recompiling QEMU while playing
> with firmware.  If you cannot mimic QEMU's behavior (which is to add
> "etc/" files), the feature is pointless, or at least I totally cannot
> understand its purpose and I'm against merging it.
> 
> Paolo

Confused.  Why does it produce the warning then?

If it's just for playing games, add a configure
switch to enable it, and disable by default.
Don't set traps for users.

-- 
MST

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Paolo Bonzini]

On 01/06/2015 12:48, Michael S. Tsirkin wrote:
> On Mon, Jun 01, 2015 at 12:43:35PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 01/06/2015 12:23, Michael S. Tsirkin wrote:
>>> Still, reserving part of the namespace for QEMU internal use
>>> is *not* policy, it's just good engineering.
>>>
>>> How about we forbid adding files under "etc/" ?
>>>
>>> That would be enough to avoid conflicts.
>>
>> I do not understand.  What we're doing is free-beer.  We can always say
>> no.  What's your worry?
> 
> Someone writes a tool using a specific path.
> We then add same path upstream, script breaks.

Who cares.  We documented it.

>> One usecase of this feature is to avoid recompiling QEMU while playing
>> with firmware.  If you cannot mimic QEMU's behavior (which is to add
>> "etc/" files), the feature is pointless, or at least I totally cannot
>> understand its purpose and I'm against merging it.
> 
> Confused.  Why does it produce the warning then?

Because someone else asked for it.  I cannot answer. :)

> If it's just for playing games, add a configure
> switch to enable it, and disable by default.
> Don't set traps for users.

What is for playing games?  What is the feature useful for, except for
developers.

Paolo

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Michael S. Tsirkin]

On Mon, Jun 01, 2015 at 12:50:50PM +0200, Paolo Bonzini wrote:
> 
> 
> On 01/06/2015 12:48, Michael S. Tsirkin wrote:
> > On Mon, Jun 01, 2015 at 12:43:35PM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 01/06/2015 12:23, Michael S. Tsirkin wrote:
> >>> Still, reserving part of the namespace for QEMU internal use
> >>> is *not* policy, it's just good engineering.
> >>>
> >>> How about we forbid adding files under "etc/" ?
> >>>
> >>> That would be enough to avoid conflicts.
> >>
> >> I do not understand.  What we're doing is free-beer.  We can always say
> >> no.  What's your worry?
> > 
> > Someone writes a tool using a specific path.
> > We then add same path upstream, script breaks.
> 
> Who cares.  We documented it.
> 
> >> One usecase of this feature is to avoid recompiling QEMU while playing
> >> with firmware.  If you cannot mimic QEMU's behavior (which is to add
> >> "etc/" files), the feature is pointless, or at least I totally cannot
> >> understand its purpose and I'm against merging it.
> > 
> > Confused.  Why does it produce the warning then?
> 
> Because someone else asked for it.  I cannot answer. :)
> 
> > If it's just for playing games, add a configure
> > switch to enable it, and disable by default.
> > Don't set traps for users.
> 
> What is for playing games?  What is the feature useful for, except for
> developers.
> 
> Paolo


OK so if it's a dveloper feature, I think a config flag
to hide it from users is a good idea?

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Gabriel L. Somlo]

On Mon, Jun 01, 2015 at 12:50:50PM +0200, Paolo Bonzini wrote:
> > Someone writes a tool using a specific path.
> > We then add same path upstream, script breaks.
> 
> Who cares.  We documented it.
> 
> >> One usecase of this feature is to avoid recompiling QEMU while playing
> >> with firmware.  If you cannot mimic QEMU's behavior (which is to add
> >> "etc/" files), the feature is pointless, or at least I totally cannot
> >> understand its purpose and I'm against merging it.
> > 
> > Confused.  Why does it produce the warning then?
> 
> Because someone else asked for it.  I cannot answer. :)
> 
> > If it's just for playing games, add a configure
> > switch to enable it, and disable by default.
> > Don't set traps for users.
> 
> What is for playing games?  What is the feature useful for, except for
> developers.

My interest in this is for its asynchronous, agent-less host->guest
communication capability. As such, I would like it to be available at
all times, not just in special builds after enabling a build-time
switch...

As for the (meta)policy aspect (i.e., warn vs. error-out when outside
"opt/"), I am happy to go with whatever consensus emerges from this
conversation. I get both sides' positions, but don't have enough
context to feel strongly either way myself... :)

Thanks much,
--Gabriel

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Paolo Bonzini]

On 01/06/2015 13:00, Michael S. Tsirkin wrote:
> > > If it's just for playing games, add a configure
> > > switch to enable it, and disable by default.
> > > Don't set traps for users.
> > 
> > What is for playing games?  What is the feature useful for, except for
> > developers.
> 
> OK so if it's a dveloper feature, I think a config flag
> to hide it from users is a good idea?

No, please, no antifeatures.

A config flag just means bitrot.  We should _remove_ them (candidates:
--enable-debug, --enable-virtfs, --enable-fdt,
--enable-{linux,bsd}-user, --enable-guest-base, --enable-pie,
--enable-coroutine-pool, --enable-gcov, --enable-quorum, --enable-vhdx,
--enable-vhost-net, --enable-sparse), not add more.

Pretty much the only justification for a --enable-* configure option is
"I don't want my binary to have a dependency on an external library".
More often than not, any other justification probably boils down to a
wrong assumption such as:

- another option is not powerful enough (e.g. --target-list doesn't
support wildcards, hence --disable-system, --disable-users, etc.)

- QEMU's configure wants to do something different from autoconf, the
outcome invariably being bug reports (e.g. stripping debug info by
default, and for a long time falling back to -O0 if you asked not to do
that)

- reviewers didn't ask themselves if other knobs already covered this
(--enable-vhdx)

- people are worried on breaking weird platforms, but you won't ever
know if it breaks unless you dare doing the change (so we have to
maintain old code: --enable-pie)

- we don't want mandatory build-time dependencies that everyone has on
their system anyway (--enable-docs)

- no one really understands what the option does (--disable-guest-base)

- sometimes reasoning about attack surface applies (--disable-kvm), but
then one wonders why we still have no --disable-tcg

- concerns about performance that should have been redirected to
/dev/null or funroll-loops.org [1]

We've already wasted more bytes in this discussion, than would be ever
wasted by a conflict in fw_cfg names.

Paolo

[1] Now at http://fun.irq.dk/funroll-loops.org/

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Laszlo Ersek]

On 06/01/15 12:42, Michael S. Tsirkin wrote:
> On Mon, Jun 01, 2015 at 12:35:34PM +0200, Laszlo Ersek wrote:
>> On 06/01/15 12:23, Michael S. Tsirkin wrote:
>>> On Mon, Jun 01, 2015 at 11:01:23AM +0200, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 01/06/2015 09:28, Michael S. Tsirkin wrote:
>>>>>> I don't feel overly strongly about it; just "mechanism, not policy"
>>>>>> looks like a good tradition (well, good excuse anyway).
>>>>>
>>>>> Most users never see warnings. We ship it, we support it.
>>>>> If we don't want to support it, let's not ship it.
>>>>
>>>> Then we should rm -rf half of QEMU. :)
>>>>
>>>> Seriously, I agree wholeheartedly with not baking policy into QEMU.  A
>>>> lot of QEMU command-line hacking really is just a shortcut to avoid
>>>> continuous recompilation.  I don't think it's reasonable to expect that
>>>> it constitutes a stable API.
>>>>
>>>> Paolo
>>>
>>> Still, reserving part of the namespace for QEMU internal use
>>> is *not* policy, it's just good engineering.
>>>
>>> How about we forbid adding files under "etc/" ?
>>>
>>> That would be enough to avoid conflicts.
>>
>> Some of the current fw_cfg files, like "bootorder", are not under
>> "etc/".
> 
> Well bootorder is there so at least it will always fail.
> We do have stuff under /rom.
> 
>> Hence the earlier proposal to restrict the user (to under opt/,
>> IIRC), rather than ourselves.
>>
>> Thanks
>> Laszlo
> 
> How about we pre-pend opt/ to user-supplied names?
> Will fix this without limiting user in any way.

(Sorry if this has been addressed in further messages down-thread; I'm
not that far yet.) The issues here are that a user implementing custom
firmware or OS-level code will have to:
(a) take this prefix in account when writing the client code,
(b) blob names have a max length of 55 chars IIRC (+ terminating NUL),
any fixed prefix would decrease that. Not necessarily a problem, but
something to be aware of, at least in the QEMU option parsing.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Jun 01, 2015 at 12:43:35PM +0200, Paolo Bonzini wrote:
>> 
>> 
>> On 01/06/2015 12:23, Michael S. Tsirkin wrote:
>> > Still, reserving part of the namespace for QEMU internal use
>> > is *not* policy, it's just good engineering.
>> > 
>> > How about we forbid adding files under "etc/" ?
>> > 
>> > That would be enough to avoid conflicts.
>> 
>> I do not understand.  What we're doing is free-beer.  We can always say
>> no.  What's your worry?
>
> Someone writes a tool using a specific path.
> We then add same path upstream, script breaks.
>
>> One usecase of this feature is to avoid recompiling QEMU while playing
>> with firmware.  If you cannot mimic QEMU's behavior (which is to add
>> "etc/" files), the feature is pointless, or at least I totally cannot
>> understand its purpose and I'm against merging it.
>> 
>> Paolo
>
> Confused.  Why does it produce the warning then?
>
> If it's just for playing games, add a configure
> switch to enable it, and disable by default.
> Don't set traps for users.

Document development aids as "use at your own risk", spit out scary
warnings on use if you like, hide them from the unsophisticated user's
view if you must, but please don't add more compile-time configuration
knobs.

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Paolo Bonzini]

On 01/06/2015 13:20, Markus Armbruster wrote:
> > If it's just for playing games, add a configure
> > switch to enable it, and disable by default.
> > Don't set traps for users.
>
> Document development aids as "use at your own risk", spit out scary
> warnings on use if you like, hide them from the unsophisticated user's
> view if you must, but please don't add more compile-time configuration
> knobs.

Subscribed.  It also turns out that the "if you like" part matches this
patchset perfectly. :)

Paolo

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Laszlo Ersek]

On 06/01/15 12:43, Paolo Bonzini wrote:
> 
> 
> On 01/06/2015 12:23, Michael S. Tsirkin wrote:
>> Still, reserving part of the namespace for QEMU internal use
>> is *not* policy, it's just good engineering.
>>
>> How about we forbid adding files under "etc/" ?
>>
>> That would be enough to avoid conflicts.
> 
> I do not understand.  What we're doing is free-beer.  We can always say
> no.  What's your worry?
> 
> One usecase of this feature is to avoid recompiling QEMU while playing
> with firmware.  If you cannot mimic QEMU's behavior (which is to add
> "etc/" files), the feature is pointless, or at least I totally cannot
> understand its purpose and I'm against merging it.

As far as I understand, the goal is indeed to expose fw_cfg to the host
user, without having to recompile QEMU. This would allow site-specific
fw_cfg files, for site-specific guest features. Gabriel wanted to
control some guest-agent like functionality (on windows too) with it,
IIRC. Matt Fleming @Intel might use it for custom OVMF development.

I think those are valid goals and should be possible to support even if
we isolate QEMU's own fw_cfg namespace strictly from the user (opt/)
namespace. This feature is not there to mess with QEMU's "own" fw_cfg
files; for those the existent command line switches should be used.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Laszlo Ersek]

On 06/01/15 12:48, Michael S. Tsirkin wrote:
> On Mon, Jun 01, 2015 at 12:43:35PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 01/06/2015 12:23, Michael S. Tsirkin wrote:
>>> Still, reserving part of the namespace for QEMU internal use
>>> is *not* policy, it's just good engineering.
>>>
>>> How about we forbid adding files under "etc/" ?
>>>
>>> That would be enough to avoid conflicts.
>>
>> I do not understand.  What we're doing is free-beer.  We can always say
>> no.  What's your worry?
> 
> Someone writes a tool using a specific path.
> We then add same path upstream, script breaks.
> 
>> One usecase of this feature is to avoid recompiling QEMU while playing
>> with firmware.  If you cannot mimic QEMU's behavior (which is to add
>> "etc/" files), the feature is pointless, or at least I totally cannot
>> understand its purpose and I'm against merging it.
>>
>> Paolo
> 
> Confused.  Why does it produce the warning then?
> 
> If it's just for playing games, add a configure
> switch to enable it, and disable by default.
> Don't set traps for users.

The site specific feature can be long-term for a given site. It might
live across several QEMU upgrades. New version of QEMU introdces a new
fw_cfg file, might conflict with user's file from earlier. Unless the
user places it under opt/. For that reason we emit a warning, but do not
forcefully prevent the user from shooting his foot off.

Laszlo

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Michael S. Tsirkin]

On Mon, Jun 01, 2015 at 01:26:53PM +0200, Laszlo Ersek wrote:
> On 06/01/15 12:48, Michael S. Tsirkin wrote:
> > On Mon, Jun 01, 2015 at 12:43:35PM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 01/06/2015 12:23, Michael S. Tsirkin wrote:
> >>> Still, reserving part of the namespace for QEMU internal use
> >>> is *not* policy, it's just good engineering.
> >>>
> >>> How about we forbid adding files under "etc/" ?
> >>>
> >>> That would be enough to avoid conflicts.
> >>
> >> I do not understand.  What we're doing is free-beer.  We can always say
> >> no.  What's your worry?
> > 
> > Someone writes a tool using a specific path.
> > We then add same path upstream, script breaks.
> > 
> >> One usecase of this feature is to avoid recompiling QEMU while playing
> >> with firmware.  If you cannot mimic QEMU's behavior (which is to add
> >> "etc/" files), the feature is pointless, or at least I totally cannot
> >> understand its purpose and I'm against merging it.
> >>
> >> Paolo
> > 
> > Confused.  Why does it produce the warning then?
> > 
> > If it's just for playing games, add a configure
> > switch to enable it, and disable by default.
> > Don't set traps for users.
> 
> The site specific feature can be long-term for a given site. It might
> live across several QEMU upgrades. New version of QEMU introdces a new
> fw_cfg file, might conflict with user's file from earlier. Unless the
> user places it under opt/. For that reason we emit a warning, but do not
> forcefully prevent the user from shooting his foot off.
> 
> Laszlo

I'm sorry - I don't understand. It's easy to do the right thing.  Just
add the opt prefix. Why insist on user doing the right thing, and punish
violations with failing at random?

If it's useful for developers somehow, add a config flag for that.

-- 
MST

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Paolo Bonzini]

On 01/06/2015 14:38, Michael S. Tsirkin wrote:
> I'm sorry - I don't understand. It's easy to do the right thing.  Just
> add the opt prefix. Why insist on user doing the right thing, and punish
> violations with failing at random?
> 
> If it's useful for developers somehow, add a config flag for that.

You've already been explained that config flags are not an answer.

Paolo

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Michael S. Tsirkin]

On Mon, Jun 01, 2015 at 02:39:17PM +0200, Paolo Bonzini wrote:
> 
> 
> On 01/06/2015 14:38, Michael S. Tsirkin wrote:
> > I'm sorry - I don't understand. It's easy to do the right thing.  Just
> > add the opt prefix. Why insist on user doing the right thing, and punish
> > violations with failing at random?
> > 
> > If it's useful for developers somehow, add a config flag for that.
> 
> You've already been explained that config flags are not an answer.
> 
> Paolo


Oh then just #define FW_CFG_USER_PREFIX "opt/".

Developers can edit that to "" if they want to play.

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Paolo Bonzini]

On 01/06/2015 14:41, Michael S. Tsirkin wrote:
> On Mon, Jun 01, 2015 at 02:39:17PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 01/06/2015 14:38, Michael S. Tsirkin wrote:
>>> I'm sorry - I don't understand. It's easy to do the right thing.  Just
>>> add the opt prefix. Why insist on user doing the right thing, and punish
>>> violations with failing at random?
>>>
>>> If it's useful for developers somehow, add a config flag for that.
>>
>> You've already been explained that config flags are not an answer.
> 
> Oh then just #define FW_CFG_USER_PREFIX "opt/".
> 
> Developers can edit that to "" if they want to play.

Shall we agree to just let Gerd decide since he's handling these patches?

Paolo

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Laszlo Ersek]

On 06/01/15 14:38, Michael S. Tsirkin wrote:
> On Mon, Jun 01, 2015 at 01:26:53PM +0200, Laszlo Ersek wrote:
>> On 06/01/15 12:48, Michael S. Tsirkin wrote:
>>> On Mon, Jun 01, 2015 at 12:43:35PM +0200, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 01/06/2015 12:23, Michael S. Tsirkin wrote:
>>>>> Still, reserving part of the namespace for QEMU internal use
>>>>> is *not* policy, it's just good engineering.
>>>>>
>>>>> How about we forbid adding files under "etc/" ?
>>>>>
>>>>> That would be enough to avoid conflicts.
>>>>
>>>> I do not understand.  What we're doing is free-beer.  We can always say
>>>> no.  What's your worry?
>>>
>>> Someone writes a tool using a specific path.
>>> We then add same path upstream, script breaks.
>>>
>>>> One usecase of this feature is to avoid recompiling QEMU while playing
>>>> with firmware.  If you cannot mimic QEMU's behavior (which is to add
>>>> "etc/" files), the feature is pointless, or at least I totally cannot
>>>> understand its purpose and I'm against merging it.
>>>>
>>>> Paolo
>>>
>>> Confused.  Why does it produce the warning then?
>>>
>>> If it's just for playing games, add a configure
>>> switch to enable it, and disable by default.
>>> Don't set traps for users.
>>
>> The site specific feature can be long-term for a given site. It might
>> live across several QEMU upgrades. New version of QEMU introdces a new
>> fw_cfg file, might conflict with user's file from earlier. Unless the
>> user places it under opt/. For that reason we emit a warning, but do not
>> forcefully prevent the user from shooting his foot off.
>>
>> Laszlo
> 
> I'm sorry - I don't understand. It's easy to do the right thing.  Just
> add the opt prefix. Why insist on user doing the right thing, and punish
> violations with failing at random?

I'm not insisting, just trying to explain / repeat the thought process
(from more than one month ago :)) that lead up to Gabriel's v4.

Thanks
Laszlo

> If it's useful for developers somehow, add a config flag for that.
> 

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Gerd Hoffmann]

On Mo, 2015-06-01 at 14:43 +0200, Paolo Bonzini wrote:
> 
> On 01/06/2015 14:41, Michael S. Tsirkin wrote:
> > On Mon, Jun 01, 2015 at 02:39:17PM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 01/06/2015 14:38, Michael S. Tsirkin wrote:
> >>> I'm sorry - I don't understand. It's easy to do the right thing.  Just
> >>> add the opt prefix. Why insist on user doing the right thing, and punish
> >>> violations with failing at random?
> >>>
> >>> If it's useful for developers somehow, add a config flag for that.
> >>
> >> You've already been explained that config flags are not an answer.
> > 
> > Oh then just #define FW_CFG_USER_PREFIX "opt/".
> > 
> > Developers can edit that to "" if they want to play.
> 
> Shall we agree to just let Gerd decide since he's handling these patches?

I can't see a strong reasons to change things.  The docs clearly
recommend to use opt/ prefix to avoid conflicts.  That is fine IMHO.

I don't feel like enforcing that in code, being able to use something
else can be useful for debugging/testing purposes.  For example there
are some etc/* things seabios looks at which qemu has no support for.
One can also supply option roms with the new switch.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Michael S. Tsirkin]

On Tue, Jun 02, 2015 at 09:37:13AM +0200, Gerd Hoffmann wrote:
> On Mo, 2015-06-01 at 14:43 +0200, Paolo Bonzini wrote:
> > 
> > On 01/06/2015 14:41, Michael S. Tsirkin wrote:
> > > On Mon, Jun 01, 2015 at 02:39:17PM +0200, Paolo Bonzini wrote:
> > >>
> > >>
> > >> On 01/06/2015 14:38, Michael S. Tsirkin wrote:
> > >>> I'm sorry - I don't understand. It's easy to do the right thing.  Just
> > >>> add the opt prefix. Why insist on user doing the right thing, and punish
> > >>> violations with failing at random?
> > >>>
> > >>> If it's useful for developers somehow, add a config flag for that.
> > >>
> > >> You've already been explained that config flags are not an answer.
> > > 
> > > Oh then just #define FW_CFG_USER_PREFIX "opt/".
> > > 
> > > Developers can edit that to "" if they want to play.
> > 
> > Shall we agree to just let Gerd decide since he's handling these patches?
> 
> I can't see a strong reasons to change things.  The docs clearly
> recommend to use opt/ prefix to avoid conflicts.  That is fine IMHO.
> 
> I don't feel like enforcing that in code, being able to use something
> else can be useful for debugging/testing purposes.  For example there
> are some etc/* things seabios looks at which qemu has no support for.
> One can also supply option roms with the new switch.
> 
> cheers,
>   Gerd

Well isn't this exactly the problem?
Once one does, then qemu gains same option rom and things break.

-- 
MST

Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Gerd Hoffmann]

  Hi,

> > I can't see a strong reasons to change things.  The docs clearly
> > recommend to use opt/ prefix to avoid conflicts.  That is fine IMHO.
> > 
> > I don't feel like enforcing that in code, being able to use something
> > else can be useful for debugging/testing purposes.  For example there
> > are some etc/* things seabios looks at which qemu has no support for.
> > One can also supply option roms with the new switch.
> > 
> > cheers,
> >   Gerd
> 
> Well isn't this exactly the problem?
> Once one does, then qemu gains same option rom and things break.

Yea, sure.  "testing/debugging purposes".  I don't mind if that breaks.
When developing stuff I do tricks which can break on
qemu/libvirt/whatever upgrade all the time for all kinds of reasons, for
example because libvirt doesn't support $new_hot_qemu_feature yet.

For production usage better don't do that and stick to the "opt/" prefix
recommendation.  If adding something outside opt/ turns out to be useful
outside testing/debugging we should add a proper qemu cmd line option
for that instead of suggesting users to fiddle with -fw_cfg.  No
question about that.

But I don't think we should break -fw_cfg usage as testing/debugging
tool by enforcing opt/ prefix.

cheers,
  Gerd