[Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host()

[Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host()

[Philippe Mathieu-Daudé]

Hi, various fw_cfg easy patches:

- First patches are trivial cleanups (and add trace events),

- patch 5 add a 'info fw_cfg' HMP command to display comprehensive list of
fw_cfg entries registered,

- patch 6 add fw_cfg_add_file_from_host(), a helper to map a file from the
host (using g_file_get_contents). This will be used by later UEFI series.

Philippe Mathieu-Daudé (6):
  hw/arm/virt: Remove null-check in virt_build_smbios()
  hw/arm: Remove unused include
  hw/i386: Remove unused include
  hw/nvram/fw_cfg: Add trace events
  hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command
  hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host()

 hmp-commands-info.hx      |  14 ++++
 hw/acpi/piix4.c           |   1 -
 hw/arm/virt-acpi-build.c  |   1 -
 hw/arm/virt.c             |   4 --
 hw/i386/acpi-build.c      |   1 -
 hw/i386/pc.c              |   1 -
 hw/nvram/fw_cfg.c         | 142 ++++++++++++++++++++++++++++++++++++++
 hw/nvram/trace-events     |   5 ++
 include/hw/nvram/fw_cfg.h |  24 +++++++
 9 files changed, 185 insertions(+), 8 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH 1/6] hw/arm/virt: Remove null-check in virt_build_smbios()

[Philippe Mathieu-Daudé]

Since af1f60a4022, the fw_cfg field is always created in machvirt_init().
There is no need to null check it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/arm/virt.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f69e7eb399..36303ed59c 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1279,10 +1279,6 @@ static void virt_build_smbios(VirtMachineState *vms)
     size_t smbios_tables_len, smbios_anchor_len;
     const char *product = "QEMU Virtual Machine";
 
-    if (!vms->fw_cfg) {
-        return;
-    }
-
     if (kvm_enabled()) {
         product = "KVM Virtual Machine";
     }
-- 
2.17.2

[Qemu-devel] [PATCH 2/6] hw/arm: Remove unused include

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/arm/virt-acpi-build.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 5785fb697c..98d7f7cf20 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -35,7 +35,6 @@
 #include "target/arm/cpu.h"
 #include "hw/acpi/acpi-defs.h"
 #include "hw/acpi/acpi.h"
-#include "hw/nvram/fw_cfg.h"
 #include "hw/acpi/bios-linker-loader.h"
 #include "hw/loader.h"
 #include "hw/hw.h"
-- 
2.17.2

[Qemu-devel] [PATCH 3/6] hw/i386: Remove unused include

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/acpi/piix4.c      | 1 -
 hw/i386/acpi-build.c | 1 -
 hw/i386/pc.c         | 1 -
 3 files changed, 3 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index e330f24c71..49a33bfcda 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -28,7 +28,6 @@
 #include "sysemu/sysemu.h"
 #include "qapi/error.h"
 #include "qemu/range.h"
-#include "hw/nvram/fw_cfg.h"
 #include "exec/address-spaces.h"
 #include "hw/acpi/piix4.h"
 #include "hw/acpi/pcihp.h"
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 236a20eaa8..1e5c371294 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -35,7 +35,6 @@
 #include "hw/acpi/acpi-defs.h"
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/cpu.h"
-#include "hw/nvram/fw_cfg.h"
 #include "hw/acpi/bios-linker-loader.h"
 #include "hw/loader.h"
 #include "hw/isa/isa.h"
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f095725dba..2221dcdb36 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -35,7 +35,6 @@
 #include "hw/ide.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_bus.h"
-#include "hw/nvram/fw_cfg.h"
 #include "hw/timer/hpet.h"
 #include "hw/smbios/smbios.h"
 #include "hw/loader.h"
-- 
2.17.2

[Qemu-devel] [PATCH 4/6] hw/nvram/fw_cfg: Add trace events

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/nvram/fw_cfg.c     | 5 +++++
 hw/nvram/trace-events | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 3cb726ff68..582653f07e 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -627,6 +627,7 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
 
 void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len)
 {
+    trace_fw_cfg_add_bytes(key, len);
     fw_cfg_add_bytes_callback(s, key, NULL, NULL, NULL, data, len, true);
 }
 
@@ -634,6 +635,7 @@ void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value)
 {
     size_t sz = strlen(value) + 1;
 
+    trace_fw_cfg_add_string(key, value);
     fw_cfg_add_bytes(s, key, g_memdup(value, sz), sz);
 }
 
@@ -643,6 +645,7 @@ void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value)
 
     copy = g_malloc(sizeof(value));
     *copy = cpu_to_le16(value);
+    trace_fw_cfg_add_i16(key, value);
     fw_cfg_add_bytes(s, key, copy, sizeof(value));
 }
 
@@ -662,6 +665,7 @@ void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value)
 
     copy = g_malloc(sizeof(value));
     *copy = cpu_to_le32(value);
+    trace_fw_cfg_add_i32(key, value);
     fw_cfg_add_bytes(s, key, copy, sizeof(value));
 }
 
@@ -671,6 +675,7 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
 
     copy = g_malloc(sizeof(value));
     *copy = cpu_to_le64(value);
+    trace_fw_cfg_add_i64(key, value);
     fw_cfg_add_bytes(s, key, copy, sizeof(value));
 }
 
diff --git a/hw/nvram/trace-events b/hw/nvram/trace-events
index 6b55ba7a09..0ee0f8d04a 100644
--- a/hw/nvram/trace-events
+++ b/hw/nvram/trace-events
@@ -7,4 +7,9 @@ nvram_write(uint32_t addr, uint32_t old, uint32_t val) "write addr %d: 0x%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, uint64_t ret) "%p = 0x%"PRIx64
+fw_cfg_add_bytes(uint16_t key, size_t len) "key 0x%04" PRIx16 " (%zu bytes)"
 fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)"
+fw_cfg_add_string(uint16_t key, const char *value) "key 0x%04" PRIx16 ", value '%s'"
+fw_cfg_add_i16(uint16_t key, uint32_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx32
+fw_cfg_add_i32(uint16_t key, uint32_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx32
+fw_cfg_add_i64(uint16_t key, uint64_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx64
-- 
2.17.2

[Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command

[Philippe Mathieu-Daudé]

$ qemu-system-x86_64 -S -monitor stdio
(qemu) info fw_cfg
        Type    Perm    Size    Specific   Order   Info
   signature      RO       4                       QEMU
          id      RO       4                       0x00000003
        uuid      RO      16                       00000000-0000-0000-0000-000000000000
    ram_size      RO       8                       0x0000000008000000
   nographic      RO       2                       0x0000
     nb_cpus      RO       2                       0x0001
        numa      RO      16
   boot_menu      RO       2                       0x0000
    max_cpus      RO       2                       0x0001
    file_dir      RO    2052
 file (id 1)      RO      36                 160   etc/acpi/rsdp
 file (id 2)      RO  131072                 130   etc/acpi/tables
 file (id 3)      RO       4                  15   etc/boot-fail-wait
 file (id 4)      RO      20                  40   etc/e820
 file (id 5)      RO      31                  30   etc/smbios/smbios-anchor
 file (id 6)      RO     320                  20   etc/smbios/smbios-tables
 file (id 7)      RO       6                  90   etc/system-states
 file (id 8)      RO    4096                 140   etc/table-loader
file (id 10)      RO    9216                  55   genroms/kvmvapic.bin
        uuid      RO       4  (arch spec)          01000000-0000-0000-0000-000000000000
    ram_size      RO     324  (arch spec)
   nographic      RO     121  (arch spec)
(qemu)

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hmp-commands-info.hx      |  14 +++++
 hw/nvram/fw_cfg.c         | 115 ++++++++++++++++++++++++++++++++++++++
 include/hw/nvram/fw_cfg.h |   2 +
 3 files changed, 131 insertions(+)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index cbee8b944d..9e86dceeb4 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -916,6 +916,20 @@ STEXI
 @item info sev
 @findex info sev
 Show SEV information.
+ETEXI
+
+    {
+        .name       = "fw_cfg",
+        .args_type  = "",
+        .params     = "",
+        .help       = "Display the table of the fw_cfg entries registered",
+        .cmd        = hmp_info_fw_cfg,
+    },
+
+STEXI
+@item info fw_cfg
+@findex info fw_cfg
+Show roms.
 ETEXI
 
 STEXI
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 582653f07e..50525ec1dd 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -36,6 +36,7 @@
 #include "qemu/config-file.h"
 #include "qemu/cutils.h"
 #include "qapi/error.h"
+#include "monitor/monitor.h"
 
 #define FW_CFG_FILE_SLOTS_DFLT 0x20
 
@@ -1164,3 +1165,117 @@ static void fw_cfg_register_types(void)
 }
 
 type_init(fw_cfg_register_types)
+
+static const char *fw_cfg_wellknown_entries[0x20] = {
+    [FW_CFG_SIGNATURE] = "signature",
+    [FW_CFG_ID] = "id",
+    [FW_CFG_UUID] = "uuid",
+    [FW_CFG_RAM_SIZE] = "ram_size",
+    [FW_CFG_NOGRAPHIC] = "nographic",
+    [FW_CFG_NB_CPUS] = "nb_cpus",
+    [FW_CFG_MACHINE_ID] = "machine_id",
+    [FW_CFG_KERNEL_ADDR] = "kernel_addr",
+    [FW_CFG_KERNEL_SIZE] = "kernel_size",
+    [FW_CFG_KERNEL_CMDLINE] = "kernel_cmdline",
+    [FW_CFG_INITRD_ADDR] = "initrd_addr",
+    [FW_CFG_INITRD_SIZE] = "initdr_size",
+    [FW_CFG_BOOT_DEVICE] = "boot_device",
+    [FW_CFG_NUMA] = "numa",
+    [FW_CFG_BOOT_MENU] = "boot_menu",
+    [FW_CFG_MAX_CPUS] = "max_cpus",
+    [FW_CFG_KERNEL_ENTRY] = "kernel_entry",
+    [FW_CFG_KERNEL_DATA] = "kernel_data",
+    [FW_CFG_INITRD_DATA] = "initrd_data",
+    [FW_CFG_CMDLINE_ADDR] = "cmdline_addr",
+    [FW_CFG_CMDLINE_SIZE] = "cmdline_size",
+    [FW_CFG_CMDLINE_DATA] = "cmdline_data",
+    [FW_CFG_SETUP_ADDR] = "setup_addr",
+    [FW_CFG_SETUP_SIZE] = "setup_size",
+    [FW_CFG_SETUP_DATA] = "setup_data",
+    [FW_CFG_FILE_DIR] = "file_dir",
+};
+
+void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict)
+{
+    FWCfgState *s = fw_cfg_find();
+    int arch, key;
+
+    monitor_printf(mon, "%12s %5s %7s %9s %6s %-24s\n",
+                   "Type", "Perm", "Size", "Specific", "Order", "Info");
+    for (arch = 0; arch < ARRAY_SIZE(s->entries); ++arch) {
+        for (key = 0; key < fw_cfg_max_entry(s); ++key) {
+            FWCfgEntry *e = &s->entries[arch][key];
+            const char *perm = e->allow_write ? "RW" : "RO";
+            const char *arch_spec = arch ? "arch_spec" : "";
+            char *type, *info = NULL;
+
+            if (!e->len) {
+                continue;
+            }
+            if (key >= FW_CFG_FILE_FIRST) {
+                int id = key - FW_CFG_FILE_FIRST;
+                const char *path = s->files->f[id].name;
+
+                type = g_strdup_printf("file (id %d)", id);
+                monitor_printf(mon, "%12s %5s %7d %10s %5d %-24s\n",
+                               type, perm, e->len, arch_spec,
+                               get_fw_cfg_order(s, path), path);
+                g_free(type);
+                continue;
+            }
+            type = g_strdup(fw_cfg_wellknown_entries[key]);
+            if (!type) {
+                type = g_strdup_printf("entry_%d", key);
+            }
+
+            switch (key) {
+            case FW_CFG_SIGNATURE:
+                info = g_strndup((const gchar *)e->data, e->len);
+                break;
+            case FW_CFG_UUID:
+                info = qemu_uuid_unparse_strdup((const QemuUUID *)e->data);
+                break;
+            case FW_CFG_ID:
+            case FW_CFG_NOGRAPHIC:
+            case FW_CFG_NB_CPUS:
+            case FW_CFG_BOOT_MENU:
+            case FW_CFG_MAX_CPUS:
+            case FW_CFG_RAM_SIZE:
+            case FW_CFG_KERNEL_ADDR:
+            case FW_CFG_KERNEL_SIZE:
+            case FW_CFG_KERNEL_CMDLINE:
+            case FW_CFG_KERNEL_ENTRY:
+            case FW_CFG_KERNEL_DATA:
+            case FW_CFG_INITRD_ADDR:
+            case FW_CFG_INITRD_SIZE:
+            case FW_CFG_INITRD_DATA:
+            case FW_CFG_CMDLINE_ADDR:
+            case FW_CFG_CMDLINE_SIZE:
+            case FW_CFG_CMDLINE_DATA:
+            case FW_CFG_SETUP_ADDR:
+            case FW_CFG_SETUP_SIZE:
+            case FW_CFG_SETUP_DATA:
+                switch (e->len) {
+                case 2:
+                    info = g_strdup_printf("0x%04x", lduw_le_p(e->data));
+                    break;
+                case 4:
+                    info = g_strdup_printf("0x%08x", ldl_le_p(e->data));
+                    break;
+                case 8:
+                    info = g_strdup_printf("0x%016" PRIx64, ldq_le_p(e->data));
+                    break;
+                default:
+                    break;
+                }
+                break;
+            default:
+                break;
+            }
+            monitor_printf(mon, "%12s %5s %7d %10s %5s %-24s\n",
+                           type, perm, e->len, arch_spec, "", info ? info : "");
+            g_free(type);
+            g_free(info);
+        }
+    }
+}
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index f5a6895a74..28630b2f26 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -226,4 +226,6 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
 FWCfgState *fw_cfg_find(void);
 bool fw_cfg_dma_enabled(void *opaque);
 
+void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict);
+
 #endif
-- 
2.17.2

[Qemu-devel] [PATCH 6/6] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host()

[Philippe Mathieu-Daudé]

Add a function to read the full content of file on the host, and add
a new 'file' name item to the fw_cfg device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/nvram/fw_cfg.c         | 22 ++++++++++++++++++++++
 include/hw/nvram/fw_cfg.h | 22 ++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 50525ec1dd..f3232fcb16 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -842,6 +842,28 @@ void fw_cfg_add_file(FWCfgState *s,  const char *filename,
     fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data, len, true);
 }
 
+void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename,
+                                const char *host_path, size_t *len)
+{
+    GError *gerr = NULL;
+    gchar *ptr = NULL;
+    gsize contents_len = 0;
+
+
+    if (g_file_get_contents(host_path, &ptr, &contents_len, &gerr)) {
+        fw_cfg_add_file(s, filename, ptr, contents_len);
+    } else {
+        error_report("failed to read file %s, %s", host_path, gerr->message);
+        g_error_free(gerr);
+        return NULL;
+    }
+    if (len) {
+        *len = contents_len;
+    }
+
+    return ptr;
+}
+
 void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
                         void *data, size_t len)
 {
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 28630b2f26..8de8d63433 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -166,6 +166,28 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
 void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
                      size_t len);
 
+/**
+ * fw_cfg_add_file_from_host:
+ * @s: fw_cfg device being modified
+ * @filename: name of new fw_cfg file item
+ * @host_path: path of the host file to read the data from
+ * @len: pointer to hold the length of the host file (optional)
+ *
+ * Read the content of a host file as a raw "blob" then add a new NAMED
+ * fw_cfg item of the file size. If @len is provided, it will contains the
+ * total length read from the host file. The data referenced by the starting
+ * pointer is only linked, NOT copied, into the data structure of the fw_cfg
+ * device.
+ * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
+ * will be used; also, a new entry will be added to the file directory
+ * structure residing at key value FW_CFG_FILE_DIR, containing the item name,
+ * data size, and assigned selector key value.
+ *
+ * Returns: pointer to the file content, or NULL if an error occured.
+ */
+void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename,
+                                const char *host_path, size_t *len);
+
 /**
  * fw_cfg_add_file_callback:
  * @s: fw_cfg device being modified
-- 
2.17.2

Re: [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command

[Eric Blake]

On 12/7/18 11:03 AM, Philippe Mathieu-Daudé wrote:
> $ qemu-system-x86_64 -S -monitor stdio
> (qemu) info fw_cfg
>          Type    Perm    Size    Specific   Order   Info
>     signature      RO       4                       QEMU
>            id      RO       4                       0x00000003
>          uuid      RO      16                       00000000-0000-0000-0000-000000000000
>      ram_size      RO       8                       0x0000000008000000
>     nographic      RO       2                       0x0000
>       nb_cpus      RO       2                       0x0001
>          numa      RO      16
>     boot_menu      RO       2                       0x0000
>      max_cpus      RO       2                       0x0001
>      file_dir      RO    2052
>   file (id 1)      RO      36                 160   etc/acpi/rsdp
>   file (id 2)      RO  131072                 130   etc/acpi/tables
>   file (id 3)      RO       4                  15   etc/boot-fail-wait
>   file (id 4)      RO      20                  40   etc/e820
>   file (id 5)      RO      31                  30   etc/smbios/smbios-anchor
>   file (id 6)      RO     320                  20   etc/smbios/smbios-tables
>   file (id 7)      RO       6                  90   etc/system-states
>   file (id 8)      RO    4096                 140   etc/table-loader
> file (id 10)      RO    9216                  55   genroms/kvmvapic.bin
>          uuid      RO       4  (arch spec)          01000000-0000-0000-0000-000000000000
>      ram_size      RO     324  (arch spec)
>     nographic      RO     121  (arch spec)
> (qemu)
> 

In general, when adding something to HMP but not QMP, it is worth a 
comment explaining WHY we did not want QMP.  (Here, I think the reason 
is that this is a debugging aid likely to be useful to developers but 
unlikely to be needed by a management app, and matches similar other 
HMP-only info commands, and is therefore not worth the effort of turning 
it into QAPI).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 3/6] hw/i386: Remove unused include

[Michael S. Tsirkin]

On Fri, Dec 07, 2018 at 06:03:57PM +0100, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/acpi/piix4.c      | 1 -
>  hw/i386/acpi-build.c | 1 -
>  hw/i386/pc.c         | 1 -
>  3 files changed, 3 deletions(-)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index e330f24c71..49a33bfcda 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -28,7 +28,6 @@
>  #include "sysemu/sysemu.h"
>  #include "qapi/error.h"
>  #include "qemu/range.h"
> -#include "hw/nvram/fw_cfg.h"
>  #include "exec/address-spaces.h"
>  #include "hw/acpi/piix4.h"
>  #include "hw/acpi/pcihp.h"

Looks right.

> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 236a20eaa8..1e5c371294 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -35,7 +35,6 @@
>  #include "hw/acpi/acpi-defs.h"
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/cpu.h"
> -#include "hw/nvram/fw_cfg.h"
>  #include "hw/acpi/bios-linker-loader.h"
>  #include "hw/loader.h"
>  #include "hw/isa/isa.h"

This one calls fw_cfg_add_file.

> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f095725dba..2221dcdb36 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -35,7 +35,6 @@
>  #include "hw/ide.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_bus.h"
> -#include "hw/nvram/fw_cfg.h"
>  #include "hw/timer/hpet.h"
>  #include "hw/smbios/smbios.h"
>  #include "hw/loader.h"


And this calls fw_cfg_add_bytes

> -- 
> 2.17.2

Re: [Qemu-devel] [PATCH 4/6] hw/nvram/fw_cfg: Add trace events

[Michael S. Tsirkin]

On Fri, Dec 07, 2018 at 06:03:58PM +0100, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Well why not.

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/nvram/fw_cfg.c     | 5 +++++
>  hw/nvram/trace-events | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 3cb726ff68..582653f07e 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -627,6 +627,7 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
>  
>  void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len)
>  {
> +    trace_fw_cfg_add_bytes(key, len);
>      fw_cfg_add_bytes_callback(s, key, NULL, NULL, NULL, data, len, true);
>  }
>  
> @@ -634,6 +635,7 @@ void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value)
>  {
>      size_t sz = strlen(value) + 1;
>  
> +    trace_fw_cfg_add_string(key, value);
>      fw_cfg_add_bytes(s, key, g_memdup(value, sz), sz);
>  }
>  
> @@ -643,6 +645,7 @@ void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value)
>  
>      copy = g_malloc(sizeof(value));
>      *copy = cpu_to_le16(value);
> +    trace_fw_cfg_add_i16(key, value);
>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>  }
>  
> @@ -662,6 +665,7 @@ void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value)
>  
>      copy = g_malloc(sizeof(value));
>      *copy = cpu_to_le32(value);
> +    trace_fw_cfg_add_i32(key, value);
>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>  }
>  
> @@ -671,6 +675,7 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
>  
>      copy = g_malloc(sizeof(value));
>      *copy = cpu_to_le64(value);
> +    trace_fw_cfg_add_i64(key, value);
>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>  }
>  
> diff --git a/hw/nvram/trace-events b/hw/nvram/trace-events
> index 6b55ba7a09..0ee0f8d04a 100644
> --- a/hw/nvram/trace-events
> +++ b/hw/nvram/trace-events
> @@ -7,4 +7,9 @@ nvram_write(uint32_t addr, uint32_t old, uint32_t val) "write addr %d: 0x%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, uint64_t ret) "%p = 0x%"PRIx64
> +fw_cfg_add_bytes(uint16_t key, size_t len) "key 0x%04" PRIx16 " (%zu bytes)"
>  fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)"
> +fw_cfg_add_string(uint16_t key, const char *value) "key 0x%04" PRIx16 ", value '%s'"
> +fw_cfg_add_i16(uint16_t key, uint32_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx32
> +fw_cfg_add_i32(uint16_t key, uint32_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx32
> +fw_cfg_add_i64(uint16_t key, uint64_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx64
> -- 
> 2.17.2

Re: [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command

[Michael S. Tsirkin]

On Fri, Dec 07, 2018 at 06:03:59PM +0100, Philippe Mathieu-Daudé wrote:
> $ qemu-system-x86_64 -S -monitor stdio
> (qemu) info fw_cfg
>         Type    Perm    Size    Specific   Order   Info

Can we do better than "Info"?

>    signature      RO       4                       QEMU
>           id      RO       4                       0x00000003
>         uuid      RO      16                       00000000-0000-0000-0000-000000000000
>     ram_size      RO       8                       0x0000000008000000
>    nographic      RO       2                       0x0000
>      nb_cpus      RO       2                       0x0001
>         numa      RO      16
>    boot_menu      RO       2                       0x0000
>     max_cpus      RO       2                       0x0001
>     file_dir      RO    2052
>  file (id 1)      RO      36                 160   etc/acpi/rsdp
>  file (id 2)      RO  131072                 130   etc/acpi/tables
>  file (id 3)      RO       4                  15   etc/boot-fail-wait
>  file (id 4)      RO      20                  40   etc/e820
>  file (id 5)      RO      31                  30   etc/smbios/smbios-anchor
>  file (id 6)      RO     320                  20   etc/smbios/smbios-tables
>  file (id 7)      RO       6                  90   etc/system-states
>  file (id 8)      RO    4096                 140   etc/table-loader
> file (id 10)      RO    9216                  55   genroms/kvmvapic.bin
>         uuid      RO       4  (arch spec)          01000000-0000-0000-0000-000000000000
>     ram_size      RO     324  (arch spec)
>    nographic      RO     121  (arch spec)

Weird. Your code has arch_spec.

> (qemu)
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>


Looks helpful but generally IMHO whatever is exposed through hmp
should be in the qmp as well.


> ---
>  hmp-commands-info.hx      |  14 +++++
>  hw/nvram/fw_cfg.c         | 115 ++++++++++++++++++++++++++++++++++++++
>  include/hw/nvram/fw_cfg.h |   2 +
>  3 files changed, 131 insertions(+)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index cbee8b944d..9e86dceeb4 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -916,6 +916,20 @@ STEXI
>  @item info sev
>  @findex info sev
>  Show SEV information.
> +ETEXI
> +
> +    {
> +        .name       = "fw_cfg",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "Display the table of the fw_cfg entries registered",

I think the help line should be a bit more verbose.
Mention it's a paravirtualized interface, and why
would one want to display it (debugging guest firmware?).


> +        .cmd        = hmp_info_fw_cfg,
> +    },
> +
> +STEXI
> +@item info fw_cfg
> +@findex info fw_cfg
> +Show roms.
>  ETEXI
>  
>  STEXI
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 582653f07e..50525ec1dd 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -36,6 +36,7 @@
>  #include "qemu/config-file.h"
>  #include "qemu/cutils.h"
>  #include "qapi/error.h"
> +#include "monitor/monitor.h"
>  
>  #define FW_CFG_FILE_SLOTS_DFLT 0x20
>  
> @@ -1164,3 +1165,117 @@ static void fw_cfg_register_types(void)
>  }
>  
>  type_init(fw_cfg_register_types)
> +
> +static const char *fw_cfg_wellknown_entries[0x20] = {
> +    [FW_CFG_SIGNATURE] = "signature",
> +    [FW_CFG_ID] = "id",
> +    [FW_CFG_UUID] = "uuid",
> +    [FW_CFG_RAM_SIZE] = "ram_size",
> +    [FW_CFG_NOGRAPHIC] = "nographic",
> +    [FW_CFG_NB_CPUS] = "nb_cpus",
> +    [FW_CFG_MACHINE_ID] = "machine_id",
> +    [FW_CFG_KERNEL_ADDR] = "kernel_addr",
> +    [FW_CFG_KERNEL_SIZE] = "kernel_size",
> +    [FW_CFG_KERNEL_CMDLINE] = "kernel_cmdline",
> +    [FW_CFG_INITRD_ADDR] = "initrd_addr",
> +    [FW_CFG_INITRD_SIZE] = "initdr_size",
> +    [FW_CFG_BOOT_DEVICE] = "boot_device",
> +    [FW_CFG_NUMA] = "numa",
> +    [FW_CFG_BOOT_MENU] = "boot_menu",
> +    [FW_CFG_MAX_CPUS] = "max_cpus",
> +    [FW_CFG_KERNEL_ENTRY] = "kernel_entry",
> +    [FW_CFG_KERNEL_DATA] = "kernel_data",
> +    [FW_CFG_INITRD_DATA] = "initrd_data",
> +    [FW_CFG_CMDLINE_ADDR] = "cmdline_addr",
> +    [FW_CFG_CMDLINE_SIZE] = "cmdline_size",
> +    [FW_CFG_CMDLINE_DATA] = "cmdline_data",
> +    [FW_CFG_SETUP_ADDR] = "setup_addr",
> +    [FW_CFG_SETUP_SIZE] = "setup_size",
> +    [FW_CFG_SETUP_DATA] = "setup_data",
> +    [FW_CFG_FILE_DIR] = "file_dir",
> +};
> +
> +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict)
> +{
> +    FWCfgState *s = fw_cfg_find();
> +    int arch, key;

Looks like this will crash on a machine without fw cfg.


> +
> +    monitor_printf(mon, "%12s %5s %7s %9s %6s %-24s\n",
> +                   "Type", "Perm", "Size", "Specific", "Order", "Info");
> +    for (arch = 0; arch < ARRAY_SIZE(s->entries); ++arch) {
> +        for (key = 0; key < fw_cfg_max_entry(s); ++key) {
> +            FWCfgEntry *e = &s->entries[arch][key];
> +            const char *perm = e->allow_write ? "RW" : "RO";
> +            const char *arch_spec = arch ? "arch_spec" : "";
> +            char *type, *info = NULL;
> +
> +            if (!e->len) {
> +                continue;
> +            }
> +            if (key >= FW_CFG_FILE_FIRST) {
> +                int id = key - FW_CFG_FILE_FIRST;
> +                const char *path = s->files->f[id].name;
> +
> +                type = g_strdup_printf("file (id %d)", id);
> +                monitor_printf(mon, "%12s %5s %7d %10s %5d %-24s\n",
> +                               type, perm, e->len, arch_spec,
> +                               get_fw_cfg_order(s, path), path);
> +                g_free(type);
> +                continue;
> +            }
> +            type = g_strdup(fw_cfg_wellknown_entries[key]);
> +            if (!type) {
> +                type = g_strdup_printf("entry_%d", key);
> +            }
> +
> +            switch (key) {
> +            case FW_CFG_SIGNATURE:
> +                info = g_strndup((const gchar *)e->data, e->len);
> +                break;
> +            case FW_CFG_UUID:
> +                info = qemu_uuid_unparse_strdup((const QemuUUID *)e->data);
> +                break;
> +            case FW_CFG_ID:
> +            case FW_CFG_NOGRAPHIC:
> +            case FW_CFG_NB_CPUS:
> +            case FW_CFG_BOOT_MENU:
> +            case FW_CFG_MAX_CPUS:
> +            case FW_CFG_RAM_SIZE:
> +            case FW_CFG_KERNEL_ADDR:
> +            case FW_CFG_KERNEL_SIZE:
> +            case FW_CFG_KERNEL_CMDLINE:
> +            case FW_CFG_KERNEL_ENTRY:
> +            case FW_CFG_KERNEL_DATA:
> +            case FW_CFG_INITRD_ADDR:
> +            case FW_CFG_INITRD_SIZE:
> +            case FW_CFG_INITRD_DATA:
> +            case FW_CFG_CMDLINE_ADDR:
> +            case FW_CFG_CMDLINE_SIZE:
> +            case FW_CFG_CMDLINE_DATA:
> +            case FW_CFG_SETUP_ADDR:
> +            case FW_CFG_SETUP_SIZE:
> +            case FW_CFG_SETUP_DATA:
> +                switch (e->len) {
> +                case 2:
> +                    info = g_strdup_printf("0x%04x", lduw_le_p(e->data));
> +                    break;
> +                case 4:
> +                    info = g_strdup_printf("0x%08x", ldl_le_p(e->data));
> +                    break;
> +                case 8:
> +                    info = g_strdup_printf("0x%016" PRIx64, ldq_le_p(e->data));
> +                    break;
> +                default:
> +                    break;
> +                }
> +                break;
> +            default:
> +                break;
> +            }
> +            monitor_printf(mon, "%12s %5s %7d %10s %5s %-24s\n",
> +                           type, perm, e->len, arch_spec, "", info ? info : "");
> +            g_free(type);
> +            g_free(info);
> +        }
> +    }
> +}
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index f5a6895a74..28630b2f26 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -226,4 +226,6 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>  FWCfgState *fw_cfg_find(void);
>  bool fw_cfg_dma_enabled(void *opaque);
>  
> +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict);
> +
>  #endif
> -- 
> 2.17.2

Re: [Qemu-devel] [PATCH 6/6] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host()

[Michael S. Tsirkin]

On Fri, Dec 07, 2018 at 06:04:00PM +0100, Philippe Mathieu-Daudé wrote:
> Add a function to read the full content of file on the host, and add
> a new 'file' name item to the fw_cfg device.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Need to be careful with options that let users play with firmware.
Way too easy to create conflicts.
Anyway, I'd rather have this included with whatever is using it.

> ---
>  hw/nvram/fw_cfg.c         | 22 ++++++++++++++++++++++
>  include/hw/nvram/fw_cfg.h | 22 ++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 50525ec1dd..f3232fcb16 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -842,6 +842,28 @@ void fw_cfg_add_file(FWCfgState *s,  const char *filename,
>      fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data, len, true);
>  }
>  
> +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename,
> +                                const char *host_path, size_t *len)
> +{
> +    GError *gerr = NULL;
> +    gchar *ptr = NULL;
> +    gsize contents_len = 0;
> +
> +
> +    if (g_file_get_contents(host_path, &ptr, &contents_len, &gerr)) {
> +        fw_cfg_add_file(s, filename, ptr, contents_len);
> +    } else {
> +        error_report("failed to read file %s, %s", host_path, gerr->message);
> +        g_error_free(gerr);
> +        return NULL;
> +    }
> +    if (len) {
> +        *len = contents_len;
> +    }
> +
> +    return ptr;
> +}
> +
>  void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
>                          void *data, size_t len)
>  {
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 28630b2f26..8de8d63433 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -166,6 +166,28 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
>  void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
>                       size_t len);
>  
> +/**
> + * fw_cfg_add_file_from_host:
> + * @s: fw_cfg device being modified
> + * @filename: name of new fw_cfg file item
> + * @host_path: path of the host file to read the data from
> + * @len: pointer to hold the length of the host file (optional)
> + *
> + * Read the content of a host file as a raw "blob" then add a new NAMED
> + * fw_cfg item of the file size. If @len is provided, it will contains the
> + * total length read from the host file. The data referenced by the starting
> + * pointer is only linked, NOT copied, into the data structure of the fw_cfg
> + * device.
> + * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
> + * will be used; also, a new entry will be added to the file directory
> + * structure residing at key value FW_CFG_FILE_DIR, containing the item name,
> + * data size, and assigned selector key value.
> + *
> + * Returns: pointer to the file content, or NULL if an error occured.
> + */
> +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename,
> +                                const char *host_path, size_t *len);
> +
>  /**
>   * fw_cfg_add_file_callback:
>   * @s: fw_cfg device being modified
> -- 
> 2.17.2

Re: [Qemu-devel] [PATCH 2/6] hw/arm: Remove unused include

[Michael S. Tsirkin]

On Fri, Dec 07, 2018 at 06:03:56PM +0100, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 5785fb697c..98d7f7cf20 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -35,7 +35,6 @@
>  #include "target/arm/cpu.h"
>  #include "hw/acpi/acpi-defs.h"
>  #include "hw/acpi/acpi.h"
> -#include "hw/nvram/fw_cfg.h"
>  #include "hw/acpi/bios-linker-loader.h"
>  #include "hw/loader.h"
>  #include "hw/hw.h"


This file uses fw_cfg_add_file, looks wrong.

> -- 
> 2.17.2

Re: [Qemu-devel] [PATCH 6/6] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host()

[Philippe Mathieu-Daudé]

On 12/7/18 6:56 PM, Michael S. Tsirkin wrote:
> On Fri, Dec 07, 2018 at 06:04:00PM +0100, Philippe Mathieu-Daudé wrote:
>> Add a function to read the full content of file on the host, and add
>> a new 'file' name item to the fw_cfg device.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Need to be careful with options that let users play with firmware.
> Way too easy to create conflicts.
> Anyway, I'd rather have this included with whatever is using it.

Yes, sure.

> 
>> ---
>>  hw/nvram/fw_cfg.c         | 22 ++++++++++++++++++++++
>>  include/hw/nvram/fw_cfg.h | 22 ++++++++++++++++++++++
>>  2 files changed, 44 insertions(+)
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 50525ec1dd..f3232fcb16 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -842,6 +842,28 @@ void fw_cfg_add_file(FWCfgState *s,  const char *filename,
>>      fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data, len, true);
>>  }
>>  
>> +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename,
>> +                                const char *host_path, size_t *len)
>> +{
>> +    GError *gerr = NULL;
>> +    gchar *ptr = NULL;
>> +    gsize contents_len = 0;
>> +
>> +
>> +    if (g_file_get_contents(host_path, &ptr, &contents_len, &gerr)) {
>> +        fw_cfg_add_file(s, filename, ptr, contents_len);
>> +    } else {
>> +        error_report("failed to read file %s, %s", host_path, gerr->message);
>> +        g_error_free(gerr);
>> +        return NULL;
>> +    }
>> +    if (len) {
>> +        *len = contents_len;
>> +    }
>> +
>> +    return ptr;
>> +}
>> +
>>  void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
>>                          void *data, size_t len)
>>  {
>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>> index 28630b2f26..8de8d63433 100644
>> --- a/include/hw/nvram/fw_cfg.h
>> +++ b/include/hw/nvram/fw_cfg.h
>> @@ -166,6 +166,28 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
>>  void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
>>                       size_t len);
>>  
>> +/**
>> + * fw_cfg_add_file_from_host:
>> + * @s: fw_cfg device being modified
>> + * @filename: name of new fw_cfg file item
>> + * @host_path: path of the host file to read the data from
>> + * @len: pointer to hold the length of the host file (optional)
>> + *
>> + * Read the content of a host file as a raw "blob" then add a new NAMED
>> + * fw_cfg item of the file size. If @len is provided, it will contains the
>> + * total length read from the host file. The data referenced by the starting
>> + * pointer is only linked, NOT copied, into the data structure of the fw_cfg
>> + * device.
>> + * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
>> + * will be used; also, a new entry will be added to the file directory
>> + * structure residing at key value FW_CFG_FILE_DIR, containing the item name,
>> + * data size, and assigned selector key value.
>> + *
>> + * Returns: pointer to the file content, or NULL if an error occured.
>> + */
>> +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename,
>> +                                const char *host_path, size_t *len);
>> +
>>  /**
>>   * fw_cfg_add_file_callback:
>>   * @s: fw_cfg device being modified
>> -- 
>> 2.17.2

Re: [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host()

[no-reply]

Patchew URL: https://patchew.org/QEMU/20181207170400.5129-1-philmd@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-quick@centos7 SHOW_ENV=1 J=8
=== TEST SCRIPT END ===

libpmem support   no
libudev           no

WARNING: Use of SDL 1.2 is deprecated and will be removed in
WARNING: future releases. Please switch to using SDL 2.0

NOTE: cross-compilers enabled:  'cc'
  GEN     x86_64-softmmu/config-devices.mak.tmp


The full log is available at
http://patchew.org/logs/20181207170400.5129-1-philmd@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH 2/6] hw/arm: Remove unused include

[Philippe Mathieu-Daudé]

Hi Michael, Peter.

On 12/7/18 6:57 PM, Michael S. Tsirkin wrote:
> On Fri, Dec 07, 2018 at 06:03:56PM +0100, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/arm/virt-acpi-build.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 5785fb697c..98d7f7cf20 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -35,7 +35,6 @@
>>  #include "target/arm/cpu.h"
>>  #include "hw/acpi/acpi-defs.h"
>>  #include "hw/acpi/acpi.h"
>> -#include "hw/nvram/fw_cfg.h"
>>  #include "hw/acpi/bios-linker-loader.h"
>>  #include "hw/loader.h"
>>  #include "hw/hw.h"
> 
> 
> This file uses fw_cfg_add_file, looks wrong.

Previously I thought if another include (here hw/loader.h) already pulls
this header, we can drop it, but Peter Maydell explained me this not
safe for refactors:

  Generally I think that if a .c file directly uses function X declared
  in header Y it should #include Y, even if it happens that it already
  includes other header Z that includes Y. Otherwise if we refactor Z
  later such that it no longer needs to include Y, it will break
  compilation of the .c file. (That is, Z including Y is a detail of
  the implementation of Z, not a guarantee made by Z to its users.)

https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg02460.html

I'm sorry I forgot to update this in the series :/

Regards,

Phil.

Re: [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command

[Philippe Mathieu-Daudé]

On 12/7/18 6:54 PM, Michael S. Tsirkin wrote:
> On Fri, Dec 07, 2018 at 06:03:59PM +0100, Philippe Mathieu-Daudé wrote:
>> $ qemu-system-x86_64 -S -monitor stdio
>> (qemu) info fw_cfg
>>         Type    Perm    Size    Specific   Order   Info
> 
> Can we do better than "Info"?

For some entry this is the "content", but for the files this is the
"path". Do you prefer "Content or path"?

> 
>>    signature      RO       4                       QEMU
>>           id      RO       4                       0x00000003
>>         uuid      RO      16                       00000000-0000-0000-0000-000000000000
>>     ram_size      RO       8                       0x0000000008000000
>>    nographic      RO       2                       0x0000
>>      nb_cpus      RO       2                       0x0001
>>         numa      RO      16
>>    boot_menu      RO       2                       0x0000
>>     max_cpus      RO       2                       0x0001
>>     file_dir      RO    2052
>>  file (id 1)      RO      36                 160   etc/acpi/rsdp
>>  file (id 2)      RO  131072                 130   etc/acpi/tables
>>  file (id 3)      RO       4                  15   etc/boot-fail-wait
>>  file (id 4)      RO      20                  40   etc/e820
>>  file (id 5)      RO      31                  30   etc/smbios/smbios-anchor
>>  file (id 6)      RO     320                  20   etc/smbios/smbios-tables
>>  file (id 7)      RO       6                  90   etc/system-states
>>  file (id 8)      RO    4096                 140   etc/table-loader
>> file (id 10)      RO    9216                  55   genroms/kvmvapic.bin
>>         uuid      RO       4  (arch spec)          01000000-0000-0000-0000-000000000000
>>     ram_size      RO     324  (arch spec)
>>    nographic      RO     121  (arch spec)
> 
> Weird. Your code has arch_spec.

Hmmm I'll check that.

> 
>> (qemu)
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> 
> Looks helpful but generally IMHO whatever is exposed through hmp
> should be in the qmp as well.

OK.

> 
> 
>> ---
>>  hmp-commands-info.hx      |  14 +++++
>>  hw/nvram/fw_cfg.c         | 115 ++++++++++++++++++++++++++++++++++++++
>>  include/hw/nvram/fw_cfg.h |   2 +
>>  3 files changed, 131 insertions(+)
>>
>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
>> index cbee8b944d..9e86dceeb4 100644
>> --- a/hmp-commands-info.hx
>> +++ b/hmp-commands-info.hx
>> @@ -916,6 +916,20 @@ STEXI
>>  @item info sev
>>  @findex info sev
>>  Show SEV information.
>> +ETEXI
>> +
>> +    {
>> +        .name       = "fw_cfg",
>> +        .args_type  = "",
>> +        .params     = "",
>> +        .help       = "Display the table of the fw_cfg entries registered",
> 
> I think the help line should be a bit more verbose.
> Mention it's a paravirtualized interface, and why
> would one want to display it (debugging guest firmware?).

OK.

> 
> 
>> +        .cmd        = hmp_info_fw_cfg,
>> +    },
>> +
>> +STEXI
>> +@item info fw_cfg
>> +@findex info fw_cfg
>> +Show roms.
>>  ETEXI
>>  
>>  STEXI
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 582653f07e..50525ec1dd 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -36,6 +36,7 @@
>>  #include "qemu/config-file.h"
>>  #include "qemu/cutils.h"
>>  #include "qapi/error.h"
>> +#include "monitor/monitor.h"
>>  
>>  #define FW_CFG_FILE_SLOTS_DFLT 0x20
>>  
>> @@ -1164,3 +1165,117 @@ static void fw_cfg_register_types(void)
>>  }
>>  
>>  type_init(fw_cfg_register_types)
>> +
>> +static const char *fw_cfg_wellknown_entries[0x20] = {
>> +    [FW_CFG_SIGNATURE] = "signature",
>> +    [FW_CFG_ID] = "id",
>> +    [FW_CFG_UUID] = "uuid",
>> +    [FW_CFG_RAM_SIZE] = "ram_size",
>> +    [FW_CFG_NOGRAPHIC] = "nographic",
>> +    [FW_CFG_NB_CPUS] = "nb_cpus",
>> +    [FW_CFG_MACHINE_ID] = "machine_id",
>> +    [FW_CFG_KERNEL_ADDR] = "kernel_addr",
>> +    [FW_CFG_KERNEL_SIZE] = "kernel_size",
>> +    [FW_CFG_KERNEL_CMDLINE] = "kernel_cmdline",
>> +    [FW_CFG_INITRD_ADDR] = "initrd_addr",
>> +    [FW_CFG_INITRD_SIZE] = "initdr_size",
>> +    [FW_CFG_BOOT_DEVICE] = "boot_device",
>> +    [FW_CFG_NUMA] = "numa",
>> +    [FW_CFG_BOOT_MENU] = "boot_menu",
>> +    [FW_CFG_MAX_CPUS] = "max_cpus",
>> +    [FW_CFG_KERNEL_ENTRY] = "kernel_entry",
>> +    [FW_CFG_KERNEL_DATA] = "kernel_data",
>> +    [FW_CFG_INITRD_DATA] = "initrd_data",
>> +    [FW_CFG_CMDLINE_ADDR] = "cmdline_addr",
>> +    [FW_CFG_CMDLINE_SIZE] = "cmdline_size",
>> +    [FW_CFG_CMDLINE_DATA] = "cmdline_data",
>> +    [FW_CFG_SETUP_ADDR] = "setup_addr",
>> +    [FW_CFG_SETUP_SIZE] = "setup_size",
>> +    [FW_CFG_SETUP_DATA] = "setup_data",
>> +    [FW_CFG_FILE_DIR] = "file_dir",
>> +};
>> +
>> +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict)
>> +{
>> +    FWCfgState *s = fw_cfg_find();
>> +    int arch, key;
> 
> Looks like this will crash on a machine without fw cfg.

Oops, good catch :)

> 
> 
>> +
>> +    monitor_printf(mon, "%12s %5s %7s %9s %6s %-24s\n",
>> +                   "Type", "Perm", "Size", "Specific", "Order", "Info");
>> +    for (arch = 0; arch < ARRAY_SIZE(s->entries); ++arch) {
>> +        for (key = 0; key < fw_cfg_max_entry(s); ++key) {
>> +            FWCfgEntry *e = &s->entries[arch][key];
>> +            const char *perm = e->allow_write ? "RW" : "RO";
>> +            const char *arch_spec = arch ? "arch_spec" : "";
>> +            char *type, *info = NULL;
>> +
>> +            if (!e->len) {
>> +                continue;
>> +            }
>> +            if (key >= FW_CFG_FILE_FIRST) {
>> +                int id = key - FW_CFG_FILE_FIRST;
>> +                const char *path = s->files->f[id].name;
>> +
>> +                type = g_strdup_printf("file (id %d)", id);
>> +                monitor_printf(mon, "%12s %5s %7d %10s %5d %-24s\n",
>> +                               type, perm, e->len, arch_spec,
>> +                               get_fw_cfg_order(s, path), path);
>> +                g_free(type);
>> +                continue;
>> +            }
>> +            type = g_strdup(fw_cfg_wellknown_entries[key]);
>> +            if (!type) {
>> +                type = g_strdup_printf("entry_%d", key);
>> +            }
>> +
>> +            switch (key) {
>> +            case FW_CFG_SIGNATURE:
>> +                info = g_strndup((const gchar *)e->data, e->len);
>> +                break;
>> +            case FW_CFG_UUID:
>> +                info = qemu_uuid_unparse_strdup((const QemuUUID *)e->data);
>> +                break;
>> +            case FW_CFG_ID:
>> +            case FW_CFG_NOGRAPHIC:
>> +            case FW_CFG_NB_CPUS:
>> +            case FW_CFG_BOOT_MENU:
>> +            case FW_CFG_MAX_CPUS:
>> +            case FW_CFG_RAM_SIZE:
>> +            case FW_CFG_KERNEL_ADDR:
>> +            case FW_CFG_KERNEL_SIZE:
>> +            case FW_CFG_KERNEL_CMDLINE:
>> +            case FW_CFG_KERNEL_ENTRY:
>> +            case FW_CFG_KERNEL_DATA:
>> +            case FW_CFG_INITRD_ADDR:
>> +            case FW_CFG_INITRD_SIZE:
>> +            case FW_CFG_INITRD_DATA:
>> +            case FW_CFG_CMDLINE_ADDR:
>> +            case FW_CFG_CMDLINE_SIZE:
>> +            case FW_CFG_CMDLINE_DATA:
>> +            case FW_CFG_SETUP_ADDR:
>> +            case FW_CFG_SETUP_SIZE:
>> +            case FW_CFG_SETUP_DATA:
>> +                switch (e->len) {
>> +                case 2:
>> +                    info = g_strdup_printf("0x%04x", lduw_le_p(e->data));
>> +                    break;
>> +                case 4:
>> +                    info = g_strdup_printf("0x%08x", ldl_le_p(e->data));
>> +                    break;
>> +                case 8:
>> +                    info = g_strdup_printf("0x%016" PRIx64, ldq_le_p(e->data));
>> +                    break;
>> +                default:
>> +                    break;
>> +                }
>> +                break;
>> +            default:
>> +                break;
>> +            }
>> +            monitor_printf(mon, "%12s %5s %7d %10s %5s %-24s\n",
>> +                           type, perm, e->len, arch_spec, "", info ? info : "");
>> +            g_free(type);
>> +            g_free(info);
>> +        }
>> +    }
>> +}
>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>> index f5a6895a74..28630b2f26 100644
>> --- a/include/hw/nvram/fw_cfg.h
>> +++ b/include/hw/nvram/fw_cfg.h
>> @@ -226,4 +226,6 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>>  FWCfgState *fw_cfg_find(void);
>>  bool fw_cfg_dma_enabled(void *opaque);
>>  
>> +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict);
>> +
>>  #endif
>> -- 
>> 2.17.2

Re: [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host()

[Philippe Mathieu-Daudé]

On 12/7/18 11:48 PM, no-reply@patchew.org wrote:
> This series failed the docker-quick@centos7 build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
> 
...
> The full log is available at
> http://patchew.org/logs/20181207170400.5129-1-philmd@redhat.com/testing.docker-quick@centos7/?type=message.

  qemu-system-x86_64: Back to tcg accelerator
  Broken pipe
  /tmp/qemu-test/src/tests/libqtest.c:125: kill_qemu() detected QEMU
death from signal 11 (Segmentation fault) (core dumped)
  GTester: last random seed: R02Sb6a4400485731f23217a8855e030b450
  Broken pipe

Probably the error Michael noticed:

>> +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict)
>> +{
>> +    FWCfgState *s = fw_cfg_find();
>> +    int arch, key;
>
>  Looks like this will crash on a machine without fw cfg.

Re: [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> $ qemu-system-x86_64 -S -monitor stdio
> (qemu) info fw_cfg
>         Type    Perm    Size    Specific   Order   Info
>    signature      RO       4                       QEMU
>           id      RO       4                       0x00000003
>         uuid      RO      16                       00000000-0000-0000-0000-000000000000
>     ram_size      RO       8                       0x0000000008000000
>    nographic      RO       2                       0x0000
>      nb_cpus      RO       2                       0x0001
>         numa      RO      16
>    boot_menu      RO       2                       0x0000
>     max_cpus      RO       2                       0x0001
>     file_dir      RO    2052
>  file (id 1)      RO      36                 160   etc/acpi/rsdp
>  file (id 2)      RO  131072                 130   etc/acpi/tables
>  file (id 3)      RO       4                  15   etc/boot-fail-wait
>  file (id 4)      RO      20                  40   etc/e820
>  file (id 5)      RO      31                  30   etc/smbios/smbios-anchor
>  file (id 6)      RO     320                  20   etc/smbios/smbios-tables
>  file (id 7)      RO       6                  90   etc/system-states
>  file (id 8)      RO    4096                 140   etc/table-loader
> file (id 10)      RO    9216                  55   genroms/kvmvapic.bin
>         uuid      RO       4  (arch spec)          01000000-0000-0000-0000-000000000000
>     ram_size      RO     324  (arch spec)
>    nographic      RO     121  (arch spec)
> (qemu)
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hmp-commands-info.hx      |  14 +++++
>  hw/nvram/fw_cfg.c         | 115 ++++++++++++++++++++++++++++++++++++++
>  include/hw/nvram/fw_cfg.h |   2 +
>  3 files changed, 131 insertions(+)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index cbee8b944d..9e86dceeb4 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -916,6 +916,20 @@ STEXI
>  @item info sev
>  @findex info sev
>  Show SEV information.
> +ETEXI
> +
> +    {
> +        .name       = "fw_cfg",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "Display the table of the fw_cfg entries registered",
> +        .cmd        = hmp_info_fw_cfg,
> +    },
> +
> +STEXI
> +@item info fw_cfg
> +@findex info fw_cfg
> +Show roms.
>  ETEXI
>  
>  STEXI
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 582653f07e..50525ec1dd 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -36,6 +36,7 @@
>  #include "qemu/config-file.h"
>  #include "qemu/cutils.h"
>  #include "qapi/error.h"
> +#include "monitor/monitor.h"
>  
>  #define FW_CFG_FILE_SLOTS_DFLT 0x20
>  
> @@ -1164,3 +1165,117 @@ static void fw_cfg_register_types(void)
>  }
>  
>  type_init(fw_cfg_register_types)
> +
> +static const char *fw_cfg_wellknown_entries[0x20] = {

That magic 0x20 is a shame; we should probably have a #define
for that max, or perhaps it can just be removed.

> +    [FW_CFG_SIGNATURE] = "signature",
> +    [FW_CFG_ID] = "id",
> +    [FW_CFG_UUID] = "uuid",
> +    [FW_CFG_RAM_SIZE] = "ram_size",
> +    [FW_CFG_NOGRAPHIC] = "nographic",
> +    [FW_CFG_NB_CPUS] = "nb_cpus",
> +    [FW_CFG_MACHINE_ID] = "machine_id",
> +    [FW_CFG_KERNEL_ADDR] = "kernel_addr",
> +    [FW_CFG_KERNEL_SIZE] = "kernel_size",
> +    [FW_CFG_KERNEL_CMDLINE] = "kernel_cmdline",
> +    [FW_CFG_INITRD_ADDR] = "initrd_addr",
> +    [FW_CFG_INITRD_SIZE] = "initdr_size",
> +    [FW_CFG_BOOT_DEVICE] = "boot_device",
> +    [FW_CFG_NUMA] = "numa",
> +    [FW_CFG_BOOT_MENU] = "boot_menu",
> +    [FW_CFG_MAX_CPUS] = "max_cpus",
> +    [FW_CFG_KERNEL_ENTRY] = "kernel_entry",
> +    [FW_CFG_KERNEL_DATA] = "kernel_data",
> +    [FW_CFG_INITRD_DATA] = "initrd_data",
> +    [FW_CFG_CMDLINE_ADDR] = "cmdline_addr",
> +    [FW_CFG_CMDLINE_SIZE] = "cmdline_size",
> +    [FW_CFG_CMDLINE_DATA] = "cmdline_data",
> +    [FW_CFG_SETUP_ADDR] = "setup_addr",
> +    [FW_CFG_SETUP_SIZE] = "setup_size",
> +    [FW_CFG_SETUP_DATA] = "setup_data",
> +    [FW_CFG_FILE_DIR] = "file_dir",
> +};
> +
> +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict)
> +{
> +    FWCfgState *s = fw_cfg_find();
> +    int arch, key;
> +
> +    monitor_printf(mon, "%12s %5s %7s %9s %6s %-24s\n",
> +                   "Type", "Perm", "Size", "Specific", "Order", "Info");
> +    for (arch = 0; arch < ARRAY_SIZE(s->entries); ++arch) {
> +        for (key = 0; key < fw_cfg_max_entry(s); ++key) {
> +            FWCfgEntry *e = &s->entries[arch][key];
> +            const char *perm = e->allow_write ? "RW" : "RO";
> +            const char *arch_spec = arch ? "arch_spec" : "";
> +            char *type, *info = NULL;
> +
> +            if (!e->len) {
> +                continue;
> +            }
> +            if (key >= FW_CFG_FILE_FIRST) {
> +                int id = key - FW_CFG_FILE_FIRST;
> +                const char *path = s->files->f[id].name;
> +
> +                type = g_strdup_printf("file (id %d)", id);
> +                monitor_printf(mon, "%12s %5s %7d %10s %5d %-24s\n",
> +                               type, perm, e->len, arch_spec,
> +                               get_fw_cfg_order(s, path), path);
> +                g_free(type);
> +                continue;
> +            }
> +            type = g_strdup(fw_cfg_wellknown_entries[key]);
> +            if (!type) {
> +                type = g_strdup_printf("entry_%d", key);
> +            }
> +
> +            switch (key) {
> +            case FW_CFG_SIGNATURE:
> +                info = g_strndup((const gchar *)e->data, e->len);
> +                break;
> +            case FW_CFG_UUID:
> +                info = qemu_uuid_unparse_strdup((const QemuUUID *)e->data);
> +                break;

It seems odd to encode the type of each entry in this switch;  I guess
it's OK if no one else needs it, else perhaps there should be a type
array.

Other than that, it's fine from the HMP side, but I agree it should
either have a QMP equivalent (in which case the structure is a bit
different) or a good justification of if it's really debug only.

Dave

> +            case FW_CFG_ID:
> +            case FW_CFG_NOGRAPHIC:
> +            case FW_CFG_NB_CPUS:
> +            case FW_CFG_BOOT_MENU:
> +            case FW_CFG_MAX_CPUS:
> +            case FW_CFG_RAM_SIZE:
> +            case FW_CFG_KERNEL_ADDR:
> +            case FW_CFG_KERNEL_SIZE:
> +            case FW_CFG_KERNEL_CMDLINE:
> +            case FW_CFG_KERNEL_ENTRY:
> +            case FW_CFG_KERNEL_DATA:
> +            case FW_CFG_INITRD_ADDR:
> +            case FW_CFG_INITRD_SIZE:
> +            case FW_CFG_INITRD_DATA:
> +            case FW_CFG_CMDLINE_ADDR:
> +            case FW_CFG_CMDLINE_SIZE:
> +            case FW_CFG_CMDLINE_DATA:
> +            case FW_CFG_SETUP_ADDR:
> +            case FW_CFG_SETUP_SIZE:
> +            case FW_CFG_SETUP_DATA:
> +                switch (e->len) {
> +                case 2:
> +                    info = g_strdup_printf("0x%04x", lduw_le_p(e->data));
> +                    break;
> +                case 4:
> +                    info = g_strdup_printf("0x%08x", ldl_le_p(e->data));
> +                    break;
> +                case 8:
> +                    info = g_strdup_printf("0x%016" PRIx64, ldq_le_p(e->data));
> +                    break;
> +                default:
> +                    break;
> +                }
> +                break;
> +            default:
> +                break;
> +            }
> +            monitor_printf(mon, "%12s %5s %7d %10s %5s %-24s\n",
> +                           type, perm, e->len, arch_spec, "", info ? info : "");
> +            g_free(type);
> +            g_free(info);
> +        }
> +    }
> +}
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index f5a6895a74..28630b2f26 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -226,4 +226,6 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>  FWCfgState *fw_cfg_find(void);
>  bool fw_cfg_dma_enabled(void *opaque);
>  
> +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict);
> +
>  #endif
> -- 
> 2.17.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/6] hw/arm/virt: Remove null-check in virt_build_smbios()

[Laszlo Ersek]

On 12/07/18 18:03, Philippe Mathieu-Daudé wrote:
> Since af1f60a4022, the fw_cfg field is always created in machvirt_init().
> There is no need to null check it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/arm/virt.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f69e7eb399..36303ed59c 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1279,10 +1279,6 @@ static void virt_build_smbios(VirtMachineState *vms)
>      size_t smbios_tables_len, smbios_anchor_len;
>      const char *product = "QEMU Virtual Machine";
>  
> -    if (!vms->fw_cfg) {
> -        return;
> -    }
> -
>      if (kvm_enabled()) {
>          product = "KVM Virtual Machine";
>      }
> 

This patch looks correct to me; however, the fact that fw_cfg is always
present on the virt board does not seem related to commit af1f60a4022
af1f60a40226 ("hw/arm/virt: remove VirtGuestInfo", 2017-01-09). Said
commit only flattened the contents of VirtGuestInfo into
VirtMachineState; the conditions under which fw_cfg would be created
were not changed.

As far as I can see (from a number of git-blame / git-show commands),
fw_cfg was added unconditionally to the virt board in commit
578f3c7b0835 ("arm: add fw_cfg to "virt" board", 2014-12-22). And the
check that you are now (correctly) removing has always been superfluous
-- it was superfluous already when it was first added in commit
c30e15658b1b ("smbios: implement smbios support for mach-virt", 2015-09-07).

As far as I can tell, anyway.

If you agree, I suggest mentioning commit c30e15658b1b in the commit
message, rather than commit af1f60a4022.

With such an update:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 4/6] hw/nvram/fw_cfg: Add trace events

[Laszlo Ersek]

On 12/07/18 18:03, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/nvram/fw_cfg.c     | 5 +++++
>  hw/nvram/trace-events | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 3cb726ff68..582653f07e 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -627,6 +627,7 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
>  
>  void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len)
>  {
> +    trace_fw_cfg_add_bytes(key, len);
>      fw_cfg_add_bytes_callback(s, key, NULL, NULL, NULL, data, len, true);
>  }
>  
> @@ -634,6 +635,7 @@ void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value)
>  {
>      size_t sz = strlen(value) + 1;
>  
> +    trace_fw_cfg_add_string(key, value);
>      fw_cfg_add_bytes(s, key, g_memdup(value, sz), sz);
>  }
>  
> @@ -643,6 +645,7 @@ void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value)
>  
>      copy = g_malloc(sizeof(value));
>      *copy = cpu_to_le16(value);
> +    trace_fw_cfg_add_i16(key, value);
>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>  }
>  
> @@ -662,6 +665,7 @@ void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value)
>  
>      copy = g_malloc(sizeof(value));
>      *copy = cpu_to_le32(value);
> +    trace_fw_cfg_add_i32(key, value);
>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>  }
>  
> @@ -671,6 +675,7 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
>  
>      copy = g_malloc(sizeof(value));
>      *copy = cpu_to_le64(value);
> +    trace_fw_cfg_add_i64(key, value);
>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>  }
>  
> diff --git a/hw/nvram/trace-events b/hw/nvram/trace-events
> index 6b55ba7a09..0ee0f8d04a 100644
> --- a/hw/nvram/trace-events
> +++ b/hw/nvram/trace-events
> @@ -7,4 +7,9 @@ nvram_write(uint32_t addr, uint32_t old, uint32_t val) "write addr %d: 0x%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, uint64_t ret) "%p = 0x%"PRIx64
> +fw_cfg_add_bytes(uint16_t key, size_t len) "key 0x%04" PRIx16 " (%zu bytes)"
>  fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)"
> +fw_cfg_add_string(uint16_t key, const char *value) "key 0x%04" PRIx16 ", value '%s'"
> +fw_cfg_add_i16(uint16_t key, uint32_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx32

For the "value" parameter, you should use "uint16_t" here (and replace
PRIx32 with PRIx16, as well).

(In practice this will make no difference, but for consistency's sake,
we should do this.)

With the update:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> +fw_cfg_add_i32(uint16_t key, uint32_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx32
> +fw_cfg_add_i64(uint16_t key, uint64_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx64
> 

Re: [Qemu-devel] [PATCH 4/6] hw/nvram/fw_cfg: Add trace events

[Philippe Mathieu-Daudé]

On 12/10/18 5:10 PM, Laszlo Ersek wrote:
> On 12/07/18 18:03, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/nvram/fw_cfg.c     | 5 +++++
>>  hw/nvram/trace-events | 5 +++++
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 3cb726ff68..582653f07e 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -627,6 +627,7 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
>>  
>>  void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len)
>>  {
>> +    trace_fw_cfg_add_bytes(key, len);
>>      fw_cfg_add_bytes_callback(s, key, NULL, NULL, NULL, data, len, true);
>>  }
>>  
>> @@ -634,6 +635,7 @@ void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value)
>>  {
>>      size_t sz = strlen(value) + 1;
>>  
>> +    trace_fw_cfg_add_string(key, value);
>>      fw_cfg_add_bytes(s, key, g_memdup(value, sz), sz);
>>  }
>>  
>> @@ -643,6 +645,7 @@ void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value)
>>  
>>      copy = g_malloc(sizeof(value));
>>      *copy = cpu_to_le16(value);
>> +    trace_fw_cfg_add_i16(key, value);
>>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>>  }
>>  
>> @@ -662,6 +665,7 @@ void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value)
>>  
>>      copy = g_malloc(sizeof(value));
>>      *copy = cpu_to_le32(value);
>> +    trace_fw_cfg_add_i32(key, value);
>>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>>  }
>>  
>> @@ -671,6 +675,7 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
>>  
>>      copy = g_malloc(sizeof(value));
>>      *copy = cpu_to_le64(value);
>> +    trace_fw_cfg_add_i64(key, value);
>>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>>  }
>>  
>> diff --git a/hw/nvram/trace-events b/hw/nvram/trace-events
>> index 6b55ba7a09..0ee0f8d04a 100644
>> --- a/hw/nvram/trace-events
>> +++ b/hw/nvram/trace-events
>> @@ -7,4 +7,9 @@ nvram_write(uint32_t addr, uint32_t old, uint32_t val) "write addr %d: 0x%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, uint64_t ret) "%p = 0x%"PRIx64
>> +fw_cfg_add_bytes(uint16_t key, size_t len) "key 0x%04" PRIx16 " (%zu bytes)"
>>  fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)"
>> +fw_cfg_add_string(uint16_t key, const char *value) "key 0x%04" PRIx16 ", value '%s'"
>> +fw_cfg_add_i16(uint16_t key, uint32_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx32
> 
> For the "value" parameter, you should use "uint16_t" here (and replace
> PRIx32 with PRIx16, as well).
> 
> (In practice this will make no difference, but for consistency's sake,
> we should do this.)

You are right :)

> 
> With the update:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo
> 
>> +fw_cfg_add_i32(uint16_t key, uint32_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx32
>> +fw_cfg_add_i64(uint16_t key, uint64_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx64
>>
> 

Re: [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command

[Laszlo Ersek]

I can only muster some random thoughts for this one, presently:

On 12/07/18 18:03, Philippe Mathieu-Daudé wrote:
> $ qemu-system-x86_64 -S -monitor stdio
> (qemu) info fw_cfg
>         Type    Perm    Size    Specific   Order   Info
>    signature      RO       4                       QEMU
>           id      RO       4                       0x00000003
>         uuid      RO      16                       00000000-0000-0000-0000-000000000000
>     ram_size      RO       8                       0x0000000008000000
>    nographic      RO       2                       0x0000
>      nb_cpus      RO       2                       0x0001
>         numa      RO      16
>    boot_menu      RO       2                       0x0000
>     max_cpus      RO       2                       0x0001
>     file_dir      RO    2052
>  file (id 1)      RO      36                 160   etc/acpi/rsdp
>  file (id 2)      RO  131072                 130   etc/acpi/tables
>  file (id 3)      RO       4                  15   etc/boot-fail-wait
>  file (id 4)      RO      20                  40   etc/e820
>  file (id 5)      RO      31                  30   etc/smbios/smbios-anchor
>  file (id 6)      RO     320                  20   etc/smbios/smbios-tables
>  file (id 7)      RO       6                  90   etc/system-states
>  file (id 8)      RO    4096                 140   etc/table-loader
> file (id 10)      RO    9216                  55   genroms/kvmvapic.bin
>         uuid      RO       4  (arch spec)          01000000-0000-0000-0000-000000000000
>     ram_size      RO     324  (arch spec)
>    nographic      RO     121  (arch spec)
> (qemu)
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hmp-commands-info.hx      |  14 +++++
>  hw/nvram/fw_cfg.c         | 115 ++++++++++++++++++++++++++++++++++++++
>  include/hw/nvram/fw_cfg.h |   2 +
>  3 files changed, 131 insertions(+)
>
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index cbee8b944d..9e86dceeb4 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -916,6 +916,20 @@ STEXI
>  @item info sev
>  @findex info sev
>  Show SEV information.
> +ETEXI
> +
> +    {
> +        .name       = "fw_cfg",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "Display the table of the fw_cfg entries registered",
> +        .cmd        = hmp_info_fw_cfg,
> +    },
> +
> +STEXI
> +@item info fw_cfg
> +@findex info fw_cfg
> +Show roms.
>  ETEXI
>
>  STEXI
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 582653f07e..50525ec1dd 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -36,6 +36,7 @@
>  #include "qemu/config-file.h"
>  #include "qemu/cutils.h"
>  #include "qapi/error.h"
> +#include "monitor/monitor.h"
>
>  #define FW_CFG_FILE_SLOTS_DFLT 0x20
>
> @@ -1164,3 +1165,117 @@ static void fw_cfg_register_types(void)
>  }
>
>  type_init(fw_cfg_register_types)
> +
> +static const char *fw_cfg_wellknown_entries[0x20] = {

(1) Using 0x20 is not wrong here (it is a well-known magic constant),
but we should refer to it by name: please refer to FW_CFG_FILE_FIRST.
(See "include/standard-headers/linux/qemu_fw_cfg.h")

> +    [FW_CFG_SIGNATURE] = "signature",
> +    [FW_CFG_ID] = "id",
> +    [FW_CFG_UUID] = "uuid",
> +    [FW_CFG_RAM_SIZE] = "ram_size",
> +    [FW_CFG_NOGRAPHIC] = "nographic",
> +    [FW_CFG_NB_CPUS] = "nb_cpus",
> +    [FW_CFG_MACHINE_ID] = "machine_id",
> +    [FW_CFG_KERNEL_ADDR] = "kernel_addr",
> +    [FW_CFG_KERNEL_SIZE] = "kernel_size",
> +    [FW_CFG_KERNEL_CMDLINE] = "kernel_cmdline",
> +    [FW_CFG_INITRD_ADDR] = "initrd_addr",
> +    [FW_CFG_INITRD_SIZE] = "initdr_size",
> +    [FW_CFG_BOOT_DEVICE] = "boot_device",
> +    [FW_CFG_NUMA] = "numa",
> +    [FW_CFG_BOOT_MENU] = "boot_menu",
> +    [FW_CFG_MAX_CPUS] = "max_cpus",
> +    [FW_CFG_KERNEL_ENTRY] = "kernel_entry",
> +    [FW_CFG_KERNEL_DATA] = "kernel_data",
> +    [FW_CFG_INITRD_DATA] = "initrd_data",
> +    [FW_CFG_CMDLINE_ADDR] = "cmdline_addr",
> +    [FW_CFG_CMDLINE_SIZE] = "cmdline_size",
> +    [FW_CFG_CMDLINE_DATA] = "cmdline_data",
> +    [FW_CFG_SETUP_ADDR] = "setup_addr",
> +    [FW_CFG_SETUP_SIZE] = "setup_size",
> +    [FW_CFG_SETUP_DATA] = "setup_data",
> +    [FW_CFG_FILE_DIR] = "file_dir",
> +};

(2) I suggest introducing a macro that generates the initializer, using
the stringify() macro from "include/qemu/compiler.h". It should go
something like

#define FW_CFG_KEY_STRINGIFY(key) [key] = stringify(key),

Feel free to disagree though; this set of predefined, fixed numeric keys
will never change (nor will it ever be extened), so we might as well
live with the currently proposed verbosity.

> +
> +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict)
> +{
> +    FWCfgState *s = fw_cfg_find();
> +    int arch, key;
> +
> +    monitor_printf(mon, "%12s %5s %7s %9s %6s %-24s\n",
> +                   "Type", "Perm", "Size", "Specific", "Order", "Info");
> +    for (arch = 0; arch < ARRAY_SIZE(s->entries); ++arch) {
> +        for (key = 0; key < fw_cfg_max_entry(s); ++key) {
> +            FWCfgEntry *e = &s->entries[arch][key];
> +            const char *perm = e->allow_write ? "RW" : "RO";
> +            const char *arch_spec = arch ? "arch_spec" : "";
> +            char *type, *info = NULL;
> +
> +            if (!e->len) {
> +                continue;
> +            }
> +            if (key >= FW_CFG_FILE_FIRST) {
> +                int id = key - FW_CFG_FILE_FIRST;
> +                const char *path = s->files->f[id].name;
> +
> +                type = g_strdup_printf("file (id %d)", id);
> +                monitor_printf(mon, "%12s %5s %7d %10s %5d %-24s\n",
> +                               type, perm, e->len, arch_spec,
> +                               get_fw_cfg_order(s, path), path);
> +                g_free(type);
> +                continue;
> +            }
> +            type = g_strdup(fw_cfg_wellknown_entries[key]);
> +            if (!type) {
> +                type = g_strdup_printf("entry_%d", key);
> +            }
> +
> +            switch (key) {
> +            case FW_CFG_SIGNATURE:
> +                info = g_strndup((const gchar *)e->data, e->len);
> +                break;
> +            case FW_CFG_UUID:
> +                info = qemu_uuid_unparse_strdup((const QemuUUID *)e->data);
> +                break;
> +            case FW_CFG_ID:
> +            case FW_CFG_NOGRAPHIC:
> +            case FW_CFG_NB_CPUS:
> +            case FW_CFG_BOOT_MENU:
> +            case FW_CFG_MAX_CPUS:
> +            case FW_CFG_RAM_SIZE:
> +            case FW_CFG_KERNEL_ADDR:
> +            case FW_CFG_KERNEL_SIZE:
> +            case FW_CFG_KERNEL_CMDLINE:
> +            case FW_CFG_KERNEL_ENTRY:
> +            case FW_CFG_KERNEL_DATA:
> +            case FW_CFG_INITRD_ADDR:
> +            case FW_CFG_INITRD_SIZE:
> +            case FW_CFG_INITRD_DATA:
> +            case FW_CFG_CMDLINE_ADDR:
> +            case FW_CFG_CMDLINE_SIZE:
> +            case FW_CFG_CMDLINE_DATA:
> +            case FW_CFG_SETUP_ADDR:
> +            case FW_CFG_SETUP_SIZE:
> +            case FW_CFG_SETUP_DATA:
> +                switch (e->len) {
> +                case 2:
> +                    info = g_strdup_printf("0x%04x", lduw_le_p(e->data));
> +                    break;
> +                case 4:
> +                    info = g_strdup_printf("0x%08x", ldl_le_p(e->data));
> +                    break;
> +                case 8:
> +                    info = g_strdup_printf("0x%016" PRIx64, ldq_le_p(e->data));
> +                    break;
> +                default:
> +                    break;
> +                }
> +                break;
> +            default:
> +                break;
> +            }
> +            monitor_printf(mon, "%12s %5s %7d %10s %5s %-24s\n",
> +                           type, perm, e->len, arch_spec, "", info ? info : "");
> +            g_free(type);
> +            g_free(info);
> +        }
> +    }
> +}

(3) In general I wouldn't try to be smart here, regarding the contents.
I suggest simply dumping arrays of uint8_t values, in hex.

(4) Also I wouldn't reuse the same column for different purposes; I
think it's easier to read if a column always means the same thing, and
if it doesn't apply, it's just left blank. Something like this:

  Selector  Well-Known Numeric  Pathname  Perm  Size  Arch-Spec  Order  Data

- "Selector" is the uint16 selector key
- "Well-Known Numeric" is the stringified name if the selector refers to
  a well-known numerically defined item.
- "Pathname" is the pathname for named files. Mutually exclusive with
  "Well-Known Numeric", but that's OK, IMO.
- "Arch-Spec" should be left blank, or marked as "set" with an asterisk.
- "Data" should be a hexdump of uint8_t values (normally limited to 8
  bytes, if the array is larger).

(5) I think this interface would benefit from being exposed over QMP /
JSON, and then the tabular presentation would be a separate question in
HMP.

(6) If we want bells and whistles, two optional parameters could be
considered: one for identifying one specific key to request info about
(identify by selector? well-known macro name? file pathname?), and
another param for placing a limit, different from 8, on the individual
hexdump size.


Important: these are not "requirements", just random ideas, food for
thought. I'm fine if you reject any subset of them, after consideration.
I don't intend this to spiral into feature creep; take whatever you
like.

Thanks!
Laszlo

> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index f5a6895a74..28630b2f26 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -226,4 +226,6 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>  FWCfgState *fw_cfg_find(void);
>  bool fw_cfg_dma_enabled(void *opaque);
>
> +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict);
> +
>  #endif
>

Re: [Qemu-devel] [PATCH 6/6] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host()

[Laszlo Ersek]

On 12/07/18 18:04, Philippe Mathieu-Daudé wrote:
> Add a function to read the full content of file on the host, and add
> a new 'file' name item to the fw_cfg device.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/nvram/fw_cfg.c         | 22 ++++++++++++++++++++++
>  include/hw/nvram/fw_cfg.h | 22 ++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 50525ec1dd..f3232fcb16 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -842,6 +842,28 @@ void fw_cfg_add_file(FWCfgState *s,  const char *filename,
>      fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data, len, true);
>  }
>  
> +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename,
> +                                const char *host_path, size_t *len)
> +{
> +    GError *gerr = NULL;
> +    gchar *ptr = NULL;
> +    gsize contents_len = 0;
> +
> +
> +    if (g_file_get_contents(host_path, &ptr, &contents_len, &gerr)) {
> +        fw_cfg_add_file(s, filename, ptr, contents_len);

Can you rename "ptr" to "data" or "contents"?

> +    } else {
> +        error_report("failed to read file %s, %s", host_path, gerr->message);

OK... most functions in this file that populate fw_cfg and can fail
(from external data) still use error_report() + a NULL retval, so this
looks consistent.

> +        g_error_free(gerr);
> +        return NULL;
> +    }
> +    if (len) {
> +        *len = contents_len;
> +    }
> +
> +    return ptr;
> +}
> +
>  void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
>                          void *data, size_t len)
>  {
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 28630b2f26..8de8d63433 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -166,6 +166,28 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
>  void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
>                       size_t len);
>  
> +/**
> + * fw_cfg_add_file_from_host:
> + * @s: fw_cfg device being modified
> + * @filename: name of new fw_cfg file item
> + * @host_path: path of the host file to read the data from
> + * @len: pointer to hold the length of the host file (optional)
> + *
> + * Read the content of a host file as a raw "blob" then add a new NAMED
> + * fw_cfg item of the file size. If @len is provided, it will contains the

s/will contains/will contain/

> + * total length read from the host file. The data referenced by the starting
> + * pointer is only linked, NOT copied, into the data structure of the fw_cfg
> + * device.

Please drop the last sentence; it does not apply here. There is no
starting pointer passed to this function.

... Should we perhaps replace the last sentence by stating that the data
read from the host filesystem is owned by the new fw_cfg entry?

> + * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
> + * will be used; also, a new entry will be added to the file directory
> + * structure residing at key value FW_CFG_FILE_DIR, containing the item name,
> + * data size, and assigned selector key value.
> + *
> + * Returns: pointer to the file content, or NULL if an error occured.
> + */
> +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename,
> +                                const char *host_path, size_t *len);
> +
>  /**
>   * fw_cfg_add_file_callback:
>   * @s: fw_cfg device being modified
> 

Hmmmm. If you do return the pointer to the data just read, directly
exposing the allocation address to the caller, then we could in theory
make this new interface consistent with the rest, and make the *caller*
own the data. Then the currently proposed documentation would be sort-of
accurate (the new fw_cfg entry would not own the data just read -- the
caller would). It's hard to divine what's the best approach here,
without seeing any call sites.

... Purely from a consistency POV, I think I'm slightly attracted to
make the fw_cfg reference again a non-owning one, and force the caller
of fw_cfg_add_file_from_host() to own the data read.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command

[Philippe Mathieu-Daudé]

Hi Michael,

On 12/10/18 10:18 AM, Philippe Mathieu-Daudé wrote:
> On 12/7/18 6:54 PM, Michael S. Tsirkin wrote:
>> On Fri, Dec 07, 2018 at 06:03:59PM +0100, Philippe Mathieu-Daudé wrote:
>>> $ qemu-system-x86_64 -S -monitor stdio
>>> (qemu) info fw_cfg
>>>         Type    Perm    Size    Specific   Order   Info
>>>    signature      RO       4                       QEMU
>>>           id      RO       4                       0x00000003
>>>         uuid      RO      16                       00000000-0000-0000-0000-000000000000
>>>     ram_size      RO       8                       0x0000000008000000
>>>    nographic      RO       2                       0x0000
>>>      nb_cpus      RO       2                       0x0001
>>>         numa      RO      16
>>>    boot_menu      RO       2                       0x0000
>>>     max_cpus      RO       2                       0x0001
>>>     file_dir      RO    2052
>>>  file (id 1)      RO      36                 160   etc/acpi/rsdp
>>>  file (id 2)      RO  131072                 130   etc/acpi/tables
>>>  file (id 3)      RO       4                  15   etc/boot-fail-wait
>>>  file (id 4)      RO      20                  40   etc/e820
>>>  file (id 5)      RO      31                  30   etc/smbios/smbios-anchor
>>>  file (id 6)      RO     320                  20   etc/smbios/smbios-tables
>>>  file (id 7)      RO       6                  90   etc/system-states
>>>  file (id 8)      RO    4096                 140   etc/table-loader
>>> file (id 10)      RO    9216                  55   genroms/kvmvapic.bin
>>>         uuid      RO       4  (arch spec)          01000000-0000-0000-0000-000000000000
>>>     ram_size      RO     324  (arch spec)
>>>    nographic      RO     121  (arch spec)
>>
>> Weird. Your code has arch_spec.
> 
> Hmmm I'll check that.

These are the entries used for Bochs:

#define FW_CFG_ACPI_TABLES (FW_CFG_ARCH_LOCAL + 0)
#define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
#define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
#define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
#define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)

static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)
{
    [...]
    fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES,
                     acpi_tables, acpi_tables_len);
    fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE,
                   kvm_allows_irq0_override());
    fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE,
                     &e820_reserve, sizeof(e820_reserve));

So what happened here is the 'type' name is incorrect (I'll fix), but
the arch_spec flag is correct. Thanks for noticing this :)

Regards,

Phil.

Re: [Qemu-devel] [PATCH 1/6] hw/arm/virt: Remove null-check in virt_build_smbios()

[Philippe Mathieu-Daudé]

Hi Laszlo,

On 12/10/18 5:02 PM, Laszlo Ersek wrote:
> On 12/07/18 18:03, Philippe Mathieu-Daudé wrote:
>> Since af1f60a4022, the fw_cfg field is always created in machvirt_init().
>> There is no need to null check it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/arm/virt.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index f69e7eb399..36303ed59c 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1279,10 +1279,6 @@ static void virt_build_smbios(VirtMachineState *vms)
>>      size_t smbios_tables_len, smbios_anchor_len;
>>      const char *product = "QEMU Virtual Machine";
>>  
>> -    if (!vms->fw_cfg) {
>> -        return;
>> -    }
>> -
>>      if (kvm_enabled()) {
>>          product = "KVM Virtual Machine";
>>      }
>>
> 
> This patch looks correct to me; however, the fact that fw_cfg is always
> present on the virt board does not seem related to commit af1f60a4022
> af1f60a40226 ("hw/arm/virt: remove VirtGuestInfo", 2017-01-09). Said
> commit only flattened the contents of VirtGuestInfo into
> VirtMachineState; the conditions under which fw_cfg would be created
> were not changed.

This is certainly not related to commit af1f60a4022... This is not the
commit I remember I wanted to refer to, I suppose I did a copy/paste
mistake, thanks for carefully reviewing me and catching this!

> As far as I can see (from a number of git-blame / git-show commands),
> fw_cfg was added unconditionally to the virt board in commit
> 578f3c7b0835 ("arm: add fw_cfg to "virt" board", 2014-12-22). And the
> check that you are now (correctly) removing has always been superfluous
> -- it was superfluous already when it was first added in commit
> c30e15658b1b ("smbios: implement smbios support for mach-virt", 2015-09-07).
> 
> As far as I can tell, anyway.
> 
> If you agree, I suggest mentioning commit c30e15658b1b in the commit
> message, rather than commit af1f60a4022.

Yes, this one makes more sense ;)

> 
> With such an update:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!

Phil.

> 
> Thanks
> Laszlo
>